Detail listeners halt on invalid signature when proxying
We write all accounting to detail files, which is then picked up by detail listener(s) and proxied to remote hosts. Recently, one of the home servers in a load balanced pool was configured with the wrong secret, triggering this code in received_proxy_response(): if (rad_verify(packet, request->proxy, request->home_server->secret) != 0) { DEBUG("Ignoring spoofed proxy reply. Signature is invalid"); return NULL; } Since the proxy response is ignored and detail_send() is never called, the detail listener stays in STATE_RUNNING. detail_recv() short-circuits during STATE_RUNNING, so the failed request is never retransmitted and all detail processing for that listener halts until FreeRADIUS is restarted. When received_proxy_response() is modified to fall through (instead of returning NULL) for detail listeners, rad_accounting() overrides request->reply->code, setting it to PW_ACCOUNTING_RESPONSE. When detail_send() is called, there's no way for it to tell that the request hasn't been processed successfully. It skips the current detail entry and continues processing. Adding more state information to the request to inform rad_accounting()'s (or detail_send()'s) decision making seems kludgy. Maybe allow received_proxy_response() to override the handler function (pfun in proxy_socket_recv()) to avoid rad_accounting() in this case? Alan, any feedback on your desired way to do this? john -- John Morrissey _o /\ ---- __o jwm@horde.net _-< \_ / \ ---- < \, www.horde.net/ __(_)/_(_)________/ \_______(_) /_(_)__
John Morrissey wrote:
Recently, one of the home servers in a load balanced pool was configured with the wrong secret, triggering this code in received_proxy_response(): ... Since the proxy response is ignored and detail_send() is never called, the detail listener stays in STATE_RUNNING. detail_recv() short-circuits during STATE_RUNNING, so the failed request is never retransmitted and all detail processing for that listener halts until FreeRADIUS is restarted.
OK. That should be fixed. The solution is to fix detail_recv() so that it retries the packet after being in the RUNNING state for "retry_interval". I'll commit a patch. Alan DeKok.
On Tue, Oct 20, 2009 at 12:13:26PM +0200, Alan DeKok wrote:
John Morrissey wrote:
Recently, one of the home servers in a load balanced pool was configured with the wrong secret, triggering this code in received_proxy_response(): ... Since the proxy response is ignored and detail_send() is never called, the detail listener stays in STATE_RUNNING. detail_recv() short-circuits during STATE_RUNNING, so the failed request is never retransmitted and all detail processing for that listener halts until FreeRADIUS is restarted.
OK. That should be fixed.
The solution is to fix detail_recv() so that it retries the packet after being in the RUNNING state for "retry_interval".
I'll commit a patch.
Thanks, Alan. FWIW, I reported this against 2.1.x but backported the fix to 2.0.x (patch below, for the list archives' sake), since we're trying to stay close to the Debian packaging. So far, we've managed to move a fairly large installation from Radiator (I'm guesstimating 600 incoming auth+acct requests/sec) with only this patch, so we decided it wasn't worth it to diverge from stock at this point. john --- freeradius/src/main/event.c.orig 2009-10-20 18:26:20.020171000 +0000 +++ freeradius/src/main/event.c 2009-10-20 18:27:41.744485000 +0000 @@ -2392,7 +2392,7 @@ when.tv_sec += 1; for (this = mainconfig.listen; this != NULL; this = this->next) { - if (this->fd >= 0) continue; + if (this->fd >= 0 && this->type != RAD_LISTEN_DETAIL) continue; /* * Try to read something. --- freeradius/src/main/detail.c.orig 2008-05-09 12:32:24.000000000 +0000 +++ freeradius/src/main/detail.c 2009-10-20 16:34:29.381519000 +0000 @@ -49,9 +49,11 @@ FILE *fp; int state; time_t timestamp; + time_t running; fr_ipaddr_t client_ip; int load_factor; /* 1..100 */ int signal; + int retry_interval; int has_rtt; int srtt; @@ -429,11 +431,18 @@ goto alloc_packet; /* - * We still have an outstanding packet. - * Don't read any more. + * Periodically check what's going on. + * If the request is taking too long, + * retry it. */ case STATE_RUNNING: - return 0; + if (time(NULL) < (data->running + data->retry_interval)) { + return 0; + } + + DEBUG("No response to detail request. Retrying"); + data->state = STATE_NO_REPLY; + /* FALL-THROUGH */ /* * If there's no reply, keep @@ -691,6 +700,7 @@ } data->state = STATE_RUNNING; + data->running = packet->timestamp; return 1; } @@ -744,6 +754,8 @@ offsetof(listen_detail_t, filename), NULL, NULL }, { "load_factor", PW_TYPE_INTEGER, offsetof(listen_detail_t, load_factor), NULL, Stringify(10)}, + { "retry_interval", PW_TYPE_INTEGER, + offsetof(listen_detail_t, retry_interval), NULL, Stringify(30)}, { NULL, -1, 0, NULL, NULL } /* end the list */ }; -- John Morrissey _o /\ ---- __o jwm@horde.net _-< \_ / \ ---- < \, www.horde.net/ __(_)/_(_)________/ \_______(_) /_(_)__
participants (2)
-
Alan DeKok -
John Morrissey