From 5b6af82e2989107559d020437c8f36d0880f1c81 Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Sat, 5 Jan 2019 11:08:16 +0100 Subject: [PATCH] Validation of mandatory choice and recursive mandatory containers. --- CHANGELOG.md | 17 +++-- apps/backend/backend_client.c | 5 +- apps/backend/backend_commit.c | 4 +- apps/netconf/netconf_main.c | 4 +- apps/restconf/README.md | 10 +-- example/example_cli.c | 2 +- lib/src/clixon_xml_map.c | 132 ++++++++++++++++++++++------------ lib/src/clixon_yang.c | 52 +++++++++++--- test/test_choice.sh | 2 +- test/test_nacm_ext.sh | 4 +- test/test_nacm_protocol.sh | 5 +- test/test_openconfig.sh | 34 ++++++--- test/test_restconf.sh | 2 +- test/test_rpc.sh | 7 +- 14 files changed, 186 insertions(+), 94 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 58ca127d..36269387 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,7 @@ ## 3.9.0 (Preliminary Target: Mid-January 2019) ### Planned new features -* [Roadmap](ROADMAP.md) (Uncommitted and unprioritized) +* [Roadmap](ROADMAP.md) ### Major New features * Correct XML namespace handling @@ -56,11 +56,6 @@ ``` * To keep previous non-strict namespace handling (backwards compatible), set CLICON_XML_NS_STRICT to false. * See https://github.com/clicon/clixon/issues/49 -* NACM extension (RFC8341) - * NACM module support (RFC8341 A1+A2) - * Recovery user "_nacm_recovery" added. - * Example use is restconf PUT when NACM edit-config is permitted, then automatic commit and discard are permitted using recovery user. - * Example user changed adm1 to andy to comply with RFC8341 example * Yang code upgrade (RFC7950) * YANG parser cardinality checked (https://github.com/clicon/clixon/issues/48) * See https://github.com/clicon/clixon/issues/84 @@ -70,13 +65,18 @@ * Openconfig yang specs parsed: https://github.com/openconfig/public * Improved "unknown" handling * More precise Yang validation and better error messages - * Example: adding bad-, missing-, or unknown-element error messages, etc instead of operation-failed, bad-element instead of "yang node not found", etc. - * + * Example: adding bad-, missing-, or unknown-element error messages, instead of operation-failed. + * Validation of mandatory choice and recursive mandatory containers * Yang load file configure options changed * `CLICON_YANG_DIR` is changed from a single directory to a path of directories * Note CLIXON_DATADIR (=/usr/local/share/clixon) need to be in the list * CLICON_YANG_MAIN_FILE Provides a filename with a single module filename. * CLICON_YANG_MAIN_DIR Provides a directory where all yang modules should be loaded. +* NACM extension (RFC8341) + * NACM module support (RFC8341 A1+A2) + * Recovery user "_nacm_recovery" added. + * Example use is restconf PUT when NACM edit-config is permitted, then automatic commit and discard are permitted using recovery user. + * Example user changed adm1 to andy to comply with RFC8341 example ### API changes on existing features (you may need to change your code) * Stricter YANG choice validation leads to enforcement of structures like: `choice c{ mandatory true; leaf x` statements. `x` was not previously enforced. @@ -95,7 +95,6 @@ * For backward compatibility, define CLICON_CLI_MODEL_TREENAME_PATCH in clixon_custom.h ### Minor changes -* Yang choice functionality improved and stricter validation for CLI generation, mandatory flags, etc. * Added new clixon-lib yang module for internal netconf protocol. Currently only extends the standard with a debug RPC. * Added three-valued return values for several validate functions where -1 is fatal error, 0 is validation failed and 1 is validation OK. * This includes: `xmldb_put`, `xml_yang_validate_all`, `xml_yang_validate_add`, `xml_yang_validate_rpc`, `api_path2xml`, `api_path2xpath` diff --git a/apps/backend/backend_client.c b/apps/backend/backend_client.c index a141f09f..2bbfbf85 100644 --- a/apps/backend/backend_client.c +++ b/apps/backend/backend_client.c @@ -430,8 +430,9 @@ from_client_edit_config(clicon_handle h, goto done; } if ((target = netconf_db_find(xn, "target")) == NULL){ - clicon_err(OE_XML, 0, "db not found"); - goto done; + if (netconf_missing_element(cbret, "protocol", "target", NULL) < 0) + goto done; + goto ok; } if ((cbx = cbuf_new()) == NULL){ clicon_err(OE_XML, errno, "cbuf_new"); diff --git a/apps/backend/backend_commit.c b/apps/backend/backend_commit.c index d7c2d6c3..8c406d69 100644 --- a/apps/backend/backend_commit.c +++ b/apps/backend/backend_commit.c @@ -117,8 +117,8 @@ generic_validate(yang_spec *yspec, for (i=0; itd_dlen; i++){ x1 = td->td_dvec[i]; ys = xml_spec(x1); - if (ys && yang_mandatory(ys)){ - if (netconf_missing_element(cbret, "protocol", xml_name(x1), "Removed mandatory variable") < 0) + if (ys && yang_mandatory(ys) && yang_config(ys)==0){ + if (netconf_missing_element(cbret, "protocol", xml_name(x1), "Missing mandatory variable") < 0) goto done; goto fail; } diff --git a/apps/netconf/netconf_main.c b/apps/netconf/netconf_main.c index 7e1163dc..19ec4ee6 100644 --- a/apps/netconf/netconf_main.c +++ b/apps/netconf/netconf_main.c @@ -97,8 +97,8 @@ netconf_input_packet(clicon_handle h, cxobj *xa; cxobj *xa2; - clicon_debug(1, "RECV"); - clicon_debug(2, "%s: RCV: \"%s\"", __FUNCTION__, cbuf_get(cb)); + clicon_debug(1, "%s", __FUNCTION__); + clicon_debug(2, "%s: \"%s\"", __FUNCTION__, cbuf_get(cb)); if ((cbret = cbuf_new()) == NULL){ clicon_err(LOG_ERR, errno, "cbuf_new"); goto done; diff --git a/apps/restconf/README.md b/apps/restconf/README.md index 2295e3fa..e951e18a 100644 --- a/apps/restconf/README.md +++ b/apps/restconf/README.md @@ -2,10 +2,10 @@ * [Installation](#installation) * [Streams](#streams) - * [Nchan Streams](#nchan-streams) + * [Nchan Streams](#nchan) * [Debugging](#debugging) -## 1. Installation +## Installation The examples are based on Nginx. Other reverse proxies should work but are not verified. @@ -76,7 +76,7 @@ Example of writing a new interfaces specification: curl -sX PUT http://localhost/restconf/data -d '{"ietf-interfaces:interfaces":{"interface":{"name":"eth1","type":"ex:eth","enabled":true}}}' ``` -## 2. Streams +## Streams Clixon have two experimental restconf event stream implementations following RFC8040 Section 6 using SSE. One native and one using Nginx @@ -125,7 +125,7 @@ You can also specify start and stop time. Start-time enables replay of existing See (stream tests)[../test/test_streams.sh] for more examples. -## 3. Nchan +## Nchan As an alternative streams implementation, Nginx/Nchan can be used. Nginx uses pub/sub channels and can be configured in a variety of @@ -180,7 +180,7 @@ curl -H "Accept: text/event-stream" -H "Last-Event-ID: 1539961709:0" -s -X GET h See (https://nchan.io/#eventsource) on more info on how to access an SSE sub endpoint. -## 4. Debugging +## Debugging Start the restconf fastcgi program with debug flag: ``` diff --git a/example/example_cli.c b/example/example_cli.c index bf24064d..ebd2305a 100644 --- a/example/example_cli.c +++ b/example/example_cli.c @@ -96,7 +96,7 @@ fib_route_rpc(clicon_handle h, /* User supplied variable in CLI command */ instance = cvec_find(cvv, "instance"); /* get a cligen variable from vector */ /* Create XML for fib-route netconf RPC */ - if (xml_parse_va(&xtop, NULL, "%s", + if (xml_parse_va(&xtop, NULL, "%sipv4", clicon_username_get(h), cv_string_get(instance)) < 0) goto done; diff --git a/lib/src/clixon_xml_map.c b/lib/src/clixon_xml_map.c index 4b51e30b..faac0666 100644 --- a/lib/src/clixon_xml_map.c +++ b/lib/src/clixon_xml_map.c @@ -482,6 +482,89 @@ xml_yang_validate_rpc(cxobj *xrpc, goto done; } +/*! Check if an xml node lacks mandatory children + * @param[in] xt XML node to be validated + * @param[in] yt xt:s yang statement + * @param[out] cbret Error buffer (set w netconf error if retval == 0) + * @retval 1 Validation OK + * @retval 0 Validation failed (cbret set) + * @retval -1 Error + */ +static int +check_mandatory(cxobj *xt, + yang_stmt *yt, + cbuf *cbret) +{ + int retval = -1; + int i; + cxobj *x; + yang_stmt *y; + yang_stmt *yc; + yang_node *yp; + + for (i=0; iys_len; i++){ + yc = yt->ys_stmt[i]; + if (!yang_mandatory(yc)) + continue; + switch (yc->ys_keyword){ + case Y_CONTAINER: + case Y_ANYDATA: + case Y_ANYXML: + case Y_LEAF: + if (yang_config(yc)==0) + break; + /* Find a child with the mandatory yang */ + x = NULL; + while ((x = xml_child_each(xt, x, CX_ELMNT)) != NULL) { + if ((y = xml_spec(x)) != NULL + && y==yc) + break; /* got it */ + } + if (x == NULL){ + if (netconf_missing_element(cbret, "application", yc->ys_argument, "Mandatory variable") < 0) + goto done; + goto fail; + } + break; + case Y_CHOICE: /* More complex because of choice/case structure */ + x = NULL; + while ((x = xml_child_each(xt, x, CX_ELMNT)) != NULL) { + if ((y = xml_spec(x)) != NULL && + (yp = yang_choice(y)) != NULL && + yp == (yang_node*)yc){ + break; /* leave loop with x set */ + } + } + if (x == NULL){ + /* @see RFC7950: 15.6 Error Message for Data That Violates + * a Mandatory "choice" Statement */ + if (cprintf(cbret, "" + "application" + "data-missing" + "missing-choice" +#ifdef NYI + // "" +#endif + "%s" + "error" + "", + yc->ys_argument) <0) + goto done; + goto fail; + } + break; + default: + break; + } /* switch */ + } + retval = 1; + done: + return retval; + fail: + retval = 0; + goto done; +} + /*! Validate a single XML node with yang specification for added entry * 1. Check if mandatory leafs present as subs. * 2. Check leaf values, eg int ranges and string regexps. @@ -510,10 +593,6 @@ xml_yang_validate_add(cxobj *xt, cg_var *cv = NULL; char *reason = NULL; yang_stmt *yt; /* yang spec of xt going in */ - yang_stmt *yc; /* yang spec of yt child */ - yang_stmt *yx; /* yang spec of xt children */ - yang_node *yp; /* yang spec of parent of yang spec of xt children */ - int i; char *body; int ret; cxobj *x; @@ -521,47 +600,12 @@ xml_yang_validate_add(cxobj *xt, /* if not given by argument (overide) use default link and !Node has a config sub-statement and it is false */ if ((yt = xml_spec(xt)) != NULL && yang_config(yt) != 0){ + if ((ret = check_mandatory(xt, yt, cbret)) < 0) + goto done; + if (ret == 0) + goto fail; + /* Check leaf values */ switch (yt->ys_keyword){ - case Y_RPC: - case Y_INPUT: - case Y_LIST: - /* fall thru */ - case Y_CONTAINER: - for (i=0; iys_len; i++){ - yc = yt->ys_stmt[i]; - switch (yc->ys_keyword){ - case Y_LEAF: - if (yang_config(yc)==0) - break; - if (yang_mandatory(yc) && xml_find(xt, yc->ys_argument)==NULL){ - if (netconf_missing_element(cbret, "application", yc->ys_argument, "Mandatory variable") < 0) - goto done; - goto fail; - } - break; - case Y_CHOICE: - if (yang_mandatory(yc)){ - /* If there is one child with this choice as parent */ - x = NULL; - while ((x = xml_child_each(xt, x, CX_ELMNT)) != NULL) { - if ((yx = xml_spec(x)) != NULL && - (yp = yang_choice(yx)) != NULL && - yp == (yang_node*)yc){ - break; /* leave loop with x set */ - } - } - if (x == NULL){ /* No hit */ - if (netconf_missing_element(cbret, "application", yc->ys_argument, "Mandatory choice") < 0) - goto done; - goto fail; - } - } - break; - default: - break; - } - } - break; case Y_LEAF: /* fall thru */ case Y_LEAF_LIST: diff --git a/lib/src/clixon_yang.c b/lib/src/clixon_yang.c index b618f037..7a81bc6b 100644 --- a/lib/src/clixon_yang.c +++ b/lib/src/clixon_yang.c @@ -2701,26 +2701,62 @@ ys_parse_sub(yang_stmt *ys, * Note: one can cache this value in ys_cvec instead of functionally evaluating it. * @retval 1 yang statement is leaf and it has a mandatory sub-stmt with value true * @retval 0 The negation of conditions for return value 1. + * @see RFC7950 Sec 3: + * o mandatory node: A mandatory node is one of: + * 1) A leaf, choice, anydata, or anyxml node with a "mandatory" + * statement with the value "true". + * 2) A list or leaf-list node with a "min-elements" statement with a + * value greater than zero. + * 3) A container node without a "presence" statement and that has at + * least one mandatory node as a child. */ int yang_mandatory(yang_stmt *ys) { yang_stmt *ym; + char *reason = NULL; + uint8_t min_elements; /* XXX change to 32 (need new cligen version) */ - if (ys->ys_keyword != Y_LEAF && ys->ys_keyword != Y_CHOICE) - return 0; - if ((ym = yang_find((yang_node*)ys, Y_MANDATORY, NULL)) != NULL){ - if (ym->ys_cv == NULL) /* shouldnt happen */ - return 0; - return cv_bool_get(ym->ys_cv); + /* 1) A leaf, choice, anydata, or anyxml node with a "mandatory" + * statement with the value "true". */ + if (ys->ys_keyword == Y_LEAF || ys->ys_keyword == Y_CHOICE || + ys->ys_keyword == Y_ANYDATA || ys->ys_keyword == Y_ANYXML){ + if ((ym = yang_find((yang_node*)ys, Y_MANDATORY, NULL)) != NULL){ + if (ym->ys_cv != NULL) /* shouldnt happen */ + return cv_bool_get(ym->ys_cv); + } + } + /* 2) A list or leaf-list node with a "min-elements" statement with a + * value greater than zero. */ + else if (ys->ys_keyword == Y_LIST || ys->ys_keyword == Y_LEAF_LIST){ + if ((ym = yang_find((yang_node*)ys, Y_MIN_ELEMENTS, NULL)) != NULL){ + /* XXX change to 32 (need new cligen version) */ + if (parse_uint8(ym->ys_argument, &min_elements, &reason) != 1){ + clicon_err(OE_YANG, EINVAL, "%s", reason?reason:"parse_uint8"); + return 0; /* XXX ignore error */ + } + return min_elements > 0; + } + } + /* 3) A container node without a "presence" statement and that has at + * least one mandatory node as a child. */ + else if (ys->ys_keyword == Y_CONTAINER && + yang_find((yang_node*)ys, Y_PRESENCE, NULL) == NULL){ + yang_stmt *yc; + int i; + for (i=0; iys_len; i++){ + yc = ys->ys_stmt[i]; + if (yang_mandatory(yc)) + return 1; + } } return 0; } /*! Return config state of this node * config statement is default true. - * Note that a node with config=false may not have a sub - * statement where config=true. And this function does not check the sttaus of a parent. + * @note a node with config=false may not have a sub statement where + * config=true. And this function does not check the status of a parent. * @retval 0 if node has a config sub-statement and it is false * @retval 1 node has not config sub-statement or it is true */ diff --git a/test/test_choice.sh b/test/test_choice.sh index c2b2f91f..1289a0f5 100755 --- a/test/test_choice.sh +++ b/test/test_choice.sh @@ -179,7 +179,7 @@ new "netconf get mandatory empty" expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 "]]>]]>" '^]]>]]>$' new "netconf validate mandatory" -expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 "]]>]]>" '^applicationmissing-elementnameerrorMandatory choice]]>]]>$' +expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 "]]>]]>" '^applicationdata-missingmissing-choicenameerror]]>]]>$' new "netconf set mandatory udp" expecteof "$clixon_netconf -qf $cfg -y $fyang" 0 ']]>]]>' "^]]>]]>$" diff --git a/test/test_nacm_ext.sh b/test/test_nacm_ext.sh index b9cef0e1..edbef77f 100755 --- a/test/test_nacm_ext.sh +++ b/test/test_nacm_ext.sh @@ -137,8 +137,8 @@ EOF new "test params: -f $cfg -y $fyang" if [ $BE -ne 0 ]; then - new "kill old backend -zf $cfg -y $fyang" - sudo clixon_backend -zf $cfg -y $fyang + new "kill old backend -zf $cfg " + sudo clixon_backend -zf $cfg if [ $? -ne 0 ]; then err fi diff --git a/test/test_nacm_protocol.sh b/test/test_nacm_protocol.sh index b4e5bc2f..8a3c298e 100755 --- a/test/test_nacm_protocol.sh +++ b/test/test_nacm_protocol.sh @@ -21,7 +21,7 @@ # automatically committed to running immediately after each successful # edit. # Which means that restconf -X DELETE /data translates to edit-config + commit -# WHICH IS allowed. +# which is allowed. APPNAME=example # include err() and new() functions and creates $dir @@ -65,7 +65,6 @@ module $APPNAME{ } } EOF - # The groups are slightly modified from RFC8341 A.1 # The rule-list is from A.2 RULES=$(cat < $cfg EOF files=$(find $OPENCONFIG -name "*.yang") -# Just cound nr of modules (exclude submodule) -let m=0; # Nr of modules +# Count nr of modules (exclude submodule) Assume "module" or "submodule" +# first word on first line +let ms=0; # Nr of modules +let ss=0; # Nr of smodules for f in $files; do - if [ -n "$(head -1 $f|grep '^module')" ]; then + let m=0; # Nr of modules + let s=0; # Nr of modules + if [ -n "$(head -15 $f|grep '^[ ]*module')" ]; then let m++; + let ms++; + elif [ -n "$(head -15 $f|grep '^[ ]*submodule')" ]; then + let s++; + let ss++; + else + echo "No module or submodule found $f" + exit + fi + if [ $m -eq 1 -a $s -eq 1 ]; then + echo "Double match $f" + exit fi done +echo "m:$ms s:$ss" new "Openconfig test: $clixon_cli -1f $cfg -y $f show version ($m modules)" for f in $files; do if [ -n "$(head -1 $f|grep '^module')" ]; then - new "cli $f" + new "$clixon_cli -1f $cfg -y $f show version" expectfn "$clixon_cli -1f $cfg -y $f show version" 0 "3." fi diff --git a/test/test_restconf.sh b/test/test_restconf.sh index 2a1fd961..66710e91 100755 --- a/test/test_restconf.sh +++ b/test/test_restconf.sh @@ -293,7 +293,7 @@ new2 "restconf rpc missing input" expecteq "$(curl -s -X POST -d '{}' http://localhost/restconf/operations/ietf-routing:fib-route)" '{"ietf-restconf:errors" : {"error": {"error-type": "rpc","error-tag": "malformed-message","error-severity": "error","error-message": "restconf RPC does not have input statement"}}} ' new "restconf rpc using POST xml" -ret=$(curl -s -X POST -H "Accept: application/yang-data+xml" -d '{"ietf-routing:input":{"routing-instance-name":"ipv4"}}' http://localhost/restconf/operations/ietf-routing:fib-route) +ret=$(curl -s -X POST -H "Accept: application/yang-data+xml" -d '{"ietf-routing:input":{"routing-instance-name":"ipv4","destination-address":{"address-family":"ipv4"}}}' http://localhost/restconf/operations/ietf-routing:fib-route) expect='ipv42.3.4.5static' match=`echo $ret | grep -EZo "$expect"` if [ -z "$match" ]; then diff --git a/test/test_rpc.sh b/test/test_rpc.sh index 45b357f1..27fb74c8 100755 --- a/test/test_rpc.sh +++ b/test/test_rpc.sh @@ -129,14 +129,13 @@ new "netconf edit-config extra arg should fail" expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' "^applicationunknown-elementextraerror]]>]]>$" new "netconf edit-config empty target should fail" -expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' '^applicationmissing-elementconfig-targeterrorMandatory choice]]>]]> -$' +expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' '^applicationdata-missingmissing-choiceconfig-targeterror]]>]]>$' new "netconf edit-config missing target should fail" -expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' "^missing-elementprotocolerrortarget]]>]]>$" +expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' '^applicationmissing-elementtargeterrorMandatory variable]]>]]>$' new "netconf edit-config missing config should fail" -expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' 'applicationmissing-elementedit-contenterrorMandatory choice]]>]]>$' +expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' '^applicationdata-missingmissing-choiceedit-contenterror]]>]]>$' new "Kill restconf daemon" sudo pkill -u www-data -f "/www-data/clixon_restconf"