From 3de87445ea39b81454e888348131dac43d65d8b6 Mon Sep 17 00:00:00 2001 From: Gil Bregman Date: Wed, 30 Oct 2024 16:15:56 +0200 Subject: [PATCH] Add a CLI command to change host DHCHAP keys. Fixes #924 Signed-off-by: Gil Bregman --- .env | 3 + .github/workflows/build-container.yml | 2 +- control/cli.py | 58 +++++- control/grpc.py | 267 +++++++++++++++++++++----- control/proto/gateway.proto | 10 + control/server.py | 4 + control/state.py | 126 ++++++++++-- tests/ha/demo_test.sh | 54 +++++- tests/test_cli_change_keys.py | 70 +++++++ tests/test_dhchap.py | 55 +++++- tests/test_psk.py | 2 +- 11 files changed, 578 insertions(+), 73 deletions(-) create mode 100644 tests/test_cli_change_keys.py diff --git a/.env b/.env index f1a35446..5b1e4c81 100644 --- a/.env +++ b/.env @@ -98,3 +98,6 @@ DHCHAP_KEY3="DHHC-1:01:eNNXGjidEHHStbUi2Gmpps0JcnofReFfy+NaulguGgt327hz:" DHCHAP_KEY4="DHHC-1:01:c8D8fVPP/wcuxxRCd8mdQQFjOWtjcS2KmspzvkeOEoF6SUm6:" DHCHAP_KEY5="DHHC-1:01:zNZ6nrs5JDIpqbH/ZP1VTAATxNf5i/rH44dci+vvjhsyI2ha:" DHCHAP_KEY6="DHHC-1:01:Bu4tZd7X2oW7XxmVH5tGCdoS30pDX6bZvexHYoudeVlJW9yz:" +DHCHAP_KEY7="DHHC-1:01:JPJkDQ2po2FfLmKYlTF/sJ2HzVO/FKWxgXKE/H6XfL8ogQ1T:" +DHCHAP_KEY8="DHHC-1:01:e0B0vDxKleDzYVtG42xqFvoWZfiufkoywmfRKrETzayRdf1j:" +DHCHAP_KEY9="DHHC-1:01:KD+sfH3/o2bRQoV0ESjBUywQlMnSaYpZISUbVa0k0nsWpNST:" diff --git a/.github/workflows/build-container.yml b/.github/workflows/build-container.yml index c8843ca9..8845fb00 100644 --- a/.github/workflows/build-container.yml +++ b/.github/workflows/build-container.yml @@ -133,7 +133,7 @@ jobs: strategy: fail-fast: false matrix: - test: ["cli", "cli_change_lb", "state", "multi_gateway", "server", "grpc", "omap_lock", "log_files", "nsid", "psk", "dhchap"] + test: ["cli", "cli_change_lb", "cli_change_keys", "state", "multi_gateway", "server", "grpc", "omap_lock", "log_files", "nsid", "psk", "dhchap"] runs-on: ubuntu-latest env: HUGEPAGES: 512 # for multi gateway test, approx 256 per gateway instance diff --git a/control/cli.py b/control/cli.py index d4b580f2..f56d3286 100644 --- a/control/cli.py +++ b/control/cli.py @@ -1167,6 +1167,50 @@ def host_del(self, args): return rc + def host_change_keys(self, args): + """Change host's inband authentication keys.""" + + rc = 0 + out_func, err_func = self.get_output_functions(args) + + if args.host_nqn == "*": + self.cli.parser.error(f"Can't change keys for host NQN '*', please use a real NQN") + + if args.dhchap_ctrlr_key: + if not args.dhchap_key: + self.cli.parser.error(f"DH-HMAC-CHAP controller keys can not be used without DH-HMAC-CHAP keys") + + req = pb2.change_host_keys_req(subsystem_nqn=args.subsystem, host_nqn=args.host_nqn, + dhchap_key=args.dhchap_key, dhchap_ctrlr_key=args.dhchap_ctrlr_key) + try: + ret = self.stub.change_host_keys(req) + except Exception as ex: + errmsg = f"Failure changing keys for host {args.host_nqn} on subsystem {args.subsystem}" + ret = pb2.req_status(status = errno.EINVAL, error_message = f"{errmsg}:\n{ex}") + + if args.format == "text" or args.format == "plain": + if ret.status == 0: + out_func(f"Changing keys for host {args.host_nqn} on subsystem {args.subsystem}: Successful") + else: + err_func(f"{ret.error_message}") + elif args.format == "json" or args.format == "yaml": + ret_str = json_format.MessageToJson( + ret, + indent=4, + including_default_value_fields=True, + preserving_proto_field_name=True) + if args.format == "json": + out_func(f"{ret_str}") + elif args.format == "yaml": + obj = json.loads(ret_str) + out_func(yaml.dump(obj)) + elif args.format == "python": + return ret + else: + assert False + + return ret.status + def host_list(self, args): """List a host for a subsystem.""" @@ -1223,19 +1267,25 @@ def host_list(self, args): ] host_add_args = host_common_args + [ argument("--host-nqn", "-t", help="Host NQN list", nargs="+", required=True), - argument("--psk", help="Hosts PSK key", required=False), - argument("--dhchap-key", help="Host DH-HMAC-CHAP key", required=False), - argument("--dhchap-ctrlr-key", help="Host DH-HMAC-CHAP controller key", required=False), + argument("--psk", "-p", help="Hosts PSK key", required=False), + argument("--dhchap-key", "-k", help="Host DH-HMAC-CHAP key", required=False), + argument("--dhchap-ctrlr-key", "-c", help="Host DH-HMAC-CHAP controller key", required=False), ] host_del_args = host_common_args + [ argument("--host-nqn", "-t", help="Host NQN list", nargs="+", required=True), ] host_list_args = host_common_args + [ ] + host_change_keys_args = host_common_args + [ + argument("--host-nqn", "-t", help="Host NQN", required=True), + argument("--dhchap-key", "-k", help="Host DH-HMAC-CHAP key", required=False), + argument("--dhchap-ctrlr-key", "-c", help="Host DH-HMAC-CHAP controller key", required=False), + ] host_actions = [] host_actions.append({"name" : "add", "args" : host_add_args, "help" : "Add host access to a subsystem"}) host_actions.append({"name" : "del", "args" : host_del_args, "help" : "Remove host access from a subsystem"}) host_actions.append({"name" : "list", "args" : host_list_args, "help" : "List subsystem's host access"}) + host_actions.append({"name" : "change_keys", "args" : host_change_keys_args, "help" : "Change host's inband authentication keys"}) host_choices = get_actions(host_actions) @cli.cmd(host_actions) def host(self, args): @@ -1246,6 +1296,8 @@ def host(self, args): return self.host_del(args) elif args.action == "list": return self.host_list(args) + elif args.action == "change_keys": + return self.host_change_keys(args) if not args.action: self.cli.parser.error(f"missing action for host command (choose from {GatewayClient.host_choices})") diff --git a/control/grpc.py b/control/grpc.py index 5d7b0b46..a01922a0 100644 --- a/control/grpc.py +++ b/control/grpc.py @@ -2208,6 +2208,68 @@ def matching_host_exists(self, context, subsys_nqn, host_nqn) -> bool: return True return False + def _create_dhchap_key_files(self, subsystem_nqn, host_nqn, dhchap_key, dhchap_ctrlr_key, err_prefix): + dhchap_file = None + dhchap_key_name = None + if dhchap_key: + dhchap_file = self.create_host_dhchap_file(subsystem_nqn, host_nqn, dhchap_key) + if not dhchap_file: + errmsg=f"{err_prefix}: Can't write DH-HMAC-CHAP file" + self.logger.error(f"{errmsg}") + return (errno.ENOENT, errmsg, None, None, None, None) + dhchap_key_name = GatewayService.construct_key_name_for_keyring( + subsystem_nqn, host_nqn, GatewayService.DHCHAP_PREFIX) + else: + # If there is no dhchap key we can't have a controller key. so we can return here + self.logger.warning(f"Can't create DH-HMAC-CHAP key files without a key value, ignore") + return (0, "", None, None, None, None) + + dhchap_ctrlr_file = None + dhchap_ctrlr_key_name = None + if dhchap_ctrlr_key: + dhchap_ctrlr_file = self.create_host_dhchap_file(subsystem_nqn, host_nqn, dhchap_ctrlr_key) + if not dhchap_ctrlr_file: + errmsg=f"{err_prefix}: Can't write DH-HMAC-CHAP controller file" + self.logger.error(f"{errmsg}") + if dhchap_file: + self.remove_host_dhchap_file(subsystem_nqn, host_nqn) + return (errno.ENOENT, errmsg, None, None, None, None) + dhchap_ctrlr_key_name = GatewayService.construct_key_name_for_keyring( + subsystem_nqn, host_nqn, GatewayService.DHCHAP_CONTROLLER_PREFIX) + + return (0, "", dhchap_file, dhchap_key_name, dhchap_ctrlr_file, dhchap_ctrlr_key_name) + + def _add_key_to_keyring(self, keytype, filename, keyname): + if not keyname or not filename: + return + keys = rpc_keyring.keyring_get_keys(self.spdk_rpc_client) + old_filename = None + for one_key in keys: + try: + if one_key["name"] == keyname: + old_filename = one_key["path"] + break + except Exception: + pass + + if old_filename: + try: + os.remove(old_filename) + except Exception: + pass + + try: + rpc_keyring.keyring_file_remove_key(self.spdk_rpc_client, keyname) + except Exception: + pass + + try: + ret = rpc_keyring.keyring_file_add_key(self.spdk_rpc_client, keyname, filename) + self.logger.debug(f"keyring_file_add_key {keyname} and file {filename}: {ret}") + self.logger.info(f"Added {keytype} key {keyname} to keyring") + except Exception: + pass + def add_host_safe(self, request, context): """Adds a host to a subsystem.""" @@ -2284,37 +2346,25 @@ def add_host_safe(self, request, context): errmsg=f"{host_failure_prefix}: Can't write PSK file" self.logger.error(f"{errmsg}") return pb2.req_status(status=errno.ENOENT, error_message=errmsg) - psk_key_name = GatewayService.construct_key_name_for_keyring(request.subsystem_nqn, request.host_nqn, GatewayService.PSK_PREFIX) + psk_key_name = GatewayService.construct_key_name_for_keyring( + request.subsystem_nqn, request.host_nqn, GatewayService.PSK_PREFIX) if len(psk_key_name) >= SubsystemHostAuth.MAX_PSK_KEY_NAME_LENGTH: errmsg=f"{host_failure_prefix}: PSK key name {psk_key_name} is too long, max length is {SubsystemHostAuth.MAX_PSK_KEY_NAME_LENGTH}" self.logger.error(f"{errmsg}") return pb2.req_status(status=errno.E2BIG, error_message=errmsg) - dhchap_file = None - dhchap_key_name = None - if request.dhchap_key: - dhchap_file = self.create_host_dhchap_file(request.subsystem_nqn, request.host_nqn, request.dhchap_key) - if not dhchap_file: - errmsg=f"{host_failure_prefix}: Can't write DH-HMAC-CHAP file" - self.logger.error(f"{errmsg}") - if psk_file: - self.remove_host_psk_file(request.subsystem_nqn, request.host_nqn) - return pb2.req_status(status=errno.ENOENT, error_message=errmsg) - dhchap_key_name = GatewayService.construct_key_name_for_keyring(request.subsystem_nqn, request.host_nqn, GatewayService.DHCHAP_PREFIX) - - dhchap_ctrlr_file = None - dhchap_ctrlr_key_name = None - if request.dhchap_ctrlr_key: - dhchap_ctrlr_file = self.create_host_dhchap_file(request.subsystem_nqn, request.host_nqn, request.dhchap_ctrlr_key) - if not dhchap_ctrlr_file: - errmsg=f"{host_failure_prefix}: Can't write DH-HMAC-CHAP controller file" - self.logger.error(f"{errmsg}") - if psk_file: - self.remove_host_psk_file(request.subsystem_nqn, request.host_nqn) - if dhchap_file: - self.remove_host_dhchap_file(request.subsystem_nqn, request.host_nqn) - return pb2.req_status(status=errno.ENOENT, error_message=errmsg) - dhchap_ctrlr_key_name = GatewayService.construct_key_name_for_keyring(request.subsystem_nqn, request.host_nqn, GatewayService.DHCHAP_CONTROLLER_PREFIX) + (key_files_status, + key_file_errmsg, + dhchap_file, + dhchap_key_name, + dhchap_ctrlr_file, + dhchap_ctrlr_key_name) = self._create_dhchap_key_files( + request.subsystem_nqn, request.host_nqn, + request.dhchap_key, request.dhchap_ctrlr_key, host_failure_prefix) + if key_files_status != 0: + if psk_file: + self.remove_host_psk_file(request.subsystem_nqn, request.host_nqn) + return pb2.req_status(status=key_files_status, error_message=key_file_errmsg) omap_lock = self.omap_lock.get_omap_lock_to_use(context) with omap_lock: @@ -2331,30 +2381,9 @@ def add_host_safe(self, request, context): else: # Allow single host access to subsystem self.logger.info( f"Received request to add host {request.host_nqn} to {request.subsystem_nqn}, psk: {request.psk}, dhchap: {request.dhchap_key}, dhchap controller: {request.dhchap_ctrlr_key}, context: {context}{peer_msg}") - if psk_file: - try: - rpc_keyring.keyring_file_remove_key(self.spdk_rpc_client, psk_key_name) - except Exception: - pass - ret = rpc_keyring.keyring_file_add_key(self.spdk_rpc_client, psk_key_name, psk_file) - self.logger.debug(f"keyring_file_add_key {psk_key_name}: {ret}") - self.logger.info(f"Added PSK key {psk_key_name} to keyring") - if dhchap_file: - try: - rpc_keyring.keyring_file_remove_key(self.spdk_rpc_client, dhchap_key_name) - except Exception: - pass - ret = rpc_keyring.keyring_file_add_key(self.spdk_rpc_client, dhchap_key_name, dhchap_file) - self.logger.debug(f"keyring_file_add_key {dhchap_key_name}: {ret}") - self.logger.info(f"Added DH-HMAC-CHAP key {dhchap_key_name} to keyring") - if dhchap_ctrlr_file: - try: - rpc_keyring.keyring_file_remove_key(self.spdk_rpc_client, dhchap_ctrlr_key_name) - except Exception: - pass - ret = rpc_keyring.keyring_file_add_key(self.spdk_rpc_client, dhchap_ctrlr_key_name, dhchap_ctrlr_file) - self.logger.debug(f"keyring_file_add_key {dhchap_ctrlr_key_name}: {ret}") - self.logger.info(f"Added DH-HMAC-CHAP controller key {dhchap_ctrlr_key_name} to keyring") + self._add_key_to_keyring("PSK", psk_file, psk_key_name) + self._add_key_to_keyring("DH-HMAC-CHAP", dhchap_file, dhchap_key_name) + self._add_key_to_keyring("DH-HMAC-CHAP controller", dhchap_ctrlr_file, dhchap_ctrlr_key_name) ret = rpc_nvmf.nvmf_subsystem_add_host( self.spdk_rpc_client, nqn=request.subsystem_nqn, @@ -2530,6 +2559,144 @@ def remove_host_safe(self, request, context): def remove_host(self, request, context=None): return self.execute_grpc_function(self.remove_host_safe, request, context) + def change_host_keys_safe(self, request, context): + """Changes host's inband authentication keys.""" + + peer_msg = self.get_peer_message(context) + failure_prefix=f"Failure changing keys for host {request.host_nqn} on subsystem {request.subsystem_nqn}" + self.logger.info( + f"Received request to change inband authentication keys for host {request.host_nqn} on subsystem {request.subsystem_nqn}, dhchap: {request.dhchap_key}, dhchap controller: {request.dhchap_ctrlr_key}, context: {context}{peer_msg}") + + if request.host_nqn == "*": + errmsg=f"{failure_prefix}: Host NQN can't be '*'" + self.logger.error(f"{errmsg}") + return pb2.req_status(status=errno.EINVAL, error_message=errmsg) + + if not GatewayState.is_key_element_valid(request.host_nqn): + errmsg = f"{failure_prefix}: Invalid host NQN \"{request.host_nqn}\", contains invalid characters" + self.logger.error(f"{errmsg}") + return pb2.req_status(status = errno.EINVAL, error_message = errmsg) + + if not GatewayState.is_key_element_valid(request.subsystem_nqn): + errmsg = f"{failure_prefix}: Invalid subsystem NQN \"{request.subsystem_nqn}\", contains invalid characters" + self.logger.error(f"{errmsg}") + return pb2.req_status(status = errno.EINVAL, error_message = errmsg) + + if self.verify_nqns: + rc = GatewayUtils.is_valid_nqn(request.host_nqn) + if rc[0] != 0: + errmsg = f"{failure_prefix}: {rc[1]}" + self.logger.error(f"{errmsg}") + return pb2.req_status(status = rc[0], error_message = errmsg) + + if GatewayUtils.is_discovery_nqn(request.host_nqn): + errmsg=f"{failure_prefix}: Can't use a discovery NQN as host's" + self.logger.error(f"{errmsg}") + return pb2.req_status(status=errno.EINVAL, error_message=errmsg) + + if request.dhchap_ctrlr_key and not request.dhchap_key: + errmsg=f"{failure_prefix}: DH-HMAC-CHAP controller key can only be used with a DH-HMAC-CHAP key" + self.logger.error(f"{errmsg}") + return pb2.req_status(status=errno.EINVAL, error_message=errmsg) + + host_already_exist = self.matching_host_exists(context, request.subsystem_nqn, request.host_nqn) + if not host_already_exist: + errmsg=f"{failure_prefix}: Can't find host on subsystem" + self.logger.error(f"{errmsg}") + return pb2.req_status(status=errno.EINVAL, error_message=errmsg) + + (key_files_status, + key_file_errmsg, + dhchap_file, + dhchap_key_name, + dhchap_ctrlr_file, + dhchap_ctrlr_key_name) = self._create_dhchap_key_files( + request.subsystem_nqn, request.host_nqn, + request.dhchap_key, request.dhchap_ctrlr_key, failure_prefix) + + if key_files_status != 0: + return pb2.req_status(status=key_files_status, error_message=key_file_errmsg) + + omap_lock = self.omap_lock.get_omap_lock_to_use(context) + with omap_lock: + host_entry = None + host_psk = None + if context: + # notice that the local state might not be up to date in case we're in the middle of update() but as the + # context is not None, we are not in an update(), the omap lock made sure that we got here with an updated local state + state = self.gateway_state.local.get_state() + host_key = GatewayState.build_host_key(request.subsystem_nqn, request.host_nqn) + try: + state_host = state[host_key] + host_entry = json.loads(state_host) + except Exception as ex: + errmsg = f"{failure_prefix}: Can't find entry for host {request.host_nqn} in {request.subsystem_nqn}" + self.logger.error(errmsg) + return pb2.req_status(status=errno.ENOENT, error_message=errmsg) + + try: + host_psk = host_entry["psk"] + except Exception: + pass + + try: + self._add_key_to_keyring("DH-HMAC-CHAP", dhchap_file, dhchap_key_name) + self._add_key_to_keyring("DH-HMAC-CHAP controller", dhchap_ctrlr_file, dhchap_ctrlr_key_name) + ret = rpc_nvmf.nvmf_subsystem_set_keys( + self.spdk_rpc_client, + request.subsystem_nqn, + request.host_nqn, + dhchap_key=dhchap_key_name, + dhchap_ctrlr_key=dhchap_ctrlr_key_name, + ) + except Exception as ex: + self.logger.exception(failure_prefix) + errmsg = f"{failure_prefix}:\n{ex}" + self.logger.error(errmsg) + resp = self.parse_json_exeption(ex) + status = errno.EINVAL + if resp: + status = resp["code"] + errmsg = f"{failure_prefix}: {resp['message']}" + return pb2.req_status(status=status, error_message=errmsg) + + # Just in case SPDK failed with no exception + if not ret: + errmsg = failure_prefix + self.logger.error(errmsg) + return pb2.req_status(status=errno.EINVAL, error_message=errmsg) + + if dhchap_key_name: + self.host_info.add_dhchap_host(request.subsystem_nqn, request.host_nqn) + else: + self.host_info.remove_dhchap_host(request.subsystem_nqn, request.host_nqn) + self.remove_all_host_key_files(request.subsystem_nqn, request.host_nqn) + self.remove_all_host_keys_from_keyring(request.subsystem_nqn, request.host_nqn) + + if context: + assert host_entry, "Host entry is None for non-update call" + # Update gateway state + try: + add_req = pb2.add_host_req(subsystem_nqn=request.subsystem_nqn, + host_nqn=request.host_nqn, + psk=host_psk, + dhchap_key=dhchap_key_name, + dhchap_ctrlr_key=dhchap_ctrlr_key_name) + json_req = json_format.MessageToJson( + add_req, preserving_proto_field_name=True, including_default_value_fields=True) + self.gateway_state.add_host(request.subsystem_nqn, request.host_nqn, json_req) + except Exception as ex: + errmsg = f"Error persisting host changet keys for host {request.host_nqn} in {request.subsystem_nqn}" + self.logger.exception(errmsg) + errmsg = f"{errmsg}:\n{ex}" + return pb2.req_status(status=errno.EINVAL, error_message=errmsg) + + return pb2.req_status(status=0, error_message=os.strerror(0)) + + def change_host_keys(self, request, context=None): + """Changes host's inband authentication keys.""" + return self.execute_grpc_function(self.change_host_keys_safe, request, context) + def list_hosts_safe(self, request, context): """List hosts.""" diff --git a/control/proto/gateway.proto b/control/proto/gateway.proto index 9622b680..45cbed3f 100644 --- a/control/proto/gateway.proto +++ b/control/proto/gateway.proto @@ -72,6 +72,9 @@ service Gateway { // Removes a host from a subsystem rpc remove_host(remove_host_req) returns (req_status) {} + // Changes a host inband authentication keys + rpc change_host_keys(change_host_keys_req) returns (req_status) {} + // List hosts rpc list_hosts(list_hosts_req) returns(hosts_info) {} @@ -206,6 +209,13 @@ message add_host_req { optional string dhchap_ctrlr_key = 5; } +message change_host_keys_req { + string subsystem_nqn = 1; + string host_nqn = 2; + optional string dhchap_key = 3; + optional string dhchap_ctrlr_key = 4; +} + message remove_host_req { string subsystem_nqn = 1; string host_nqn = 2; diff --git a/control/server.py b/control/server.py index 9aac809b..94a24ea6 100644 --- a/control/server.py +++ b/control/server.py @@ -760,3 +760,7 @@ def gateway_rpc_caller(self, requests, is_add_req): else: req = json_format.Parse(val, pb2.namespace_delete_host_req(), ignore_unknown_fields=True) self.gateway_rpc.namespace_delete_host(req) + elif key.startswith(GatewayState.HOST_KEYS_PREFIX): + if is_add_req: + req = json_format.Parse(val, pb2.change_host_keys_req(), ignore_unknown_fields=True) + self.gateway_rpc.change_host_keys(req) diff --git a/control/state.py b/control/state.py index 0683e0d4..483d072f 100644 --- a/control/state.py +++ b/control/state.py @@ -31,10 +31,11 @@ class GatewayState(ABC): NAMESPACE_PREFIX = "namespace" + OMAP_KEY_DELIMITER SUBSYSTEM_PREFIX = "subsystem" + OMAP_KEY_DELIMITER HOST_PREFIX = "host" + OMAP_KEY_DELIMITER + HOST_KEYS_PREFIX = "keys-host" + OMAP_KEY_DELIMITER LISTENER_PREFIX = "listener" + OMAP_KEY_DELIMITER NAMESPACE_QOS_PREFIX = "qos" + OMAP_KEY_DELIMITER NAMESPACE_LB_GROUP_PREFIX = "lbgroup" + OMAP_KEY_DELIMITER - NAMESPACE_HOST_PREFIX = "ns_host" + OMAP_KEY_DELIMITER + NAMESPACE_HOST_PREFIX = "ns-host" + OMAP_KEY_DELIMITER def is_key_element_valid(s: str) -> bool: if type(s) != str: @@ -77,6 +78,12 @@ def build_host_key(subsystem_nqn: str, host_nqn: str) -> str: key += GatewayState.OMAP_KEY_DELIMITER + host_nqn return key + def build_host_keys_key(subsystem_nqn: str, host_nqn: str) -> str: + key = GatewayState.HOST_KEYS_PREFIX + subsystem_nqn + if host_nqn is not None: + key += GatewayState.OMAP_KEY_DELIMITER + host_nqn + return key + def build_partial_listener_key(subsystem_nqn: str, host: str) -> str: key = GatewayState.LISTENER_PREFIX + subsystem_nqn if host: @@ -750,6 +757,39 @@ def namespace_only_lb_group_id_changed(self, old_val, new_val): return (False, None) return (True, new_req.anagrpid) + def host_only_keys_changed(self, old_val, new_val): + old_req = None + new_req = None + try: + old_req = json_format.Parse(old_val, pb2.add_host_req(), ignore_unknown_fields=True ) + except Exception as ex: + self.logger.exception(f"Got exception parsing {old_val}") + return (False, None, None) + try: + new_req = json_format.Parse(new_val, pb2.add_host_req(), ignore_unknown_fields=True) + except Exception as ex: + self.logger.exeption(f"Got exception parsing {new_val}") + return (False, None, None) + if not old_req or not new_req: + self.logger.debug(f"Failed to parse requests, old: {old_val} -> {old_req}, new: {new_val} -> {new_req}") + return (False, None, None) + assert old_req != new_req, f"Something was wrong we shouldn't get identical old and new values ({old_req})" + # Because of Json formatting of empty fields we might get a difference here, so just use the same values for empty + if not old_req.dhchap_key: + old_req.dhchap_key = "" + if not old_req.dhchap_ctrlr_key: + old_req.dhchap_ctrlr_key = "" + if not new_req.dhchap_key: + new_req.dhchap_key = "" + if not new_req.dhchap_ctrlr_key: + new_req.dhchap_ctrlr_key = "" + old_req.dhchap_key = new_req.dhchap_key + old_req.dhchap_ctrlr_key = new_req.dhchap_ctrlr_key + if old_req != new_req: + # Something besides the keys is different + return (False, None, None) + return (True, new_req.dhchap_key, new_req.dhchap_ctrlr_key) + def break_namespace_key(self, ns_key: str): if not ns_key.startswith(GatewayState.NAMESPACE_PREFIX): self.logger.warning(f"Invalid namespace key \"{ns_key}\", can't find key parts") @@ -771,6 +811,26 @@ def break_namespace_key(self, ns_key: str): return (nqn, nsid) + def break_host_key(self, host_key: str): + if not host_key.startswith(GatewayState.HOST_PREFIX): + self.logger.warning(f"Invalid host key \"{host_key}\", can't find key parts") + return (None, None) + key_end = host_key[len(GatewayState.HOST_PREFIX) : ] + key_parts = key_end.split(GatewayState.OMAP_KEY_DELIMITER) + if len(key_parts) != 2: + self.logger.warning(f"Invalid host key \"{host_key}\", can't find key parts") + return (None, None) + if not GatewayUtils.is_valid_nqn(key_parts[0]): + self.logger.warning(f"Invalid subsystem NQN \"{key_parts[0]}\" found for host key \"{host_key}\", can't find key parts") + return (None, None) + subsys_nqn = key_parts[0] + if key_parts[1] != "*" and not GatewayUtils.is_valid_nqn(key_parts[1]): + self.logger.warning(f"Invalid host NQN \"{key_parts[0]}\" found for host key \"{host_key}\", can't find key parts") + return (None, None) + host_nqn = key_parts[1] + + return (subsys_nqn, host_nqn) + def get_str_from_bytes(val): val_str = val.decode() if type(val) == type(b'') else val return val_str @@ -827,19 +887,31 @@ def update(self) -> bool: # Handle namespace changes in which only the load balancing group id was changed only_lb_group_changed = [] + only_keys_changed = [] ns_prefix = GatewayState.build_namespace_key("nqn", None) + host_prefix = GatewayState.build_host_key("nqn", None) for key in changed.keys(): - if not key.startswith(ns_prefix): - continue - try: - (should_process, new_lb_grp_id) = self.namespace_only_lb_group_id_changed(local_state_dict[key], - omap_state_dict[key]) - if should_process: - assert new_lb_grp_id, "Shouldn't get here with en empty lb group id" - self.logger.debug(f"Found {key} where only the load balancing group id has changed. The new group id is {new_lb_grp_id}") - only_lb_group_changed.insert(0, (key, new_lb_grp_id)) - except Exception as ex: - self.logger.warning("Got exception checking namespace for load balancing group id change") + if key.startswith(ns_prefix): + try: + (should_process, new_lb_grp_id) = self.namespace_only_lb_group_id_changed(local_state_dict[key], + omap_state_dict[key]) + if should_process: + assert new_lb_grp_id, "Shouldn't get here with an empty lb group id" + self.logger.debug(f"Found {key} where only the load balancing group id has changed. The new group id is {new_lb_grp_id}") + only_lb_group_changed.insert(0, (key, new_lb_grp_id)) + except Exception as ex: + self.logger.warning("Got exception checking namespace for load balancing group id change") + elif key.startswith(host_prefix): + try: + (should_process, + new_dhchap_key, + new_dhchap_ctrl_key) = self.host_only_keys_changed(local_state_dict[key], omap_state_dict[key]) + if should_process: + assert new_dhchap_key, "Shouldn't get here with an empty dhchap key" + self.logger.debug(f"Found {key} where only the keys have changed. The new dhchap key is {new_dhchap_key}, the new dhchap controller key is {new_dhchap_ctrl_key}") + only_keys_changed.insert(0, (key, new_dhchap_key, new_dhchap_ctrl_key)) + except Exception as ex: + self.logger.warning("Got exception checking host for keys change") for ns_key, new_lb_grp in only_lb_group_changed: ns_nqn = None @@ -860,9 +932,35 @@ def update(self) -> bool: except Exception as ex: self.logger.error(f"Exception formatting change namespace load balancing group request:\n{ex}") - if len(only_lb_group_changed) > 0: + for host_key, new_dhchap_key, new_dhchap_ctrl_key in only_keys_changed: + subsys_nqn = None + host_nqn = None + try: + changed.pop(host_key) + (subsys_nqn, host_nqn) = self.break_host_key(host_key) + except Exception as ex: + self.logger.error(f"Exception removing {host_key} from {changed}:\n{ex}") + if host_nqn == "*": + self.logger.warning(f"Something went wrong, host \"*\" can't have DH-HMAC-CHAP keys, ignore") + continue + if subsys_nqn and host_nqn: + try: + host_keys_key = GatewayState.build_host_keys_key(subsys_nqn, host_nqn) + req = pb2.change_host_keys_req(subsystem_nqn=subsys_nqn, host_nqn=host_nqn, + dhchap_key=new_dhchap_key, + dhchap_ctrlr_key=new_dhchap_ctrl_key) + json_req = json_format.MessageToJson(req, preserving_proto_field_name=True, + including_default_value_fields=True) + added[host_keys_key] = json_req + except Exception as ex: + self.logger.error(f"Exception formatting change host keys request:\n{ex}") + + if len(only_lb_group_changed) > 0 or len(only_keys_changed) > 0: grouped_changed = self._group_by_prefix(changed, prefix_list) - prefix_list += [GatewayState.NAMESPACE_LB_GROUP_PREFIX] + if len(only_lb_group_changed) > 0: + prefix_list += [GatewayState.NAMESPACE_LB_GROUP_PREFIX] + if len(only_keys_changed) > 0: + prefix_list += [GatewayState.HOST_KEYS_PREFIX] grouped_added = self._group_by_prefix(added, prefix_list) # Find OMAP removals diff --git a/tests/ha/demo_test.sh b/tests/ha/demo_test.sh index edf1ec69..dd5feb1d 100755 --- a/tests/ha/demo_test.sh +++ b/tests/ha/demo_test.sh @@ -45,9 +45,11 @@ function demo_test_dhchap() echo -n "${DHCHAP_KEY4}" > /tmp/temp-dhchap/dhchap/${NQN}/key1 echo -n "${DHCHAP_KEY5}" > /tmp/temp-dhchap/dhchap/${NQN}/key2 echo -n "${DHCHAP_KEY6}" > /tmp/temp-dhchap/dhchap/${NQN}/key3 + echo -n "${DHCHAP_KEY7}" > /tmp/temp-dhchap/dhchap/${NQN}/key4 chmod 0600 /tmp/temp-dhchap/dhchap/${NQN}/key1 chmod 0600 /tmp/temp-dhchap/dhchap/${NQN}/key2 chmod 0600 /tmp/temp-dhchap/dhchap/${NQN}/key3 + chmod 0600 /tmp/temp-dhchap/dhchap/${NQN}/key4 make demosecuredhchap OPTS=-T HOSTNQN="${NQN}host" HOSTNQN2="${NQN}host2" HOSTNQN3="${NQN}host3" NVMEOF_IO_PORT2=${port2} NVMEOF_IO_PORT3=${port3} DHCHAPKEY1="${DHCHAP_KEY4}" DHCHAPKEY2="${DHCHAP_KEY5}" DHCHAPKEY3="${DHCHAP_KEY6}" } @@ -318,6 +320,7 @@ function demo_bdevperf_dhchap() make exec SVC=bdevperf OPTS=-T CMD="chmod 0600 ${dhchap_path}/key1" make exec SVC=bdevperf OPTS=-T CMD="chmod 0600 ${dhchap_path}/key2" make exec SVC=bdevperf OPTS=-T CMD="chmod 0600 ${dhchap_path}/key3" + make exec SVC=bdevperf OPTS=-T CMD="chmod 0600 ${dhchap_path}/key4" rm -rf /tmp/temp-dhchap echo "ℹ️ bdevperf add DHCHAP key name key1 to keyring" @@ -326,6 +329,8 @@ function demo_bdevperf_dhchap() make -s exec SVC=bdevperf OPTS=-T CMD="$rpc -v -s $BDEVPERF_SOCKET keyring_file_add_key key2 ${dhchap_path}/key2" echo "ℹ️ bdevperf add DHCHAP controller key name key3 to keyring" make -s exec SVC=bdevperf OPTS=-T CMD="$rpc -v -s $BDEVPERF_SOCKET keyring_file_add_key key3 ${dhchap_path}/key3" + echo "ℹ️ bdevperf add DHCHAP key name key4 to keyring" + make -s exec SVC=bdevperf OPTS=-T CMD="$rpc -v -s $BDEVPERF_SOCKET keyring_file_add_key key4 ${dhchap_path}/key4" echo "ℹ️ bdevperf list keyring" make -s exec SVC=bdevperf OPTS=-T CMD="$rpc -s $BDEVPERF_SOCKET keyring_get_keys" @@ -426,11 +431,54 @@ function demo_bdevperf_dhchap() controllers=`make -s exec SVC=bdevperf OPTS=-T CMD="$rpc -v -s $BDEVPERF_SOCKET bdev_nvme_get_controllers"` [[ "${controllers}" == "[]" ]] + echo "ℹ️ keep keys before change" + dhchap_key_list_pre_change=`make -s exec SVC=nvmeof OPTS=-T CMD="/usr/local/bin/spdk_rpc -s /var/tmp/spdk.sock keyring_get_keys"` + path1_pre=`echo ${dhchap_key_list_pre_change} | jq -r '.[0].path'` + path2_pre=`echo ${dhchap_key_list_pre_change} | jq -r '.[1].path'` + path3_pre=`echo ${dhchap_key_list_pre_change} | jq -r '.[2].path'` + name1_pre=`echo ${dhchap_key_list_pre_change} | jq -r '.[0].name'` + name2_pre=`echo ${dhchap_key_list_pre_change} | jq -r '.[1].name'` + name3_pre=`echo ${dhchap_key_list_pre_change} | jq -r '.[2].name'` + make exec SVC=nvmeof OPTS=-T CMD="test -f ${path1_pre}" + + echo "ℹ️ change the key for host ${NQN}host" + cephnvmf_func host change_keys --subsystem $NQN --host-nqn ${NQN}host --dhchap-key "${DHCHAP_KEY7}" + make exec SVC=nvmeof OPTS=-T CMD="test ! -f ${path1_pre}" + + echo "ℹ️ bdevperf tcp connect ip: $NVMEOF_IP_ADDRESS port: ${NVMEOF_IO_PORT} nqn: ${NQN}host using previous DHCHAP key" + set +e + make exec SVC=bdevperf OPTS=-T CMD="$rpc -v -s $BDEVPERF_SOCKET bdev_nvme_attach_controller -b Nvme4 -t tcp -a $NVMEOF_IP_ADDRESS -s ${NVMEOF_IO_PORT} -f ipv4 -n ${NQN} -q ${NQN}host -l -1 -o 10 --dhchap-key key1" + if [[ $? -eq 0 ]]; then + echo "Connecting using the previous DHCAP key should fail" + exit 1 + fi + set -e + + echo "ℹ️ bdevperf tcp connect ip: $NVMEOF_IP_ADDRESS port: ${NVMEOF_IO_PORT} nqn: ${NQN}host using the new DHCHAP key" + make exec SVC=bdevperf OPTS=-T CMD="$rpc -v -s $BDEVPERF_SOCKET bdev_nvme_attach_controller -b Nvme5 -t tcp -a $NVMEOF_IP_ADDRESS -s ${NVMEOF_IO_PORT} -f ipv4 -n ${NQN} -q ${NQN}host -l -1 -o 10 --dhchap-key key4" + make exec SVC=bdevperf OPTS=-T CMD="$rpc -v -s $BDEVPERF_SOCKET bdev_nvme_detach_controller Nvme5" + echo "ℹ️ verify DHCHAP key files removal" dhchap_key_list=`make -s exec SVC=nvmeof OPTS=-T CMD="/usr/local/bin/spdk_rpc -s /var/tmp/spdk.sock keyring_get_keys"` path1=`echo ${dhchap_key_list} | jq -r '.[0].path'` path2=`echo ${dhchap_key_list} | jq -r '.[1].path'` path3=`echo ${dhchap_key_list} | jq -r '.[2].path'` + name1=`echo ${dhchap_key_list} | jq -r '.[0].name'` + name2=`echo ${dhchap_key_list} | jq -r '.[1].name'` + name3=`echo ${dhchap_key_list} | jq -r '.[2].name'` + [[ "$path1_pre" != "$path1" ]] + [[ "$path1_pre" != "$path2" ]] + [[ "$path1_pre" != "$path3" ]] + [[ "$name1_pre" != "$name1" ]] + [[ "$name1_pre" != "$name2" ]] + [[ "$name1_pre" == "$name3" ]] + [[ "$path2_pre" == "$path1" ]] + [[ "$name2_pre" == "$name1" ]] + [[ "$path3_pre" == "$path2" ]] + [[ "$name3_pre" == "$name2" ]] + [[ "$path3" != "$path1_pre" ]] + [[ "$path3" != "$path2_pre" ]] + [[ "$path3" != "$path3_pre" ]] subsys_dir=`dirname ${path1}` [[ `echo $dhchap_key_list | jq -r '.[0].removed'` == "false" ]] [[ `echo $dhchap_key_list | jq -r '.[1].removed'` == "false" ]] @@ -441,11 +489,11 @@ function demo_bdevperf_dhchap() make exec SVC=nvmeof OPTS=-T CMD="test -f ${path3}" make exec SVC=nvmeof OPTS=-T CMD="test -d ${subsys_dir}" cephnvmf_func host del --subsystem $NQN --host-nqn ${NQN}host2 - make exec SVC=nvmeof OPTS=-T CMD="test -f ${path1}" + make exec SVC=nvmeof OPTS=-T CMD="test ! -f ${path1}" make exec SVC=nvmeof OPTS=-T CMD="test ! -f ${path2}" - make exec SVC=nvmeof OPTS=-T CMD="test ! -f ${path3}" + make exec SVC=nvmeof OPTS=-T CMD="test -f ${path3}" cephnvmf_func subsystem del --subsystem $NQN --force - make exec SVC=nvmeof OPTS=-T CMD="test ! -f ${path1}" + make exec SVC=nvmeof OPTS=-T CMD="test ! -f ${path3}" make exec SVC=nvmeof OPTS=-T CMD="test ! -d ${subsys_dir}" dhchap_key_list=`make -s exec SVC=nvmeof OPTS=-T CMD="/usr/local/bin/spdk_rpc -s /var/tmp/spdk.sock keyring_get_keys"` [[ `echo $dhchap_key_list | jq -r '.[0]'` == "null" ]] diff --git a/tests/test_cli_change_keys.py b/tests/test_cli_change_keys.py new file mode 100644 index 00000000..c08046fb --- /dev/null +++ b/tests/test_cli_change_keys.py @@ -0,0 +1,70 @@ +import pytest +from control.server import GatewayServer +from control.cli import main as cli +from control.cli import main_test as cli_test +from control.cephutils import CephUtils +import spdk.rpc.nvmf as rpc_nvmf +import grpc +from control.proto import gateway_pb2 as pb2 +from control.proto import gateway_pb2_grpc as pb2_grpc +import copy +import time + +image = "mytestdevimage" +pool = "rbd" +subsystem = "nqn.2016-06.io.spdk:cnode1" +anagrpid = "1" +anagrpid2 = "2" +uuid = "9dee1f89-e950-4a2f-b984-244ea73f1851" +uuid2 = "9dee1f89-e950-4a2f-b984-244ea73f1852" +config = "ceph-nvmeof.conf" + +@pytest.fixture(scope="module") +def two_gateways(config): + """Sets up and tears down two Gateways""" + nameA = "GatewayAA" + nameB = "GatewayBB" + sockA = f"spdk_{nameA}.sock" + sockB = f"spdk_{nameB}.sock" + config.config["gateway-logs"]["log_level"] = "debug" + config.config["gateway"]["group"] = "" + addr = config.get("gateway", "addr") + configA = copy.deepcopy(config) + configB = copy.deepcopy(config) + configA.config["gateway"]["name"] = nameA + configA.config["gateway"]["override_hostname"] = nameA + configA.config["spdk"]["rpc_socket_name"] = sockA + configA.config["spdk"]["tgt_cmd_extra_args"] = "-m 0x03" + portA = configA.getint("gateway", "port") + 1 + configA.config["gateway"]["port"] = str(portA) + discPortA = configA.getint("discovery", "port") + 1 + configA.config["discovery"]["port"] = str(discPortA) + configB.config["gateway"]["name"] = nameB + configB.config["gateway"]["override_hostname"] = nameB + configB.config["spdk"]["rpc_socket_name"] = sockB + portB = portA + 1 + discPortB = discPortA + 1 + configB.config["gateway"]["port"] = str(portB) + configB.config["discovery"]["port"] = str(discPortB) + configB.config["spdk"]["tgt_cmd_extra_args"] = "-m 0x0C" + + ceph_utils = CephUtils(config) + with (GatewayServer(configA) as gatewayA, GatewayServer(configB) as gatewayB): + ceph_utils.execute_ceph_monitor_command("{" + f'"prefix":"nvme-gw create", "id": "{nameA}", "pool": "{pool}", "group": ""' + "}") + ceph_utils.execute_ceph_monitor_command("{" + f'"prefix":"nvme-gw create", "id": "{nameB}", "pool": "{pool}", "group": ""' + "}") + gatewayA.serve() + gatewayB.serve() + + channelA = grpc.insecure_channel(f"{addr}:{portA}") + stubA = pb2_grpc.GatewayStub(channelA) + channelB = grpc.insecure_channel(f"{addr}:{portB}") + stubB = pb2_grpc.GatewayStub(channelB) + + yield gatewayA, stubA, gatewayB, stubB + gatewayA.gateway_rpc.gateway_state.delete_state() + gatewayB.gateway_rpc.gateway_state.delete_state() + gatewayA.server.stop(grace=1) + gatewayB.server.stop(grace=1) + +def test_to_be_done(caplog, two_gateways): + pass diff --git a/tests/test_dhchap.py b/tests/test_dhchap.py index 0dd0138a..faebe46e 100644 --- a/tests/test_dhchap.py +++ b/tests/test_dhchap.py @@ -115,7 +115,7 @@ def test_create_secure_no_key(caplog, gateway): rc = int(str(sysex)) pass assert rc == 2 - assert f"error: argument --dhchap-key: expected one argument" in caplog.text + assert f"error: argument --dhchap-key/-k: expected one argument" in caplog.text def test_dhchap_controller_key(caplog, gateway): caplog.clear() @@ -187,3 +187,56 @@ def test_list_listeners(caplog, gateway): else: assert False assert found == 2 + +def test_add_key_to_host(caplog, gateway): + caplog.clear() + found = False + hosts = cli_test(["host", "list", "--subsystem", subsystem]) + for h in hosts.hosts: + if h.nqn == hostnqn7: + found = True + assert not h.use_dhchap + break + assert found + caplog.clear() + cli(["host", "change_keys", "--subsystem", subsystem, "--host-nqn", hostnqn7, "--dhchap-key", hostdhchap6]) + assert f"Changing keys for host {hostnqn7} on subsystem {subsystem}: Successful" in caplog.text + caplog.clear() + found = False + hosts = cli_test(["host", "list", "--subsystem", subsystem]) + for h in hosts.hosts: + if h.nqn == hostnqn7: + found = True + assert h.use_dhchap + break + assert found + +def change_key_to_all_hosts(caplog, gateway): + caplog.clear() + rc = 0 + try: + cli(["host", "change_keys", "--subsystem", subsystem, "--host-nqn", "*", "--dhchap-key", hostdhchap1]) + except SystemExit as sysex: + rc = int(str(sysex)) + pass + assert rc == 2 + assert f"error: Can't change keys for host NQN '*', please use a real NQN" in caplog.text + +def change_key_just_controller(caplog, gateway): + caplog.clear() + rc = 0 + try: + cli(["host", "change_keys", "--subsystem", subsystem, "--host-nqn", hostnqn7, "--dhchap-ctrlr-key", hostdhchap1]) + except SystemExit as sysex: + rc = int(str(sysex)) + pass + assert rc == 2 + assert f"error: DH-HMAC-CHAP controller keys can not be used without DH-HMAC-CHAP keys" in caplog.text + +def change_key_for_host(caplog, gateway): + caplog.clear() + cli(["host", "change_keys", "--subsystem", subsystem, "--host-nqn", hostnqn7, "--dhchap-key", hostdhchap1]) + assert f"Changing keys for host {hostnqn7} on subsystem {subsystem}: Successful" in caplog.text + caplog.clear() + cli(["host", "change_keys", "--subsystem", subsystem, "--host-nqn", hostnqn7, "--dhchap-key", hostdhchap1, "--dhchap-ctrlr-key", hostdhchap2]) + assert f"Changing keys for host {hostnqn7} on subsystem {subsystem}: Successful" in caplog.text diff --git a/tests/test_psk.py b/tests/test_psk.py index f6ff0996..fc8ed64f 100644 --- a/tests/test_psk.py +++ b/tests/test_psk.py @@ -134,7 +134,7 @@ def test_create_secure_no_key(caplog, gateway): rc = int(str(sysex)) pass assert rc == 2 - assert f"error: argument --psk: expected one argument" in caplog.text + assert f"error: argument --psk/-p: expected one argument" in caplog.text def test_list_psk_hosts(caplog, gateway): caplog.clear()