From 864ed3af3d0c69a1f62444a7cd056db137841a77 Mon Sep 17 00:00:00 2001 From: Chuck Rolke Date: Thu, 31 May 2018 13:35:49 -0400 Subject: [PATCH 1/3] DISPATCH-1011: Improve user name substitution token logic and code Remove code flagged by Coverity. Add scheme that specifies precisely where user name substitution goes. --- .../policy/policy_local.py | 66 ++- src/policy.c | 497 ++++++++++++++---- src/policy.h | 11 + src/policy_internal.h | 3 +- src/remote_sasl.c | 4 +- tests/policy_test.c | 90 +++- tests/router_policy_test.py | 8 +- 7 files changed, 553 insertions(+), 126 deletions(-) diff --git a/python/qpid_dispatch_internal/policy/policy_local.py b/python/qpid_dispatch_internal/policy/policy_local.py index f651dcc293..566e3fb080 100644 --- a/python/qpid_dispatch_internal/policy/policy_local.py +++ b/python/qpid_dispatch_internal/policy/policy_local.py @@ -89,6 +89,16 @@ class PolicyKeys(object): # policy stats controlled by C code but referenced by settings KW_CSTATS = "denialCounts" + # Username subsitituion token in link source and target names and patterns + KC_TOKEN_USER = "${user}" + + # Link target/source name wildcard tuple keys + KC_TUPLE_ABSENT = 'a' + KC_TUPLE_PREFIX = 'p' + KC_TUPLE_SUFFIX = 's' + KC_TUPLE_EMBED = 'e' + KC_TUPLE_WILDCARD = '*' + # # class PolicyCompiler(object): @@ -291,8 +301,60 @@ def compile_app_settings(self, vhostname, usergroup, policy_in, policy_out, warn (vhostname, usergroup, key, val, type(val))) # deduplicate address lists val = list(set(val)) - # output result is CSV string with no white space between values: 'abc,def,mytarget' - policy_out[key] = ','.join(val) + # val is CSV string with no white space between values: 'abc,def,mytarget,tmp-${user}' + if key == PolicyKeys.KW_USERS: + # user name list items are literal strings and need no special handling + policy_out[key] = ','.join(val) + else: + # source and target names get special handling for the '${user}' substitution token + # The literal string is translated to a (key, prefix, suffix) set of three strings. + # C code does not have to search for the username token and knows with authority + # how to construct match strings. + # A wildcard is also signaled. + utoken = PolicyKeys.KC_TOKEN_USER + eVal = [] + for v in val: + vcount = v.count(utoken) + if vcount > 1: + errors.append("Policy vhost '%s' user group '%s' policy key '%s' item '%s' contains multiple user subtitution tokens" % + (vhostname, usergroup, key, v)) + return False + elif vcount == 1: + # a single token is present as a prefix, suffix, or embedded + # construct cChar, S1, S2 encodings to be added to eVal description + if v.startswith(utoken): + # prefix + eVal.append(PolicyKeys.KC_TUPLE_PREFIX) + eVal.append('') + eVal.append(v) + elif v.endswith(utoken): + # suffix + eVal.append(PolicyKeys.KC_TUPLE_SUFFIX) + eVal.append(v) + eVal.append('') + else: + # embedded + if key in [PolicyKeys.KW_SOURCE_PATTERN, + PolicyKeys.KW_TARGET_PATTERN]: + errors.append("Policy vhost '%s' user group '%s' policy key '%s' item '%s' may contain match pattern '%s' as a prefix or a suffix only." % + (vhostname, usergroup, key, v, utoken)) + return False + eVal.append(PolicyKeys.KC_TUPLE_EMBED) + eVal.append(v[0:v.find(utoken)]) + eVal.append(v[v.find(utoken) + len(utoken):]) + else: + # ${user} token is absent + if v == PolicyKeys.KC_TUPLE_WILDCARD: + eVal.append(PolicyKeys.KC_TUPLE_WILDCARD) + eVal.append('') + eVal.append('') + else: + eVal.append(PolicyKeys.KC_TUPLE_ABSENT) + eVal.append(v) + eVal.append('') + policy_out[key] = ','.join(eVal) + print "Val: ", val # hack alert + print "eVal:", eVal # hack alert if key == PolicyKeys.KW_SOURCES: user_sources = True diff --git a/src/policy.c b/src/policy.c index c7f9e5025c..58338c545c 100644 --- a/src/policy.c +++ b/src/policy.c @@ -54,6 +54,23 @@ static char* CONNECTION_DISALLOWED = "connection disallowed by local pol static char* SESSION_DISALLOWED = "session disallowed by local policy"; static char* LINK_DISALLOWED = "link disallowed by local policy"; +// +// username substitution key shared with configuration files and python code +// substitution triplet keys shared with python code +// +static const char * const user_subst_key = "${user}"; +static const char * const user_subst_i_absent = "a"; +static const char * const user_subst_i_prefix = "p"; +static const char * const user_subst_i_embed = "e"; +static const char * const user_subst_i_suffix = "s"; +static const char * const user_subst_i_wildcard = "*"; + +// interface shared with parse tree +// TODO: get parse_tree to export these values +static const char parse_spec_sep1 = '.'; +static const char parse_spec_sep2 = '/'; + + // // Policy configuration/statistics management interface // @@ -253,11 +270,14 @@ void qd_policy_socket_close(qd_policy_t *policy, const qd_connection_t *conn) // C in the CSV string static const char* QPALN_COMMA_SEP =","; + // // Given a CSV string defining parser tree specs for allowed sender or // receiver links, return a parse_tree // // @param config_spec CSV string with link name match patterns +// The patterns consist of ('key', 'prefix', 'suffix') triplets describing +// the match pattern. // @return pointer to parse tree // qd_parse_tree_t * qd_policy_parse_tree(const char *config_spec) @@ -270,18 +290,53 @@ qd_parse_tree_t * qd_policy_parse_tree(const char *config_spec) if (!tree) return NULL; - // Add CSV's values to the tree. - // Note that tree's pattern is unused. This code uses a dummy '1'. + // make a writable, disposable copy of the csv string char * dup = strdup(config_spec); + if (!dup) + return NULL; char * dupend = dup + strlen(dup); + char * pch = dup; while (pch < dupend) { - size_t vsize = strcspn(pch, QPALN_COMMA_SEP); - if (vsize > 0) { - pch[vsize] = '\0'; - qd_parse_tree_add_pattern_str(tree, pch, (void *)1); - } - pch += vsize + 1; + // the tuple strings + char *pChar, *pS1, *pS2; + size_t sChar, sS1, sS2; + + // extract control field + sChar = strcspn(pch, QPALN_COMMA_SEP); + if (sChar != 1) { assert(false); break;} + pChar = pch; + pChar[sChar] = '\0'; + pch += sChar + 1; + if (pch >= dupend) { assert(false); break; } + + // extract prefix field S1 + sS1 = strcspn(pch, QPALN_COMMA_SEP); + pS1 = pch; + pS1[sS1] = '\0'; + pch += sS1 + 1; + if (pch > dupend) { assert(false); break; } + + // extract suffix field S2 + sS2 = strcspn(pch, QPALN_COMMA_SEP); + pS2 = pch; + pch += sS2 + 1; + pS2[sS2] = '\0'; + + size_t sName = sS1 + strlen(user_subst_key) + sS2 + 1; // large enough to handle any case + char *pName = (char *)malloc(sName); + + if (!strcmp(pChar, user_subst_i_absent)) + snprintf(pName, sName, "%s", pS1); + else if (!strcmp(pChar, user_subst_i_prefix)) + snprintf(pName, sName, "%s%s", user_subst_key, pS2); + else if (!strcmp(pChar, user_subst_i_embed)) + snprintf(pName, sName, "%s%s%s", pS1, user_subst_key, pS2); + else + snprintf(pName, sName, "%s%s", pS1, user_subst_key); + qd_parse_tree_add_pattern_str(tree, pName, (void *)1); + + free(pName); } free(dup); return tree; @@ -508,62 +563,34 @@ void _qd_policy_deny_amqp_receiver_link(pn_link_t *pn_link, qd_connection_t *qd_ } -// -// -#define MIN(a,b) (((a)<(b))?(a):(b)) - -char * _qd_policy_link_user_name_subst(const char *uname, const char *proposed, char *obuf, int osize) -{ - if (strlen(uname) == 0) - return NULL; - - const char duser[] = "${user}"; - char *retptr = obuf; - const char *wiptr = proposed; - const char *findptr = strstr(proposed, uname); - if (findptr == NULL) { - return NULL; - } - - // Copy leading before match - int segsize = findptr - wiptr; - int copysize = MIN(osize, segsize); - if (copysize) - strncpy(obuf, wiptr, copysize); - wiptr += copysize; - osize -= copysize; - obuf += copysize; - - // Copy the substitution string - segsize = sizeof(duser) - 1; - copysize = MIN(osize, segsize); - if (copysize) - strncpy(obuf, duser, copysize); - wiptr += strlen(uname); - osize -= copysize; - obuf += copysize; - - // Copy trailing after match - strncpy(obuf, wiptr, osize); - return retptr; -} - - // // // Size of 'easy' temporary copy of allowed input string #define QPALN_SIZE 1024 -// Size of user-name-substituted proposed string. -#define QPALN_USERBUFSIZE 300 // Wildcard character #define QPALN_WILDCARD '*' +#define QPALN_USERBUFSIZE 300 +#define MIN(a,b) (((a)<(b))?(a):(b))\ + +/** + * Given a username and a list of allowed link names + * decide if the proposed link name is approved. + * @param[in] username the user name + * @param[in] allowed csv of (key, prefix, suffix) tuples + * @param[in] proposed the link source/target name to be approved + * @return true if the user is allowed to open this link source/target name + * + * Concrete example + * user: 'bob', allowed (from spec): 'A,B,tmp-${user},C', proposed: 'tmp-bob' + * note that allowed above is now a tuple and not simple string fron the spec. + */ bool _qd_policy_approve_link_name(const char *username, const char *allowed, const char *proposed) { // Verify string sizes are usable size_t p_len = strlen(proposed); if (p_len == 0) { - // degenerate case of blank name being opened. will never match anything. + // degenerate case of blank proposed name being opened. will never match anything. return false; } size_t a_len = strlen(allowed); @@ -572,71 +599,265 @@ bool _qd_policy_approve_link_name(const char *username, const char *allowed, con return false; } - // Create a temporary writable copy of incoming allowed list - char t_allow[QPALN_SIZE + 1]; // temporary buffer for normal allow lists - char * pa = t_allow; - if (a_len > QPALN_SIZE) { - pa = (char *)malloc(a_len + 1); // malloc a buffer for larger allow lists + size_t username_len = strlen(username); + + // make a writable, disposable copy of the csv string + char * dup = strdup(allowed); + if (!dup) { + return false; } - if (!pa) + char * dupend = dup + strlen(dup); + char * pch = dup; + + // get a scratch buffer for writing temporary match strings + char * pName = (char *)malloc(QPALN_SIZE); + if (!pName) { + free(dup); return false; + } + size_t pName_sz = QPALN_SIZE; - strcpy(pa, allowed); /* We know we have allocated enoough space */ - pa[a_len] = 0; - // Do reverse user substitution into proposed - char substbuf[QPALN_USERBUFSIZE]; - char * prop2 = _qd_policy_link_user_name_subst(username, proposed, substbuf, QPALN_USERBUFSIZE); - char *toknext = 0; - char *tok = strtok_r(pa, QPALN_COMMA_SEP, &toknext); - assert (tok); bool result = false; - while (tok != NULL) { - if (*tok == QPALN_WILDCARD) { - result = true; - break; - } - int matchlen = p_len; - int len = strlen(tok); - if (tok[len-1] == QPALN_WILDCARD) { - matchlen = len - 1; - assert(len > 0); + + while (pch < dupend) { + // the tuple strings + char *pChar, *pS1, *pS2; + size_t sChar, sS1, sS2; + + // extract control field + sChar = strcspn(pch, QPALN_COMMA_SEP); + if (sChar != 1) { assert(false); break;} + pChar = pch; + pChar[sChar] = '\0'; + pch += sChar + 1; + if (pch >= dupend) { assert(false); break; } + + // extract prefix field S1 + sS1 = strcspn(pch, QPALN_COMMA_SEP); + pS1 = pch; + pS1[sS1] = '\0'; + pch += sS1 + 1; + if (pch > dupend) { assert(false); break; } + + // extract suffix field S2 + sS2 = strcspn(pch, QPALN_COMMA_SEP); + pS2 = pch; + pch += sS2 + 1; + pS2[sS2] = '\0'; + + // compute size of generated string and make sure + // temporary buffer is big enough to hold it. + size_t sName = sS1 + username_len + sS2 + 1; + if (sName > pName_sz) { + size_t newSize = sName + QPALN_SIZE; + char * newPtr = (char *)realloc(pName, newSize); + if (!newPtr) { + break; + } + pName = newPtr; + pName_sz = newSize; } - if (strncmp(tok, proposed, matchlen) == 0) { + + // if wildcard then check no more + if (*pChar == *user_subst_i_wildcard) { result = true; break; } - if (prop2 && strncmp(tok, prop2, matchlen) == 0) { - result = true; + // From the rule clause construct what the rule is allowing + // given the user name associated with this request. + int snpN; + if (*pChar == *user_subst_i_absent) + snpN = snprintf(pName, sName, "%s", pS1); + else if (*pChar == *user_subst_i_prefix) + snpN = snprintf(pName, sName, "%s%s", username, pS2); + else if (*pChar == *user_subst_i_embed) + snpN = snprintf(pName, sName, "%s%s%s", pS1, username, pS2); + else if (*pChar == *user_subst_i_suffix) + snpN = snprintf(pName, sName, "%s%s", pS1, username); + else { + assert(false); break; } - tok = strtok_r(NULL, QPALN_COMMA_SEP, &toknext); - } - if (pa != t_allow) { - free(pa); + + size_t rule_len = MIN(snpN, sName); + if (pName[rule_len-1] != QPALN_WILDCARD) { + // Rule clauses that do not end with wildcard + // must match entire proposed name string. + // pName=tmp-bob-5, proposed can be only 'tmp-bob-5' + result = strcmp(proposed, pName) == 0; + } else { + // Rule clauses that end with wildcard + // must match only as many characters as the cluase without the '*'. + // pName=tmp*, will match proposed 'tmp', 'tmp-xxx', 'tmp-bob', ... + result = strncmp(proposed, pName, rule_len - 1) == 0; + } + if (result) + break; } + free(pName); + free(dup); + return result; } -bool _qd_policy_approve_link_name_tree(const char *username, qd_parse_tree_t *tree, const char *proposed) +bool _qd_policy_approve_link_name_tree(const char *username, const char *allowed, const char *proposed, + qd_parse_tree_t *tree) { // Verify string sizes are usable - size_t p_len = strlen(proposed); - if (p_len == 0) { - // degenerate case of blank name being opened. will never match anything. + size_t proposed_len = strlen(proposed); + if (proposed_len == 0) { + // degenerate case of blank proposed name being opened. will never match anything. + return false; + } + size_t a_len = strlen(allowed); + if (a_len == 0) { + // no names in 'allowed'. return false; } - void * unused_payload = 0; - if (qd_parse_tree_retrieve_match_str(tree, proposed, &unused_payload)) - return true; + // Regardless of how many rule clauses are specified only three match + // patterns must be checked: no user subst, prefix subst, and suffix subst. + bool need_check_nosubst = true; + bool need_check_prefix = true; + bool need_check_suffix = true; - // Do reverse user substitution into proposed - char substbuf[QPALN_USERBUFSIZE]; - char * prop2 = _qd_policy_link_user_name_subst(username, proposed, substbuf, QPALN_USERBUFSIZE); - if (prop2 && qd_parse_tree_retrieve_match_str(tree, prop2, &unused_payload)) - return true; - return false; + size_t username_len = strlen(username); + size_t usersubst_len = strlen(user_subst_key); + + // make a writable, disposable copy of the csv string + char * dup = strdup(allowed); + if (!dup) { + return false; + } + char * dupend = dup + strlen(dup); + char * pch = dup; + + // get a scratch buffer for writing temporary match strings + char * pName = (char *)malloc(QPALN_SIZE); + if (!pName) { + free(dup); + return false; + } + size_t pName_sz = QPALN_SIZE; + + bool result = false; + + while (pch < dupend) { + // the tuple strings + char *pChar, *pS1, *pS2; + size_t sChar, sS1, sS2; + + // extract control field + sChar = strcspn(pch, QPALN_COMMA_SEP); + if (sChar != 1) { assert(false); break;} + pChar = pch; + pChar[sChar] = '\0'; + pch += sChar + 1; + if (pch >= dupend) { assert(false); break; } + + // extract prefix field S1 + sS1 = strcspn(pch, QPALN_COMMA_SEP); + pS1 = pch; + pS1[sS1] = '\0'; + pch += sS1 + 1; + if (pch > dupend) { assert(false); break; } + + // extract suffix field S2 + sS2 = strcspn(pch, QPALN_COMMA_SEP); + pS2 = pch; + pch += sS2 + 1; + pS2[sS2] = '\0'; + + // compute size of generated string and make sure + // temporary buffer is big enough to hold it. + size_t sName = proposed_len + usersubst_len + 1; + if (sName > pName_sz) { + size_t newSize = sName + QPALN_SIZE; + char * newPtr = (char *)realloc(pName, newSize); + if (!newPtr) { + break; + } + pName = newPtr; + pName_sz = newSize; + } + + // From the rule clause construct what the rule is allowing + // given the user name associated with this request. + if (*pChar == *user_subst_i_absent && need_check_nosubst) { + need_check_nosubst = false; + // Substitution spec is absent. The search string is the literal + // S1 in the rule. + snprintf(pName, sName, "%s", proposed); + } + else if (*pChar == *user_subst_i_prefix && need_check_prefix) { + need_check_prefix = false; + // Substitution spec is prefix. + if (strncmp(proposed, username, username_len) != 0) + continue; // Denied. Proposed does not have username prefix. + // Check that username is not part of a larger token. + if (username_len == proposed_len) { + // If username is the whole link name then allow if lookup is ok + } else { + // Proposed is longer than username. Make sure that proposed + // is delimited after user name. + if (proposed[username_len] != parse_spec_sep1 && + proposed[username_len] != parse_spec_sep2) { + continue; // denied. proposed has username prefix it it not a delimited user name + } + } + snprintf(pName, sName, "%s%s", user_subst_key, proposed + username_len); + } + else if (*pChar == *user_subst_i_embed) { + assert(false); // not supported + } + else if (*pChar == *user_subst_i_suffix && need_check_suffix) { + need_check_suffix = false; + // Check that link name has username suffix + if (username_len > proposed_len) { + continue; // denied. proposed name is too short to hold username + } else { + //--- + // if (username_len == proposed_len) { ... } + // unreachable code. substitution-only rule clause is handled by prefix + //--- + if (proposed[proposed_len - username_len - 1] != parse_spec_sep1 && + proposed[proposed_len - username_len - 1] != parse_spec_sep2) { + continue; // denied. proposed suffix it it not a delimited user name + } + if (strncmp(&proposed[proposed_len - username_len], username, username_len) != 0) + continue; // denied. username is not the suffix + } + if (strncmp(proposed, username, username_len) != 0) + continue; // denied. proposed does not have username suffix + // check that username is not part of a larger token + if (username_len == proposed_len) { + // if username is the whole link name. this is allowed. + } else { + // proposed is longer than username. make sure that proposed + // is delimited after user name + if (proposed[username_len] != parse_spec_sep1 && + proposed[username_len] != parse_spec_sep2) { + continue; // denied. proposed has username prefix it it not a delimited user name + } + } + strncat(pName, proposed, proposed_len - usersubst_len); + strcat(pName, user_subst_key); + } + else { + assert(false); + break; + } + + void * unused_payload = 0; + result = qd_parse_tree_retrieve_match_str(tree, pName, &unused_payload); + if (result) + break; + } + free(pName); + free(dup); + + return result; } @@ -818,13 +1039,13 @@ bool qd_policy_approve_link_name(const char *username, { if (isReceiver) { if (settings->sourceParseTree) { - return _qd_policy_approve_link_name_tree(username, settings->sourceParseTree, proposed); + return _qd_policy_approve_link_name_tree(username, settings->sourcePattern, proposed, settings->sourceParseTree); } else if (settings->sources) { return _qd_policy_approve_link_name(username, settings->sources, proposed); } } else { if (settings->targetParseTree) { - return _qd_policy_approve_link_name_tree(username, settings->targetParseTree, proposed); + return _qd_policy_approve_link_name_tree(username, settings->targetPattern, proposed, settings->targetParseTree); } else if (settings->targets) { return _qd_policy_approve_link_name(username, settings->targets, proposed); } @@ -879,3 +1100,83 @@ char * qd_policy_host_pattern_lookup(qd_policy_t *policy, char *hostPattern) hostPattern, (payload ? (char *)payload : "null")); return payload; } + + +// Convert naked CSV allow list into parsed settings 3-tuple +// Note that this logic is also present in python compile_app_settings. +char * qd_policy_compile_allowed_csv(char * csv) +{ + size_t csv_len = strlen(csv); + size_t usersubst_len = strlen(user_subst_key); + + size_t n_commas = 0; + char * pch = strchr(csv, *QPALN_COMMA_SEP); + while (pch != NULL) { + n_commas++; + pch = strchr(pch + 1, *QPALN_COMMA_SEP); + } + + size_t result_size = csv_len + 3 * (n_commas + 1) + 1; // each token gets ctrl char and 2 commas + char * result = (char *)malloc(result_size); + if (!result) + return NULL; + result[0] = '\0'; + + char * dup = strdup(csv); + if (!dup) { + free(result); + return NULL; + } + char * dupend = dup + csv_len; + + size_t tok_size = 0; + char * sep = ""; + for (pch = dup; pch < dupend; pch += tok_size + 1) { + // isolate token + char * pcomma = strchr(pch, *QPALN_COMMA_SEP); + if (!pcomma) pcomma = dupend; + *pcomma = '\0'; + tok_size = pcomma - pch; + + strcat(result, sep); + sep = ","; + char * psubst = strstr(pch, user_subst_key); + if (psubst) { + // substitute token is present + if (psubst == pch) { + // token is a prefix + strcat(result, user_subst_i_prefix); + strcat(result, ",,"); + strcat(result, pch + usersubst_len); + } else if (psubst == pch + tok_size - usersubst_len) { + // token is a suffix + strcat(result, user_subst_i_suffix); + strcat(result, ","); + strncat(result, pch, tok_size - usersubst_len); + strcat(result, ","); + } else { + // token is embedded + strcat(result, user_subst_i_embed); + strcat(result, ","); + strncat(result, pch, psubst - pch); + strcat(result, ","); + strncat(result, psubst + usersubst_len, tok_size - (psubst - pch) - usersubst_len); + } + } else { + // substitute token is absent + if (strcmp(pch, user_subst_i_wildcard) == 0) { + // token is wildcard + strcat(result, user_subst_i_wildcard); + strcat(result, ",,"); + } else { + // token is ordinary string + strcat(result, user_subst_i_absent); + strcat(result, ","); + strcat(result, pch); + strcat(result, ","); + } + } + } + free(dup); + return result; +} diff --git a/src/policy.h b/src/policy.h index 98500ec66d..c24ad6b7fc 100644 --- a/src/policy.h +++ b/src/policy.h @@ -202,4 +202,15 @@ void qd_policy_host_pattern_remove(qd_policy_t *policy, char *hostPattern); * @return the name of the ruleset whose hostname pattern matched this actual hostname */ char * qd_policy_host_pattern_lookup(qd_policy_t *policy, char *hostPattern); + +/** + * Compile raw CSV spec of allowed sources/targets and return + * the string of tuples used by policy runtime. + * The returned string is allocated here and freed by the caller. + * This function does no error checking or logging. + * + * @param[in] csv the CSV allowed list + * @return the ruleset string to be used in policy settings. + */ +char * qd_policy_compile_allowed_csv(char * csv); #endif diff --git a/src/policy_internal.h b/src/policy_internal.h index 100338fb52..c075a75f5b 100644 --- a/src/policy_internal.h +++ b/src/policy_internal.h @@ -110,6 +110,7 @@ bool _qd_policy_approve_link_name(const char *username, const char *allowed, con * @param[in] username authenticated user name * @param[in] allowed policy settings source/target string in packed CSV form. * @param[in] proposed the link target name to be approved + * @param[in] tree the parse tree for this source/target names */ -bool _qd_policy_approve_link_name_tree(const char *username, qd_parse_tree_t *tree, const char *proposed); +bool _qd_policy_approve_link_name_tree(const char *username, const char *allowed, const char *proposed, qd_parse_tree_t *tree); #endif diff --git a/src/remote_sasl.c b/src/remote_sasl.c index fd9da26594..239891cb2b 100644 --- a/src/remote_sasl.c +++ b/src/remote_sasl.c @@ -249,10 +249,10 @@ static void set_policy_settings(pn_connection_t* conn, permissions_t* permission ZERO(qd_conn->policy_settings->denialCounts); if (permissions->targets.start && permissions->targets.capacity) { - qd_conn->policy_settings->targets = strdup(permissions->targets.start); + qd_conn->policy_settings->targets = qd_policy_compile_allowed_csv(permissions->targets.start); } if (permissions->sources.start && permissions->sources.capacity) { - qd_conn->policy_settings->sources = strdup(permissions->sources.start); + qd_conn->policy_settings->sources = qd_policy_compile_allowed_csv(permissions->sources.start); } qd_conn->policy_settings->allowDynamicSource = true; qd_conn->policy_settings->allowAnonymousSender = true; diff --git a/tests/policy_test.c b/tests/policy_test.c index ca73f3246f..377e4743ae 100644 --- a/tests/policy_test.c +++ b/tests/policy_test.c @@ -26,6 +26,11 @@ static char *test_link_name_lookup(void *context) { + // DISPATCH-1011: approval specifications are now CSV concatenated 3-tuples: + // (user-subst-code, prefix, suffix) + // codes are 'a'bsent, 'p'refix, 's'uffix, 'e'mbedded, '*'wildcard + // OLD: 'joe', NEW: 'a,joe,' + // Degenerate blank names if (_qd_policy_approve_link_name("a", "a", "")) return "blank proposed name not rejected"; @@ -33,31 +38,34 @@ static char *test_link_name_lookup(void *context) return "blank allowed list not rejected"; // Easy matches - if (!_qd_policy_approve_link_name("", "joe", "joe")) + if (!_qd_policy_approve_link_name("", "a,joe,", "joe")) return "proposed link 'joe' should match allowed links 'joe' but does not"; - if (_qd_policy_approve_link_name("", "joe", "joey")) + if (_qd_policy_approve_link_name("", "a,joe,", "joey")) return "proposed link 'joey' should not match allowed links 'joe' but does"; // Wildcard matches - if (!_qd_policy_approve_link_name("", "joe*", "joey")) + if (!_qd_policy_approve_link_name("", "a,joe*,", "joey")) return "proposed link 'joey' should match allowed links 'joe*' but does not"; - if (!_qd_policy_approve_link_name("", "joe*", "joezzzZZZ")) + if (!_qd_policy_approve_link_name("", "a,joe*,", "joezzzZZZ")) return "proposed link 'joezzzZZZ' should match allowed links 'joe*' but does not"; - if (!_qd_policy_approve_link_name("", "joe,*", "joey")) + if (!_qd_policy_approve_link_name("", "a,joe,,*,,", "joey")) return "proposed link 'joey' should match allowed links 'joe,*' but does not"; // Deeper match - if (!_qd_policy_approve_link_name("", "no1,no2,no3,yes,no4", "yes")) + if (!_qd_policy_approve_link_name("", "a,no1,,a,no2,,a,no3,,a,yes,,a,no4,", "yes")) return "proposed link 'yes' should match allowed links 'no1,no2,no3,yes,no4' but does not"; // Deeeper match - triggers malloc/free internal handler - char * bufp = (char *)malloc(512 * 5 + 6); +#define BIG_N 512 + char * bufp = (char *)malloc(BIG_N * 8 + 6); + if (!bufp) + return "failed to allocate buffer for large link name test"; char * wp = bufp; int i; - for (i=0; i<512; i++) { - wp += sprintf(wp, "n%03d,", i); + for (i=0; i Date: Thu, 31 May 2018 14:02:18 -0400 Subject: [PATCH 2/3] DISPATCH-1011: Describe user name substitution changes in doc --- doc/book/policy.adoc | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/doc/book/policy.adoc b/doc/book/policy.adoc index 888611cc88..a4eb99b7da 100644 --- a/doc/book/policy.adoc +++ b/doc/book/policy.adoc @@ -279,12 +279,19 @@ a * character, a # character, or a sequence of characters that do not include /, *, or #. The * token matches any single token. The # token matches zero or more tokens. +The user name substitution token may be used in a sourcePattern or in a +targetPattern subject to the following restrictions: + +* The user name substitution token must be the first or last token in the rule clause. +* The user name substitution token must stand alone within its delimited field. + It may not be concatenated with literal text prefixes or suffixes. + For each rule definition multiple patterns may be specified in a comma separated list. [options="nowrap"] ---- - sourcePattern: tmp_${user}, temp/#, ${user}-home/* + sourcePattern: tmp.${user}, temp/#, ${user}.home/* ---- [NOTE] From 7c9798135e728143377be54ce18313bf2c60894c Mon Sep 17 00:00:00 2001 From: Chuck Rolke Date: Thu, 31 May 2018 15:52:39 -0400 Subject: [PATCH 3/3] DISPATCH-1011: Parse tree exports separators. Improve self tests. Test sourcePattern suffixes found latent paste error in code. More through test cases to hit more conditional code paths. --- .../policy/policy_local.py | 2 - src/parse_tree.c | 5 ++ src/parse_tree.h | 3 +- src/policy.c | 41 +++++----- tests/policy_test.c | 33 ++++++++- tests/system_tests_policy.py | 74 +++++++++++++++++++ 6 files changed, 126 insertions(+), 32 deletions(-) diff --git a/python/qpid_dispatch_internal/policy/policy_local.py b/python/qpid_dispatch_internal/policy/policy_local.py index 566e3fb080..8936f3ffec 100644 --- a/python/qpid_dispatch_internal/policy/policy_local.py +++ b/python/qpid_dispatch_internal/policy/policy_local.py @@ -353,8 +353,6 @@ def compile_app_settings(self, vhostname, usergroup, policy_in, policy_out, warn eVal.append(v) eVal.append('') policy_out[key] = ','.join(eVal) - print "Val: ", val # hack alert - print "eVal:", eVal # hack alert if key == PolicyKeys.KW_SOURCES: user_sources = True diff --git a/src/parse_tree.c b/src/parse_tree.c index c02b74a25f..189f316fae 100644 --- a/src/parse_tree.c +++ b/src/parse_tree.c @@ -91,6 +91,11 @@ static void token_iterator_next(token_iterator_t *t) } +const char address_token_sep[] = "./"; +const char *qd_parse_address_token_sep() { + return address_token_sep; +} + static bool token_iterator_done(const token_iterator_t *t) { return t->token.begin == t->terminator; diff --git a/src/parse_tree.h b/src/parse_tree.h index 8a06fc9643..26613a3de2 100644 --- a/src/parse_tree.h +++ b/src/parse_tree.h @@ -28,8 +28,6 @@ typedef struct qd_parse_node qd_parse_tree_t; -//extern const char * const QD_PARSE_TREE_TOKEN_SEP; - // Pattern matching algorithms // ADDRESS - configured address prefix/pattern matching // token separators: '.' or '/' @@ -54,6 +52,7 @@ typedef enum { qd_parse_tree_t *qd_parse_tree_new(qd_parse_tree_type_t type); void qd_parse_tree_free(qd_parse_tree_t *tree); qd_parse_tree_type_t qd_parse_tree_type(const qd_parse_tree_t *tree); +const char *qd_parse_address_token_sep(); // verify the pattern is in a legal format for the given tree's match algorithm bool qd_parse_tree_validate_pattern(const qd_parse_tree_t *tree, diff --git a/src/policy.c b/src/policy.c index 58338c545c..bb785e626b 100644 --- a/src/policy.c +++ b/src/policy.c @@ -65,12 +65,6 @@ static const char * const user_subst_i_embed = "e"; static const char * const user_subst_i_suffix = "s"; static const char * const user_subst_i_wildcard = "*"; -// interface shared with parse tree -// TODO: get parse_tree to export these values -static const char parse_spec_sep1 = '.'; -static const char parse_spec_sep2 = '/'; - - // // Policy configuration/statistics management interface // @@ -563,6 +557,18 @@ void _qd_policy_deny_amqp_receiver_link(pn_link_t *pn_link, qd_connection_t *qd_ } +/** + * Given a char return true if it is a parse_tree token separater + */ +bool is_token_sep(char testc) +{ + for (const char *ptr = qd_parse_address_token_sep(); *ptr != '\0'; ptr++) { + if (*ptr == testc) + return true; + } + return false; +} + // // // Size of 'easy' temporary copy of allowed input string @@ -801,8 +807,7 @@ bool _qd_policy_approve_link_name_tree(const char *username, const char *allowed } else { // Proposed is longer than username. Make sure that proposed // is delimited after user name. - if (proposed[username_len] != parse_spec_sep1 && - proposed[username_len] != parse_spec_sep2) { + if (!is_token_sep(proposed[username_len])) { continue; // denied. proposed has username prefix it it not a delimited user name } } @@ -821,27 +826,15 @@ bool _qd_policy_approve_link_name_tree(const char *username, const char *allowed // if (username_len == proposed_len) { ... } // unreachable code. substitution-only rule clause is handled by prefix //--- - if (proposed[proposed_len - username_len - 1] != parse_spec_sep1 && - proposed[proposed_len - username_len - 1] != parse_spec_sep2) { + if (!is_token_sep(proposed[proposed_len - username_len - 1])) { continue; // denied. proposed suffix it it not a delimited user name } - if (strncmp(&proposed[proposed_len - username_len], username, username_len) != 0) + if (strncmp(&proposed[proposed_len - username_len], username, username_len) != 0) { continue; // denied. username is not the suffix - } - if (strncmp(proposed, username, username_len) != 0) - continue; // denied. proposed does not have username suffix - // check that username is not part of a larger token - if (username_len == proposed_len) { - // if username is the whole link name. this is allowed. - } else { - // proposed is longer than username. make sure that proposed - // is delimited after user name - if (proposed[username_len] != parse_spec_sep1 && - proposed[username_len] != parse_spec_sep2) { - continue; // denied. proposed has username prefix it it not a delimited user name } } - strncat(pName, proposed, proposed_len - usersubst_len); + pName[0] = '\0'; + strncat(pName, proposed, proposed_len - username_len); strcat(pName, user_subst_key); } else { diff --git a/tests/policy_test.c b/tests/policy_test.c index 377e4743ae..3dbd478701 100644 --- a/tests/policy_test.c +++ b/tests/policy_test.c @@ -98,19 +98,44 @@ static char *test_link_name_tree_lookup(void *context) qd_parse_tree_add_pattern_str(node, "${user}.xyz", payload); if (!_qd_policy_approve_link_name_tree("chuck", "p,,.xyz", "chuck.xyz", node)) - return "proposed link 'chuck.xyz' should tree-match allowed links with ${user} but does not"; + return "proposed link 'chuck.xyz' should tree-match allow links with ${user} but does not"; if (_qd_policy_approve_link_name_tree("chuck", "p,,.xyz", "chuck.xyz.ynot", node)) - return "proposed link 'chuck.xyz.ynot' should not tree-match allowed links with ${user} but does"; + return "proposed link 'chuck.xyz.ynot' should not tree-match allow links with ${user} but does"; qd_parse_tree_add_pattern_str(node, "${user}.#", payload); if (!_qd_policy_approve_link_name_tree("motronic", "p,,.#", "motronic", node)) - return "proposed link 'motronic' should tree-match allowed links with ${user} but does not"; + return "proposed link 'motronic' should tree-match allow links with ${user} but does not"; if (!_qd_policy_approve_link_name_tree("motronic", "p,,.#", "motronic.stubs.wobbler", node)) - return "proposed link 'motronic.stubs.wobbler' should tree-match allowed links with ${user} but does not"; + return "proposed link 'motronic.stubs.wobbler' should tree-match allow links with ${user} but does not"; + qd_parse_tree_t *node2 = qd_parse_tree_new(QD_PARSE_TREE_ADDRESS); + qd_parse_tree_add_pattern_str(node2, "abc.${user}", payload); + + if (!_qd_policy_approve_link_name_tree("chuck", "s,abc.,", "abc.chuck", node2)) + return "proposed link 'abc.chuck' should tree-match allow links with ${user} but does not"; + + if (_qd_policy_approve_link_name_tree("chuck", "s,abc.,", "abc.ynot.chuck", node2)) + return "proposed link 'abc.ynot.chuck' should not tree-match allow links with ${user} but does"; + + if (_qd_policy_approve_link_name_tree("chuck", "s,abc.,", "abc.achuck", node2)) + return "proposed link 'abc.achuck' should not tree-match allow links with ${user} but does"; + + if (_qd_policy_approve_link_name_tree("chuckginormous", "s,abc.,", "abc.chuck", node2)) + return "proposed link 'abc.chuck' should not tree-match allow links with ${user} but does"; + + qd_parse_tree_t *node3 = qd_parse_tree_new(QD_PARSE_TREE_ADDRESS); + qd_parse_tree_add_pattern_str(node3, "${user}", payload); + + if (!_qd_policy_approve_link_name_tree("chuck", "p,,", "chuck", node3)) + return "proposed link 'chuck' should tree-match allow links with ${user} but does not"; + + qd_parse_tree_free(node); + qd_parse_tree_free(node2); + qd_parse_tree_free(node3); + return 0; } diff --git a/tests/system_tests_policy.py b/tests/system_tests_policy.py index 92555d03ee..99ba611785 100644 --- a/tests/system_tests_policy.py +++ b/tests/system_tests_policy.py @@ -713,6 +713,60 @@ def disallowed_target(self): } } } +""" + + def disallowed_source_pattern1(self): + return """ +{ + "id": "DISPATCH-1993-3", + "maxConnections": 3, + "maxConnectionsPerHost": 3, + "maxConnectionsPerUser": 3, + "allowUnknownUser": true, + "groups": { + "$default": { + "allowAnonymousSender": true, + "maxReceivers": 99, + "users": "*", + "maxSessionWindow": 1000000, + "maxFrameSize": 222222, + "sourcePattern": "public, private, $management, abc-${user}.xyz", + "maxMessageSize": 222222, + "allowDynamicSource": true, + "remoteHosts": "*", + "maxSessions": 2, + "targetPattern": "public, private, $management", + "maxSenders": 22 + } + } +} +""" + + def disallowed_source_pattern2(self): + return """ +{ + "id": "DISPATCH-1993-3", + "maxConnections": 3, + "maxConnectionsPerHost": 3, + "maxConnectionsPerUser": 3, + "allowUnknownUser": true, + "groups": { + "$default": { + "allowAnonymousSender": true, + "maxReceivers": 99, + "users": "*", + "maxSessionWindow": 1000000, + "maxFrameSize": 222222, + "sourcePattern": "public, private, $management, abc/${user}.xyz", + "maxMessageSize": 222222, + "allowDynamicSource": true, + "remoteHosts": "*", + "maxSessions": 2, + "targetPattern": "public, private, $management", + "maxSenders": 22 + } + } +} """ def test_link_name_parse_tree_patterns(self): @@ -744,6 +798,26 @@ def test_link_name_parse_tree_patterns(self): self.assertTrue("InternalServerErrorStatus: PolicyError: \"Policy 'DISPATCH-1993-3' is invalid:" in e.message) self.assertTrue(exception) + # attempt another create that should be rejected - name subst must whole token + qdm_out = "" + exception = False + try: + qdm_out = self.run_qdmanage('create --type=vhost --name=DISPATCH-1993-3 --stdin', input=self.disallowed_source_pattern1()) + except Exception, e: + exception = True + self.assertTrue("InternalServerErrorStatus: PolicyError: \"Policy 'DISPATCH-1993-3' is invalid:" in e.message) + self.assertTrue(exception) + + # attempt another create that should be rejected - name subst must be prefix or suffix + qdm_out = "" + exception = False + try: + qdm_out = self.run_qdmanage('create --type=vhost --name=DISPATCH-1993-3 --stdin', input=self.disallowed_source_pattern2()) + except Exception, e: + exception = True + self.assertTrue("InternalServerErrorStatus: PolicyError: \"Policy 'DISPATCH-1993-3' is invalid:" in e.message) + self.assertTrue(exception) + class PolicyHostamePatternTest(TestCase): """