2.2.1: if-block leads to reject
Hi, another issue in 2.2.1 (and the current v2.x.x git): if-blocks in an authenticate section cause to wrong rejects. Example with minimal config: authorize { update control { Auth-Type := 'Test' } } authenticate { if (1) { #noop #update reply { # Reply-Message := "bla" #} } ok } } debug output:
rad_recv: Access-Request packet from host 127.1.0.5 port 33185, id=115, length=55 User-Name = "test-user" User-Password = "test-password" NAS-Port = 12345 # Executing section authorize from file /etc/ui-freeradius/sites/test.conf +group authorize { ++update control { ++} # update control = noop +} # group authorize = notfound Found Auth-Type = Test # Executing group from file /etc/ui-freeradius/sites/test.conf +group Test { ++? if (1) ? Evaluating (1) -> TRUE ++? if (1) -> TRUE ++if (1) { ++} # if (1) = reject +} # group Test = reject Failed to authenticate the user. Login incorrect: [test-user/test-password] (from client Arcor_test port 12345) Sending Access-Reject of id 115 to 127.1.0.5 port 33185 Finished request 0.
So, for some reason, freeradius thinks that the if-block returns with a "reject", which is at least counterintuitive (though I just noticed that 2.2.0 behaved the same, so it's ok probably). If I activate the "noop" (corresponding instance of rlm_always, as the above "ok") in the if-block, it works:
+group Test { ++? if (1) ? Evaluating (1) -> TRUE ++? if (1) -> TRUE ++if (1) { +++[noop] = noop ++} # if (1) = noop ++[ok] = ok +} # group Test = ok
The update section is not suffucient, though:
+group Test { ++? if (1) ? Evaluating (1) -> TRUE ++? if (1) -> TRUE ++if (1) { +++update reply { +++} # update reply = noop ++} # if (1) = reject +} # group Test = reject
This is the actual issue I encountered. We use several if-blocks with update sections and this always worked (including 2.2.0). Running 2.2.1 with our live config, I even get a return value of "noop", which leads to a different problem:
++? if (control:X-Attr-Id && (control:X-Attr-Down || control:X-Attr-Up)) ? Evaluating (control:X-Attr-Id ) -> TRUE ?? Evaluating (control:X-Attr-Down ) -> TRUE ?? Skipping (control:X-Attr-Up) ++? if (control:X-Attr-Id && (control:X-Attr-Down || control:X-Attr-Up)) -> TRUE ++if (control:X-Attr-Id && (control:X-Attr-Down || control:X-Attr-Up)) { +++update reply { expand: %{control:X-Attr-ID} -> 1 expand: %{control:X-Attr-Down} -> 2 expand: %{control:X-Attr-Up} -> 3 +++} # update reply = noop ++} # if (control:X-Attr-Id && (control:X-Attr-Down || control:X-Attr-Up)) = noop ++[ok] = ok +} # group SP-Auth = noop
The if block returns a noop, which the following "ok" cannot override, so the whole group returns with "noop", which make freeradius reject the request. This worked in 2.2.0 (and looked a bit different): ...
++? if (control:X-Attr-Id && (control:X-Attr-Down || control:X-Attr-Up)) -> TRUE ++- entering if (control:X-Attr-Id && (control:X-Attr-Down || control:X-Attr-Up)) {...} expand: %{control:X-Attr-Id} -> 1 expand: %{control:X-Attr-Down} -> 2 expand: %{control:X-Attr-Up} -> 3 +++[reply] returns noop ++- if (control:SP-Drossel-DTAG-ID && (control:SP-Drossel-Down || control:SP-Drossel-Up)) returns noop ++[ok] returns ok Login OK: [xxxx-xxxx] (from client client_test port 12345)
Regards Jakob
H
Hi,
another issue in 2.2.1 (and the current v2.x.x git): if-blocks in an authenticate section cause to wrong rejects.
Example with minimal config:
authorize { update control { Auth-Type := 'Test' } }
authenticate { if (1) { #noop #update reply { # Reply-Message := "bla" #} } ok } }
Have you tried ok if (1) { etc... ? -Arran Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team
Hi, Arran Cudbard-Bell, 2013-09-24 17:22:
Have you tried
ok if (1) { etc...
?
This actually works:
+group Test { ++[ok] = ok ++? if (1) ? Evaluating (1) -> TRUE ++? if (1) -> TRUE ++if (1) { ++} # if (1) = ok ++[ok] = ok +} # group Test = ok
Now I know why there was a noop with our live config: We use a (custom) module that returns noop, which is called first in our auth section. It seems that the if-block "manifests" a previous return code in a way that it cannot be overriden by a following ok/noop, but only if there is no module called in the block itself. A ok in a following if-block will override the return value, though. Examples: ok if (1) { } noop -> ok ok if (1) { ok } noop -> noop if (1) { } ok -> reject after the if-block noop if (1) { } ok -> noop noop if (1) { } if (1) { ok } -> ok This looks quite strange to me... Regards, Jakob
On 28.09.2013 16:44, Alan DeKok wrote:
This looks quite strange to me... I've pushed a fix. Please try the v2.x.x branch on git. If it's OK, we'll release 2.2.1 on Monday.
Thanks, but it doesn't seem to fix it: version: FreeRADIUS Version 2.2.2 (git #f1c2629) config: authenticate { Auth-Type Test { if (1) { update reply { Reply-Message := "some message" } } ok } } debug output: +group Test { ++? if (1) ? Evaluating (1) -> TRUE ++? if (1) -> TRUE ++if (1) { +++update reply { +++} # update reply = noop ++} # if (1) = reject +} # group Test = reject Failed to authenticate the user. Regards, Jakob
Jakob Hirsch wrote:
On 28.09.2013 16:44, Alan DeKok wrote:
This looks quite strange to me... I've pushed a fix. Please try the v2.x.x branch on git. If it's OK, we'll release 2.2.1 on Monday.
Thanks, but it doesn't seem to fix it:
OK. I had a similar test case. Some code searching later... it turns out that the v2.2.1 (and previous) code worked sort of by accident. There was an underlying bug which was highlighted by the new code. i.e. everything should work like X. But the "update" sections didn't. So... there was a bug. The old code had many special cases, and worked almost by magic. It's nice that it's gone. Your test case should now work. Alan DeKok.
On 29.09.2013 15:19, Alan DeKok wrote:
Your test case should now work.
It does indeed. Thanks for the quick fixing. I will test tomorrow with our live config. It's a little odd that an empty if-section still returns a reject, i.e. if (1) { # nothing in here } gives: +group Test { ++? if (1) ? Evaluating (1) -> TRUE ++? if (1) -> TRUE ++if (1) { ++} # if (1) = reject +} # group Test = reject That's just like in previos versions, so it's no change of behaviour. The only use-case for that I can think of is some kind of autogenerating config system where someone uses $INCLUDE in an if-block. If the included file happens to be empty, the result might be a little surprising, and "surprising" is nothing you want in your system :) Regards, Jakob
Jakob Hirsch wrote:
It's a little odd that an empty if-section still returns a reject, i.e.
if (1) { # nothing in here }
Yes... it's a limitation of the underlying assumptions.
That's just like in previos versions, so it's no change of behaviour. The only use-case for that I can think of is some kind of autogenerating config system where someone uses $INCLUDE in an if-block. If the included file happens to be empty, the result might be a little surprising, and "surprising" is nothing you want in your system :)
In v3, blocks which are never evaluated are silently deleted: ... if (0) { blah1 blah2 blah3 } ... The entire "if" section will disappear from the running server, as if it never existed. Alan DeKok.
Alan DeKok, 2013-09-29 15:19:
Your test case should now work.
As I wrote, it does. But now reject is not working if the status is already set to noop, e.g. by an update section: authenticate { Auth-Type Test { if (1) { update reply { Reply-Message := "some message" } reject } ok } } -> +group Test { ++? if (1) ? Evaluating (1) -> TRUE ++? if (1) -> TRUE ++if (1) { +++update reply { +++} # update reply = noop +++[reject] = reject ++} # if (1) = reject ++[ok] = ok +} # group Test = ok Login OK: [test-user] (from client TESTlocalhost port 12345) If I replace the update section with a simple noop, the result is the same: +group Test { ++? if (1) ? Evaluating (1) -> TRUE ++? if (1) -> TRUE ++if (1) { +++[noop] = noop +++[reject] = reject ++} # if (1) = reject ++[ok] = ok +} # group Test = ok Login OK: [test-user] (from client TESTlocalhost port 12345)
Alan DeKok, 2013-09-30 17:04:
Your test case should now work. As I wrote, it does. But now reject is not working if the status is already set to noop, e.g. by an update section: OK. I've pushed a fix.
Ok, works fine now, thanks. I also did some more tests, which all passed.
participants (3)
-
Alan DeKok -
Arran Cudbard-Bell -
Jakob Hirsch