Wouldnt it make more sense if these operators matched if no attributes were present that matched or that no attributes were present at all? After all, == and =~ DONT match if there is no attribute present. If one wanted to check that the attribute was present but did not contain value X, they would use Attribute =* "", Attribute != "X" Fixing the other problems mentioned below while keeping the questioned behavior would seem to require keeping track that the auth_item was ever not null in some other variable before the "goto try_again;" The attached patch for src/main/valuepair.c changes the behavior of paircompare() in these ways * if !* matched, no further check attributes were tried * if !* or != or !~ operators and the attribute is not found, consider that a successfull match and try the next check attribute * if =* and we find the attribute in the request there is no need to actually compare the value * It should be considered an error condition to have matching case statements for !* or =* after the call to compare_pair(), since they should have been handled already. * != and !~ are considered successfull as soon as there is an attribute in the request that does not match. If there was an attribute that matched, and then another one that didnt (or vice versa), it would succeed. The patch changes that If check != request try next one If check == request return -1 for not match I have tested it to an extent and would appreciate feedback. Thanks, Joe --- ../../cvs/cvs-20051029/radiusd/src/main/valuepair.c 2005-10-18 19:52:49.000000000 -0400 +++ src/main/valuepair.c 2005-11-05 19:32: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 @@ -487,7 +497,128 @@ return compare_pair( req, first, second, NULL, NULL ); } +/* + * Compare ONE attribute from a (potential) list. + * Return the first matching one. + */ + +static VALUE_PAIR * paircmpmatchone(REQUEST *req, VALUE_PAIR *first, VALUE_PAIR *second) +{ + VALUE_PAIR *fnext = NULL; + VALUE_PAIR *snext = NULL; + int result = 0; + + for(; second; second = second->next){ + for(; first; first = first->next){ + if(first->attribute == second->attribute){ + /* No matter what, compare only THESE items in chain */ + fnext = first->next; + first->next = NULL; + snext = second->next; + second->next = NULL; + result = simplepaircmp(req, first, second); + first->next = fnext; + second->next = snext; + + if(!result) + return first; + } + } + } + return NULL; +} + +#ifdef HAVE_REGEX_H +/* + * Regex Compare ONE attribute from a (potential) list. + * Return the first matching one. + */ + +static VALUE_PAIR * pairregmatchone(REQUEST *req, VALUE_PAIR *first, VALUE_PAIR *second) +{ + int result = 0; + regex_t reg; + + for(; second; second = second->next){ + for(; first; first = first->next){ + if(first->attribute == second->attribute) { + if (second->operator == T_OP_REG_EQ || + second->operator == T_OP_REG_NE || + second->operator == T_OP_SUB_REG) + { + regcomp(®, (char *)second->vp_strvalue, + REG_EXTENDED); + result = regexec(®, (char *)first->vp_strvalue, + 0, NULL, 0); + regfree(®); + + if(!result) + return first; + } + if (first->operator == T_OP_REG_EQ || + first->operator == T_OP_REG_NE || + first->operator == T_OP_SUB_REG) + { + + regcomp(®, (char *)first->vp_strvalue, + REG_EXTENDED); + result = regexec(®, (char *)second->vp_strvalue, + 0, NULL, 0); + regfree(®); + + if(!result) + return second; + } + } + } + } + return NULL; +} +#endif + +/* + * Deletes matching attributes. + */ +static void pairdeletematch(REQUEST *req,VALUE_PAIR **first,VALUE_PAIR *second) +{ + VALUE_PAIR *vp, *i; + + while((vp = paircmpmatchone(req,*first,second))){ + if(*first == vp) /* new head of chain */ + *first = vp->next; + else + for(i = *first; i; i = i->next){ + if(i->next == vp){ /* dis-own */ + i->next = vp->next; + break; + } + } + pairbasicfree(vp); + } +} + +#ifdef HAVE_REGEX_H +/* + * deletes matching regex pairs +*/ +static void pairdeleteregmatch(REQUEST *req,VALUE_PAIR **first,VALUE_PAIR *second) +{ + VALUE_PAIR *vp, *i; + while((vp = pairregmatchone(req,*first,second))){ + if(*first == vp) /* new head of chain */ + *first = vp->next; + else + for(i = *first; i; i = i->next){ + if(i->next == vp){ /* dis-own */ + i->next = vp->next; + break; + } + } + pairbasicfree(vp); + } +} +#endif /* * Move pairs, replacing/over-writing them, and doing xlat. */ @@ -505,6 +636,7 @@ * Point "tailto" to the end of the "to" list. */ tailto = to; + for(i = *to; i; i = i->next) { tailto = &i->next; } @@ -545,30 +677,57 @@ switch (i->operator) { /* - * If a similar attribute is found, - * delete it. + * If a similar attribute are found, + * delete them. */ case T_OP_SUB: /* -= */ if (found) { - if (!i->vp_strvalue[0] || - (strcmp((char *)found->vp_strvalue, - (char *)i->vp_strvalue) == 0)){ - pairdelete(to, found->attribute); - - /* - * 'tailto' may have been - * deleted... - */ + if (!i->vp_strvalue[0]) + goto delete_all; + + pairdeletematch(req,to,i); + /* + * 'tailto' may have been + * deleted... + */ + tailto = to; + for(j = *to; j; j = j->next) { + tailto = &j->next; + } + + } + tailfrom = i; + continue; + break; + case T_OP_SUB_ALL: /* -* */ + if (found) { +delete_all: + pairdelete(to, found->attribute); tailto = to; for(j = *to; j; j = j->next) { tailto = &j->next; } - } } tailfrom = i; continue; break; +#ifdef HAVE_REGEX_H + case T_OP_SUB_REG: /* -~ */ + if (found) { + + pairdeleteregmatch(req,to,i); + + tailto = to; + for(j = *to; j; j = j->next) { + tailto = &j->next; + } + + tailfrom = i; + continue; + break; + } +#endif /* * Add it, if it's not already there. */
Joe Maimon wrote:
The attached patch for src/main/valuepair.c changes the behavior of
org/list/devel.html Sorry about that, forgot to edit the patch, which was mixed with another patch. Attached find clean version. --- ../../cvs/cvs-20051029/radiusd/src/main/valuepair.c 2005-10-18 19:52:49.000000000 -0400 +++ src/main/valuepair.c 2005-11-05 19:32: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
Joe Maimon <jmaimon@ttec.com> wrote:
Wouldnt it make more sense if these operators matched if no attributes were present that matched or that no attributes were present at all?
It's difficult to say.
If one wanted to check that the attribute was present but did not contain value X, they would use
Attribute =* "", Attribute != "X"
The same thing goes for "==". Having the attribute not exist is technically an exception, not a failed match.
I have tested it to an extent and would appreciate feedback.
My main concern is changing the behavior of the server. I'd prefer to move these kinds of checks to rlm_policy, as it's new, and won't break existing systems. Alan DeKok.
Alan DeKok wrote:
The same thing goes for "==". Having the attribute not exist is technically an exception, not a failed match.
When you put it that way, it actually becomes clearer. It seems to be quite unreasonable for == to return success if the attribute did not exist. Indeed, it only returns match if there is an attribute that exists and matches the supplied value. So to != should only return match if there was no attribute that existed and compared successfully with != value. A requirement that the attribute existed but did not match the value is not quite an inversion of == !* is properly an inversion of =*
I have tested it to an extent and would appreciate feedback.
My main concern is changing the behavior of the server.
Does this mean you think the patch would be a good idea otherwise? With some changes it can avoid changing the behavior of != !~ when no pairs are found. But is that a good idea? Perhaps a small poll to find out if anyone present relies on the current behavior of != ? Anyways, I thought you were doing a 2.0.0 :-) ? If the operator behavior is problematic, perhaps users would be best served correcting it as early as possible in the server history. So far I have seen -= , != , !* , !~ that could possibly be changed. I also have added -* , -~ operators that I find usefull, especialy in the policy module.
I'd prefer to move these kinds of checks to rlm_policy, as it's new, and won't break existing systems.
The policy module, while I managed to get it to work for my own purposes, wasnt being called complete or stable. And it stands to reason that paircompare could/should be used by all modules who wish to maintain comparable comparison rules as used in the users file and other modules. Also the policy manages to duplicate a nice portion of the paircompare() functionality within evaluate_condition(). That means two sets of code need to be maintained to present as consistent an experience to the user as possible. Additionaly, != is handled the same way in the policy module as in paircompare(), if it doesnt exist it returns FALSE. I guess what I am getting at is the impression that equaly important as being consistent with past usage should be internal consistency with current usage. Not like my opinion actually matters much.
Alan DeKok. - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
Joe Maimon <jmaimon@ttec.com> wrote:
The same thing goes for "==". Having the attribute not exist is technically an exception, not a failed match.
When you put it that way, it actually becomes clearer.
Yeah, I spent a while doing similar rule-based approaches at a previous company.
So to != should only return match if there was no attribute that existed and compared successfully with != value.
I'm not sure that makes sense.. == means "exists AND matches" By boolean inversion, != means "does not exist OR does not match" That makes sense. It gets more difficult once multiple attributes of the same name are in a list. I'll have to think about that some more.
Does this mean you think the patch would be a good idea otherwise?
Yes.
With some changes it can avoid changing the behavior of != !~ when no pairs are found.
But is that a good idea?
Perhaps a small poll to find out if anyone present relies on the current behavior of != ?
Sure. Since it's not really documented, I'm not sure what the existing systems *expect*.
So far I have seen -= , != , !* , !~ that could possibly be changed.
I also have added -* , -~ operators that I find usefull, especialy in the policy module.
The use there is probably better done by something like: ... reply -= { foo == bar, ... } i.e. "remove from reply attributes that match these conditions". The behavior of '==' and '=~' is always the same, which helps.
The policy module, while I managed to get it to work for my own purposes, wasnt being called complete or stable. And it stands to reason that paircompare could/should be used by all modules who wish to maintain comparable comparison rules as used in the users file and other modules.
I would prefer to fix the policy module so it's complete and stable. With a little bit of effort, it can solve many of the same problems that the existing modules do, in a more general manner.
Also the policy manages to duplicate a nice portion of the paircompare() functionality within evaluate_condition(). That means two sets of code need to be maintained to present as consistent an experience to the user as possible.
Yeah, that's bad. But the main reason is that paircompare() has it's problems. It's meant to support the "users" file, and extending it for any other purpose is a bad idea.
Additionaly, != is handled the same way in the policy module as in paircompare(), if it doesnt exist it returns FALSE.
So long as rlm_policy is documented and consistent, that shouldn't be a problem.
I guess what I am getting at is the impression that equaly important as being consistent with past usage should be internal consistency with current usage.
rlm_policy is new and experimental. I don't mind changing existing deployments if we can figure out what the heck is the right thing to do. Changing the interpretation of the "users" file, and paircompare() at this stage is not a good idea.
Not like my opinion actually matters much.
<shrug> See the number of bufs fixes that have gonr in recently based on your comments. Alan DeKok.
Alan DeKok wrote:
That makes sense. It gets more difficult once multiple attributes of the same name are in a list. I'll have to think about that some more.
Codewise (well at least the patch I tried) its easier to deal with mutliple attributes if no attribute is also a successfull no match
Perhaps a small poll to find out if anyone present relies on the current behavior of != ?
Well I recall being bitten by != unexepcted behavior in the past and switching to !* (which also didnt work, which you took my 2 liner, which was also buggy, which the last patch includes a fix for) Yep, its in my users file to deny certain users who dont have a Connect-Info == "value" attribute. So its unpatched behavior is actually NOT what I wanted/expected until I read man 5 users. Perhaps the direction to be taken here is to add another way to negate the == test that behaves like the patches != ?
Sure. Since it's not really documented, I'm not sure what the existing systems *expect*.
man 5 users states that Attribute != Value As a check item, matches if the given attribute is in the request, AND does not have the given value. Not allowed as a reply item.
The use there is probably better done by something like:
... reply -= { foo == bar, ... }
i.e. "remove from reply attributes that match these conditions". The behavior of '==' and '=~' is always the same, which helps.
That syntax never occurred to me. It would be nice if it worked, so I tested it. /etc/freeradius/policy.txt[8]: Unexpected token -= But policy is not complete, hence this portion of discussion.
I would prefer to fix the policy module so it's complete and stable. With a little bit of effort, it can solve many of the same problems that the existing modules do, in a more general manner.
The policy module is a very powerfull concept, allowing one to use pseduo code to manipulate the attribute lists. I assume you mean it can make redundant modules like attr_filter attr_rewrite and others as well.
Yeah, that's bad. But the main reason is that paircompare() has it's problems. It's meant to support the "users" file, and extending it for any other purpose is a bad idea.
paircompare() supports xlat, supports registered comparison functions, and seems to be the code equivalent to man 5 users which is the freeradius authoritative source as to how operators are to be used. I havent checked but I would hope that rlm_sql and friends also used it as well, since their goal is to reproduce the users file environment in a db friendly environment. If you mean paircompare() should be used whenever one want users file like handling of comparisons, I would tend to agree.
Not like my opinion actually matters much.
<shrug> See the number of bufs fixes that have gonr in recently based on your comments.
Drop in the bucket compared to what the you and the existing contributors have done.
Alan DeKok. - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
participants (2)
-
Alan DeKok -
Joe Maimon