Re: Possible bug in configurable failover
Alan DeKok wrote:
The "update" section should behave identically to the "if".
It doesn't at the moment. Version 1 policy.conf: testing_module { if (1) { update reply { Reply-Message += "Foo" } } if (1) { update reply { Reply-Message += "Bar" } } ok } Results: $ bin/radtest steve badpass localhost 1 testing123 Sending Access-Request of id 69 to 127.0.0.1 port 1812 User-Name = "steve" User-Password = "badpass" NAS-IP-Address = 127.0.0.1 NAS-Port = 1 rad_recv: Access-Reject packet from host 127.0.0.1 port 1812, id=69, length=25 Reply-Message = "Foo" $ Version 2 policy.conf: testing_module { #if (1) { update reply { Reply-Message += "Foo" } #} #if (1) { update reply { Reply-Message += "Bar" } #} ok } Results: $ bin/radtest steve badpass localhost 1 testing123 Sending Access-Request of id 33 to 127.0.0.1 port 1812 User-Name = "steve" User-Password = "badpass" NAS-IP-Address = 127.0.0.1 NAS-Port = 1 rad_recv: Access-Accept packet from host 127.0.0.1 port 1812, id=33, length=81 Service-Type = Framed-User Framed-Protocol = PPP Framed-IP-Address = 172.16.3.33 Framed-IP-Netmask = 255.255.255.0 Framed-Routing = Broadcast-Listen Filter-Id = "std.ppp" Framed-MTU = 1500 Framed-Compression = Van-Jacobson-TCP-IP Reply-Message = "Foo" Reply-Message = "Bar" $ So: without the 'ifs', the three sections are run sequentially (update, update, ok). With the 'ifs', only the first section is run. Authenticate section has: Auth-Type PAP { pap { ok = return reject = 1 } testing_module } Regards, Brian.
On Tue, Mar 15, 2011 at 08:44:22AM +0000, Brian Candler wrote:
Alan DeKok wrote:
The "update" section should behave identically to the "if".
It doesn't at the moment.
I understand it a little bit more now. Here's a version which combines both behaviours in one. testing_module { update reply { Reply-Message += "Foo" } if (1) { update reply { Reply-Message += "Bar" } } ok } And here's the debug (it doesn't reach the 'ok', but it does add both Reply-Message attributes): Tue Mar 15 10:56:48 2011 : Info: ++- entering policy testing_module {...} Tue Mar 15 10:56:48 2011 : Info: +++[reply] returns reject Tue Mar 15 10:56:48 2011 : Info: +++? if (1) Tue Mar 15 10:56:48 2011 : Info: ? Evaluating (1) -> TRUE Tue Mar 15 10:56:48 2011 : Info: +++? if (1) -> TRUE Tue Mar 15 10:56:48 2011 : Info: +++- entering if (1) {...} Tue Mar 15 10:56:48 2011 : Info: ++++[reply] returns reject Tue Mar 15 10:56:48 2011 : Info: +++- if (1) returns reject Tue Mar 15 10:56:48 2011 : Info: ++- policy testing_module returns reject Tue Mar 15 10:56:48 2011 : Info: Failed to authenticate the user. It seems that two things are going on here: 1. The 'update reply' statement is returning 'reject' 2. At the top level of the policy group, the 'reject' is not causing an immediate return - but it does at the end of an 'if' block. Taking point (1) first, one thing I notice is here: if (child->type == MOD_UPDATE) { ... if (rcode != RLM_MODULE_UPDATED) { myresult = rcode; } else { /* * FIXME: Set priority based on * previous priority, so that we * don't stop on reject when the * default priority was to * continue... * */ } goto handle_result; So the 'updated' status from MOD_UPDATE is being explicitly ignored. In that case, myresult defaults to... well, at the top of modcall it's initialised to: myresult = stack.result[0] = default_component_results[component]; Since we're being called from module_authenticate (from rad_check_password) we're passed component == 0, and the default is RLM_MODULE_REJECT. However I don't see the reason why myresult = RLM_MODULE_UPDATED can't be set after an update statement. It's certainly fine for authentication; in auth.c the only return value from module_authenticate which causes an Access-Accept is RLM_MODULE_OK. So there must be some other case this was intended to deal with. As for point (2), this appears to be fortuitously because I had mapped the 'reject' result to a different code: Auth-Type PAP { pap { ok = return reject = 1 } testing_module } Which in turn begs the question, why does the 'if' statement still act on the reject? I'm wondering if the child->actions table is not inherited inside the 'if' body, because I can make it work like this: testing_module { update reply { Reply-Message += "Foo" } if (1) { reject = 1 <<<<< EXTRA LINE update reply { Reply-Message += "Bar" } } ok } But then, the expected actions seemed to get from the pap { ... } block to testing_module... Here's what I actually see on entry to modcall (without the extra line shown above): (gdb) print ((modgroup*)c)->mc $1 = {parent = 0x0, next = 0x0, name = 0x66f482 "PAP", type = MOD_GROUP, method = 0, actions = {-1, 1, -1, -1, 1, -1, -1, 1, 1}} (gdb) print ((modgroup*)c)->children $2 = (modcallable *) 0x7a3030 (gdb) print *(((modgroup*)c)->children) $3 = {parent = 0x7a2640, next = 0x7a25d0, name = 0x66f638 "pap", type = MOD_SINGLE, method = 0, actions = {1, -1, -1, -1, -1, -1, 1, 2, 4}} (gdb) print *(((modgroup*)c)->children->next) $4 = {parent = 0x7a2640, next = 0x0, name = 0x6543f8 "testing_module", type = MOD_POLICY, method = 0, actions = {-1, -1, 3, -1, -1, -1, 1, 2, 4}} (gdb) print ((modgroup*)(((modgroup*)c)->children->next))->mc $5 = {parent = 0x7a2640, next = 0x0, name = 0x6543f8 "testing_module", type = MOD_POLICY, method = 0, actions = {-1, -1, 3, -1, -1, -1, 1, 2, 4}} (gdb) print ((modgroup*)(((modgroup*)c)->children->next))->children $6 = (modcallable *) 0x7a3740 (gdb) print *(((modgroup*)(((modgroup*)c)->children->next))->children) $7 = {parent = 0x7a25d0, next = 0x7a37b0, name = 0x65457f "reply", type = MOD_UPDATE, method = 0, actions = {0, 0, 0, 0, 0, 0, 0, 0, 0}} (gdb) print *(((modgroup*)(((modgroup*)c)->children->next))->children->next) $8 = {parent = 0x7a25d0, next = 0x7a4460, name = 0x6547cb "(1)", type = MOD_IF, method = 0, actions = {-1, -1, 3, -1, -1, -1, 1, 2, 4}} (gdb) print *(((modgroup*)(((modgroup*)c)->children->next))->children->next->next) $9 = {parent = 0x7a25d0, next = 0x0, name = 0x654b50 "ok", type = MOD_SINGLE, method = 0, actions = {-1, -1, 3, -1, -1, -1, 1, 2, 4}} (gdb) print *((modgroup*)(((modgroup*)(((modgroup*)c)->children->next))->children->next))->children $10 = {parent = 0x7a37b0, next = 0x0, name = 0x65494f "reply", type = MOD_UPDATE, method = 0, actions = {0, 0, 0, 0, 0, 0, 0, 0, 0}} I see the actions table isn't set for MOD_UPDATE. As far as I can tell, the reject action is actions[0]. This is 1 in the pap{} section, but is -1 in testing_module, both at the top level and inside the MOD_IF node. So this rather spoils my theory :-( Regards, Brian.
Brian Candler wrote:
It seems that two things are going on here:
1. The 'update reply' statement is returning 'reject'
No. Read the REST of the debug output. The *previous* module is returning "reject", and the "update reply" statement doesn't change that. Alan DeKok.
Pasted below is the output from 'dump_tree'. By the way, you need to change RDEBUG to DEBUG inside dump_mc() and dump_tree() for it to compile successfully: patch at https://github.com/candlerb/freeradius-server/commit/3a90c3e2721e3e3794df698... Observations: * For 'if' and 'ok', the action codes I the ones I would expect for authorize, not authenticate (e.g. noop=2, ok=3, updated=4). This appears to be an explicit decision: for (i = 0; i < RLM_MODULE_NUMCODES; i++) { if (!c->actions[i]) { if (!parent || (component != RLM_COMPONENT_AUTH)) { c->actions[i] = defaultactions[component][parentgrouptype][i]; } else { /* inside Auth-Type has different rules */ c->actions[i] = defaultactions[RLM_COMPONENT_AUTZ][parentgrouptype][i]; } } } But I'm not sure why it's choosing the defaultactions for AUTZ when inside an AUTH group, rather than just setting the right values in defaultactions[RLM_COMPONENT_AUTH]. I see the same logic is in both do_compile_modsingle and do_compile_modgroup. This doesn't seem to be mentioned in doc/configurable_failover.rst ("For authenticate, the default is to return on success *or* reject, and only try the second and following items if the first one fails") * For 'update' the action codes are all zero, which means no return and no update to the current result. Perhaps that's intentional, but it means that results like RLM_MODULE_INVALID or RLM_MODULE_FAIL from an update are likely to be silently ignored. * I see the parser has reject=return both inside policy { } and if { }, which is consistent. So I think what's happening in my example case is: - reject is set on entry (default for auth module) - the update { } block returns RLM_MODULE_UPDATED, but this is ignored, so the result remains as reject - on exit from the update { } block, the action code 0 means that the reject is ignored - if the update was inside an if block, then on exit from the if block the reject causes an immediate return - if the update was at the top level policy { }, the action code 0 means that the reject is ignored and then we step to the next entry in the group Phew. I think it would be simpler if: 1. MOD_UPDATE had a set of non-zero default actions, including reject=return 2. a successful update { } block caused myresult to be set to RLM_UPDATED but maybe there are reasons why these aren't done today. And possibly, default actions could be copied from parent node rather than setting defaults at every level, such as inside an 'if' block. Regards, Brian. [authenticate] group { pap { reject = 1 fail = return ok = return handled = return invalid = return userlock = return notfound = 1 noop = 2 updated = 4 } policy { update { reject = 0 fail = 0 ok = 0 handled = 0 invalid = 0 userlock = 0 notfound = 0 noop = 0 updated = 0 } if { update { reject = 0 fail = 0 ok = 0 handled = 0 invalid = 0 userlock = 0 notfound = 0 noop = 0 updated = 0 } reject = return fail = return ok = 3 handled = return invalid = return userlock = return notfound = 1 noop = 2 updated = 4 } ok { reject = return fail = return ok = 3 handled = return invalid = return userlock = return notfound = 1 noop = 2 updated = 4 } reject = return fail = return ok = 3 handled = return invalid = return userlock = return notfound = 1 noop = 2 updated = 4 } reject = return fail = 1 ok = return handled = return invalid = 1 userlock = return notfound = return noop = 1 updated = 1 }
Alan DeKok wrote:
1. The 'update reply' statement is returning 'reject'
No. Read the REST of the debug output. The *previous* module is returning "reject", and the "update reply" statement doesn't change that.
I understand that, so let me write it more clearly: 1. the 'reject' state is being set at the beginning 2. the 'update' module returns 'updated' but this is discarded, hence the return code remains at 'reject' 3. the 'reject' result is being *ignored* when the update{} block completes 4. the 'reject' result causes a return when the outer if{} block completes So: an update inside an 'if' causes a reject return; an update outside of an 'if' leaves the result as reject but doesn't return. If that's the designed behaviour, it's pretty obscure. Regards, Brian.
Brian Candler wrote:
I understand that, so let me write it more clearly:
1. the 'reject' state is being set at the beginning
No. It definitely doesn't do that.
2. the 'update' module returns 'updated' but this is discarded, hence the return code remains at 'reject'
"updated" is a lower priority than "reject". See doc/configurable_failover. It's obscure, but useful.
3. the 'reject' result is being *ignored* when the update{} block completes 4. the 'reject' result causes a return when the outer if{} block completes
Well, yes.
So: an update inside an 'if' causes a reject return; an update outside of an 'if' leaves the result as reject but doesn't return.
If that's the designed behaviour, it's pretty obscure.
Well, yes. As always, patches are welcome for suggested behavior. Alan DeKok.
I understand that, so let me write it more clearly:
1. the 'reject' state is being set at the beginning
No. It definitely doesn't do that.
Here's the code: int modcall(int component, modcallable *c, REQUEST *request) { int myresult; ... myresult = stack.result[0] = default_component_results[component]; But to be 100% precise: the way I configured it, I'm calling pap {..} before testing_module, and so pap is also setting reject. But if I comment this out: authenticate { # # PAP authentication, when a back-end database listed # in the 'authorize' section supplies a password. The # password can be clear-text, or encrypted. Auth-Type PAP { #pap { # ok = return # reject = 1 #} testing_module } then what I said was correct (reject is being set up-front when modcall is first entered)
2. the 'update' module returns 'updated' but this is discarded, hence the return code remains at 'reject'
"updated" is a lower priority than "reject". See doc/configurable_failover. It's obscure, but useful.
That's correct but irrelevant. Here is the code again: if (child->type == MOD_UPDATE) { int rcode; modgroup *g = mod_callabletogroup(child); rcode = radius_update_attrlist(request, g->cs, g->vps, child->name); if (rcode != RLM_MODULE_UPDATED) { myresult = rcode; That is: after a MOD_UPDATE, a return value of RLM_MODULE_UPDATED is *explicitly* discarded. It's nothing to do with the relative priorities of updated and reject.
As always, patches are welcome for suggested behavior.
I am happy to do so, I was just trying to understand if there was any reasoning behind the current behaviour. In particular: (1) why is a return of RLM_MODULE_UPDATED from MOD_UPDATE being discarded? (src/main/modcall.c line 468) (2) why does RLM_COMPONENT_AUTH use the RLM_COMPONENT_AUTZ actions table? (modcall.c line 1950 and 2131) (3) why does an update {} section not set any actions? (modcall.c, do_compile_update, doesn't copy actions) (3) may be an oversight, but (1) and (2) are explicitly coded that way, so there may be some reason for them which I can't see. I guess I can patch it to change it, and see what breaks :-) Regards, Brian.
As always, patches are welcome for suggested behavior.
For the delayed-return-when-you-don't-expect-it case: authenticate { ... previous module sets reject without returning (reject=1) if (cond) { update request { ... ok so far } !! return happens here !! } } It seems to me that if the reject suppressed the return previously, then the 'if' block should suppress it too. In fact, an 'if' block could behave like an 'update' and simply perform no action at its end. I've made a patch for if/elsif/else/case/switch: https://github.com/candlerb/freeradius-server/commit/5d4b9923010de8f24dff266... This makes the authenticate { } section behave how I expected it to in the first place. You can still turn on the old behaviour if you want: if (cond) { ... reject = return } I arrived at that patch after examining why the 'update' block has a zero actions array. This turns out to be the right thing: if you *did* have the standard actions for 'update', and a 'reject' was set by a previous module, then a subsequent update{...} statement will trigger an immediately return. That's not very useful. Furthermore, the syntax for 'update' doesn't allow overriding return codes like this: update reply { reject = 1 Reply-Message += "foo" } Hence it's important that update shouldn't take any action based on the return code from the previous module. What about update setting the result to 'updated', instead of propagating the result of the previous module? I tried that, and it was OK in simple cases, but is probably undesirable in 'redundant' groups. After all, you don't want a simple update { } to mark the request as if it had done a successful database lookup. However, I still haven't worked out why the default AUTZ actions are being used for AUTH modules in some circumstances. The following patch would undo it, but it's probably a bad idea (I just don't understand why): --- a/src/main/modcall.c +++ b/src/main/modcall.c @@ -1950,13 +1938,8 @@ static modcallable *do_compile_modsingle(modcallable *parent, csingle = mod_singletocallable(single); csingle->parent = parent; csingle->next = NULL; - if (!parent || (component != RLM_COMPONENT_AUTH)) { - memcpy(csingle->actions, defaultactions[component][grouptype], - sizeof csingle->actions); - } else { /* inside Auth-Type has different rules */ - memcpy(csingle->actions, defaultactions[RLM_COMPONENT_AUTZ][grouptype], - sizeof csingle->actions); - } + memcpy(csingle->actions, defaultactions[component][grouptype], + sizeof csingle->actions); rad_assert(modrefname != NULL); csingle->name = modrefname; csingle->type = MOD_SINGLE; @@ -2132,11 +2115,7 @@ static modcallable *do_compile_modgroup(modcallable *parent, */ for (i = 0; i < RLM_MODULE_NUMCODES; i++) { if (!c->actions[i]) { - if (!parent || (component != RLM_COMPONENT_AUTH)) { - c->actions[i] = defaultactions[component][parentgrouptype][i]; - } else { /* inside Auth-Type has different rules */ - c->actions[i] = defaultactions[RLM_COMPONENT_AUTZ][parentgrouptype][i]; - } + c->actions[i] = defaultactions[component][parentgrouptype][i]; } } Redundant and append groups are identical between AUTH and AUTZ anyway, so the only difference is in the behaviour of a simple group, depending on whether it's at the top level or not (!parent) Regards, Brian.
participants (2)
-
Alan DeKok -
Brian Candler