From 0626de9431ba8e2595c53722a48e1041c45a02b8 Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Tue, 16 Nov 2021 22:09:16 +0100 Subject: [PATCH] * RPC replies now verified with YANG * Stricter checking of outgoing RPC replies from server * See [RPC output not verified by yang](https://github.com/clicon/clixon/issues/283) * This lead to some corrections of RPC replies in system code --- CHANGELOG.md | 5 ++ apps/backend/backend_client.c | 17 ++-- apps/netconf/netconf_rpc.c | 9 +- apps/restconf/restconf_methods_post.c | 12 ++- lib/clixon/clixon_plugin.h | 3 +- lib/clixon/clixon_validate.h | 2 + lib/clixon/clixon_xml.h | 2 +- lib/src/clixon_plugin.c | 32 +++++-- lib/src/clixon_validate.c | 124 ++++++++++++++++++++++++++ lib/src/clixon_xml_bind.c | 15 +++- test/test_c++.sh | 2 +- 11 files changed, 197 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7fd27e05..788559b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,9 @@ Thanks netgate for providing the dispatcher code! Users may have to change how they access the system +* RPC replies now verified with YANG + * Stricter checking of outgoing RPC replies from server + * See [RPC output not verified by yang](https://github.com/clicon/clixon/issues/283) * XML to JSON CDATA translation is NOT stripped * Example, assume XML: ` x & x < y ]]>` * Previous bevavior: @@ -100,6 +103,7 @@ Developers may need to change their code * Any changes to context are logged at loglevel WARNING * [OpenConfig path compression](https://github.com/clicon/clixon/pull/276) * C API: Added set/get pointer API to clixon_data: + * Changed signature of `rpc_callback_call()` * Added json/cli support for cli save/load * clicon_ptr_get(), clicon_ptr_set(), * Restconf YANG PATCH according to RFC 8072 @@ -110,6 +114,7 @@ Developers may need to change their code ### Corrected Bugs +* [RPC output not verified by yang](https://github.com/clicon/clixon/issues/283) * [Statements given in "load set" are order dependent](https://github.com/clicon/clixon/issues/287) * Modify ordering of XML encoding to put sub-elements with YANG WHEN statements last * [RPC get-conf method returned some content not specified by select filter](https://github.com/clicon/clixon/issues/281) diff --git a/apps/backend/backend_client.c b/apps/backend/backend_client.c index cad66831..20700d49 100644 --- a/apps/backend/backend_client.c +++ b/apps/backend/backend_client.c @@ -217,14 +217,15 @@ clixon_stats_get_db(clicon_handle h, xt = xmldb_cache_get(h, dbname); } if (xt == NULL){ - cprintf(cb, "%s00", dbname); + cprintf(cb, "%s00", + CLIXON_LIB_NS, dbname); } else{ if (xml_stats(xt, &nr, &sz) < 0) goto done; - cprintf(cb, "%s%" PRIu64 "" + cprintf(cb, "%s%" PRIu64 "" "%zu", - dbname, nr, sz); + CLIXON_LIB_NS, dbname, nr, sz); } retval = 0; done: @@ -1001,7 +1002,7 @@ from_client_stats(clicon_handle h, cprintf(cbret, "", NETCONF_BASE_NAMESPACE); nr=0; xml_stats_global(&nr); - cprintf(cbret, "%" PRIu64 "", nr); + cprintf(cbret, "%" PRIu64 "", CLIXON_LIB_NS, nr); if (clixon_stats_get_db(h, "running", cbret) < 0) goto done; if (clixon_stats_get_db(h, "candidate", cbret) < 0) @@ -1136,6 +1137,7 @@ from_client_hello(clicon_handle h, return retval; } + /*! An internal clicon message has arrived from a client. Receive and dispatch. * @param[in] h Clicon handle * @param[in] s Socket where message arrived. read from this. @@ -1168,6 +1170,7 @@ from_client_msg(clicon_handle h, char *rpcname; char *rpcprefix; char *namespace = NULL; + int nr = 0; clicon_debug(1, "%s", __FUNCTION__); yspec = clicon_dbspec_yang(h); @@ -1295,13 +1298,15 @@ from_client_msg(clicon_handle h, goto reply; } clicon_err_reset(); - if ((ret = rpc_callback_call(h, xe, cbret, ce)) < 0){ + if ((ret = rpc_callback_call(h, xe, ce, &nr, cbret)) < 0){ if (netconf_operation_failed(cbret, "application", clicon_err_reason)< 0) goto done; clicon_log(LOG_NOTICE, "%s Error in rpc_callback_call:%s", __FUNCTION__, xml_name(xe)); goto reply; /* Dont quit here on user callbacks */ } - if (ret == 0){ /* not handled by callback */ + if (ret == 0) + goto reply; + if (nr == 0){ /* not handled by callback */ if (netconf_operation_not_supported(cbret, "application", "RPC operation not supported")< 0) goto done; goto reply; diff --git a/apps/netconf/netconf_rpc.c b/apps/netconf/netconf_rpc.c index 8ad952cf..30cd7073 100644 --- a/apps/netconf/netconf_rpc.c +++ b/apps/netconf/netconf_rpc.c @@ -588,6 +588,7 @@ netconf_application_rpc(clicon_handle h, cbuf *cb = NULL; cbuf *cbret = NULL; int ret; + int nr = 0; /* First check system / netconf RPC:s */ if ((cb = cbuf_new()) == NULL){ @@ -623,9 +624,13 @@ netconf_application_rpc(clicon_handle h, if (yrpc != NULL){ /* No need to check xn arguments with input statement since already bound and validated. */ /* Look for local (client-side) netconf plugins. */ - if ((ret = rpc_callback_call(h, xn, cbret, NULL)) < 0) + if ((ret = rpc_callback_call(h, xn, NULL, &nr, cbret)) < 0) goto done; - if (ret > 0){ /* Handled locally */ + if (ret == 0){ + if (clixon_xml_parse_string(cbuf_get(cbret), YB_NONE, NULL, xret, NULL) < 0) + goto done; + } + else if (nr > 0){ /* Handled locally */ if (clixon_xml_parse_string(cbuf_get(cbret), YB_NONE, NULL, xret, NULL) < 0) goto done; } diff --git a/apps/restconf/restconf_methods_post.c b/apps/restconf/restconf_methods_post.c index 23a627d5..31976596 100644 --- a/apps/restconf/restconf_methods_post.c +++ b/apps/restconf/restconf_methods_post.c @@ -712,6 +712,7 @@ api_operations_post(clicon_handle h, char *id = NULL; yang_stmt *ys = NULL; char *namespace = NULL; + int nr = 0; clicon_debug(1, "%s json:\"%s\" path:\"%s\"", __FUNCTION__, data, api_path); /* 1. Initialize */ @@ -826,9 +827,16 @@ api_operations_post(clicon_handle h, /* Look for local (client-side) restconf plugins. * -1:Error, 0:OK local, 1:OK backend */ - if ((ret = rpc_callback_call(h, xbot, cbret, req)) < 0) + if ((ret = rpc_callback_call(h, xbot, req, &nr, cbret)) < 0) goto done; - if (ret > 0){ /* Handled locally */ + if (ret == 0){ + if (clixon_xml_parse_string(cbuf_get(cbret), YB_NONE, NULL, &xe, NULL) < 0) + goto done; + if (api_return_err(h, req, xe, pretty, media_out, 0) < 0) + goto done; + goto ok; + } + else if (nr > 0){ /* Handled locally */ if (clixon_xml_parse_string(cbuf_get(cbret), YB_NONE, NULL, &xret, NULL) < 0) goto done; /* Local error: return it and quit */ diff --git a/lib/clixon/clixon_plugin.h b/lib/clixon/clixon_plugin.h index c7294881..feddc4a6 100644 --- a/lib/clixon/clixon_plugin.h +++ b/lib/clixon/clixon_plugin.h @@ -395,8 +395,7 @@ int clixon_plugin_datastore_upgrade_all(clicon_handle h, const char *db, cxobj * /* rpc callback API */ int rpc_callback_register(clicon_handle h, clicon_rpc_cb cb, void *arg, const char *ns, const char *name); -int rpc_callback_call(clicon_handle h, cxobj *xe, cbuf *cbret, void *arg); - +int rpc_callback_call(clicon_handle h, cxobj *xe, void *arg, int *nrp, cbuf *cbret); /* upgrade callback API */ int upgrade_callback_reg_fn(clicon_handle h, clicon_upgrade_cb cb, const char *strfn, const char *ns, void *arg); int upgrade_callback_call(clicon_handle h, cxobj *xt, char *ns, uint16_t op, uint32_t from, uint32_t to, cbuf *cbret); diff --git a/lib/clixon/clixon_validate.h b/lib/clixon/clixon_validate.h index 45720d39..7af5c338 100644 --- a/lib/clixon/clixon_validate.h +++ b/lib/clixon/clixon_validate.h @@ -44,9 +44,11 @@ * Prototypes */ int xml_yang_validate_rpc(clicon_handle h, cxobj *xrpc, cxobj **xret); +int xml_yang_validate_rpc_reply(clicon_handle h, cxobj *xrpc, cxobj **xret); int xml_yang_validate_add(clicon_handle h, cxobj *xt, cxobj **xret); int xml_yang_validate_list_key_only(cxobj *xt, cxobj **xret); int xml_yang_validate_all(clicon_handle h, cxobj *xt, cxobj **xret); int xml_yang_validate_all_top(clicon_handle h, cxobj *xt, cxobj **xret); +int rpc_reply_check(clicon_handle h, char *rpcname, cbuf *cbret); #endif /* _CLIXON_VALIDATE_H_ */ diff --git a/lib/clixon/clixon_xml.h b/lib/clixon/clixon_xml.h index 03766c92..f77a5690 100644 --- a/lib/clixon/clixon_xml.h +++ b/lib/clixon/clixon_xml.h @@ -147,7 +147,7 @@ enum yang_bind{ * Ie, xml looks like: ... where "x" is a top-level symbol in a module */ YB_PARENT, /* Assume yang binding of existing parent and match its children by name */ - YB_RPC, /* Assume top-level xml is an netconf RPC message (or hello) */ + YB_RPC, /* Assume top-level xml is an incoming netconf RPC message (or hello) */ }; typedef enum yang_bind yang_bind; diff --git a/lib/src/clixon_plugin.c b/lib/src/clixon_plugin.c index 2bebbcf3..a14cdf03 100644 --- a/lib/src/clixon_plugin.c +++ b/lib/src/clixon_plugin.c @@ -66,6 +66,7 @@ #include "clixon_xml.h" #include "clixon_xml_nsctx.h" #include "clixon_yang_module.h" +#include "clixon_validate.h" #include "clixon_plugin.h" /* @@ -1093,13 +1094,14 @@ rpc_callback_delete_all(clicon_handle h) /*! Search RPC callbacks and invoke if XML match with tag * - * @param[in] h clicon handle - * @param[in] xn Sub-tree (under xorig) at child of rpc: . - * @param[out] cbret Return XML (as string in CLIgen buffer), error or OK - * @param[in] arg Domain-speific arg (eg client_entry) + * @param[in] h clicon handle + * @param[in] xn Sub-tree (under xorig) at child of rpc: . + * @param[in] arg Domain-speific arg (eg client_entry) + * @param[out] nr Number of callbacks handled: 0, 1, n (retval = 1) + * @param[out] cbret Return XML (as string in CLIgen buffer), error or OK * @retval -1 Error - * @retval 0 OK, not found handler. - * @retval n OK, handler called + * @retval 0 Failed, error return in cbret + * @retval 1 OK, see nr * @see rpc_callback_register which register a callback function * @note that several callbacks can be registered. They need to cooperate on * return values, ie if one writes cbret, the other needs to handle that by @@ -1108,8 +1110,9 @@ rpc_callback_delete_all(clicon_handle h) int rpc_callback_call(clicon_handle h, cxobj *xe, - cbuf *cbret, - void *arg) + void *arg, + int *nrp, + cbuf *cbret) { int retval = -1; rpc_callback_t *rc; @@ -1119,6 +1122,7 @@ rpc_callback_call(clicon_handle h, int nr = 0; /* How many callbacks */ plugin_module_struct *ms = plugin_module_struct_get(h); plugin_context_t *pc = NULL; + int ret; if (ms == NULL){ clicon_err(OE_PLUGIN, EINVAL, "plugin module not initialized"); @@ -1148,12 +1152,22 @@ rpc_callback_call(clicon_handle h, } rc = NEXTQ(rpc_callback_t *, rc); } while (rc != ms->ms_rpc_callbacks); - retval = nr; /* 0: none found, >0 nr of handlers called */ + if (nr){ + if ((ret = rpc_reply_check(h, name, cbret)) < 0) + goto done; + if (ret == 0) + goto fail; + } + *nrp = nr; + retval = 1; /* 0: none found, >0 nr of handlers called */ done: clicon_debug(1, "%s retval:%d", __FUNCTION__, retval); if (pc) free(pc); return retval; + fail: + retval = 0; + goto done; } /*-------------------------------------------------------------------- diff --git a/lib/src/clixon_validate.c b/lib/src/clixon_validate.c index 743b213e..b8a988fd 100644 --- a/lib/src/clixon_validate.c +++ b/lib/src/clixon_validate.c @@ -69,6 +69,7 @@ #include "clixon_log.h" #include "clixon_yang.h" #include "clixon_xml.h" +#include "clixon_data.h" #include "clixon_netconf_lib.h" #include "clixon_options.h" #include "clixon_xml_nsctx.h" @@ -78,6 +79,7 @@ #include "clixon_yang_module.h" #include "clixon_yang_type.h" #include "clixon_xml_map.h" +#include "clixon_xml_bind.h" #include "clixon_validate.h" /*! Validate xml node of type leafref, ensure the value is one of that path's reference @@ -419,6 +421,67 @@ xml_yang_validate_rpc(clicon_handle h, goto done; } +int +xml_yang_validate_rpc_reply(clicon_handle h, + cxobj *xrpc, + cxobj **xret) +{ + int retval = -1; + yang_stmt *yn=NULL; /* rpc name */ + cxobj *xn; /* rpc name */ + char *rpcprefix; + char *namespace = NULL; + int ret; + + if (strcmp(xml_name(xrpc), "rpc-reply")){ + clicon_err(OE_XML, EINVAL, "Expected RPC"); + goto done; + } + rpcprefix = xml_prefix(xrpc); + if (xml2ns(xrpc, rpcprefix, &namespace) < 0) + goto done; + /* Only accept resolved NETCONF base namespace */ + if (namespace == NULL || strcmp(namespace, NETCONF_BASE_NAMESPACE) != 0){ + if (xret && netconf_unknown_namespace_xml(xret, "protocol", + rpcprefix?rpcprefix:"null", + "No appropriate namespace associated with prefix")< 0) + goto done; + goto fail; + } + xn = NULL; + /* xn is name of rpc, ie */ + while ((xn = xml_child_each(xrpc, xn, CX_ELMNT)) != NULL) { + /* OK and rpc-error are not explicit in ietf-netconf.yang. Why are they hardcoded ? */ + if (strcmp(xml_name(xn), "ok") == 0 || strcmp(xml_name(xn), "rpc-error") == 0){ + continue; + } + if ((yn = xml_spec(xn)) == NULL){ + if (xret && netconf_unknown_element_xml(xret, "application", xml_name(xn), NULL) < 0) + goto done; + goto fail; + } + if ((ret = xml_yang_validate_all(h, xn, xret)) < 0) + goto done; /* error or validation fail */ + if (ret == 0) + goto fail; + if ((ret = xml_yang_validate_add(h, xn, xret)) < 0) + goto done; /* error or validation fail */ + if (ret == 0) + goto fail; + if (xml_default_recurse(xn, 0) < 0) + goto done; + } + // ok: /* pass validation */ + retval = 1; + done: + return retval; + fail: + if (xret && *xret && clixon_xml_attr_copy(xrpc, *xret, "message-id") < 0) + goto done; + retval = 0; + goto done; +} + /*! Check if an xml node is a part of a choice and have >1 siblings * @param[in] xt XML node to be validated * @param[in] yt xt:s yang statement @@ -1405,3 +1468,64 @@ xml_yang_validate_all_top(clicon_handle h, return ret; return 1; } + +/*! Check validity of outgoing RPC + * + * Rewrite return message if errors + * @param[in,out] cbret + * @note Parses cbret which seems one time too many + */ +int +rpc_reply_check(clicon_handle h, + char *rpcname, + cbuf *cbret) +{ + int retval = -1; + cxobj *x = NULL; + cxobj *xret = NULL; + int ret; + yang_stmt *yspec; + + if ((yspec = clicon_dbspec_yang(h)) == NULL){ + clicon_err(OE_YANG, ENOENT, "No yang spec9"); + goto done; + } + /* Validate outgoing RPC */ + if ((ret = clixon_xml_parse_string(cbuf_get(cbret), YB_NONE, NULL, &x, NULL)) < 0) + goto done; + if (xml_child_nr(x) == 0){ + cbuf_reset(cbret); + if (netconf_operation_failed(cbret, "application", + "Internal error: Outgoing reply is empty")< 0) + goto done; + goto fail; + } + if (xml_rootchild(x, 0, &x) < 0) + goto done; + if ((ret = xml_bind_yang_rpc_reply(x, rpcname, yspec, &xret)) < 0) + goto done; + if (ret == 0){ + cbuf_reset(cbret); + if (clicon_xml2cbuf(cbret, xret, 0, 0, -1) < 0) + goto done; + goto fail; + } + if ((ret = xml_yang_validate_rpc_reply(h, x, &xret)) < 0) + goto done; + if (ret == 0){ + cbuf_reset(cbret); + if (clicon_xml2cbuf(cbret, xret, 0, 0, -1) < 0) + goto done; + goto fail; + } + retval = 1; + done: + if (x) + xml_free(x); + if (xret) + xml_free(xret); + return retval; + fail: + retval = 0; + goto done; +} diff --git a/lib/src/clixon_xml_bind.c b/lib/src/clixon_xml_bind.c index c625e93d..5245751b 100644 --- a/lib/src/clixon_xml_bind.c +++ b/lib/src/clixon_xml_bind.c @@ -521,11 +521,11 @@ xml_bind_yang0(cxobj *xt, * @retval -1 Error * The * @code - * if (xml_bind_yang_rpc(x, NULL) < 0) + * if ((ret = xml_bind_yang_rpc(x, NULL, &xerr)) < 0) * err; * @endcode * @see xml_bind_yang For other generic cases - * @see xml_bind_yang_rpc_reply + * @see xml_bind_yang_rpc_reply */ int xml_bind_yang_rpc(cxobj *xrpc, @@ -666,7 +666,7 @@ xml_bind_yang_rpc(cxobj *xrpc, * @retval -1 Error * * @code - * if (xml_bind_yang_rpc_reply(x, "get-config", yspec, name) < 0) + * if ((ret = xml_bind_yang_rpc_reply(x, "get-config", yspec, &xerr)) < 0) * err; * @endcode * @see xml_bind_yang For other generic cases @@ -686,6 +686,7 @@ xml_bind_yang_rpc_reply(cxobj *xrpc, cxobj *xerr1 = NULL; char *opname; cbuf *cberr = NULL; + cxobj *xc; opname = xml_name(xrpc); if (strcmp(opname, "rpc-reply")){ @@ -716,6 +717,13 @@ xml_bind_yang_rpc_reply(cxobj *xrpc, } if (yo != NULL){ xml_spec_set(xrpc, yo); + /* Special case for ok and rpc-error */ + if ((xc = xml_child_i_type(xrpc, 0, CX_ELMNT)) != NULL && + (strcmp(xml_name(xc),"rpc-error") == 0 + || strcmp(xml_name(xc),"ok") == 0 + )){ + goto ok; + } /* Use a temporary xml error tree since it is stringified in the original error on error */ if ((ret = xml_bind_yang(xrpc, YB_PARENT, NULL, &xerr1)) < 0) goto done; @@ -732,6 +740,7 @@ xml_bind_yang_rpc_reply(cxobj *xrpc, goto fail; } } + ok: retval = 1; done: if (cberr) diff --git a/test/test_c++.sh b/test/test_c++.sh index c9be6f2a..ad4725ac 100755 --- a/test/test_c++.sh +++ b/test/test_c++.sh @@ -101,7 +101,7 @@ int example_rpc(clicon_handle h, clicon_err(OE_XML, ENOENT, "No namespace given in rpc %s", xml_name(xe)); goto done; } - cprintf(cbret, ""); + cprintf(cbret, "", NETCONF_BASE_NAMESPACE); if (!xml_child_nr_type(xe, CX_ELMNT)) cprintf(cbret, ""); else