remove Acct-Delay-Time

If we receive an accounting packet, add Event-Timestamp if it's
not already in the packet.

If the packet contains Acct-Delay-Time, then subtract that from
Event-Timestamp, and delete Acct-Delay-Time.

Acct-Delay-Time causes too many issues with proxying and retransmissions.

diff --git a/raddb/sites-available/default b/raddb/sites-available/default
index b23a4105e2ac4882ae046f5681c996fa1dcd9fe1..ed88025ff62db0d5c916e935388061385ff5d7cc 100644 (file)
--- a/raddb/sites-available/default
+++ b/raddb/sites-available/default
@@ -1344,17 +1344,6 @@ recv Accounting-Request {
        #
 #      &request.FreeRADIUS-Acct-Session-Start-Time = "%{(&Event-Timestamp || %l) - &Acct-Session-Time - &Acct-Delay-Time}"
 
-       #
-       #  The packet should have a timestamp.  If not, use "now" from the server.
-       #
-       if (!&Event-Timestamp) {
-               &request.Event-Timestamp := %{%l() - &Acct-Delay-Time}
-
-       } elsif (!&Acct-Delay-Time && &request.Event-Timestamp && (&request.Event-Timestamp < %l())) {
-               &request.Acct-Delay-Time := %{%l() - &Event-Timestamp}
-
-       }
-
        #
        #  Ensure that we have a semi-unique identifier for every
        #  request, as many NAS boxes are broken.

diff --git a/src/modules/rlm_radius/rlm_radius_udp.c b/src/modules/rlm_radius/rlm_radius_udp.c
index a686f5b29e8013ec677be8294278ac87be6878a0..77e1029fb345392bcef6e69f6b41864e68d288f9 100644 (file)
--- a/src/modules/rlm_radius/rlm_radius_udp.c
+++ b/src/modules/rlm_radius/rlm_radius_udp.c
@@ -202,7 +202,6 @@ fr_dict_autoload_t rlm_radius_udp_dict[] = {
        { NULL }
 };
 
-static fr_dict_attr_t const *attr_acct_delay_time;
 static fr_dict_attr_t const *attr_error_cause;
 static fr_dict_attr_t const *attr_event_timestamp;
 static fr_dict_attr_t const *attr_extended_attribute_1;
@@ -217,7 +216,6 @@ static fr_dict_attr_t const *attr_packet_type;
 
 extern fr_dict_attr_autoload_t rlm_radius_udp_dict_attr[];
 fr_dict_attr_autoload_t rlm_radius_udp_dict_attr[] = {
-       { .out = &attr_acct_delay_time, .name = "Acct-Delay-Time", .type = FR_TYPE_UINT32, .dict = &dict_radius},
        { .out = &attr_error_cause, .name = "Error-Cause", .type = FR_TYPE_UINT32, .dict = &dict_radius },
        { .out = &attr_event_timestamp, .name = "Event-Timestamp", .type = FR_TYPE_DATE, .dict = &dict_radius},
        { .out = &attr_extended_attribute_1, .name = "Extended-Attribute-1", .type = FR_TYPE_TLV, .dict = &dict_radius},
@@ -1307,50 +1305,6 @@ static int encode(rlm_radius_udp_t const *inst, request_t *request, udp_request_
                fr_pair_append(&u->extra, vp);
        }
 
-       /*
-        *      Ensure that we update the Acct-Delay-Time based on the
-        *      time difference between now, and when we originally
-        *      received the request.
-        */
-       if ((u->code == FR_RADIUS_CODE_ACCOUNTING_REQUEST) &&
-           (fr_pair_find_by_da(&request->request_pairs, NULL, attr_acct_delay_time) != NULL)) {
-               uint8_t *attr, *end;
-               uint32_t delay;
-               fr_time_t now;
-
-               /*
-                *      Change Acct-Delay-Time in the packet, but not
-                *      in the debug output.  Oh well.  We don't want
-                *      to edit the incoming VPs, and we want to
-                *      update the encoded version of Acct-Delay-Time.
-                *      So we just walk through the packet to find it.
-                */
-               end = u->packet + packet_len;
-
-               for (attr = u->packet + RADIUS_HEADER_LENGTH;
-                    attr < end;
-                    attr += attr[1]) {
-                       if (attr[0] != attr_acct_delay_time->attr) continue;
-                       if (attr[1] != 6) continue;
-
-                       now = u->retry.updated;
-
-                       /*
-                        *      Add in the time between when
-                        *      we received the packet, and
-                        *      when we're sending the packet.
-                        */
-                       memcpy(&delay, attr + 2, 4);
-                       delay = ntohl(delay);
-                       delay += fr_time_delta_to_sec(fr_time_sub(now, u->recv_time));
-                       delay = htonl(delay);
-                       memcpy(attr + 2, &delay, 4);
-                       break;
-               }
-
-               u->can_retransmit = false;
-       }
-
        /*
         *      Now that we're done mangling the packet, sign it.
         */

diff --git a/src/process/radius/base.c b/src/process/radius/base.c
index b128e1848edc16d9f456988e21c34b872375e046..e2bbf7263cad257e834ef81b9f7e64f69a4b9a3f 100644 (file)
--- a/src/process/radius/base.c
+++ b/src/process/radius/base.c
@@ -69,6 +69,8 @@ static fr_dict_attr_t const *attr_user_name;
 static fr_dict_attr_t const *attr_user_password;
 static fr_dict_attr_t const *attr_original_packet_code;
 static fr_dict_attr_t const *attr_error_cause;
+static fr_dict_attr_t const *attr_event_timestamp;
+static fr_dict_attr_t const *attr_acct_delay_time;
 
 extern fr_dict_attr_autoload_t process_radius_dict_attr[];
 fr_dict_attr_autoload_t process_radius_dict_attr[] = {
@@ -91,6 +93,9 @@ fr_dict_attr_autoload_t process_radius_dict_attr[] = {
        { .out = &attr_original_packet_code, .name = "Extended-Attribute-1.Original-Packet-Code", .type = FR_TYPE_UINT32, .dict = &dict_radius },
        { .out = &attr_error_cause, .name = "Error-Cause", .type = FR_TYPE_UINT32, .dict = &dict_radius },
 
+       { .out = &attr_event_timestamp, .name = "Event-Timestamp", .type = FR_TYPE_DATE, .dict = &dict_radius },
+       { .out = &attr_acct_delay_time, .name = "Acct-Delay-Time", .type = FR_TYPE_UINT32, .dict = &dict_radius },
+
        { NULL }
 };
 
@@ -610,6 +615,48 @@ RESUME(access_challenge)
        RETURN_MODULE_OK;
 }
 
+/** A wrapper around recv generic which stores fields from the request
+ */
+RECV(accounting_request)
+{
+       module_ctx_t                    our_mctx = *mctx;
+       fr_pair_t                       *acct_delay, *event_timestamp;
+
+       fr_assert_msg(!mctx->rctx, "rctx not expected here");
+       our_mctx.rctx = radius_request_pairs_store(request);
+       mctx = &our_mctx;       /* Our mutable mctx */
+
+       /*
+        *      Acct-Delay-Time is horrific.  Its existence in a packet means that any retransmissions can't
+        *      be retransmissions!  Instead, we have to send a brand new packet each time.  This rewriting is
+        *      expensive, causes ID churn, over-allocation of IDs, and makes it more difficult to discover
+        *      end-to-end failures.
+        *
+        *      As a result, we delete Acct-Delay-Time, and replace it with Event-Timestamp.
+        */
+       event_timestamp = fr_pair_find_by_da(&request->request_pairs, NULL, attr_event_timestamp);
+       if (!event_timestamp) {
+               MEM(event_timestamp = fr_pair_afrom_da(request->request_ctx, attr_event_timestamp));
+               fr_pair_append(&request->request_pairs, event_timestamp);
+               event_timestamp->vp_date = fr_time_to_unix_time(request->packet->timestamp);
+
+               RDEBUG("Accounting-Request packet is missing Event-Timestamp.  Adding it to packet as %pP.", event_timestamp);
+       }
+
+       acct_delay = fr_pair_find_by_da(&request->request_pairs, NULL, attr_event_timestamp);
+       if (acct_delay) {
+               if (acct_delay->vp_uint32 < ((365 * 86400))) {
+                       event_timestamp->vp_date = fr_unix_time_sub_time_delta(event_timestamp->vp_date, fr_time_delta_from_sec(acct_delay->vp_uint32));
+
+                       RDEBUG("Accounting-Request packet contains Acct-Delay-Time.  Removing %pP, and updating %pP",
+                              acct_delay, event_timestamp);
+               }
+               (void) fr_pair_delete_by_da(&request->request_pairs, attr_acct_delay_time);
+       }
+
+       return CALL_RECV(generic);
+}
+
 RESUME(acct_type)
 {
        static const fr_process_rcode_t acct_type_rcode = {
@@ -948,7 +995,7 @@ static fr_process_state_t const process_state[] = {
                        [RLM_MODULE_DISALLOW]   = FR_RADIUS_CODE_DO_NOT_RESPOND
                },
                .rcode = RLM_MODULE_NOOP,
-               .recv = recv_generic_radius_request,
+               .recv = recv_accounting_request,
                .resume = resume_accounting_request,
                .section_offset = offsetof(process_radius_sections_t, accounting_request),
        },

diff --git a/src/protocols/radius/attrs.h b/src/protocols/radius/attrs.h
index 0369762f21fcf7a6bffd45489210cf05f52a6ede..1cdf526e62bf22d5cd9d78e69fdb9ac0ab5ac4b3 100644 (file)
--- a/src/protocols/radius/attrs.h
+++ b/src/protocols/radius/attrs.h
@@ -33,7 +33,6 @@ extern HIDDEN fr_dict_t const *dict_radius;
 
 extern HIDDEN fr_dict_attr_t const *attr_packet_type;
 extern HIDDEN fr_dict_attr_t const *attr_packet_authentication_vector;
-extern HIDDEN fr_dict_attr_t const *attr_raw_attribute;
 extern HIDDEN fr_dict_attr_t const *attr_chap_challenge;
 extern HIDDEN fr_dict_attr_t const *attr_chargeable_user_identity;
 extern HIDDEN fr_dict_attr_t const *attr_eap_message;

diff --git a/src/protocols/radius/base.c b/src/protocols/radius/base.c
index 51bac0f3982f45e3721214f7ca468a25f5201623..027987f746e6052a9804dd649a5ff451b1428eab 100644 (file)
--- a/src/protocols/radius/base.c
+++ b/src/protocols/radius/base.c
@@ -51,7 +51,6 @@ fr_dict_autoload_t libfreeradius_radius_dict[] = {
 
 fr_dict_attr_t const *attr_packet_type;
 fr_dict_attr_t const *attr_packet_authentication_vector;
-fr_dict_attr_t const *attr_raw_attribute;
 fr_dict_attr_t const *attr_chap_challenge;
 fr_dict_attr_t const *attr_chargeable_user_identity;
 fr_dict_attr_t const *attr_eap_message;

diff --git a/src/protocols/radius/radius.h b/src/protocols/radius/radius.h
index f3ae728aa297aef5bc0800eb2f257340210be16f..b82cf31dd0016d8f8afcd885e808006ee65bca86 100644 (file)
--- a/src/protocols/radius/radius.h
+++ b/src/protocols/radius/radius.h
@@ -155,7 +155,6 @@ typedef struct {
        int                     salt_offset;            //!< for tunnel passwords
 
 
-       uint32_t                acct_delay_time;        //!< additional time to add to acct_delay_time
        uint8_t                 tag;                    //!< current tag for encoding
 
        uint8_t                 request_code;