lib/rpc: add RPC allow list

Add an optional allowlist for RPC methods: if the method is not listed,
it is not allowed to be called or visible. This can be used to restrict
accidental mis-configurations, and generally helps locking down the
configuration surface.

Signed-off-by: John Levon <john.levon@nutanix.com>
Change-Id: Ied78fc4b14b60cb94ed0852b92deb6df545cbec4
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/15275
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Aleksey Marchuk <alexeymar@nvidia.com>
Community-CI: Mellanox Build Bot
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>

CHANGELOG.md

@@ -37,6 +37,13 @@ Added a command line switch to disable CPU locks on SPDK startup.
 Added RPCs framework_enable_cpumask_locks and framework_disable_cpumask_locks to enable
 and disable CPU core locks in runtime.
 
+Added --rpcs-allowed command line option. Users can specify a comma-separated list of RPC
+names with this option to restrict allowed RPCs to only that list.
+
+### rpc
+
+Added spdk_rpc_set_allowlist to restrict allowed RPCs to the specified list.
+
 ## v22.09
 
 ### accel

autotest.sh

@@ -162,8 +162,7 @@ if [ $SPDK_RUN_FUNCTIONAL_TEST -eq 1 ]; then
 	run_test "event" test/event/event.sh
 	run_test "thread" test/thread/thread.sh
 	run_test "accel" test/accel/accel.sh
-	# Uncomment this line when ready
-	# run_test "app_cmdline" test/app/cmdline.sh
+	run_test "app_cmdline" test/app/cmdline.sh
 
 	if [ $SPDK_TEST_BLOCKDEV -eq 1 ]; then
 		run_test "blockdev_general" test/bdev/blockdev.sh

include/spdk/event.h

@@ -152,8 +152,13 @@ struct spdk_app_opts {
 	 * Default is `SPDK_DEFAULT_MSG_MEMPOOL_SIZE`.
 	 */
 	size_t msg_mempool_size;
+
+	/*
+	 *  If non-NULL, a string array of allowed RPC methods.
+	 */
+	const char **rpc_allowlist;
 } __attribute__((packed));
-SPDK_STATIC_ASSERT(sizeof(struct spdk_app_opts) == 200, "Incorrect size");
+SPDK_STATIC_ASSERT(sizeof(struct spdk_app_opts) == 208, "Incorrect size");
 
 /**
  * Initialize the default value of opts

include/spdk/rpc.h

@@ -131,6 +131,13 @@ void spdk_rpc_set_state(uint32_t state_mask);
  */
 uint32_t spdk_rpc_get_state(void);
 
+/*
+ * Mark only the given RPC methods as allowed.
+ *
+ * \param rpc_allowlist string array of method names, terminated with a NULL.
+ */
+void spdk_rpc_set_allowlist(const char **rpc_allowlist);
+
 #ifdef __cplusplus
 }
 #endif

lib/event/app.c

@@ -38,6 +38,7 @@ struct spdk_app {
 	bool				json_config_ignore_errors;
 	bool				stopped;
 	const char			*rpc_addr;
+	const char			**rpc_allowlist;
 	int				shm_id;
 	spdk_app_shutdown_cb		shutdown_cb;
 	int				rc;
@@ -123,6 +124,8 @@ static const struct option g_cmdline_options[] = {
 	{"env-context",			required_argument,	NULL, ENV_CONTEXT_OPT_IDX},
 #define DISABLE_CPUMASK_LOCKS_OPT_IDX	267
 	{"disable-cpumask-locks",	no_argument,		NULL, DISABLE_CPUMASK_LOCKS_OPT_IDX},
+#define RPCS_ALLOWED_OPT_IDX	268
+	{"rpcs-allowed",		required_argument,	NULL, RPCS_ALLOWED_OPT_IDX}
 };
 
 static void
@@ -205,6 +208,7 @@ spdk_app_opts_init(struct spdk_app_opts *opts, size_t opts_size)
 	SET_FIELD(delay_subsystem_init, false);
 	SET_FIELD(disable_signal_handlers, false);
 	SET_FIELD(msg_mempool_size, SPDK_DEFAULT_MSG_MEMPOOL_SIZE);
+	SET_FIELD(rpc_allowlist, NULL);
 #undef SET_FIELD
 }
 
@@ -264,6 +268,8 @@ app_start_rpc(int rc, void *arg1)
 		return;
 	}
 
+	spdk_rpc_set_allowlist(g_spdk_app.rpc_allowlist);
+
 	rc = spdk_rpc_initialize(g_spdk_app.rpc_addr);
 	if (rc) {
 		spdk_app_stop(rc);
@@ -459,6 +465,8 @@ bootstrap_fn(void *arg1)
 		if (!g_delay_subsystem_init) {
 			spdk_subsystem_init(app_start_rpc, NULL);
 		} else {
+			spdk_rpc_set_allowlist(g_spdk_app.rpc_allowlist);
+
 			rc = spdk_rpc_initialize(g_spdk_app.rpc_addr);
 			if (rc) {
 				spdk_app_stop(rc);
@@ -507,10 +515,11 @@ app_copy_opts(struct spdk_app_opts *opts, struct spdk_app_opts *opts_user, size_
 	SET_FIELD(base_virtaddr);
 	SET_FIELD(disable_signal_handlers);
 	SET_FIELD(msg_mempool_size);
+	SET_FIELD(rpc_allowlist);
 
 	/* You should not remove this statement, but need to update the assert statement
 	 * if you add a new field, and also add a corresponding SET_FIELD statement */
-	SPDK_STATIC_ASSERT(sizeof(struct spdk_app_opts) == 200, "Incorrect size");
+	SPDK_STATIC_ASSERT(sizeof(struct spdk_app_opts) == 208, "Incorrect size");
 
 #undef SET_FIELD
 }
@@ -662,6 +671,7 @@ spdk_app_start(struct spdk_app_opts *opts_user, spdk_msg_fn start_fn,
 	g_spdk_app.json_config_file = opts->json_config_file;
 	g_spdk_app.json_config_ignore_errors = opts->json_config_ignore_errors;
 	g_spdk_app.rpc_addr = opts->rpc_addr;
+	g_spdk_app.rpc_allowlist = opts->rpc_allowlist;
 	g_spdk_app.shm_id = opts->shm_id;
 	g_spdk_app.shutdown_cb = opts->shutdown_cb;
 	g_spdk_app.rc = 0;
@@ -844,6 +854,7 @@ usage(void (*app_usage)(void))
 	printf("     --base-virtaddr <addr>      the base virtual address for DPDK (default: 0x200000000000)\n");
 	printf("     --num-trace-entries <num>   number of trace entries for each core, must be power of 2, setting 0 to disable trace (default %d)\n",
 	       SPDK_APP_DEFAULT_NUM_TRACE_ENTRIES);
+	printf("     --rpcs-allowed	   comma-separated list of permitted RPCS\n");
 	printf("     --env-context         Opaque context for use of the env implementation\n");
 	spdk_log_usage(stdout, "-L");
 	spdk_trace_mask_usage(stdout, "-e");
@@ -1094,6 +1105,14 @@ spdk_app_parse_args(int argc, char **argv, struct spdk_app_opts *opts,
 		case ENV_CONTEXT_OPT_IDX:
 			opts->env_context = optarg;
 			break;
+		case RPCS_ALLOWED_OPT_IDX:
+			opts->rpc_allowlist = (const char **)spdk_strarray_from_string(optarg, ",");
+			if (opts->rpc_allowlist == NULL) {
+				SPDK_ERRLOG("Invalid --rpcs-allowed argument\n");
+				usage(app_usage);
+				goto out;
+			}
+			break;
 		case VERSION_OPT_IDX:
 			printf(SPDK_VERSION_STRING"\n");
 			retval = SPDK_APP_PARSE_ARGS_HELP;
@@ -1127,6 +1146,8 @@ out:
 		opts->pci_blocked = NULL;
 		free(opts->pci_allowed);
 		opts->pci_allowed = NULL;
+		spdk_strarray_free((char **)opts->rpc_allowlist);
+		opts->rpc_allowlist = NULL;
 	}
 	free(cmdline_short_opts);
 	free(cmdline_options);

lib/rpc/Makefile

@@ -8,7 +8,7 @@ SPDK_ROOT_DIR := $(abspath $(CURDIR)/../..)
 include $(SPDK_ROOT_DIR)/mk/spdk.common.mk
 
 SO_VER := 4
-SO_MINOR := 1
+SO_MINOR := 2
 
 C_SRCS = rpc.c
 LIBNAME = rpc

lib/rpc/rpc.c

@@ -23,6 +23,7 @@ static int g_rpc_lock_fd = -1;
 static struct spdk_jsonrpc_server *g_jsonrpc_server = NULL;
 static uint32_t g_rpc_state;
 static bool g_rpcs_correct = true;
+static char **g_rpcs_allowlist = NULL;
 
 struct spdk_rpc_method {
 	const char *name;
@@ -48,6 +49,25 @@ spdk_rpc_get_state(void)
 	return g_rpc_state;
 }
 
+static bool
+rpc_is_allowed(const char *name)
+{
+	size_t i;
+
+	if (g_rpcs_allowlist == NULL) {
+		return true;
+	}
+
+	for (i = 0; g_rpcs_allowlist[i] != NULL; i++) {
+		if (strcmp(name, g_rpcs_allowlist[i]) == 0) {
+			return true;
+		}
+	}
+
+	return false;
+}
+
+
 static struct spdk_rpc_method *
 _get_rpc_method(const struct spdk_json_val *method)
 {
@@ -55,6 +75,9 @@ _get_rpc_method(const struct spdk_json_val *method)
 
 	SLIST_FOREACH(m, &g_rpc_methods, slist) {
 		if (spdk_json_strequal(method, m->name)) {
+			if (!rpc_is_allowed(m->name)) {
+				return NULL;
+			}
 			return m;
 		}
 	}
@@ -259,6 +282,10 @@ spdk_rpc_is_method_allowed(const char *method, uint32_t state_mask)
 {
 	struct spdk_rpc_method *m;
 
+	if (!rpc_is_allowed(method)) {
+		return -ENOENT;
+	}
+
 	SLIST_FOREACH(m, &g_rpc_methods, slist) {
 		if (strcmp(m->name, method) != 0) {
 			continue;
@@ -289,6 +316,20 @@ spdk_rpc_get_method_state_mask(const char *method, uint32_t *state_mask)
 	return -ENOENT;
 }
 
+void
+spdk_rpc_set_allowlist(const char **rpc_allowlist)
+{
+	spdk_strarray_free(g_rpcs_allowlist);
+
+	if (rpc_allowlist == NULL) {
+		g_rpcs_allowlist = NULL;
+		return;
+	}
+
+	g_rpcs_allowlist = spdk_strarray_dup(rpc_allowlist);
+	assert(g_rpcs_allowlist != NULL);
+}
+
 void
 spdk_rpc_close(void)
 {
@@ -344,6 +385,9 @@ rpc_get_methods(struct spdk_jsonrpc_request *request, const struct spdk_json_val
 	w = spdk_jsonrpc_begin_result(request);
 	spdk_json_write_array_begin(w);
 	SLIST_FOREACH(m, &g_rpc_methods, slist) {
+		if (!rpc_is_allowed(m->name)) {
+			continue;
+		}
 		if (m->is_alias_of != NULL && !req.include_aliases) {
 			continue;
 		}

lib/rpc/spdk_rpc.map

@@ -12,6 +12,7 @@
 	spdk_rpc_get_method_state_mask;
 	spdk_rpc_set_state;
 	spdk_rpc_get_state;
+	spdk_rpc_set_allowlist;
 
 	local: *;
 };

test/app/cmdline.sh

@@ -1,22 +1,28 @@
 #!/usr/bin/env bash
 #  SPDX-License-Identifier: BSD-3-Clause
 #  Copyright (C) 2022 Intel Corporation
+#  Copyright (C) 2022 Nutanix Inc.
 #  All rights reserved.
 #
 
+# Test --rpcs-allowed: only the given RPCs are allowed and reported.
+
 testdir=$(readlink -f $(dirname $0))
 rootdir=$(readlink -f $testdir/../..)
 source $rootdir/test/common/autotest_common.sh
 
 trap 'killprocess $spdk_tgt_pid; exit 1' ERR
 
-# Add an allowlist here...
-$SPDK_BIN_DIR/spdk_tgt &
+$SPDK_BIN_DIR/spdk_tgt --rpcs-allowed spdk_get_version,rpc_get_methods &
 spdk_tgt_pid=$!
 waitforlisten $spdk_tgt_pid
 
-# Do both some positive and negative testing on that allowlist here...
-# You can use the NOT() function to help with the negative test
-# $rootdir/scripts/rpc.py some_rpc
+$rootdir/scripts/rpc.py spdk_get_version
+declare -a methods=($(rpc_cmd rpc_get_methods | jq -rc ".[]"))
+[[ "${methods[0]}" = "spdk_get_version" ]]
+[[ "${methods[1]}" = "rpc_get_methods" ]]
+[[ "${#methods[@]}" = 2 ]]
+
+NOT $rootdir/scripts/rpc.py env_dpdk_get_mem_stats
 
 killprocess $spdk_tgt_pid

test/unit/lib/event/app.c/app_ut.c

@@ -22,6 +22,7 @@ DEFINE_STUB_V(spdk_rpc_register_alias_deprecated, (const char *method, const cha
 DEFINE_STUB_V(spdk_rpc_set_state, (uint32_t state));
 DEFINE_STUB(spdk_rpc_get_state, uint32_t, (void), SPDK_RPC_RUNTIME);
 DEFINE_STUB(spdk_rpc_initialize, int, (const char *listen_addr), 0);
+DEFINE_STUB_V(spdk_rpc_set_allowlist, (const char **rpc_allowlist));
 DEFINE_STUB_V(spdk_rpc_finish, (void));
 DEFINE_STUB_V(spdk_subsystem_init_from_json_config, (const char *json_config_file,
 		const char *rpc_addr,