# HG changeset patch # User Sebastien Decugis # Date 1283750933 -32400 # Node ID 825a2992e3b9cabe797cc08a973ccbc454998cff # Parent be646053706b445d567d6f9cbbb5c66f627b3055 Improved duplicate detection in RADIUS/Diameter gw. It will be changed again soon diff -r be646053706b -r 825a2992e3b9 extensions/app_radgw/rgw_clients.c --- a/extensions/app_radgw/rgw_clients.c Fri Sep 03 16:50:04 2010 +0900 +++ b/extensions/app_radgw/rgw_clients.c Mon Sep 06 14:28:53 2010 +0900 @@ -81,13 +81,18 @@ size_t len; } key; - /* information of previous msg received, for duplicate checks. */ + /* information of previous msg received, for duplicate checks -- we keep the last DUPLICATE_MESSAGES_BUFFER messages on each port. */ +#define DUPLICATE_MESSAGES_BUFFER 200 /* This should actually be replaced with a time-based dynamic list! TODO... */ struct { - uint16_t port; - uint8_t id; - uint8_t auth[16]; /* we also compare the request authenticator to avoid buggy NASes */ - struct radius_msg * ans; /* to be able to resend a lost answer */ - } last[2]; /*[0] for auth, [1] for acct. */ + int cnt; /* Counts the number of (different) requests we received */ + struct { + uint16_t port; /* The source UDP port of the request */ + uint8_t id; /* The identifier in the request */ + uint8_t auth[16]; /* The request authenticator, because some NAS are not using identifier properly. */ + struct radius_msg * ans; /* When the answer has been sent already, keep it so we can send it back */ + int nbdup; /* count the number of duplicate RADIUS requests we received on this message */ + } msg_info[DUPLICATE_MESSAGES_BUFFER]; + } duplicates_info[2]; /*[0] for auth, [1] for acct. */ }; @@ -168,6 +173,19 @@ free(client->fqdn); free(client->sa); free(client->key.data); + + /* Free the duplicate info */ + for (idx=0; idx <= 1; idx++){ + int i = 0; + for (i = 0; i < DUPLICATE_MESSAGES_BUFFER; i++) { + if (client->duplicates_info[idx].msg_info[i].ans) { + /* Free this RADIUS message */ + radius_msg_free(client->duplicates_info[idx].msg_info[i].ans); + free(client->duplicates_info[idx].msg_info[i].ans); + } + } + } + free(client); } } @@ -268,47 +286,59 @@ int rgw_clients_check_dup(struct rgw_radius_msg_meta **msg, struct rgw_client *cli) { - int idx; + int p, i, dup = 0; TRACE_ENTRY("%p %p", msg, cli); CHECK_PARAMS( msg && cli ); if ((*msg)->serv_type == RGW_PLG_TYPE_AUTH) - idx = 0; + p = 0; else - idx = 1; + p = 1; - if ((cli->last[idx].id == (*msg)->radius.hdr->identifier) - && (cli->last[idx].port == (*msg)->port) - && !memcmp(&cli->last[idx].auth[0], &(*msg)->radius.hdr->authenticator[0], 16)) { - /* Duplicate! */ - TRACE_DEBUG(INFO, "Received duplicated RADIUS message (id: %02hhx, port: %hu).", (*msg)->radius.hdr->identifier, ntohs((*msg)->port)); - if (cli->last[idx].ans) { - /* Resend the answer */ - CHECK_FCT_DO( rgw_servers_send((*msg)->serv_type, cli->last[idx].ans->buf, cli->last[idx].ans->buf_used, cli->sa, (*msg)->port), ); + /* Check in the previous DUPLICATE_MESSAGES_BUFFER messages if we have received the same identifier / authenticator / port combination */ + for (i = cli->duplicates_info[p].cnt - 1; i >= cli->duplicates_info[p].cnt - DUPLICATE_MESSAGES_BUFFER; i--) { + if ( (cli->duplicates_info[p].msg_info[i % DUPLICATE_MESSAGES_BUFFER].id == (*msg)->radius.hdr->identifier) + && (cli->duplicates_info[p].msg_info[i % DUPLICATE_MESSAGES_BUFFER].port == (*msg)->port) + && !memcmp(&cli->duplicates_info[p].msg_info[i % DUPLICATE_MESSAGES_BUFFER].auth[0], &(*msg)->radius.hdr->authenticator[0], 16)) { + /* We already received this request */ + cli->duplicates_info[p].msg_info[i % DUPLICATE_MESSAGES_BUFFER].nbdup++; + dup = 1; + TRACE_DEBUG(INFO, "Received duplicated RADIUS message (id: %02hhx, port: %hu, dup #%d).", + (*msg)->radius.hdr->identifier, + ntohs((*msg)->port), + cli->duplicates_info[p].msg_info[i % DUPLICATE_MESSAGES_BUFFER].nbdup); + if (cli->duplicates_info[p].msg_info[i % DUPLICATE_MESSAGES_BUFFER].ans) { + /* Resend the answer */ + CHECK_FCT_DO( rgw_servers_send((*msg)->serv_type, + cli->duplicates_info[p].msg_info[i % DUPLICATE_MESSAGES_BUFFER].ans->buf, + cli->duplicates_info[p].msg_info[i % DUPLICATE_MESSAGES_BUFFER].ans->buf_used, + cli->sa, + (*msg)->port), ); + } + rgw_msg_free(msg); + break; } - rgw_msg_free(msg); - } else { - /* We have not just received this message already */ - if (cli->last[idx].port == 0) { /* first message from this client */ - /* Just add the new information */ - ASSERT(cli->last[idx].ans == NULL); - cli->last[idx].id = (*msg)->radius.hdr->identifier; - cli->last[idx].port = (*msg)->port; - memcpy(&cli->last[idx].auth[0], &(*msg)->radius.hdr->authenticator[0], 16); - } else { - /* We have got previous message(s), update the info only if answered already */ - if (cli->last[idx].ans) { - cli->last[idx].id = (*msg)->radius.hdr->identifier; - cli->last[idx].port = (*msg)->port; - memcpy(&cli->last[idx].auth[0], &(*msg)->radius.hdr->authenticator[0], 16); - /* Free the previous answer */ - radius_msg_free(cli->last[idx].ans); - free(cli->last[idx].ans); - cli->last[idx].ans = NULL; - } + } + + /* If we did no already receive this request, save it for later */ + if (!dup) { + /* It's a new request, save its data */ + int i = cli->duplicates_info[p].cnt % DUPLICATE_MESSAGES_BUFFER; + + cli->duplicates_info[p].msg_info[i].port = (*msg)->port; + cli->duplicates_info[p].msg_info[i].id = (*msg)->radius.hdr->identifier; + memcpy(&cli->duplicates_info[p].msg_info[i].auth[0], &(*msg)->radius.hdr->authenticator[0], 16); + if (cli->duplicates_info[p].msg_info[i].ans) { + /* Free the old answer */ + radius_msg_free(cli->duplicates_info[p].msg_info[i].ans); + free(cli->duplicates_info[p].msg_info[i].ans); + cli->duplicates_info[p].msg_info[i].ans = NULL; } + cli->duplicates_info[p].msg_info[i].nbdup = 0; + + cli->duplicates_info[p].cnt += 1; } return 0; @@ -799,7 +829,7 @@ int rgw_client_finish_send(struct radius_msg ** msg, struct rgw_radius_msg_meta * req, struct rgw_client * cli) { - int idx; + int p,i; TRACE_ENTRY("%p %p %p", msg, req, cli); CHECK_PARAMS( msg && *msg && cli ); @@ -827,18 +857,35 @@ /* update the duplicate cache in rgw_clients */ if (req->serv_type == RGW_PLG_TYPE_AUTH) - idx = 0; + p = 0; else - idx = 1; - if (cli->last[idx].ans) { - /* Free it */ - radius_msg_free(cli->last[idx].ans); - free(cli->last[idx].ans); + p = 1; + for (i = cli->duplicates_info[p].cnt - 1; i >= cli->duplicates_info[p].cnt - DUPLICATE_MESSAGES_BUFFER; i--) { + /* Search the entry corresponding to the request */ + if ( (cli->duplicates_info[p].msg_info[i % DUPLICATE_MESSAGES_BUFFER].id == req->radius.hdr->identifier) + && (cli->duplicates_info[p].msg_info[i % DUPLICATE_MESSAGES_BUFFER].port == req->port) + && !memcmp(&cli->duplicates_info[p].msg_info[i % DUPLICATE_MESSAGES_BUFFER].auth[0], &req->radius.hdr->authenticator[0], 16)) { + /* This should not happen, but just in case */ + if (cli->duplicates_info[p].msg_info[i % DUPLICATE_MESSAGES_BUFFER].ans) { + radius_msg_free(cli->duplicates_info[p].msg_info[i % DUPLICATE_MESSAGES_BUFFER].ans); + free(cli->duplicates_info[p].msg_info[i % DUPLICATE_MESSAGES_BUFFER].ans); + } + /* Now save the answer message */ + cli->duplicates_info[p].msg_info[i % DUPLICATE_MESSAGES_BUFFER].ans = *msg; + *msg = NULL; + break; + } } - cli->last[idx].ans = *msg; - cli->last[idx].id = req->radius.hdr->identifier; - cli->last[idx].port = req->port; - *msg = NULL; + + /* If we have not found the request in our circular buffer, it is probably too small */ + if (*msg) { + TODO("Implement a dynamic list for RADIUS duplicates detection based on expiry time instead of number of messages"); + TRACE_DEBUG(INFO, "The circular buffer has circled before the Diameter answer was received, you should definitely increase DUPLICATE_MESSAGES_BUFFER value."); + /* We don't re-save the value */ + radius_msg_free(*msg); + free(*msg); + *msg = NULL; + } /* Finished */ return 0; diff -r be646053706b -r 825a2992e3b9 extensions/app_radgw/rgwx_acct.c --- a/extensions/app_radgw/rgwx_acct.c Fri Sep 03 16:50:04 2010 +0900 +++ b/extensions/app_radgw/rgwx_acct.c Mon Sep 06 14:28:53 2010 +0900 @@ -826,7 +826,20 @@ CHECK_FCT( fd_msg_avp_add ( *diam_fw, MSG_BRW_LAST_CHILD, avp) ); /* While here, we also add the Accouting-Record-Number AVP. - We don't have a dedicated counter nor a state, so we just use the Diameter message End-to-end id here, which fits the conditions on the value. */ + The Accounting-Record-Number AVP (AVP Code 485) is of type Unsigned32 + and identifies this record within one session. As Session-Id AVPs + are globally unique, the combination of Session-Id and Accounting- + Record-Number AVPs is also globally unique, and can be used in + matching accounting records with confirmations. An easy way to + produce unique numbers is to set the value to 0 for records of type + EVENT_RECORD and START_RECORD, and set the value to 1 for the first + INTERIM_RECORD, 2 for the second, and so on until the value for + STOP_RECORD is one more than for the last INTERIM_RECORD. + + -- we actually use the end-to-end id of the message here, which remains constant + if we send a duplicate, so it has the same properties as the suggested algorithm. + Anyway, it assumes that we are not converting twice the same RADIUS message. + . */ CHECK_FCT( fd_msg_avp_new ( cs->dict.Accounting_Record_Number, 0, &avp ) ); value.u32 = e2eid; CHECK_FCT( fd_msg_avp_setvalue ( avp, &value ) );