Detail listeners halt on invalid signature when proxying

[John Morrissey]

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 at horde.net        _-< \_          /  \       ----  <  \,
www.horde.net/    __(_)/_(_)________/    \_______(_) /_(_)__