From e89049c75f31a0e6c42d1538cb63767e7e6e2dd9 Mon Sep 17 00:00:00 2001 From: "Alan T. DeKok" Date: Sun, 11 Aug 2024 22:09:24 -0400 Subject: [PATCH] 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. --- raddb/sites-available/default | 11 ------ src/modules/rlm_radius/rlm_radius_udp.c | 46 ----------------------- src/process/radius/base.c | 49 ++++++++++++++++++++++++- src/protocols/radius/attrs.h | 1 - src/protocols/radius/base.c | 1 - src/protocols/radius/radius.h | 1 - 6 files changed, 48 insertions(+), 61 deletions(-) diff --git a/raddb/sites-available/default b/raddb/sites-available/default index b23a4105e2..ed88025ff6 100644 --- 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 a686f5b29e..77e1029fb3 100644 --- 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 b128e1848e..e2bbf7263c 100644 --- 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 0369762f21..1cdf526e62 100644 --- 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 51bac0f398..027987f746 100644 --- 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 f3ae728aa2..b82cf31dd0 100644 --- 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; -- 2.47.3