Bogus default attrs file?
Hi all! Default attrs file used by rlm_attr_filter contains following DEFAULT section: DEFAULT Service-Type == Framed-User, Service-Type == Login-User, Login-Service == Telnet, Login-Service == Rlogin, Login-Service == TCP-Clear, [ ... ] Framed-Protocol == PPP, Framed-Protocol == SLIP, [ ... ] This syntax, accompanied with tisp realm example, suggests that the attribute e.g. Framed-Protocol will be preserved in proxy reply if it's value is either PPP or SLIP. However, it does not seem to be true. Framed-Protocol will not pass this filter at all. Documentation quite clearly states: "an attribute must pass *all* the rules which affect it in order to make it past the filter". Hence using == rule twice with different values for same attribute should be the same as using !*. Or am I missing anything? Proposal for fix: check_pair function after comparison always increments either fail or pass counter for given attribute. It may be modified to not increment fail for '==' operator (and also '=~') when compare fails. This should be consistent with final " fail == 0 && pass > 0 " check and default attrs file should work as expected. Untested patch: For released code: --- rlm_attr_filter.c 2005-08-11 23:06:38.000000000 +0200 +++ rlm_attr_filter.c.new 2007-01-10 13:32:55.000000000 +0100 @@ -68,8 +68,6 @@ case T_OP_CMP_EQ: if (comp == 0) { ++*(pa); - } else { - ++*(fa); } break; @@ -120,8 +118,6 @@ regfree(®); if (comp == 0) { ++*(pa); - } else { - ++*(fa); } break; For CVS code (some #ifdef HAVE_REGEX_H should probably appear here): --- rlm_attr_filter.c 2006-11-22 22:44:19.000000000 +0100 +++ rlm_attr_filter.c.new 2007-01-10 13:37:05.000000000 +0100 @@ -68,7 +68,8 @@ compare = paircmp(check_item, reply_item); if (compare == 1) { ++*(pass); - } else { + } else if (check_item->operator != T_OP_CMP_EQ && + check_item->operator != T_OP_REG_EQ) { ++*(fail); } Ideas? th.
Tomas Hoger wrote:
Default attrs file used by rlm_attr_filter contains following DEFAULT section:
That's certainly a possibility...
Proposal for fix:
Could you check the code in the CVS head? It was updated significantly, to clarify some of these issues. I think it may work a little better. Alan DeKok. -- http://deployingradius.com - The web site of the book http://deployingradius.com/blog/ - The blog
Hi Alan! Thanks for reply. On Wed, Jan 10, 2007 at 09:32:37AM -0500, Alan DeKok wrote:
Could you check the code in the CVS head? It was updated significantly, to clarify some of these issues. I think it may work a little better.
I have not tried latest CVS code yet, but I have read it. It seems to have same issue. That's why I also included patch for CVS version in my previous email, but I guess it has been overlooked ;). Here is slightly improved version: --- rlm_attr_filter.c 2006-11-22 22:44:19.000000000 +0100 +++ rlm_attr_filter.c.new 2007-01-10 23:52:21.000000000 +0100 @@ -68,7 +68,11 @@ compare = paircmp(check_item, reply_item); if (compare == 1) { ++*(pass); - } else { + } else if (check_item->operator != T_OP_CMP_EQ +#ifdef HAVE_REGEX_H + && check_item->operator != T_OP_REG_EQ +#endif + ) { ++*(fail); } --- 8< --- Code still runs every check_item - reply_item combination through check_pair function, which uses paircmp to do the hard work, but always increments pass or fail counter. After checking filter rules: attribute == value1, attribute == value2 Value of fail will always be >0 and attribute will be dropped. However, I think it would be nice to have way to enumerate list of permitted values (without having to write regex). That's why I suggested possible change of semantics of == and =~ operators in attr filter rules. Single failed comparison will not drop attribute and any matching value will suffice. Are there any other changes in CVS version of rlm_attr_filter I have overlooked which can make it behave as described in attrs file? th.
participants (2)
-
Alan DeKok -
Tomas Hoger