Skip to content

Commit

Permalink
Check subsystem and host NQN validity before passing them to SPDK.
Browse files Browse the repository at this point in the history 
Fixes #364

Signed-off-by: Gil Bregman <gbregman@il.ibm.com>
  • Loading branch information
@gbregman
gbregman committed Jan 18, 2024
1 parent c97b09b commit e285604
Show file tree
Hide file tree
Showing 2 changed files with 153 additions and 24 deletions.
140 changes: 120 additions & 20 deletions control/grpc.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,99 @@ def __init__(self, config, gateway_state, omap_lock, spdk_rpc_client) -> None:
self.gateway_group = self.config.get("gateway", "group")
self._init_cluster_context()

def is_valid_uuid(uuid_val) -> bool:
UUID_STRING_LENGTH = len(str(uuid.uuid4()))

if len(uuid_val) != UUID_STRING_LENGTH:
return False

uuid_parts = uuid_val.split("-")
if len(uuid_parts) != 5:
return False
if len(uuid_parts[0]) != 8:
return False
if len(uuid_parts[1]) != 4:
return False
if len(uuid_parts[2]) != 4:
return False
if len(uuid_parts[3]) != 4:
return False
if len(uuid_parts[4]) != 12:
return False

for u in uuid_parts:
try:
n = int(u, 16)
except ValueError:
return False

return True

def is_valid_nqn(nqn):
NQN_MIN_LENGTH = 11
NQN_MAX_LENGTH = 223
NQN_PREFIX = "nqn."
UUID_STRING_LENGTH = len(str(uuid.uuid4()))
NQN_UUID_PREFIX = "nqn.2014-08.org.nvmexpress:uuid:"
NQN_UUID_PREFIX_LENGTH = len(NQN_UUID_PREFIX)

if type(nqn) != str:
return pb2.req_status(status=errno.EINVAL, error_message=f"Invalid type {type(nqn)} for NQN, must be a string")

if not nqn.isascii():
return pb2.req_status(status=errno.EINVAL, error_message=f"Invalid NQN \"{nqn}\", must be an ASCII string")

if len(nqn) < NQN_MIN_LENGTH:
return pb2.req_status(status=errno.EINVAL, error_message=f"NQN \"{nqn}\" is too short, minimal length is {NQN_MIN_LENGTH}")

if len(nqn) > NQN_MAX_LENGTH:
return pb2.req_status(status=errno.EINVAL, error_message=f"NQN \"{nqn}\" is too long, maximal length is {NQN_MAX_LENGTH}")
if GatewayService.is_discovery_nqn(nqn):
# The NQN is technically valid but we will probably reject it later as being a discovery one
return pb2.req_status(status=0, error_message=os.strerror(0))

if nqn.startswith(NQN_UUID_PREFIX):
if len(nqn) != NQN_UUID_PREFIX_LENGTH + UUID_STRING_LENGTH:
return pb2.req_status(status=errno.EINVAL, error_message=f"Invalid NQN \"{nqn}\": UUID is not the correct length")
uuid_part = nqn[NQN_UUID_PREFIX_LENGTH : ]
if not GatewayService.is_valid_uuid(uuid_part):
return pb2.req_status(status=errno.EINVAL, error_message=f"Invalid NQN \"{nqn}\": UUID is not formatted correctly")
return pb2.req_status(status=0, error_message=os.strerror(0))

if not nqn.startswith(NQN_PREFIX):
return pb2.req_status(status=errno.EINVAL, error_message=f"Invalid NQN \"{nqn}\", doesn't start with \"{NQN_PREFIX}\"")

nqn_no_prefix = nqn[len(NQN_PREFIX) : ]
date_part = nqn_no_prefix[ : 8]
rev_domain_part = nqn_no_prefix[8 : ]
if not date_part.endswith("."):
return pb2.req_status(status=errno.EINVAL, error_message=f"Invalid NQN \"{nqn}\": invalid date code")
date_part = date_part[ : -1]
try:
year_part, month_part = date_part.split("-")
if len(year_part) != 4 or len(month_part) != 2:
return pb2.req_status(status=errno.EINVAL, error_message=f"Invalid NQN \"{nqn}\": invalid date code")
n = int(year_part)
n = int(month_part)
except ValueError:
return pb2.req_status(status=errno.EINVAL, error_message=f"Invalid NQN \"{nqn}\": invalid date code")

try:
rev_domain_part, user_part = rev_domain_part.split(":")
except ValueError:
return pb2.req_status(status=errno.EINVAL, error_message=f"Invalid NQN \"{nqn}\": must contain a user specified name starting with a \":\"")

if not user_part:
return pb2.req_status(status=errno.EINVAL, error_message=f"Invalid NQN \"{nqn}\": must contain a user specified name starting with a \":\"")

########### need to verify reverse domain part ####################
return pb2.req_status(status=0, error_message=os.strerror(0))

def is_valid_host_nqn(nqn):
if nqn == "*":
return pb2.req_status(status=0, error_message=os.strerror(0))
return GatewayService.is_valid_nqn(nqn)

def parse_json_exeption(self, ex):
if type(ex) != JSONRPCException:
return None
Expand Down Expand Up @@ -299,7 +392,7 @@ def delete_bdev(self, bdev_name):

return pb2.req_status(status=0, error_message=os.strerror(0))

def is_discovery_nqn(self, nqn) -> bool:
def is_discovery_nqn(nqn) -> bool:
return nqn == GatewayConfig.DISCOVERY_NQN

def subsystem_already_exists(self, context, nqn) -> bool:
Expand Down Expand Up @@ -344,24 +437,26 @@ def create_subsystem_safe(self, request, context):
f"Received request to create subsystem {request.subsystem_nqn}, enable_ha: {request.enable_ha}, ana reporting: {request.ana_reporting}, context: {context}")

errmsg = ""
if self.is_discovery_nqn(request.subsystem_nqn):
rc = GatewayService.is_valid_nqn(request.subsystem_nqn)
if rc.status != 0:
errmsg = f"{create_subsystem_error_prefix}: {rc.error_message}"
self.logger.error(f"{errmsg}")
return pb2.req_status(status = rc.status, error_message = errmsg)
if GatewayService.is_discovery_nqn(request.subsystem_nqn):
errmsg = f"{create_subsystem_error_prefix}: Can't create a discovery subsystem"
ret = pb2.req_status(status = errno.EINVAL, error_message = errmsg)
self.logger.error(f"{errmsg}")
return ret
return pb2.req_status(status = errno.EINVAL, error_message = errmsg)
if request.enable_ha and not request.ana_reporting:
errmsg = f"{create_subsystem_error_prefix}: HA is enabled but ANA reporting is disabled"
ret = pb2.req_status(status = errno.EINVAL, error_message = errmsg)
self.logger.error(f"{errmsg}")
return ret
return pb2.req_status(status = errno.EINVAL, error_message = errmsg)

min_cntlid = self.config.getint_with_default("gateway", "min_controller_id", 1)
max_cntlid = self.config.getint_with_default("gateway", "max_controller_id", 65519)
if min_cntlid > max_cntlid:
errmsg = f"{create_subsystem_error_prefix}: Min controller id {min_cntlid} is bigger than max controller id {max_cntlid}"
ret = pb2.req_status(status = errno.EINVAL, error_message = errmsg)
self.logger.error(f"{errmsg}")
return ret
return pb2.req_status(status = errno.EINVAL, error_message = errmsg)

if not request.serial_number:
random.seed()
Expand Down Expand Up @@ -509,11 +604,10 @@ def delete_subsystem(self, request, context=None):
delete_subsystem_error_prefix = f"Failure deleting subsystem {request.subsystem_nqn}"
self.logger.info(f"Received request to delete subsystem {request.subsystem_nqn}, context: {context}")

if self.is_discovery_nqn(request.subsystem_nqn):
if GatewayService.is_discovery_nqn(request.subsystem_nqn):
errmsg = f"{delete_subsystem_error_prefix}: Can't delete a discovery subsystem"
ret = pb2.req_status(status = errno.EINVAL, error_message = errmsg)
self.logger.error(f"{errmsg}")
return ret
return pb2.req_status(status = errno.EINVAL, error_message = errmsg)

ns_list = []
if context:
Expand Down Expand Up @@ -560,7 +654,7 @@ def create_namespace(self, subsystem_nqn, bdev_name, nsid, anagrpid, uuid, conte
self.logger.error(errmsg)
return pb2.nsid_status(status=errno.EINVAL, error_message=errmsg)

if self.is_discovery_nqn(subsystem_nqn):
if GatewayService.is_discovery_nqn(subsystem_nqn):
errmsg = f"{add_namespace_error_prefix}: Can't add namespaces to a discovery subsystem"
self.logger.error(errmsg)
return pb2.nsid_status(status=errno.EINVAL, error_message=errmsg)
Expand Down Expand Up @@ -808,7 +902,7 @@ def remove_namespace(self, subsystem_nqn, nsid, context):
namespace_failure_prefix = f"Failure removing namespace {nsid} from {subsystem_nqn}"
self.logger.info(f"Received request to remove namespace {nsid} from {subsystem_nqn}")

if self.is_discovery_nqn(subsystem_nqn):
if GatewayService.is_discovery_nqn(subsystem_nqn):
errmsg=f"{namespace_failure_prefix}: Can't remove a namespace from a discovery subsystem"
self.logger.error(f"{errmsg}")
return pb2.req_status(status=errno.EINVAL, error_message=errmsg)
Expand Down Expand Up @@ -1261,15 +1355,21 @@ def add_host_safe(self, request, context):
all_host_failure_prefix=f"Failure allowing open host access to {request.subsystem_nqn}"
host_failure_prefix=f"Failure adding host {request.host_nqn} to {request.subsystem_nqn}"

if self.is_discovery_nqn(request.subsystem_nqn):
rc = GatewayService.is_valid_host_nqn(request.host_nqn)
if rc.status != 0:
errmsg = f"{host_failure_prefix}: {rc.error_message}"
self.logger.error(f"{errmsg}")
return pb2.req_status(status = rc.status, error_message = errmsg)

if GatewayService.is_discovery_nqn(request.subsystem_nqn):
if request.host_nqn == "*":
errmsg=f"{all_host_failure_prefix}: Can't allow host access to a discovery subsystem"
else:
errmsg=f"{host_failure_prefix}: Can't add host to a discovery subsystem"
self.logger.error(f"{errmsg}")
return pb2.req_status(status=errno.EINVAL, error_message=errmsg)

if self.is_discovery_nqn(request.host_nqn):
if GatewayService.is_discovery_nqn(request.host_nqn):
errmsg=f"{host_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)
Expand Down Expand Up @@ -1367,15 +1467,15 @@ def remove_host_safe(self, request, context):
all_host_failure_prefix=f"Failure disabling open host access to {request.subsystem_nqn}"
host_failure_prefix=f"Failure removing host {request.host_nqn} access from {request.subsystem_nqn}"

if self.is_discovery_nqn(request.subsystem_nqn):
if GatewayService.is_discovery_nqn(request.subsystem_nqn):
if request.host_nqn == "*":
errmsg=f"{all_host_failure_prefix}: Can't disable open host access to a discovery subsystem"
else:
errmsg=f"{host_failure_prefix}: Can't remove host access from a discovery subsystem"
self.logger.error(f"{errmsg}")
return pb2.req_status(status=errno.EINVAL, error_message=errmsg)

if self.is_discovery_nqn(request.host_nqn):
if GatewayService.is_discovery_nqn(request.host_nqn):
if request.host_nqn == "*":
errmsg=f"{all_host_failure_prefix}: Can't use a discovery NQN as host's"
else:
Expand Down Expand Up @@ -1656,7 +1756,7 @@ def create_listener_safe(self, request, context):
f" {trtype} {adrfam} listener for {request.nqn} at"
f" {traddr}:{request.trsvcid}, auto HA state: {auto_ha_state}, context: {context}")

if self.is_discovery_nqn(request.nqn):
if GatewayService.is_discovery_nqn(request.nqn):
errmsg=f"{create_listener_error_prefix}: Can't create a listener for a discovery subsystem"
self.logger.error(f"{errmsg}")
return pb2.req_status(status=errno.EINVAL, error_message=errmsg)
Expand Down Expand Up @@ -1812,7 +1912,7 @@ def delete_listener_safe(self, request, context):
f" {trtype} listener for {request.nqn} at"
f" {traddr}:{request.trsvcid}, context: {context}")

if self.is_discovery_nqn(request.nqn):
if GatewayService.is_discovery_nqn(request.nqn):
errmsg=f"{delete_listener_error_prefix}: Can't delete a listener from a discovery subsystem"
self.logger.error(errmsg)
return pb2.req_status(status=errno.EINVAL, error_message=errmsg)
Expand Down Expand Up @@ -1912,7 +2012,7 @@ def list_subsystems_safe(self, request, context):
if request.subsystem_nqn:
self.logger.info(f"Received request to list subsystem {request.subsystem_nqn}, context: {context}")
else:
self.logger.info(f"Received request to list subsystems{ser_msg}, context: {context}")
self.logger.info(f"Received request to list the subsystem{ser_msg}, context: {context}")

subsystems = []
try:
Expand Down
37 changes: 33 additions & 4 deletions tests/test_cli.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,31 @@ def test_get_gateway_info(self, caplog, gateway):

class TestCreate:
def test_create_subsystem(self, caplog, gateway):
caplog.clear()
cli(["subsystem", "add", "--subsystem", "nqn.2016"])
assert f'NQN "nqn.2016" is too short, minimal length is 11' in caplog.text
caplog.clear()
cli(["subsystem", "add", "--subsystem",
"nqn.2016-06XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"])
assert f"is too long, maximal length is 223" in caplog.text
caplog.clear()
cli(["subsystem", "add", "--subsystem", "nqn.2014-08.org.nvmexpress:uuid:0"])
assert f"UUID is not the correct length" in caplog.text
caplog.clear()
cli(["subsystem", "add", "--subsystem", "nqn.2014-08.org.nvmexpress:uuid:9e9134-3cb431-4f3e-91eb-a13cefaabebf"])
assert f"UUID is not formatted correctly" in caplog.text
caplog.clear()
cli(["subsystem", "add", "--subsystem", "qqn.2016-06.io.spdk:cnode1"])
assert f"doesn't start with" in caplog.text
caplog.clear()
cli(["subsystem", "add", "--subsystem", "nqn.016-206.io.spdk:cnode1"])
assert f"invalid date code" in caplog.text
caplog.clear()
cli(["subsystem", "add", "--subsystem", "nqn.2X16-06.io.spdk:cnode1"])
assert f"invalid date code" in caplog.text
caplog.clear()
cli(["subsystem", "add", "--subsystem", "nqn.2016-06.io.spdk:"])
assert f"must contain a user specified name starting with" in caplog.text
caplog.clear()
cli(["subsystem", "add", "--subsystem", subsystem])
assert f"create_subsystem {subsystem}: True" in caplog.text
Expand Down Expand Up @@ -162,10 +187,6 @@ def test_create_subsystem(self, caplog, gateway):
assert f"Failure listing subsystems: No such device" in caplog.text
assert f'"nqn": "JUNK"' in caplog.text
caplog.clear()
cli(["subsystem", "add", "--subsystem", "JUNK"])
assert f"Failure creating subsystem JUNK: Unable to create subsystem JUNK" in caplog.text
assert f'"nqn": "JUNK"' in caplog.text
caplog.clear()
subs_list = cli_test(["--format", "text", "subsystem", "list"])
assert subs_list != None
assert subs_list.status == 0
Expand Down Expand Up @@ -407,6 +428,14 @@ def test_add_host(self, caplog, host):
else:
assert f"Adding host {host} to {subsystem}: Successful" in caplog.text

def test_add_host_invalid_nqn(self, caplog):
caplog.clear()
cli(["host", "add", "--subsystem", subsystem, "--host", "nqn.2016"])
assert f'NQN "nqn.2016" is too short, minimal length is 11' in caplog.text
caplog.clear()
cli(["host", "add", "--subsystem", subsystem, "--host", "nqn.2X16-06.io.spdk:host1"])
assert f"invalid date code" in caplog.text

@pytest.mark.parametrize("listener", listener_list)
def test_create_listener(self, caplog, listener, gateway):
caplog.clear()
Expand Down

0 comments on commit e285604

Please sign in to comment.