diff --git a/CHANGELOG.md b/CHANGELOG.md index 32285f71..e3544744 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,6 +63,7 @@ Users may have to change how they access the system ### Minor changes +* Added more sanity checks on incoming top-level rpc and hello messages, including verifying top-level namespace * Added inline state field to clixon-example.yang * Added stricter check on schema-node identifier checking, such as for augments. * These checks are now made at YANG loading time @@ -70,7 +71,8 @@ Users may have to change how they access the system ### Corrected Bugs -* Fixed [Crash seen with startup mode as running with the XML_DB format being set to JSON. [clixon : 4.7.0] #138](https://github.com/clicon/clixon/issues/138) +* Fixed: [namespace prefix nc is not supported](https://github.com/clicon/clixon/issues/143) +* Fixed: [Crash seen with startup mode as running with the XML_DB format being set to JSON. [clixon : 4.7.0] #138](https://github.com/clicon/clixon/issues/138) * Fixed: Performance enhancement of unique list check (of duplicate keys) * Fixed: Validate/commit error with false positive yang choice changes detected in validation found in ietf-ipfix-psamp.yang. * Fixed: Accepted added subtrees containing lists with duplicate keys. diff --git a/apps/backend/backend_client.c b/apps/backend/backend_client.c index d931730d..bd50abc9 100644 --- a/apps/backend/backend_client.c +++ b/apps/backend/backend_client.c @@ -1594,6 +1594,9 @@ from_client_msg(clicon_handle h, cxobj *xret = NULL; uint32_t id; enum nacm_credentials_t creds; + char *rpcname; + char *rpcprefix; + char *namespace; clicon_debug(1, "%s", __FUNCTION__); yspec = clicon_dbspec_yang(h); @@ -1615,18 +1618,47 @@ from_client_msg(clicon_handle h, goto done; goto reply; } - if ((x = xpath_first(xt, NULL, "/rpc")) == NULL){ - if ((x = xpath_first(xt, NULL, "/hello")) != NULL){ - if ((ret = from_client_hello(h, x, ce, cbret)) <0) - goto done; - goto reply; - } - else{ - if (netconf_malformed_message(cbret, "rpc keyword expected")< 0) + /* Check for empty frame (no mesaages), return empty message, not clear from RFC what to do */ + if (xml_child_nr_type(xt, CX_ELMNT) == 0){ + if (netconf_malformed_message(cbret, "Empty message in netconf rpc frame")< 0) + goto done; + goto reply; + } + /* Check for multi-messages in frame */ + if (xml_child_nr_type(xt, CX_ELMNT) != 1){ + if (netconf_malformed_message(cbret, "More than one message in netconf rpc frame")< 0) + goto done; + goto reply; + } + if ((x = xml_child_i_type(xt, 0, CX_ELMNT)) == NULL){ /* Shouldnt happen */ + clicon_err(OE_XML, EFAULT, "No xml req (shouldnt happen)"); + goto done; + } + rpcname = xml_name(x); + rpcprefix = xml_prefix(x); + if (0) { /* XXX notyet (4.8) restconf seems not to produce right namespace */ + if (xml2ns(x, rpcprefix, &namespace) < 0) + goto done; + /* Only accept resolved NETCONF base namespace */ + if (namespace == NULL || strcmp(namespace, NETCONF_BASE_NAMESPACE) != 0){ + if (netconf_unknown_namespace(cbret, "protocol", rpcprefix, "No appropriate namespace associated with prefix")< 0) goto done; goto reply; } } + if (strcmp(rpcname, "rpc") == 0){ + ; /* continue below */ + } + else if (strcmp(rpcname, "hello") == 0){ + if ((ret = from_client_hello(h, x, ce, cbret)) <0) + goto done; + goto reply; + } + else{ + if (netconf_unknown_element(cbret, "protocol", rpcname, "Unrecognized netconf operation")< 0) + goto done; + goto reply; + } ce->ce_id = id; if ((ret = xml_yang_validate_rpc(h, x, &xret)) < 0) goto done; diff --git a/apps/netconf/netconf_main.c b/apps/netconf/netconf_main.c index 56dd32c9..7b26968e 100644 --- a/apps/netconf/netconf_main.c +++ b/apps/netconf/netconf_main.c @@ -79,7 +79,8 @@ static int ignore_packet_errors = 1; static int -netconf_hello(cxobj *xn) +netconf_hello(clicon_handle h, + cxobj *xn) { #ifdef nyi cxobj *x; @@ -93,125 +94,230 @@ netconf_hello(cxobj *xn) return 0; } +/*! Process incoming Netconf RPC netconf message + * @param[in] h Clicon handle + * @param[in] xreq XML tree containing netconf RPC message + * @param[in] yspec YANG spec + * @retval 0 OK + * @retval -1 Error + */ int -netconf_hello_dispatch(cxobj *xn) +netconf_rpc_message(clicon_handle h, + cxobj *xrpc, + yang_stmt *yspec) { - cxobj *xp; - int retval = -1; + int retval = -1; + cxobj *xret = NULL; /* Return (out) */ + int ret; + cbuf *cbret = NULL; + cxobj *xc; + cxobj *xa; + cxobj *xa2; - if ((xp = xpath_first(xn, NULL, "//hello")) != NULL) - retval = netconf_hello(xp); + if ((ret = xml_bind_yang_rpc(xrpc, yspec, &xret)) < 0) + goto done; + if (ret > 0 && + (ret = xml_yang_validate_rpc(h, xrpc, &xret)) < 0) + goto done; + if (ret == 0){ + if ((cbret = cbuf_new()) == NULL){ + clicon_err(LOG_ERR, errno, "cbuf_new"); + goto done; + } + clicon_xml2cbuf(cbret, xret, 0, 0, -1); + netconf_output_encap(1, cbret, "rpc-error"); + goto ok; + } + if (netconf_rpc_dispatch(h, xrpc, &xret) < 0){ + goto done; + } + else{ /* there is a return message in xret */ + if (xret == NULL){ + if ((cbret = cbuf_new()) == NULL){ + clicon_err(LOG_ERR, errno, "cbuf_new"); + goto done; + } + if (netconf_operation_failed(cbret, "rpc", "Internal error: no xml return")< 0) + goto done; + netconf_output_encap(1, cbret, "rpc-error"); + goto done; + } + if ((xc = xml_child_i(xret, 0))!=NULL){ + xa=NULL; + /* Copy message-id attribute from incoming to reply. + * RFC 6241: + * If additional attributes are present in an element, a NETCONF + * peer MUST return them unmodified in the element. This + * includes any "xmlns" attributes. + */ + while ((xa = xml_child_each(xrpc, xa, CX_ATTR)) != NULL){ + /* If attribute already exists, dont copy it */ + if (xml_find_type(xc, NULL, xml_name(xa), CX_ATTR) != NULL) + continue; + if ((xa2 = xml_dup(xa)) ==NULL) + goto done; + if (xml_addsub(xc, xa2) < 0) + goto done; + } + if ((cbret = cbuf_new()) == NULL){ + clicon_err(LOG_ERR, errno, "cbuf_new"); + goto done; + } + clicon_xml2cbuf(cbret, xml_child_i(xret,0), 0, 0, -1); + if (netconf_output_encap(1, cbret, "rpc-reply") < 0){ + cbuf_free(cbret); + goto done; + } + } + } + ok: + retval = 0; + done: + if (cbret) + cbuf_free(cbret); + if (xret) + xml_free(xret); return retval; } -/*! Process incoming packet - * @param[in] h Clicon handle - * @param[in] cb Packet buffer +/*! Process incoming a single netconf message parsed as XML + * Identify what netconf message it is + * @param[in] h Clicon handle + * @param[in] xreq XML tree containing netconf + * @param[in] yspec YANG spec + * @retval 0 OK + * @retval -1 Error */ static int -netconf_input_packet(clicon_handle h, - cbuf *cb) +netconf_input_packet(clicon_handle h, + cxobj *xreq, + yang_stmt *yspec) { int retval = -1; - char *str; - char *str0; - cxobj *xreq = NULL; /* Request (in) */ - int isrpc = 0; /* either hello or rpc */ cbuf *cbret = NULL; + char *rpcname; + char *rpcprefix; + char *namespace = NULL; + + clicon_debug(1, "%s", __FUNCTION__); + rpcname = xml_name(xreq); + rpcprefix = xml_prefix(xreq); + if (xml2ns(xreq, rpcprefix, &namespace) < 0) + goto done; + /* Only accept resolved NETCONF base namespace */ + if (namespace == NULL || strcmp(namespace, NETCONF_BASE_NAMESPACE) != 0){ + if ((cbret = cbuf_new()) == NULL){ + clicon_err(LOG_ERR, errno, "cbuf_new"); + goto done; + } + if (netconf_unknown_namespace(cbret, "protocol", rpcprefix, "No appropriate namespace associated with prefix")< 0) + goto done; + netconf_output_encap(1, cbret, "rpc-error"); + } + if (strcmp(rpcname, "rpc") == 0){ + if (netconf_rpc_message(h, xreq, yspec) < 0) + goto done; + } + else if (strcmp(rpcname, "hello") == 0){ + if (netconf_hello(h, xreq) < 0) + goto done; + } + else{ + if ((cbret = cbuf_new()) == NULL){ + clicon_err(LOG_ERR, errno, "cbuf_new"); + goto done; + } + if (netconf_unknown_element(cbret, "protocol", rpcname, "Unrecognized netconf operation")< 0) + goto done; + netconf_output_encap(1, cbret, "rpc-error"); + } + retval = 0; + done: + if (cbret) + cbuf_free(cbret); + return retval; +} + +/*! Process incoming frame, ie a char message framed by ]]>]]> + * Parse string to xml, check only one netconf message within a frame + * @param[in] h Clicon handle + * @param[in] cb Packet buffer + * @retval 0 OK + * @retval -1 Fatal error + */ +static int +netconf_input_frame(clicon_handle h, + cbuf *cb) +{ + int retval = -1; + char *str = NULL; + cxobj *xtop = NULL; /* Request (in) */ + cxobj *xreq = NULL; cxobj *xret = NULL; /* Return (out) */ - cxobj *xrpc; - cxobj *xc; + cbuf *cbret = NULL; yang_stmt *yspec; int ret; - cxobj *xa; - cxobj *xa2; 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"); + yspec = clicon_dbspec_yang(h); + if ((str = strdup(cbuf_get(cb))) == NULL){ + clicon_err(OE_UNIX, errno, "strdup"); goto done; } - yspec = clicon_dbspec_yang(h); - if ((str0 = strdup(cbuf_get(cb))) == NULL){ - clicon_log(LOG_ERR, "%s: strdup: %s", __FUNCTION__, strerror(errno)); - return -1; - } - str = str0; /* Parse incoming XML message */ - if (clixon_xml_parse_string(str, YB_MODULE, yspec, &xreq, NULL) < 0){ - free(str0); + if ((ret = clixon_xml_parse_string(str, YB_RPC, yspec, &xtop, &xret)) < 0){ + if ((cbret = cbuf_new()) == NULL){ + clicon_err(OE_UNIX, errno, "cbuf_new"); + goto done; + } if (netconf_operation_failed(cbret, "rpc", clicon_err_reason)< 0) goto done; + netconf_output_encap(1, cbret, "rpc-error"); /* XXX */ + goto ok; + } + if (ret == 0){ + if ((cbret = cbuf_new()) == NULL){ + clicon_err(LOG_ERR, errno, "cbuf_new"); + goto done; + } + clicon_xml2cbuf(cbret, xret, 0, 0, -1); netconf_output_encap(1, cbret, "rpc-error"); + goto ok; + } + /* Check for empty frame (no mesaages), return empty message, not clear from RFC what to do */ + if (xml_child_nr_type(xtop, CX_ELMNT) == 0){ + if ((cbret = cbuf_new()) == NULL){ + clicon_err(OE_UNIX, errno, "cbuf_new"); + goto done; + } + netconf_output_encap(1, cbret, "rpc-error"); + goto ok; + } + /* Check for multi-messages in frame */ + if (xml_child_nr_type(xtop, CX_ELMNT) != 1){ + if ((cbret = cbuf_new()) == NULL){ + clicon_err(OE_UNIX, errno, "cbuf_new"); + goto done; + } + if (netconf_malformed_message(cbret, "More than one message in netconf rpc frame")< 0) + goto done; + netconf_output_encap(1, cbret, "rpc-error"); + goto ok; + } + if ((xreq = xml_child_i_type(xtop, 0, CX_ELMNT)) == NULL){ /* Shouldnt happen */ + clicon_err(OE_XML, EFAULT, "No xml req (shouldnt happen)"); goto done; } - free(str0); - if ((xrpc=xpath_first(xreq, NULL, "//rpc")) != NULL){ - isrpc++; - if ((ret = xml_bind_yang_rpc(xrpc, yspec, &xret)) < 0) - goto done; - if (ret > 0 && - (ret = xml_yang_validate_rpc(h, xrpc, &xret)) < 0) - goto done; - if (ret == 0){ - clicon_xml2cbuf(cbret, xret, 0, 0, -1); - netconf_output_encap(1, cbret, "rpc-error"); - goto ok; - } - } - else - if (xpath_first(xreq, NULL, "//hello") != NULL) - ; - else{ - clicon_log(LOG_WARNING, "Invalid netconf msg: neither rpc or hello: dropped"); - goto done; - } - if (!isrpc){ /* hello */ - if (netconf_hello_dispatch(xreq) < 0) - goto done; - } - else /* rpc */ - if (netconf_rpc_dispatch(h, xrpc, &xret) < 0){ - goto done; - } - else{ /* there is a return message in xret */ - - if (xret == NULL){ - if (netconf_operation_failed(cbret, "rpc", "Internal error: no xml return")< 0) - goto done; - netconf_output_encap(1, cbret, "rpc-error"); - goto done; - } - if ((xc = xml_child_i(xret, 0))!=NULL){ - xa=NULL; - /* Copy message-id attribute from incoming to reply. - * RFC 6241: - * If additional attributes are present in an element, a NETCONF - * peer MUST return them unmodified in the element. This - * includes any "xmlns" attributes. - */ - while ((xa = xml_child_each(xrpc, xa, CX_ATTR)) != NULL){ - /* If attribute already exists, dont copy it */ - if (xml_find_type(xc, NULL, xml_name(xa), CX_ATTR) != NULL) - continue; - if ((xa2 = xml_dup(xa)) ==NULL) - goto done; - if (xml_addsub(xc, xa2) < 0) - goto done; - } - clicon_xml2cbuf(cbret, xml_child_i(xret,0), 0, 0, -1); - if (netconf_output_encap(1, cbret, "rpc-reply") < 0){ - cbuf_free(cbret); - goto done; - } - } - } + if (netconf_input_packet(h, xreq, yspec) < 0) + goto done; ok: retval = 0; - done: - if (xreq) - xml_free(xreq); + done: + if (str) + free(str); + if (xtop) + xml_free(xtop); if (xret) xml_free(xret); if (cbret) @@ -269,7 +375,7 @@ netconf_input_cb(int s, /* OK, we have an xml string from a client */ /* Remove trailer */ *(((char*)cbuf_get(cb)) + cbuf_len(cb) - strlen("]]>]]>")) = '\0'; - if (netconf_input_packet(h, cb) < 0 && + if (netconf_input_frame(h, cb) < 0 && !ignore_packet_errors) // default is to ignore errors goto done; if (cc_closed) diff --git a/lib/src/clixon_xml_io.c b/lib/src/clixon_xml_io.c index 07260727..9ed13812 100644 --- a/lib/src/clixon_xml_io.c +++ b/lib/src/clixon_xml_io.c @@ -461,7 +461,7 @@ _xml_parse(const char *str, clicon_debug(2, "%s", __FUNCTION__); if (strlen(str) == 0) - return 0; /* OK */ + return 1; /* OK */ if (xt == NULL){ clicon_err(OE_XML, errno, "Unexpected NULL XML"); return -1; diff --git a/test/lib.sh b/test/lib.sh index a69a61e0..eb3aa166 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -433,7 +433,7 @@ expectpart(){ # Pipe stdin to command # Arguments: # - Command -# - expected command return value (0 if OK) +# - expected command return value (0 if OK) XXX SHOULD SWITCH w next # - stdin input # - expected stdout outcome # Use this if you want regex eg ^foo$ diff --git a/test/test_api.sh b/test/test_api.sh index 9cc70707..d922b866 100755 --- a/test/test_api.sh +++ b/test/test_api.sh @@ -222,7 +222,7 @@ if [ $BE -ne 0 ]; then err fi new "start backend" - start_backend -s running -f $cfg + start_backend -s init -f $cfg fi new "waiting" diff --git a/test/test_netconf.sh b/test/test_netconf.sh index 0cb5ca63..36c70c64 100755 --- a/test/test_netconf.sh +++ b/test/test_netconf.sh @@ -49,12 +49,40 @@ if [ $BE -ne 0 ]; then wait_backend fi -new "netconf hello" -expecteof "$clixon_netconf -f $cfg" 0 "]]>]]>" "^urn:ietf:params:netconf:base:1.0urn:ietf:params:netconf:capability:yang-library:1.0?revision=2016-06-21&module-set-id=42urn:ietf:params:netconf:capability:candidate:1.0urn:ietf:params:netconf:capability:validate:1.1urn:ietf:params:netconf:capability:startup:1.0urn:ietf:params:netconf:capability:xpath:1.0urn:ietf:params:netconf:capability:notification:1.0[0-9]*]]>]]>]]>]]>$" +# Framing. with -q to inhibit rcv hello +new "Empty frame" +expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' ']]>]]>' -new "netconf hello, disable RFC7895/ietf-yang-library" +if [ $valgrindtest -eq 0 ]; then # Some leakage in lex / error handling difficult to catch +new "Frame invalid non-xml" +expecteof "$clixon_netconf -qf $cfg" 0 "This is not XML]]>]]>" 'rpcoperation-failederrorxml_parse: line 0: syntax error: at or before: This]]>]]>' 2> /dev/null +fi + +new "Frame with two messages" +expecteof "$clixon_netconf -qf $cfg" 0 "urn:ietf:params:netconf:base:1.0]]>]]>" 'rpcmalformed-messageerrorMore than one message in netconf rpc frame]]>]]>' + +new "Frame with unknown message" +expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" '^protocolunknown-elementxxxerrorUnrecognized netconf operation]]>]]>$' + +# Hello +new "Netconf snd hello with xmldecl" +expecteof "$clixon_netconf -qf $cfg" 0 "urn:ietf:params:netconf:base:1.0]]>]]>" '^$' + +new "Netconf snd hello with wrong prefix" +expecteof "$clixon_netconf -qf $cfg" 0 "urn:ietf:params:netconf:base:1.0]]>]]>" 'protocolunknown-namespacexxerrorNo appropriate namespace associated with prefix]]>]]>' + +new "Netconf snd hello with prefix" +expecteof "$clixon_netconf -qf $cfg" 0 "urn:ietf:params:netconf:base:1.0]]>]]>" '^$' + +new "netconf snd + rcv hello" +expecteof "$clixon_netconf -f $cfg" 0 "urn:ietf:params:netconf:base:1.0]]>]]>" "^urn:ietf:params:netconf:base:1.0urn:ietf:params:netconf:capability:yang-library:1.0?revision=2016-06-21&module-set-id=42urn:ietf:params:netconf:capability:candidate:1.0urn:ietf:params:netconf:capability:validate:1.1urn:ietf:params:netconf:capability:startup:1.0urn:ietf:params:netconf:capability:xpath:1.0urn:ietf:params:netconf:capability:notification:1.0[0-9]*]]>]]>$" + +new "netconf rcv hello, disable RFC7895/ietf-yang-library" expecteof "$clixon_netconf -f $cfg -o CLICON_MODULE_LIBRARY_RFC7895=0" 0 "]]>]]>" "^urn:ietf:params:netconf:base:1.0urn:ietf:params:netconf:capability:candidate:1.0urn:ietf:params:netconf:capability:validate:1.1urn:ietf:params:netconf:capability:startup:1.0urn:ietf:params:netconf:capability:xpath:1.0urn:ietf:params:netconf:capability:notification:1.0[0-9]*]]>]]>]]>]]>$" +new "netconf get-config prefix" +expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" "^]]>]]>$" + new "netconf get-config double quotes" expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" "^]]>]]>$" diff --git a/test/test_xml.sh b/test/test_xml.sh index d31f1dd2..a294c0d0 100755 --- a/test/test_xml.sh +++ b/test/test_xml.sh @@ -141,7 +141,7 @@ expecteof "$clixon_util_xml -o" 0 '' '' new "prolog element misc*" expecteof "$clixon_util_xml -o" 0 '' '' - + # We allow it as an internal necessity for parsing of xml fragments #new "double element error" #expecteof "$clixon_util_xml" 255 '' '' @@ -179,6 +179,8 @@ EOF ) expecteof "$clixon_util_xml -o" 0 "$XML" '^Cheaper by the Dozen1568491379$' +endtest + rm -rf $dir # unset conditional parameters