From: Alan T. DeKok Date: Wed, 12 Jul 2023 12:22:51 +0000 (-0400) Subject: parse conditional filters in attributes X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=dbb79089e1860be4f1a61401a15449ffa5845a1e;p=thirdparty%2Ffreeradius-server.git parse conditional filters in attributes &Foo[&bar == baz] None of this works for a bunch of reason. See filter.txt for a discussion. * filtering on leaf attributes needs to be double-checked for good and bad things ok: &User-Name[&User-Name == 'bar'] better: &USer-Name[=='bar'] bad: &User-Name[&Filter-Id == 'bar] It's probably simplest to just allow the first syntax. It's redundant, but not wrong. We then need to update the tokenizer to walk over the condition, and ensure that the only attribute used in the condition is the leaf attribute which we are checking. * the tests are for conditions, but we do want to allow filtering on group attributes: &TLS-Certificate[&Common-Name == 'foo'] but the conditions don't allow comparisons on groups: &TLS-Certificate == ... So it makes sense to forbid &TLS-Certificate[&Common-Name == 'foo'] == ... until such time as we do allow conditions on groups. * tmpl_dcursor now has an assertion if you try to use conditions in filters. We still need to write the run-time evaluation code which checks if the condition matches. --- diff --git a/src/lib/server/tmpl.h b/src/lib/server/tmpl.h index 4671bafaf71..d3c5aca7d57 100644 --- a/src/lib/server/tmpl.h +++ b/src/lib/server/tmpl.h @@ -403,11 +403,13 @@ FR_DLIST_TYPES(tmpl_attr_list) typedef enum { TMPL_ATTR_FILTER_TYPE_NONE = 0, //!< No filter present. TMPL_ATTR_FILTER_TYPE_INDEX, //!< Filter is an index type. + TMPL_ATTR_FILTER_TYPE_CONDITION, //!< Filter is a condition } tmpl_attr_filter_type_t; typedef struct { tmpl_attr_filter_type_t _CONST type; //!< Type of filter this is. int16_t _CONST num; //!< For array references. + xlat_exp_head_t _CONST *cond; //!< xlat condition } tmpl_attr_filter_t; /** An element in a list of nested attribute references @@ -524,10 +526,12 @@ static inline bool ar_is_raw(tmpl_attr_t const *ar) } #define ar_num filter.num +#define ar_cond filter.cond #define ar_filter_type filter.type #define ar_filter_is_none(_ar) ((_ar)->ar_filter_type == TMPL_ATTR_FILTER_TYPE_NONE) #define ar_filter_is_num(_ar) ((_ar)->ar_filter_type == TMPL_ATTR_FILTER_TYPE_INDEX) +#define ar_filter_is_cond(_ar) ((_ar)->ar_filter_type == TMPL_ATTR_FILTER_TYPE_CONDITION) /** @} */ /** A source or sink of value data. @@ -1028,6 +1032,7 @@ typedef enum { ///< to the one specified. TMPL_ATTR_ERROR_FILTER_NOT_ALLOWED, //!< Filters disallowed by rules. TMPL_ATTR_ERROR_INVALID_ARRAY_INDEX, //!< Invalid array index. + TMPL_ATTR_ERROR_INVALID_FILTER, //!< Invalid filter TMPL_ATTR_ERROR_NESTING_TOO_DEEP, //!< Too many levels of nesting. TMPL_ATTR_ERROR_MISSING_TERMINATOR, //!< Unexpected text found after attribute reference TMPL_ATTR_ERROR_BAD_CAST //!< Specified cast was invalid. diff --git a/src/lib/server/tmpl_dcursor.c b/src/lib/server/tmpl_dcursor.c index e977d73f0a4..f4e75dd6a5d 100644 --- a/src/lib/server/tmpl_dcursor.c +++ b/src/lib/server/tmpl_dcursor.c @@ -133,6 +133,8 @@ fr_pair_t *_tmpl_cursor_eval(fr_pair_t *curr, tmpl_dcursor_ctx_t *cc) ar = ns->ar; vp = fr_dcursor_current(&ns->cursor); + fr_assert(!ar || ar_filter_is_none(ar) || ar_filter_is_num(ar)); /* @todo - add evaluation of conditions */ + if (ar) switch (ar->ar_num) { /* * Get the first instance diff --git a/src/lib/server/tmpl_tokenize.c b/src/lib/server/tmpl_tokenize.c index dc732f0645a..7d54c306f95 100644 --- a/src/lib/server/tmpl_tokenize.c +++ b/src/lib/server/tmpl_tokenize.c @@ -1349,29 +1349,93 @@ static fr_slen_t tmpl_attr_parse_filter(tmpl_attr_error_t *err, tmpl_attr_t *ar, { fr_sbuff_parse_error_t sberr = FR_SBUFF_PARSE_OK; fr_sbuff_t tmp = FR_SBUFF(&our_name); - - if (fr_sbuff_out(&sberr, &ar->ar_num, &tmp) < 0) { - if (sberr == FR_SBUFF_PARSE_ERROR_NOT_FOUND) { - fr_strerror_const("Invalid array index"); + ssize_t rcode; + fr_slen_t slen; + tmpl_rules_t t_rules; + fr_sbuff_parse_rules_t p_rules; + fr_sbuff_term_t const filter_terminals = FR_SBUFF_TERMS(L("]")); + + rcode = fr_sbuff_out(&sberr, &ar->ar_num, &tmp); + if ((rcode > 0) && (fr_sbuff_is_char(&tmp, ']'))) { + if ((ar->ar_num > 1000) || (ar->ar_num < 0)) { + fr_strerror_printf("Invalid array index '%hi' (should be between 0-1000)", ar->ar_num); + ar->ar_num = 0; if (err) *err = TMPL_ATTR_ERROR_INVALID_ARRAY_INDEX; goto error; } + fr_sbuff_set(&our_name, &tmp); /* Advance name _AFTER_ doing checks */ + break; + } + + /* + * Temporary parsing hack: &User-Name[a] does _not_ match a condition 'a'. + */ + if (!fr_sbuff_is_char(&tmp, '&')) { fr_strerror_const("Invalid array index"); if (err) *err = TMPL_ATTR_ERROR_INVALID_ARRAY_INDEX; goto error; } - if ((ar->ar_num > 1000) || (ar->ar_num < 0)) { - fr_strerror_printf("Invalid array index '%hi' (should be between 0-1000)", ar->ar_num); - ar->ar_num = 0; - if (err) *err = TMPL_ATTR_ERROR_INVALID_ARRAY_INDEX; + /* + * For now, we don't allow filtering on leaf values. e.g. + * + * &User-Name[&User-Name == foo] + * + * @todo - find some sane way of allowing this, without mangling the xlat expression + * parser too badly. The simplest way is likely to just parse the expression, and then + * walk over it, complaining if it contains attribute references other than &User-Name. + * + * Anything else is likely too hard. + * + * We also want to disallow basic conditions like "true" or "false". The conditions + * should only be using attribute references, and those attribute references should be + * limited to certain attributes. + * + * And if the filter attribute is a TLV, the condition code complains with 'Nesting types + * such as groups or TLVs cannot be used in condition comparisons'. This is reasonable, + * as we can't currently compare things like; + * + * &Group-Thingy == { &foo = bar } + * + * Which would involve creating the RHS list, doing an element-by-element comparison, and + * then returning. + * + * In order to fix that, we have to + */ + if (!fr_type_is_structural(ar->ar_da->type)) { + fr_strerror_printf("Invalid filter - cannot use filter on leaf attributes"); + ar->ar_num = 0; + if (err) *err = TMPL_ATTR_ERROR_INVALID_ARRAY_INDEX; + goto error; + } + + fr_assert(ar->ar_da != NULL); + fr_assert(fr_type_is_structural(ar->ar_da->type)); + + tmp = FR_SBUFF(&our_name); + t_rules = (tmpl_rules_t) {}; + t_rules.attr = *at_rules; + t_rules.attr.namespace = ar->ar_da; + + p_rules = (fr_sbuff_parse_rules_t) { + .terminals = &filter_terminals, + .escapes = NULL + }; + + /* + * Check if it's a condition. + */ + slen = xlat_tokenize_condition(ar, &ar->ar_cond, &tmp, &p_rules, &t_rules); + if (slen < 0) { goto error; } + + ar->ar_filter_type = TMPL_ATTR_FILTER_TYPE_CONDITION; fr_sbuff_set(&our_name, &tmp); /* Advance name _AFTER_ doing checks */ - } break; } + } /* * Always advance here, so the error @@ -4222,6 +4286,7 @@ fr_slen_t tmpl_attr_print(fr_sbuff_t *out, tmpl_t const *vpt, tmpl_attr_prefix_t break; default: + fr_assert(0); return 0; } @@ -4346,30 +4411,38 @@ fr_slen_t tmpl_attr_print(fr_sbuff_t *out, tmpl_t const *vpt, tmpl_attr_prefix_t } } - /* - * Add array subscript. - * - * Will later be complex filters. - */ - switch (ar->ar_num) { - case NUM_UNSPEC: - break; + if (ar_filter_is_none(ar)) { + /* do nothing */ - case NUM_ALL: - FR_SBUFF_IN_STRCPY_LITERAL_RETURN(&our_out, "[*]"); - break; + } else if (ar_filter_is_num(ar)) { + switch (ar->ar_num) { + case NUM_UNSPEC: + break; - case NUM_COUNT: - FR_SBUFF_IN_STRCPY_LITERAL_RETURN(&our_out, "[#]"); - break; + case NUM_ALL: + FR_SBUFF_IN_STRCPY_LITERAL_RETURN(&our_out, "[*]"); + break; - case NUM_LAST: - FR_SBUFF_IN_STRCPY_LITERAL_RETURN(&our_out, "[n]"); - break; + case NUM_COUNT: + FR_SBUFF_IN_STRCPY_LITERAL_RETURN(&our_out, "[#]"); + break; - default: - FR_SBUFF_IN_SPRINTF_RETURN(&our_out, "[%i]", ar->ar_num); - break; + case NUM_LAST: + FR_SBUFF_IN_STRCPY_LITERAL_RETURN(&our_out, "[n]"); + break; + + default: + FR_SBUFF_IN_SPRINTF_RETURN(&our_out, "[%i]", ar->ar_num); + break; + } + + } else if (ar_filter_is_cond(ar)) { + FR_SBUFF_IN_STRCPY_LITERAL_RETURN(&our_out, "["); + (void) xlat_print(&our_out, ar->ar_cond, NULL); + FR_SBUFF_IN_STRCPY_LITERAL_RETURN(&our_out, "]"); + + } else { + fr_assert(0); } if (tmpl_attr_list_next(tmpl_attr(vpt), ar)) FR_SBUFF_IN_CHAR_RETURN(&our_out, '.'); diff --git a/src/tests/unit/condition/filter.txt b/src/tests/unit/condition/filter.txt new file mode 100644 index 00000000000..884c666ad5a --- /dev/null +++ b/src/tests/unit/condition/filter.txt @@ -0,0 +1,55 @@ +# +# Tests for parsing attribute filers +# +# $Id$ +# + +proto-dictionary radius + +condition &User-Name[0] == "foo" +match &User-Name[0] == "foo" + +# +# This is a run-time error. [#] returns an integer. +# +condition &User-Name[#] == "foo" +match &User-Name[#] == "foo" + +# +# This is a run-time error. [*] returns a list. +# +condition &User-Name[*] == "foo" +match &User-Name[*] == "foo" + +condition &User-Name[n] == "foo" +match &User-Name[n] == "foo" + +condition &User-Name[-1] == "foo" +match ERROR offset 12: Invalid array index '-1' (should be between 0-1000) + +# +# tmpl_dcursor.c has an assert which fails if you try to use +# conditional filters. +# +# And we do not (yet) allow for filters on leaf attributes. See +# @todo in tmpl_attr_parse_filter(() +# +condition &User-Name[&User-Name == 'bar'] == "foo" +match ERROR offset 12: Invalid filter - cannot use filter on leaf attributes + +# +# The condition code doesn't allow &Group-Thingy == { &foo = bar } +# +# Arguably it could, but that wouldn't help here. +# +# What we really want to allow is a filter for use with 'foreach': +# +# foreach &TLS-Certificate[&Common-Name == 'user@example.com'] { ... } +# +# which is a loop that returns &TLS-Certificate. +# +condition &TLS-Certificate[&Common-Name == 'user@example.com'] == bar +match ERROR offset 1: Nesting types such as groups or TLVs cannot be used in condition comparisons + +count +match 13