users file and "!*" comparisons
I've encountered an unintuitive behaviour in the users file processing . I wanted to do something similar to: DEFAULT Some-Attr !* "", Other-Attr !* "" ... I.e. if neither "Some-Attr" nor "Other-Attr" is present, it should match. However, this doesn't work, as paircmp in valuepair.c immediately returns the result as soon as an operator "!*" is found in the check list. No other check items in the user entry is checked. This seems to apply to both 1.1 and 2.0 branches. Before I try to patch this, is this intended for some reason?
This issue comes up now and again. This is the patch I have been using for the past year or so. Daniel Larsson wrote:
This little patch solves my problem.
------------------------------------------------------------------------
Index: src/main/valuepair.c =================================================================== RCS file: /source/radiusd/src/main/valuepair.c,v retrieving revision 1.60.2.4.2.1 diff -u -r1.60.2.4.2.1 valuepair.c --- src/main/valuepair.c 9 Feb 2007 15:56:12 -0000 1.60.2.4.2.1 +++ src/main/valuepair.c 7 Aug 2007 16:52:12 -0000 @@ -284,7 +284,7 @@ * to not find it, then we succeeded. */ if (check_item->operator == T_OP_CMP_FALSE) - return 0; + continue; else return -1; }
------------------------------------------------------------------------
- List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
#! /bin/sh /usr/share/dpatch/dpatch-run ## 200-cmp-operators-fix.dpatch by <joe@nameserver3.ttec.com> ## ## All lines beginning with `## DP:' are a description of the patch. ## DP: No description. @DPATCH@ diff -urNad freeradius-1.1.0~/src/main/valuepair.c freeradius-1.1.0/src/main/valuepair.c --- freeradius-1.1.0~/src/main/valuepair.c 2005-11-06 00:40:42.000000000 -0500 +++ freeradius-1.1.0/src/main/valuepair.c 2005-11-06 00:40:46.000000000 -0500 @@ -298,8 +298,13 @@ * Didn't find it. If we were *trying* * to not find it, then we succeeded. */ - if (check_item->operator == T_OP_CMP_FALSE) - return 0; + if (check_item->operator == T_OP_CMP_FALSE || +#ifdef HAVE_REGEX_H + check_item->operator == T_OP_REG_NE || +#endif + check_item->operator == T_OP_NE + ) + continue; else return -1; } @@ -310,7 +315,10 @@ */ if (check_item->operator == T_OP_CMP_FALSE) return -1; - + + /* no comparison needed if it exists */ + if (check_item->operator == T_OP_CMP_TRUE) + continue; /* * We've got to xlat the string before doing @@ -342,14 +350,14 @@ radlog(L_INFO, "Invalid operator for item %s: " "reverting to '=='", check_item->name); /*FALLTHRU*/ - case T_OP_CMP_TRUE: /* compare always == 0 */ - case T_OP_CMP_FALSE: /* compare always == 1 */ case T_OP_CMP_EQ: if (compare != 0) result = -1; break; case T_OP_NE: - if (compare == 0) result = -1; + /* != only succeeds if NO reply values match*/ + if (compare == 0) return -1; + result = -1; /*check the rest for no match */ break; case T_OP_LT: @@ -457,7 +465,9 @@ compare = regexec(®, (char *)auth_item->vp_strvalue, 0, NULL, 0); regfree(®); - if (compare == 0) result = -1; + /* !~ only succeeds if NO matches are found in reply pair */ + if (compare == 0) return -1; + result = -1; /*check the rest for no match */ break; #endif
Daniel Larsson wrote:
I've encountered an unintuitive behaviour in the users file processing . I wanted to do something similar to:
DEFAULT Some-Attr !* "", Other-Attr !* "" ...
I.e. if neither "Some-Attr" nor "Other-Attr" is present, it should match.
However, this doesn't work, as paircmp in valuepair.c immediately returns the result as soon as an operator "!*" is found in the check list. No other check items in the user entry is checked. This seems to apply to both 1.1 and 2.0 branches.
Before I try to patch this, is this intended for some reason?
In the users file, the top line conditions are evaluated in sequence until one evaluates to false. It doesn't matter which operators you use. I actually find it quite helpful, and *more* intuitive that way, also saves on processing for a line which could never match ... Really if you want to be doing anything complicated with conditions I would try the CVS head and look at 'man unlang'. -- Arran Cudbard-Bell (A.Cudbard-Bell@sussex.ac.uk) Authentication, Authorisation and Accounting Officer Infrastructure Services | ENG1 E1-1-08 University Of Sussex, Brighton EXT:01273 873900 | INT: 3900
On Tue, 2007-08-07 at 18:08 +0100, Arran Cudbard-Bell wrote:
Daniel Larsson wrote:
I've encountered an unintuitive behaviour in the users file processing . I wanted to do something similar to:
DEFAULT Some-Attr !* "", Other-Attr !* "" ...
I.e. if neither "Some-Attr" nor "Other-Attr" is present, it should match.
However, this doesn't work, as paircmp in valuepair.c immediately returns the result as soon as an operator "!*" is found in the check list. No other check items in the user entry is checked. This seems to apply to both 1.1 and 2.0 branches.
Before I try to patch this, is this intended for some reason?
In the users file, the top line conditions are evaluated in sequence until one evaluates to false. It doesn't matter which operators you use.
Yes, this was my understanding too, but tests with the !* operator indicated it didn't work this way. Looking at the code seemed to confirm this too (see the patch).
I actually find it quite helpful, and *more* intuitive that way, also saves on processing for a line which could never match ...
Really if you want to be doing anything complicated with conditions I would try the CVS head and look at 'man unlang'.
Daniel Larsson wrote:
I've encountered an unintuitive behaviour in the users file processing . I wanted to do something similar to:
DEFAULT Some-Attr !* "", Other-Attr !* "" ...
I.e. if neither "Some-Attr" nor "Other-Attr" is present, it should match.
No. That is not the way it's documented. See "man users". Each item in the check list is ANDed. What you wrote is: Some-Attr doesn't exist AND some other attribute doesn't exist. If Some-Attr *does* exist, the first check fails, and the next one isn't even looked at.
Before I try to patch this, is this intended for some reason?
It's both intended and documented. If you want more complex comparisons, see the CVS head. There, you can do: if (!Some-Attr || !Other-Attr) ... Which is what you seem to want. Alan DeKok.
On Tue, 2007-08-07 at 13:33 -0400, Alan DeKok wrote:
Daniel Larsson wrote:
I've encountered an unintuitive behaviour in the users file processing . I wanted to do something similar to:
DEFAULT Some-Attr !* "", Other-Attr !* "" ...
I.e. if neither "Some-Attr" nor "Other-Attr" is present, it should match.
No. That is not the way it's documented. See "man users". Each item in the check list is ANDed. What you wrote is:
Some-Attr doesn't exist AND some other attribute doesn't exist.
If Some-Attr *does* exist, the first check fails, and the next one isn't even looked at.
The second one isn't looked at in any case. Even if Some-Attr does *not* exist, the check returns false immediately, regardless of the presence of Other-Attr.
Before I try to patch this, is this intended for some reason?
It's both intended and documented.
If you want more complex comparisons, see the CVS head. There, you can do:
if (!Some-Attr || !Other-Attr) ...
Which is what you seem to want.
No, perhaps my wording was confused. I wanted a match if neither was present.
Daniel Larsson wrote:
The second one isn't looked at in any case. Even if Some-Attr does *not* exist, the check returns false immediately, regardless of the presence of Other-Attr.
OK, that would seem to be a bug. In any case, the "unlang" feature in 2.0 has an entirely new implementation of the operator matching (well tested, too.). It's a lot easier to understand, and it works. Alan DeKok.
On Thu, 2007-08-09 at 09:25 -0400, Alan DeKok wrote:
Daniel Larsson wrote:
The second one isn't looked at in any case. Even if Some-Attr does *not* exist, the check returns false immediately, regardless of the presence of Other-Attr.
OK, that would seem to be a bug.
In any case, the "unlang" feature in 2.0 has an entirely new implementation of the operator matching (well tested, too.). It's a lot easier to understand, and it works.
Yes, looking forward to it :)
Alan DeKok. - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
participants (4)
-
Alan DeKok -
Arran Cudbard-Bell -
Daniel Larsson -
Joe Maimon