* 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

@@ -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: `<s><![CDATA[  z > x  & x < y ]]></s>` 
   * 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)

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, "<datastore><name>%s</name><nr>0</nr><size>0</size></datastore>", dbname);
+	cprintf(cb, "<datastore xmlns=\"%s\"><name>%s</name><nr>0</nr><size>0</size></datastore>",
+		CLIXON_LIB_NS, dbname);
     }
     else{
 	if (xml_stats(xt, &nr, &sz) < 0)
 	    goto done;
-	cprintf(cb, "<datastore><name>%s</name><nr>%" PRIu64 "</nr>"
+	cprintf(cb, "<datastore xmlns=\"%s\"><name>%s</name><nr>%" PRIu64 "</nr>"
 		"<size>%zu</size></datastore>",
-		dbname, nr, sz);
+		CLIXON_LIB_NS, dbname, nr, sz);
     }
     retval = 0;
  done:
@@ -1001,7 +1002,7 @@ from_client_stats(clicon_handle h,
     cprintf(cbret, "<rpc-reply xmlns=\"%s\">", NETCONF_BASE_NAMESPACE);
     nr=0;
     xml_stats_global(&nr);
-    cprintf(cbret, "<global><xmlnr>%" PRIu64 "</xmlnr></global>", nr);
+    cprintf(cbret, "<global xmlns=\"%s\"><xmlnr>%" PRIu64 "</xmlnr></global>", 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;

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;
 	}

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 */

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);

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_ */

lib/clixon/clixon_xml.h

@@ -147,7 +147,7 @@ enum yang_bind{
 		  * Ie, xml looks like: <top><config><x>... 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;

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]   h       clicon handle
- * @param[in]  xn      Sub-tree (under xorig) at child of rpc: <rpc><xn></rpc>.
+ * @param[in]   xn      Sub-tree (under xorig) at child of rpc: <rpc><xn></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]  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  0   Failed, error return in cbret
- * @retval  n   OK, <n> handler called 
+ * @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;
 }
 
 /*--------------------------------------------------------------------

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 <rcp><xn/></rpc> */
+    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;
+}

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)

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, "<rpc-reply>");
+    cprintf(cbret, "<rpc-reply xmlns=\"%s\">", NETCONF_BASE_NAMESPACE);
     if (!xml_child_nr_type(xe, CX_ELMNT))
 	      cprintf(cbret, "<ok/>");
     else