From 6656678cdd66dd07ac421c526fe6f68dbe1092cd Mon Sep 17 00:00:00 2001 From: ruffsl Date: Thu, 30 Apr 2020 22:13:01 -0700 Subject: [PATCH 1/8] Update GeneratePolicyVerb for enclaves Signed-off-by: ruffsl --- sros2/sros2/verb/generate_policy.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/sros2/sros2/verb/generate_policy.py b/sros2/sros2/verb/generate_policy.py index 80811fde..dc71643e 100644 --- a/sros2/sros2/verb/generate_policy.py +++ b/sros2/sros2/verb/generate_policy.py @@ -43,7 +43,7 @@ def FilesCompleter(*, allowednames, directories): _HIDDEN_NODE_PREFIX = '_' -_NodeName = namedtuple('NodeName', ('node', 'ns', 'fqn')) +_NodeName = namedtuple('NodeName', ('node', 'ns', 'fqn', 'path')) _TopicInfo = namedtuple('Topic', ('fqn', 'type')) @@ -73,7 +73,15 @@ def get_policy(self, policy_file_path): return policy def get_profile(self, policy, node_name): - profile = policy.find( + enclave = policy.find( + path='enclaves/enclave[@path="{path}"]'.format( + enclave=node_name.path)) + if enclave is None: + enclave = etree.Element('enclave') + enclave.attrib['path'] = node_name.path + enclaves = policy.find('enclaves') + enclaves.append(enclave) + profile = enclave.find( path='profiles/profile[@ns="{ns}"][@node="{node}"]'.format( ns=node_name.ns, node=node_name.node)) @@ -147,13 +155,14 @@ def main(self, *, args): def _get_node_names(*, node, include_hidden_nodes=False): - node_names_and_namespaces = node.get_node_names_and_namespaces() + node_names_and_namespaces_with_enclaves = node.get_node_names_and_namespaces_with_enclaves() return [ _NodeName( node=t[0], ns=t[1], - fqn=t[1] + ('' if t[1].endswith('/') else '/') + t[0]) - for t in node_names_and_namespaces + fqn=t[1] + ('' if t[1].endswith('/') else '/') + t[0], + path=t[2]) + for t in node_names_and_namespaces_with_enclaves if ( include_hidden_nodes or (t[0] and not t[0].startswith(_HIDDEN_NODE_PREFIX)) From 0d3accfb72151f667528f293e40841ec580d0af9 Mon Sep 17 00:00:00 2001 From: ruffsl Date: Thu, 30 Apr 2020 22:13:34 -0700 Subject: [PATCH 2/8] Reactivate generate_policy verb Signed-off-by: ruffsl --- sros2/setup.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/sros2/setup.py b/sros2/setup.py index 98f0746d..e2838408 100644 --- a/sros2/setup.py +++ b/sros2/setup.py @@ -65,10 +65,7 @@ def package_files(directory): 'create_permission = sros2.verb.create_permission' ':CreatePermissionVerb', 'generate_artifacts = sros2.verb.generate_artifacts:GenerateArtifactsVerb', - # TODO(ivanpauno): Reactivate this after having a way to introspect - # enclave names in rclpy. - # Related with https://github.com/ros2/rclpy/issues/529. - # 'generate_policy = sros2.verb.generate_policy:GeneratePolicyVerb', + 'generate_policy = sros2.verb.generate_policy:GeneratePolicyVerb', 'list_keys = sros2.verb.list_keys:ListKeysVerb', ], }, From a936ad90daf4a46f73650ae6474e8137f65d3e0a Mon Sep 17 00:00:00 2001 From: ruffsl Date: Thu, 30 Apr 2020 22:17:20 -0700 Subject: [PATCH 3/8] Reactivate generate_policy test Signed-off-by: ruffsl --- .../sros2/commands/security/verbs/test_generate_policy.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/sros2/test/sros2/commands/security/verbs/test_generate_policy.py b/sros2/test/sros2/commands/security/verbs/test_generate_policy.py index c21ab264..56d58020 100644 --- a/sros2/test/sros2/commands/security/verbs/test_generate_policy.py +++ b/sros2/test/sros2/commands/security/verbs/test_generate_policy.py @@ -24,8 +24,6 @@ from test_msgs.srv import Empty -# TODO(ivanpauno): reactivate this test after updating generate policy -@pytest.mark.skip(reason='temporarily deactivated') def test_generate_policy_topics(): with tempfile.TemporaryDirectory() as tmpdir: # Create a test-specific context so that generate_policy can still init @@ -67,8 +65,6 @@ def test_generate_policy_topics(): assert len([t for t in topics if t.text == 'test_generate_policy_topics_pub']) == 0 -# TODO(ivanpauno): reactivate this test after updating generate policy -@pytest.mark.skip(reason='temporarily deactivated') def test_generate_policy_services(): with tempfile.TemporaryDirectory() as tmpdir: # Create a test-specific context so that generate_policy can still init @@ -110,8 +106,6 @@ def test_generate_policy_services(): assert len([s for s in services if s.text == 'test_generate_policy_services_server']) == 0 -# TODO(ivanpauno): reactivate this test after updating generate policy -@pytest.mark.skip(reason='temporarily deactivated') # TODO(jacobperron): On Windows, this test is flakey due to nodes left-over from tests in # other packages. # See: https://github.com/ros2/sros2/issues/143 From 46ff9539047faf997e3dbe4c144d33abd8a52591 Mon Sep 17 00:00:00 2001 From: ruffsl Date: Thu, 30 Apr 2020 22:33:37 -0700 Subject: [PATCH 4/8] Update generate_policy tests Signed-off-by: ruffsl --- .../sros2/commands/security/verbs/test_generate_policy.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sros2/test/sros2/commands/security/verbs/test_generate_policy.py b/sros2/test/sros2/commands/security/verbs/test_generate_policy.py index 56d58020..60cbe128 100644 --- a/sros2/test/sros2/commands/security/verbs/test_generate_policy.py +++ b/sros2/test/sros2/commands/security/verbs/test_generate_policy.py @@ -47,7 +47,7 @@ def test_generate_policy_topics(): # Load the policy and pull out the allowed publications and subscriptions policy = load_policy(os.path.join(tmpdir, 'test-policy.xml')) profile = policy.find( - path='profiles/profile[@ns="/"][@node="test_generate_policy_topics_node"]') + path='enclaves/enclave[@path="/"]/profiles/profile[@ns="/"][@node="test_generate_policy_topics_node"]') assert profile is not None topics_publish_allowed = profile.find(path='topics[@publish="ALLOW"]') assert topics_publish_allowed is not None @@ -88,7 +88,7 @@ def test_generate_policy_services(): # Load the policy and pull out allowed replies and requests policy = load_policy(os.path.join(tmpdir, 'test-policy.xml')) profile = policy.find( - path='profiles/profile[@ns="/"][@node="test_generate_policy_services_node"]') + path='enclaves/enclave[@path="/"]/profiles/profile[@ns="/"][@node="test_generate_policy_services_node"]') assert profile is not None service_reply_allowed = profile.find(path='services[@reply="ALLOW"]') assert service_reply_allowed is not None From bc7a5ade6999bba68b3eeda3c1ebca1e42718615 Mon Sep 17 00:00:00 2001 From: ruffsl Date: Thu, 30 Apr 2020 23:38:30 -0700 Subject: [PATCH 5/8] Fix missing element Signed-off-by: ruffsl --- sros2/sros2/verb/generate_policy.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/sros2/sros2/verb/generate_policy.py b/sros2/sros2/verb/generate_policy.py index dc71643e..17a6b740 100644 --- a/sros2/sros2/verb/generate_policy.py +++ b/sros2/sros2/verb/generate_policy.py @@ -66,19 +66,21 @@ def get_policy(self, policy_file_path): if os.path.isfile(policy_file_path): return load_policy(policy_file_path) else: - profiles = etree.Element('profiles') + enclaves = etree.Element('enclaves') policy = etree.Element('policy') policy.attrib['version'] = POLICY_VERSION - policy.append(profiles) + policy.append(enclaves) return policy def get_profile(self, policy, node_name): enclave = policy.find( path='enclaves/enclave[@path="{path}"]'.format( - enclave=node_name.path)) + path=node_name.path)) if enclave is None: enclave = etree.Element('enclave') enclave.attrib['path'] = node_name.path + profiles = etree.Element('profiles') + enclave.append(profiles) enclaves = policy.find('enclaves') enclaves.append(enclave) profile = enclave.find( @@ -89,7 +91,7 @@ def get_profile(self, policy, node_name): profile = etree.Element('profile') profile.attrib['ns'] = node_name.ns profile.attrib['node'] = node_name.node - profiles = policy.find('profiles') + profiles = enclave.find('profiles') profiles.append(profile) return profile From 7baa7aaea778081f175c116b0a714e84c965c979 Mon Sep 17 00:00:00 2001 From: ruffsl Date: Fri, 1 May 2020 00:27:49 -0700 Subject: [PATCH 6/8] Remove use of concat concat take 2+ args, only @path is needed to sort enclaves Signed-off-by: ruffsl --- sros2/sros2/policy/templates/policy.xsl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sros2/sros2/policy/templates/policy.xsl b/sros2/sros2/policy/templates/policy.xsl index 395fa15f..134b53e8 100644 --- a/sros2/sros2/policy/templates/policy.xsl +++ b/sros2/sros2/policy/templates/policy.xsl @@ -23,7 +23,7 @@ - + From ff3503be530c7f488dffccaa33b0a62247761732 Mon Sep 17 00:00:00 2001 From: Mikael Arguedas Date: Fri, 1 May 2020 10:46:00 +0200 Subject: [PATCH 7/8] make tests more modular and test different enclave paths Signed-off-by: Mikael Arguedas --- .../security/verbs/test_generate_policy.py | 55 ++++++++++++------- 1 file changed, 36 insertions(+), 19 deletions(-) diff --git a/sros2/test/sros2/commands/security/verbs/test_generate_policy.py b/sros2/test/sros2/commands/security/verbs/test_generate_policy.py index 60cbe128..8a137355 100644 --- a/sros2/test/sros2/commands/security/verbs/test_generate_policy.py +++ b/sros2/test/sros2/commands/security/verbs/test_generate_policy.py @@ -26,16 +26,19 @@ def test_generate_policy_topics(): with tempfile.TemporaryDirectory() as tmpdir: + TEST_ENCLAVE = '/foo/bar' + TEST_NODE_NAMESPACE = '/' + TEST_NAME = 'test_generate_policy_topics' + TEST_NODE_NAME = TEST_NAME + '_node' # Create a test-specific context so that generate_policy can still init context = rclpy.Context() - rclpy.init(context=context) - node = rclpy.create_node('test_generate_policy_topics_node', context=context) + rclpy.init(context=context, args=['--ros-args', '-e', TEST_ENCLAVE]) + node = rclpy.create_node(TEST_NODE_NAME, context=context) try: # Create a publisher and subscription - node.create_publisher(Strings, 'test_generate_policy_topics_pub', 1) - node.create_subscription( - Strings, 'test_generate_policy_topics_sub', lambda msg: None, 1) + node.create_publisher(Strings, TEST_NAME + '_pub', 1) + node.create_subscription(Strings, TEST_NAME + '_sub', lambda msg: None, 1) # Generate the policy for the running node assert cli.main( @@ -47,7 +50,10 @@ def test_generate_policy_topics(): # Load the policy and pull out the allowed publications and subscriptions policy = load_policy(os.path.join(tmpdir, 'test-policy.xml')) profile = policy.find( - path='enclaves/enclave[@path="/"]/profiles/profile[@ns="/"][@node="test_generate_policy_topics_node"]') + path=f'enclaves/enclave[@path="{TEST_ENCLAVE}"]' + + f'/profiles/profile[@ns="{TEST_NODE_NAMESPACE}"]' + + f'[@node="{TEST_NODE_NAME}"]' + ) assert profile is not None topics_publish_allowed = profile.find(path='topics[@publish="ALLOW"]') assert topics_publish_allowed is not None @@ -56,26 +62,34 @@ def test_generate_policy_topics(): # Verify that the allowed publications include topic_pub and not topic_sub topics = topics_publish_allowed.findall('topic') - assert len([t for t in topics if t.text == 'test_generate_policy_topics_pub']) == 1 - assert len([t for t in topics if t.text == 'test_generate_policy_topics_sub']) == 0 + assert len([t for t in topics if t.text == TEST_NAME + '_pub']) == 1 + assert len([t for t in topics if t.text == TEST_NAME + '_sub']) == 0 # Verify that the allowed subscriptions include topic_sub and not topic_pub topics = topics_subscribe_allowed.findall('topic') - assert len([t for t in topics if t.text == 'test_generate_policy_topics_sub']) == 1 - assert len([t for t in topics if t.text == 'test_generate_policy_topics_pub']) == 0 + assert len([t for t in topics if t.text == TEST_NAME + '_sub']) == 1 + assert len([t for t in topics if t.text == TEST_NAME + '_pub']) == 0 def test_generate_policy_services(): with tempfile.TemporaryDirectory() as tmpdir: # Create a test-specific context so that generate_policy can still init context = rclpy.Context() - rclpy.init(context=context) - node = rclpy.create_node('test_generate_policy_services_node', context=context) + TEST_ENCLAVE = '/foo' + TEST_NODE_NAMESPACE = '/node_ns' + TEST_NAME = 'test_generate_policy_services' + TEST_NODE_NAME = TEST_NAME + '_node' + rclpy.init(context=context, args=['--ros-args', '-e', TEST_ENCLAVE]) + node = rclpy.create_node( + TEST_NODE_NAME, + namespace=TEST_NODE_NAMESPACE, + context=context + ) try: # Create a server and client - node.create_client(Empty, 'test_generate_policy_services_client') - node.create_service(Empty, 'test_generate_policy_services_server', lambda request, + node.create_client(Empty, TEST_NAME + '_client') + node.create_service(Empty, TEST_NAME + '_server', lambda request, response: response) # Generate the policy for the running node @@ -88,7 +102,10 @@ def test_generate_policy_services(): # Load the policy and pull out allowed replies and requests policy = load_policy(os.path.join(tmpdir, 'test-policy.xml')) profile = policy.find( - path='enclaves/enclave[@path="/"]/profiles/profile[@ns="/"][@node="test_generate_policy_services_node"]') + path=f'enclaves/enclave[@path="{TEST_ENCLAVE}"]' + + f'/profiles/profile[@ns="{TEST_NODE_NAMESPACE}"]' + + f'[@node="{TEST_NODE_NAME}"]' + ) assert profile is not None service_reply_allowed = profile.find(path='services[@reply="ALLOW"]') assert service_reply_allowed is not None @@ -97,13 +114,13 @@ def test_generate_policy_services(): # Verify that the allowed replies include service_server and not service_client services = service_reply_allowed.findall('service') - assert len([s for s in services if s.text == 'test_generate_policy_services_server']) == 1 - assert len([s for s in services if s.text == 'test_generate_policy_services_client']) == 0 + assert len([s for s in services if s.text == TEST_NAME + '_server']) == 1 + assert len([s for s in services if s.text == TEST_NAME + '_client']) == 0 # Verify that the allowed requests include service_client and not service_server services = service_request_allowed.findall('service') - assert len([s for s in services if s.text == 'test_generate_policy_services_client']) == 1 - assert len([s for s in services if s.text == 'test_generate_policy_services_server']) == 0 + assert len([s for s in services if s.text == TEST_NAME + '_client']) == 1 + assert len([s for s in services if s.text == TEST_NAME + '_server']) == 0 # TODO(jacobperron): On Windows, this test is flakey due to nodes left-over from tests in From cab6123d7dafe8a9d16e48266c229bb884960346 Mon Sep 17 00:00:00 2001 From: Mikael Arguedas Date: Fri, 1 May 2020 15:19:17 +0200 Subject: [PATCH 8/8] gotta format 'em all Signed-off-by: Mikael Arguedas --- sros2/sros2/verb/generate_policy.py | 12 +--- .../security/verbs/test_generate_policy.py | 62 +++++++++---------- 2 files changed, 34 insertions(+), 40 deletions(-) diff --git a/sros2/sros2/verb/generate_policy.py b/sros2/sros2/verb/generate_policy.py index 17a6b740..5808620a 100644 --- a/sros2/sros2/verb/generate_policy.py +++ b/sros2/sros2/verb/generate_policy.py @@ -74,8 +74,7 @@ def get_policy(self, policy_file_path): def get_profile(self, policy, node_name): enclave = policy.find( - path='enclaves/enclave[@path="{path}"]'.format( - path=node_name.path)) + path=f'enclaves/enclave[@path="{node_name.path}"]') if enclave is None: enclave = etree.Element('enclave') enclave.attrib['path'] = node_name.path @@ -84,9 +83,7 @@ def get_profile(self, policy, node_name): enclaves = policy.find('enclaves') enclaves.append(enclave) profile = enclave.find( - path='profiles/profile[@ns="{ns}"][@node="{node}"]'.format( - ns=node_name.ns, - node=node_name.node)) + path=f'profiles/profile[@ns="{node_name.ns}"][@node="{node_name.node}"]') if profile is None: profile = etree.Element('profile') profile.attrib['ns'] = node_name.ns @@ -97,10 +94,7 @@ def get_profile(self, policy, node_name): def get_permissions(self, profile, permission_type, rule_type, rule_qualifier): permissions = profile.find( - path='{permission_type}s[@{rule_type}="{rule_qualifier}"]'.format( - permission_type=permission_type, - rule_type=rule_type, - rule_qualifier=rule_qualifier)) + path=f'{permission_type}s[@{rule_type}="{rule_qualifier}"]') if permissions is None: permissions = etree.Element(permission_type + 's') permissions.attrib[rule_type] = rule_qualifier diff --git a/sros2/test/sros2/commands/security/verbs/test_generate_policy.py b/sros2/test/sros2/commands/security/verbs/test_generate_policy.py index 8a137355..b077e219 100644 --- a/sros2/test/sros2/commands/security/verbs/test_generate_policy.py +++ b/sros2/test/sros2/commands/security/verbs/test_generate_policy.py @@ -26,19 +26,19 @@ def test_generate_policy_topics(): with tempfile.TemporaryDirectory() as tmpdir: - TEST_ENCLAVE = '/foo/bar' - TEST_NODE_NAMESPACE = '/' - TEST_NAME = 'test_generate_policy_topics' - TEST_NODE_NAME = TEST_NAME + '_node' + test_enclave = '/foo/bar' + test_node_namespace = '/' + test_name = 'test_generate_policy_topics' + test_node_name = f'{test_name}_node' # Create a test-specific context so that generate_policy can still init context = rclpy.Context() - rclpy.init(context=context, args=['--ros-args', '-e', TEST_ENCLAVE]) - node = rclpy.create_node(TEST_NODE_NAME, context=context) + rclpy.init(context=context, args=['--ros-args', '-e', test_enclave]) + node = rclpy.create_node(test_node_name, context=context) try: # Create a publisher and subscription - node.create_publisher(Strings, TEST_NAME + '_pub', 1) - node.create_subscription(Strings, TEST_NAME + '_sub', lambda msg: None, 1) + node.create_publisher(Strings, f'{test_name}_pub', 1) + node.create_subscription(Strings, f'{test_name}_sub', lambda msg: None, 1) # Generate the policy for the running node assert cli.main( @@ -50,9 +50,9 @@ def test_generate_policy_topics(): # Load the policy and pull out the allowed publications and subscriptions policy = load_policy(os.path.join(tmpdir, 'test-policy.xml')) profile = policy.find( - path=f'enclaves/enclave[@path="{TEST_ENCLAVE}"]' - + f'/profiles/profile[@ns="{TEST_NODE_NAMESPACE}"]' - + f'[@node="{TEST_NODE_NAME}"]' + path=f'enclaves/enclave[@path="{test_enclave}"]' + + f'/profiles/profile[@ns="{test_node_namespace}"]' + + f'[@node="{test_node_name}"]' ) assert profile is not None topics_publish_allowed = profile.find(path='topics[@publish="ALLOW"]') @@ -62,34 +62,34 @@ def test_generate_policy_topics(): # Verify that the allowed publications include topic_pub and not topic_sub topics = topics_publish_allowed.findall('topic') - assert len([t for t in topics if t.text == TEST_NAME + '_pub']) == 1 - assert len([t for t in topics if t.text == TEST_NAME + '_sub']) == 0 + assert len([t for t in topics if t.text == f'{test_name}_pub']) == 1 + assert len([t for t in topics if t.text == f'{test_name}_sub']) == 0 # Verify that the allowed subscriptions include topic_sub and not topic_pub topics = topics_subscribe_allowed.findall('topic') - assert len([t for t in topics if t.text == TEST_NAME + '_sub']) == 1 - assert len([t for t in topics if t.text == TEST_NAME + '_pub']) == 0 + assert len([t for t in topics if t.text == f'{test_name}_sub']) == 1 + assert len([t for t in topics if t.text == f'{test_name}_pub']) == 0 def test_generate_policy_services(): with tempfile.TemporaryDirectory() as tmpdir: # Create a test-specific context so that generate_policy can still init context = rclpy.Context() - TEST_ENCLAVE = '/foo' - TEST_NODE_NAMESPACE = '/node_ns' - TEST_NAME = 'test_generate_policy_services' - TEST_NODE_NAME = TEST_NAME + '_node' - rclpy.init(context=context, args=['--ros-args', '-e', TEST_ENCLAVE]) + test_enclave = '/foo' + test_node_namespace = '/node_ns' + test_name = 'test_generate_policy_services' + test_node_name = test_name + '_node' + rclpy.init(context=context, args=['--ros-args', '-e', test_enclave]) node = rclpy.create_node( - TEST_NODE_NAME, - namespace=TEST_NODE_NAMESPACE, + test_node_name, + namespace=test_node_namespace, context=context ) try: # Create a server and client - node.create_client(Empty, TEST_NAME + '_client') - node.create_service(Empty, TEST_NAME + '_server', lambda request, + node.create_client(Empty, f'{test_name}_client') + node.create_service(Empty, f'{test_name}_server', lambda request, response: response) # Generate the policy for the running node @@ -102,9 +102,9 @@ def test_generate_policy_services(): # Load the policy and pull out allowed replies and requests policy = load_policy(os.path.join(tmpdir, 'test-policy.xml')) profile = policy.find( - path=f'enclaves/enclave[@path="{TEST_ENCLAVE}"]' - + f'/profiles/profile[@ns="{TEST_NODE_NAMESPACE}"]' - + f'[@node="{TEST_NODE_NAME}"]' + path=f'enclaves/enclave[@path="{test_enclave}"]' + + f'/profiles/profile[@ns="{test_node_namespace}"]' + + f'[@node="{test_node_name}"]' ) assert profile is not None service_reply_allowed = profile.find(path='services[@reply="ALLOW"]') @@ -114,13 +114,13 @@ def test_generate_policy_services(): # Verify that the allowed replies include service_server and not service_client services = service_reply_allowed.findall('service') - assert len([s for s in services if s.text == TEST_NAME + '_server']) == 1 - assert len([s for s in services if s.text == TEST_NAME + '_client']) == 0 + assert len([s for s in services if s.text == f'{test_name}_server']) == 1 + assert len([s for s in services if s.text == f'{test_name}_client']) == 0 # Verify that the allowed requests include service_client and not service_server services = service_request_allowed.findall('service') - assert len([s for s in services if s.text == TEST_NAME + '_client']) == 1 - assert len([s for s in services if s.text == TEST_NAME + '_server']) == 0 + assert len([s for s in services if s.text == f'{test_name}_client']) == 1 + assert len([s for s in services if s.text == f'{test_name}_server']) == 0 # TODO(jacobperron): On Windows, this test is flakey due to nodes left-over from tests in