Possible bug in configurable failover

[Brian Candler]

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.