Configurable timeout in rlm_exec
Hi list, Currently rlm_exec has a hardcoded timeout of 10s when waiting for termination of the executed command. We have a case where this is is not enough and I'd like to have it configurable. I changed the following: A new timeout argument is added to exec_program: int radius_exec_program(const char *cmd, REQUEST *request, int exec_wait, + int timeout, ...); In all other invocations of radius_exe_program where there's no configuration option the following constant is used: +#define RADIUS_EXEC_DEFAULT_TIMEOUT 10 I could have also renamed the exec_wait to timeout and use a special value (e.g. 0) as a timeout to create the same behavior. Any comments? btw: I'll send the patch as a separate mail. Philipp
--- scripts/exec-program-wait | 1 + src/include/radiusd.h | 3 ++- src/main/evaluate.c | 2 +- src/main/exec.c | 6 ++++-- src/main/modcall.c | 2 +- src/main/tls.c | 4 ++-- src/modules/rlm_exec/rlm_exec.c | 10 ++++++---- src/modules/rlm_mschap/rlm_mschap.c | 1 + 8 files changed, 18 insertions(+), 11 deletions(-) diff --git a/scripts/exec-program-wait b/scripts/exec-program-wait index ea303bc..db3ea73 100755 --- a/scripts/exec-program-wait +++ b/scripts/exec-program-wait @@ -17,6 +17,7 @@ # exec { # program = "/path/to/program/exec-program-wait" # wait = yes +# timeout = 10 # input_pairs = request # output_pairs = reply # } diff --git a/src/include/radiusd.h b/src/include/radiusd.h index 686bef6..b15c110 100644 --- a/src/include/radiusd.h +++ b/src/include/radiusd.h @@ -674,6 +674,7 @@ int rad_authenticate (REQUEST *); int rad_postauth(REQUEST *); /* exec.c */ +#define RADIUS_EXEC_DEFAULT_TIMEOUT 10 pid_t radius_start_program(const char *cmd, REQUEST *request, int exec_wait, int *input_fd, @@ -681,7 +682,7 @@ pid_t radius_start_program(const char *cmd, REQUEST *request, VALUE_PAIR *input_pairs, int shell_escape); int radius_readfrom_program(int fd, pid_t pid, int timeout, char *answer, int left); -int radius_exec_program(const char *, REQUEST *, int, +int radius_exec_program(const char *, REQUEST *, int exec_wait, int timeout, char *user_msg, int msg_len, VALUE_PAIR *input_pairs, VALUE_PAIR **output_pairs, diff --git a/src/main/evaluate.c b/src/main/evaluate.c index 99fa7a0..29a1d35 100644 --- a/src/main/evaluate.c +++ b/src/main/evaluate.c @@ -80,7 +80,7 @@ static const char *expand_string(char *buffer, size_t sizeof_buffer, return value; case T_BACK_QUOTED_STRING: - result = radius_exec_program(value, request, 1, + result = radius_exec_program(value, request, 1, RADIUS_EXEC_DEFAULT_TIMEOUT, buffer, sizeof_buffer, NULL, NULL, 0); if (result != 0) { diff --git a/src/main/exec.c b/src/main/exec.c index 16155d8..a13685e 100644 --- a/src/main/exec.c +++ b/src/main/exec.c @@ -599,6 +599,7 @@ int radius_readfrom_program(int fd, pid_t pid, int timeout, char *answer, * then each individual argv part is xlat'ed. * @param request current request. * @param exec_wait set to 1 if you want to read from or write to child + * @param timeout Seconds to wait before terminating child * @param user_msg buffer to append plaintext (non valuepair) output. * @param msg_len length of user_msg buffer. * @param input_pairs list of value pairs - these will be put into @@ -610,6 +611,7 @@ int radius_readfrom_program(int fd, pid_t pid, int timeout, char *answer, */ int radius_exec_program(const char *cmd, REQUEST *request, int exec_wait, + int timeout, char *user_msg, int msg_len, VALUE_PAIR *input_pairs, VALUE_PAIR **output_pairs, @@ -636,7 +638,7 @@ int radius_exec_program(const char *cmd, REQUEST *request, return 0; #ifndef __MINGW32__ - done = radius_readfrom_program(from_child, pid, 10, answer, sizeof(answer)); + done = radius_readfrom_program(from_child, pid, timeout, answer, sizeof(answer)); if (done < 0) { /* * failure - radius_readfrom_program will @@ -844,5 +846,5 @@ void exec_trigger(REQUEST *request, CONF_SECTION *cs, const char *name, int quen } DEBUG("Trigger %s -> %s", name, value); - radius_exec_program(value, request, 0, NULL, 0, vp, NULL, 1); + radius_exec_program(value, request, 0, 0, NULL, 0, vp, NULL, 1); } diff --git a/src/main/modcall.c b/src/main/modcall.c index 5e74334..8c9a639 100644 --- a/src/main/modcall.c +++ b/src/main/modcall.c @@ -631,7 +631,7 @@ int modcall(int component, modcallable *c, REQUEST *request) } else { RDEBUG("`%s`", mx->xlat_name); radius_exec_program(mx->xlat_name, request, - 0, NULL, 0, + 0, 0, NULL, 0, request->packet->vps, NULL, 1); } diff --git a/src/main/tls.c b/src/main/tls.c index 44fdb21..6825205 100644 --- a/src/main/tls.c +++ b/src/main/tls.c @@ -1587,7 +1587,7 @@ int cbtls_verify(int ok, X509_STORE_CTX *ctx) RDEBUG("Verifying client certificate: %s", conf->verify_client_cert_cmd); if (radius_exec_program(conf->verify_client_cert_cmd, - request, 1, NULL, 0, + request, 1, RADIUS_EXEC_DEFAULT_TIMEOUT, NULL, 0, request->packet->vps, NULL, 1) != 0) { radlog(L_AUTH, "rlm_eap_tls: Certificate CN (%s) fails external verification!", common_name); @@ -2137,7 +2137,7 @@ fr_tls_server_conf_t *tls_server_conf_parse(CONF_SECTION *cs) if ((stat(conf->make_cert_command, &buf) == 0) && (stat(conf->certificate_file, &buf) < 0) && (errno == ENOENT) && - (radius_exec_program(conf->make_cert_command, NULL, 1, + (radius_exec_program(conf->make_cert_command, NULL, 1, RADIUS_EXEC_DEFAULT_TIMEOUT, NULL, 0, NULL, NULL, 0) != 0)) { goto error; } diff --git a/src/modules/rlm_exec/rlm_exec.c b/src/modules/rlm_exec/rlm_exec.c index 5f16319..9ab7e08 100644 --- a/src/modules/rlm_exec/rlm_exec.c +++ b/src/modules/rlm_exec/rlm_exec.c @@ -34,6 +34,7 @@ typedef struct rlm_exec_t { char *xlat_name; int bare; int wait; + int timeout; char *program; char *input; char *output; @@ -53,6 +54,7 @@ typedef struct rlm_exec_t { */ static const CONF_PARSER module_config[] = { { "wait", PW_TYPE_BOOLEAN, offsetof(rlm_exec_t,wait), NULL, "yes" }, + { "timeout", PW_TYPE_INTEGER, offsetof(rlm_exec_t,timeout), NULL, "10" }, { "program", PW_TYPE_STRING_PTR, offsetof(rlm_exec_t,program), NULL, NULL }, { "input_pairs", PW_TYPE_STRING_PTR, @@ -138,7 +140,7 @@ static size_t exec_xlat(void *instance, REQUEST *request, * FIXME: Do xlat of program name? */ RDEBUG2("Executing %s", fmt); - result = radius_exec_program(fmt, request, inst->wait, + result = radius_exec_program(fmt, request, inst->wait, inst->timeout, out, outlen, *input_pairs, NULL, inst->shell_escape); RDEBUG2("result %d", result); if (result != 0) { @@ -327,7 +329,7 @@ static int exec_dispatch(void *instance, REQUEST *request) * into something else. */ result = radius_exec_program(inst->program, request, - inst->wait, NULL, 0, + inst->wait, inst->timeout, NULL, 0, *input_pairs, &answer, inst->shell_escape); if (result < 0) { radlog(L_ERR, "rlm_exec (%s): External script failed", @@ -380,7 +382,7 @@ static int exec_postauth(void *instance, REQUEST *request) } tmp = NULL; - result = radius_exec_program(vp->vp_strvalue, request, exec_wait, + result = radius_exec_program(vp->vp_strvalue, request, exec_wait, inst->timeout, NULL, 0, request->packet->vps, &tmp, inst->shell_escape); @@ -444,7 +446,7 @@ static int exec_accounting(void *instance, REQUEST *request) } if (!vp) return RLM_MODULE_NOOP; - result = radius_exec_program(vp->vp_strvalue, request, exec_wait, + result = radius_exec_program(vp->vp_strvalue, request, exec_wait, inst->timeout, NULL, 0, request->packet->vps, NULL, inst->shell_escape); if (result != 0) { diff --git a/src/modules/rlm_mschap/rlm_mschap.c b/src/modules/rlm_mschap/rlm_mschap.c index d56636b..da7065c 100644 --- a/src/modules/rlm_mschap/rlm_mschap.c +++ b/src/modules/rlm_mschap/rlm_mschap.c @@ -1089,6 +1089,7 @@ static int do_mschap(rlm_mschap_t *inst, */ result = radius_exec_program(inst->ntlm_auth, request, TRUE, /* wait */ + RADIUS_EXEC_DEFAULT_TIMEOUT, buffer, sizeof(buffer), NULL, NULL, 1); if (result != 0) { -- 1.7.2.5
hi, 2 small things 1) you seem to be adding this exec patch to other places where external execution of code takes place - eg verification of the EAP cert - was that intentional? 2) it appears that you are choosing to set the value to '0' when running some commands in debug mode - surely not the intention (or my misreading of those few lines).... I agree that the timeout should be configurable.... perhaps, in an ideal world you could have a minimum and maximum. server starts with minimum value...if the reply doesnt happen quick enough and theres a failure, it could log the error and then incremement the value by one second...up until it reaches the ceiling value. errors messages would then alert admin to the issue and hopefully external systems fixed before the 'hard' error boundary reached (which is all we have right now..a hard boundary) alan
Hi Alan, 1) you seem to be adding this exec patch to other places where external > execution of code takes place - eg verification of the EAP cert - was that > intentional? > > Yes, this is intended. I had to change radius_exec_program function which is used in various places. The verification of the EAP cert is executed with the default timeout (10s) which is exactly the same behavior as before. - (radius_exec_program(conf->make_cert_command, NULL, 1, + (radius_exec_program(conf->make_cert_command, NULL, 1, RADIUS_EXEC_DEFAULT_TIMEOUT, > 2) it appears that you are choosing to set the value to '0' when running > some commands in debug mode - surely not the intention (or my misreading > of those few lines).... > The timeout value is ignored if exec_wait is set to zero. > > I agree that the timeout should be configurable.... perhaps, in an ideal > world > you could have a minimum and maximum. server starts with minimum value...if > the reply doesnt happen quick enough and theres a failure, it could log > the error > and then incremement the value by one second...up until it reaches the > ceiling value. > errors messages would then alert admin to the issue and hopefully external > systems > fixed before the 'hard' error boundary reached (which is all we have right > now..a hard > boundary) > > I think it would be easier to have a timeout_warn and timeout_hard value instead of dynamically changing it. But for now, I'd prefer to just have one timeout value. Philipp
On 09/24/2012 09:24 AM, Philipp Hug wrote:
Hi list,
Currently rlm_exec has a hardcoded timeout of 10s when waiting for termination of the executed command. We have a case where this is is not enough and I'd like to have it configurable.
That's not completely unreasonable, but be aware that the server thread is entirely blocked whilst waiting for the exec to complete. If all threads in the pool are blocked, you'll have serious problems. Basically: 10 seconds is a long to wait, so waiting even longer... I'm kind of curious what you're doing? Anyway, enough people already get confused about this w.r.t. SQL/LDAP databases that are slow - the complain "the server is slow", when in fact the server is *blocked*. So you might want to surround this config option with BIG BOLD TEXT explaining this ;o) Personally I think the option might be more generally useful for *decreasing* the timeout!
Hi Phil, That's not completely unreasonable, but be aware that the server thread is
entirely blocked whilst waiting for the exec to complete. If all threads in the pool are blocked, you'll have serious problems.
Basically: 10 seconds is a long to wait, so waiting even longer... I'm kind of curious what you're doing?
Well, the script is waiting for user interaction to authorize the radius transaction. (which will take up to 2 minutes) And in our proof of concept we're using a shell script with rlm_exec. If there's another way to achieve this. e.g. by letting freeradius invoke the shell script like every 10s and check if the request is still pending or already accepted or denied, that would also be an acceptable solution. Philipp
Anyway, enough people already get confused about this w.r.t. SQL/LDAP databases that are slow - the complain "the server is slow", when in fact the server is *blocked*. So you might want to surround this config option with BIG BOLD TEXT explaining this ;o)
Personally I think the option might be more generally useful for *decreasing* the timeout! - List info/subscribe/unsubscribe? See http://www.freeradius.org/** list/devel.html <http://www.freeradius.org/list/devel.html>
On 25/09/12 12:01, Philipp Hug wrote:
Hi Phil,
That's not completely unreasonable, but be aware that the server thread is entirely blocked whilst waiting for the exec to complete. If all threads in the pool are blocked, you'll have serious problems.
Basically: 10 seconds is a long to wait, so waiting even longer... I'm kind of curious what you're doing?
Well, the script is waiting for user interaction to authorize the radius transaction. (which will take up to 2 minutes)
Woah. That's an *enormous* timeout. With a thread pool of 32 threads, one authentication every 4 seconds will eventually eat all your threads.
And in our proof of concept we're using a shell script with rlm_exec.
If there's another way to achieve this. e.g. by letting freeradius invoke the shell script like every 10s and check if the request is still pending or already accepted or denied, that would also be an acceptable solution.
I think you're going about this entirely the wrong way, personally. I can think of a couple of alternatives. 1. Just authenticate the user straight away - don't wait - but put them into a network with no access. Once the manual authorization is complete, send a CoA request to move the existing session into the "working" network. This should work on any NAS with CoA support, and is the "proper" RADIUS way to do it. 2. More complex and error-prone - insert the authorization request into a SQL table and send an Access-Challenge with some attributes including State, and a "retry" delay. Have your NAS / the client "continue" at intervals of $retry, and keep sending Access-Challenge until the SQL row reads "accepted" or "rejected". This will only work if you have control of the NAS, and you'll have to implement the challenge sending/logic yourselves. Not a very clean solution. What network protocol / NAS are you using here? I'd use CoA to solve this, if at all possible. 2 minute blocking timeouts on external "exec" are just crazy! Cheers, Phil
Hi again,
Woah. That's an *enormous* timeout.
With a thread pool of 32 threads, one authentication every 4 seconds will eventually eat all your threads.
Well, we'd change the thread pool size according to our requirements. But, it isn't a nice solution, I know that.
And in our proof of concept we're using a shell script with rlm_exec.
If there's another way to achieve this. e.g. by letting freeradius invoke the shell script like every 10s and check if the request is still pending or already accepted or denied, that would also be an acceptable solution.
I think you're going about this entirely the wrong way, personally.
I can think of a couple of alternatives.
1. Just authenticate the user straight away - don't wait - but put them into a network with no access. Once the manual authorization is complete, send a CoA request to move the existing session into the "working" network. This should work on any NAS with CoA support, and is the "proper" RADIUS way to do it.
This could work in some cases, where the NAS supports Change-of-auth but it's not really intuitive to the user as he'll not be notified when he's moved to the "working" network.
2. More complex and error-prone - insert the authorization request into a SQL table and send an Access-Challenge with some attributes including State, and a "retry" delay. Have your NAS / the client "continue" at intervals of $retry, and keep sending Access-Challenge until the SQL row reads "accepted" or "rejected". This will only work if you have control of the NAS, and you'll have to implement the challenge sending/logic yourselves. Not a very clean solution.
We don't have control over which NAS are used. It should work with different NAS implementations. How about 3. (Similar to 2 but without Access-Challenge) Instead of blocking we return immediatly, insert the request into an SQL table but don't respond to the NAS. The NAS is configured to retry every n seconds. When the NAS retries we check if there's already a request and if yes, we check if it already succeeded and we send a response back and if not we'll ignore it again.
What network protocol / NAS are you using here? I'd use CoA to solve this, if at all possible. 2 minute blocking timeouts on external "exec" are just crazy!
We use for example a VPN gateway which uses Radius, but it should also work with different NAS.
Philipp
On 25/09/12 13:08, Philipp Hug wrote:
We use for example a VPN gateway which uses Radius, but it should also work with different NAS.
This has gotten a bit off-topic, which is partly my fault. I can't see what you want to do working, but it's your choice. Configurable exec timeout is not a bad idea generally. I think you're crazy to set it to two minutes, but that's your choice! Anyway, good luck.
Philipp Hug wrote:
We don't have control over which NAS are used. It should work with different NAS implementations.
Then a 2 minute timeout WILL NOT WORK. Pretty much every single NAS in existence will give up after about 30 seconds. So sending an Access-Accept after that time is uselss.
We use for example a VPN gateway which uses Radius, but it should also work with different NAS.
The only real solution is to let the user in, but block them at a captive portal. Once they authenticate, change the permissions in the captive portal to let them access the network. Alan DeKok.
Hi again, Back to the original patch. Even if it doesn't make sense for most users to increase the timeout it still makes sense to have the timeout configurable. Maybe someone wants only 1s timeout instead of the hardcoded 10s. I can add a warning about the blocked worker threads if you want. What do you think? Philipp
Philipp Hug wrote:
Even if it doesn't make sense for most users to increase the timeout it still makes sense to have the timeout configurable. Maybe someone wants only 1s timeout instead of the hardcoded 10s.
Sure... I'm wary of adding more configuration options to the server, tho. It's already getting a bit silly. My $0.02 is that the 10s timeout is a helpful feature for the admin. It means that bad things won't cause the system to go down. But as Phil said, the timer should probably be less. And documented in BIG LETTERS. If your script takes more than 100ms to run, it's probably broken. Fix it. Adding configurable timers is just putting bandaids on a nuclear bomb. They're cute. They don't do much. Alan DeKok.
Philipp Hug wrote:
Currently rlm_exec has a hardcoded timeout of 10s when waiting for termination of the executed command. We have a case where this is is not enough and I'd like to have it configurable.
Hmm... I agree with Phil here. A 10s timeout is very, very, bad. Increasing is is worse. I would suggest instead to talk about the problem. What problem are you trying to solve? There may be an alternate solution. Alan DeKok.
participants (4)
-
alan buxey -
Alan DeKok -
Phil Mayers -
Philipp Hug