Update generate_policy verb for enclaves #203

ruffsl · 2020-05-01T05:37:12Z

Updates policy auto generation now that ros2/rclpy#529 is merged. Was disabled in #177.

Signed-off-by: ruffsl <[email protected]>

ruffsl · 2020-05-01T05:42:01Z

There shouldn't be much more to this, but I'm not sure why I'm seeing xmlrpc in this traceback:

$ ros2 security generate_policy foo.xml
Traceback (most recent call last):
  File "/home/ruffsl/ws/sros2/install/ros2cli/bin/ros2", line 11, in <module>
    load_entry_point('ros2cli', 'console_scripts', 'ros2')()
  File "/home/ruffsl/ws/sros2/build/ros2cli/ros2cli/cli.py", line 67, in main
    rc = extension.main(parser=parser, args=args)
  File "/home/ruffsl/ws/sros2/build/sros2/sros2/command/security.py", line 36, in main
    return extension.main(args=args)
  File "/home/ruffsl/ws/sros2/build/sros2/sros2/verb/generate_policy.py", line 126, in main
    node_names = _get_node_names(node=node, include_hidden_nodes=False)
  File "/home/ruffsl/ws/sros2/build/sros2/sros2/verb/generate_policy.py", line 158, in _get_node_names
    node_names_and_namespaces_with_enclaves = node.get_node_names_and_namespaces_with_enclaves()
  File "/usr/lib/python3.8/xmlrpc/client.py", line 1109, in __call__
    return self.__send(self.__name, args)
  File "/usr/lib/python3.8/xmlrpc/client.py", line 1450, in __request
    response = self.__transport.request(
  File "/usr/lib/python3.8/xmlrpc/client.py", line 1153, in request
    return self.single_request(host, handler, request_body, verbose)
  File "/usr/lib/python3.8/xmlrpc/client.py", line 1169, in single_request
    return self.parse_response(resp)
  File "/usr/lib/python3.8/xmlrpc/client.py", line 1341, in parse_response
    return u.close()
  File "/usr/lib/python3.8/xmlrpc/client.py", line 655, in close
    raise Fault(**self._stack[0])
xmlrpc.client.Fault: <Fault 1: '<class \'Exception\'>:method "get_node_names_and_namespaces_with_enclaves" is not supported'>

Are one of our from/imports now colliding with a standard python library?

Update

I think that xmlrpc issues as from my cli daemon being out of sync with my current workspace.
Also had to fix a bug in the policy xslt transform for apha sorting the xml tree.

sros2/test/sros2/commands/security/verbs/test_generate_policy.py

Signed-off-by: ruffsl <[email protected]>

mikaelarguedas · 2020-05-01T07:23:34Z

There shouldn't be much more to this, but I'm not sure why I'm seeing xmlrpc in this traceback:

It looks like the ros2 daemon uses xmlrpc and needs explicit registration of functions to use to know how to deal with them, I opened ros2/ros2cli#501 that seems to address that problem.

Now I get more "normal" XSL errors:

  File "/root/ws/install/ros2cli/bin/ros2", line 11, in <module>
    load_entry_point('ros2cli==0.9.0', 'console_scripts', 'ros2')()
  File "/root/ws/install/ros2cli/lib/python3.8/site-packages/ros2cli/cli.py", line 67, in main
    rc = extension.main(parser=parser, args=args)
  File "/root/ws/install/sros2/lib/python3.8/site-packages/sros2/command/security.py", line 36, in main
    return extension.main(args=args)
  File "/root/ws/install/sros2/lib/python3.8/site-packages/sros2/verb/generate_policy.py", line 155, in main
    dump_policy(policy, stream)
  File "/root/ws/install/sros2/lib/python3.8/site-packages/sros2/policy/__init__.py", line 77, in dump_policy
    policy = policy_xsl(policy)
  File "src/lxml/xslt.pxi", line 600, in lxml.etree.XSLT.__call__
lxml.etree.XSLTApplyError: Invalid number of arguments

concat take 2+ args, only @path is needed to sort enclaves Signed-off-by: ruffsl <[email protected]>

ruffsl · 2020-05-01T07:32:09Z

Now I get more "normal" XSL errors:

@mikaelarguedas , I found that too, and fixed it in 7baa7aa .

Now the policy output looks good:

<policy version="0.2.0">
  <enclaves>
    <enclave path="/">
      <profiles>
        <profile node="listener" ns="/">
          <services reply="ALLOW">
            <service>~/describe_parameters</service>
            <service>~/get_parameter_types</service>
            <service>~/get_parameters</service>
            <service>~/list_parameters</service>
            <service>~/set_parameters</service>
            <service>~/set_parameters_atomically</service>
          </services>
          <topics subscribe="ALLOW">
            <topic>chatter</topic>
            <topic>parameter_events</topic>
          </topics>
          <topics publish="ALLOW">
            <topic>parameter_events</topic>
            <topic>rosout</topic>
          </topics>
        </profile>
        <profile node="talker" ns="/">
          <services reply="ALLOW">
            <service>~/describe_parameters</service>
            <service>~/get_parameter_types</service>
            <service>~/get_parameters</service>
            <service>~/list_parameters</service>
            <service>~/set_parameters</service>
            <service>~/set_parameters_atomically</service>
          </services>
          <topics subscribe="ALLOW">
            <topic>parameter_events</topic>
          </topics>
          <topics publish="ALLOW">
            <topic>chatter</topic>
            <topic>parameter_events</topic>
            <topic>rosout</topic>
          </topics>
        </profile>
      </profiles>
    </enclave>
  </enclaves>
</policy>

mikaelarguedas · 2020-05-01T07:36:57Z

yeah it seems to work with ros2 run demo_nodes_py talker but not with ros2 topic pub -p 100 -r 0.1 /chatter std_msgs/msg/String "data: Hello World"

Is it expected ?

otherwise looks good 👍

(as a side note there is a --no-daemon flag we can use to avoid daemon issues when testing)

ruffsl · 2020-05-01T07:41:26Z

Is it expected ?

I think ros2 topic pub falls under this condtion as being a hidden node with

sros2/sros2/sros2/verb/generate_policy.py

Lines 168 to 170 in 7baa7aa

    
           if ( 
        
               include_hidden_nodes or 
        
               (t[0] and not t[0].startswith(_HIDDEN_NODE_PREFIX))

Try setting the include_hidden_nodes to True.
That doesn't seem to be user controllable though.

mikaelarguedas · 2020-05-01T07:43:07Z

That doesn't seem to be user controllable though.

yeah it used to be exposed in most cli before but is not the case anymore. We could consider exposing it in the future if we want to support these use cases

ruffsl · 2020-05-01T07:48:43Z

Those hidden node names from the CLI are normally unique with an ephemeral UID at the end anyhow. Not sure it's a big issue given those UIDs would have to be manually edited to deal with.

mikaelarguedas · 2020-05-01T08:04:51Z

Those hidden node names from the CLI are normally unique with an ephemeral UID at the end anyhow. Not sure it's a big issue given those UIDs would have to be manually edited to deal with.

Not sure how much we care about the suffix though, what you want are the permissions associated with it. the actual node name will not impact the fact that a participant using these policies wil be able to behave the same way right ? even if the node name is different?

It does seem weird to have a include_hidden_nodes argument but without ever exposing it.

Both these points are features and unrelated to this bugfix so they should not impact this PR

Signed-off-by: Mikael Arguedas <[email protected]>

codecov · 2020-05-01T09:05:39Z

Codecov Report

Merging #203 into master will increase coverage by 17.48%.
The diff coverage is 92.85%.

@@             Coverage Diff             @@
##           master     #203       +/-   ##
===========================================
+ Coverage   60.00%   77.48%   +17.48%     
===========================================
  Files          16       16               
  Lines         565      573        +8     
  Branches       52       53        +1     
===========================================
+ Hits          339      444      +105     
+ Misses        212      109      -103     
- Partials       14       20        +6

Flag	Coverage Δ
#unittests	`77.48% <92.85%> (+17.48%)`	⬆️

Impacted Files	Coverage Δ
sros2/sros2/verb/generate_policy.py	`86.48% <92.85%> (+86.48%)`	⬆️
sros2/sros2/policy/__init__.py	`84.61% <0.00%> (+23.07%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7bb8a45...cab6123. Read the comment docs.

mikaelarguedas · 2020-05-01T09:54:36Z

Linux
Linux-aarch64
macOS
Windows

Another linux build to confirm the cosmetic commit didnt break anything:

Linux

mikaelarguedas

lgtm with green ci

ivanpauno

LGTM, thanks for this @ruffsl !

Considering that sros2 is a leaf package, and the verb modified here (generate_policies) isn't covered by any of the tutorials, I think it's safe to merge this.

@jacobperron as rosboss for approval.

ivanpauno · 2020-05-01T12:27:24Z

@ruffsl According to the checks, test_generate_policies_no_nodes is failing.

mikaelarguedas · 2020-05-01T12:36:32Z

According to the checks, test_generate_policies_no_nodes is failing.

This is because the automatic checks don't use ros2/ros2cli#501

I launched a round of CI in #203 (comment) that leverage the fix on ros2 daemon so these should pass 🤞

sros2/sros2/verb/generate_policy.py

sros2/test/sros2/commands/security/verbs/test_generate_policy.py

Signed-off-by: Mikael Arguedas <[email protected]>

kyrofa

Excellent, thanks guys!

jacobperron · 2020-05-01T16:26:21Z

Considering that sros2 is a leaf package, and the verb modified here (generate_policies) isn't covered by any of the tutorials, I think it's safe to merge this.

@jacobperron as rosboss for approval.

This PR is also re-enabling the verb.

ruffsl added 4 commits April 30, 2020 22:37

Update GeneratePolicyVerb for enclaves

6656678

Signed-off-by: ruffsl <[email protected]>

Reactivate generate_policy verb

0d3accf

Signed-off-by: ruffsl <[email protected]>

Reactivate generate_policy test

a936ad9

Signed-off-by: ruffsl <[email protected]>

Update generate_policy tests

46ff953

Signed-off-by: ruffsl <[email protected]>

ruffsl force-pushed the generate_policy branch from eeb7282 to 46ff953 Compare May 1, 2020 05:37

ruffsl commented May 1, 2020

View reviewed changes

sros2/test/sros2/commands/security/verbs/test_generate_policy.py Show resolved Hide resolved

ruffsl requested review from ivanpauno, kyrofa and mikaelarguedas May 1, 2020 05:56

Fix missing element

bc7a5ad

Signed-off-by: ruffsl <[email protected]>

mikaelarguedas mentioned this pull request May 1, 2020

add support for get_node_names_and_namespaces_with_enclaves ros2/ros2cli#501

Merged

Remove use of concat

7baa7aa

concat take 2+ args, only @path is needed to sort enclaves Signed-off-by: ruffsl <[email protected]>

ruffsl marked this pull request as ready for review May 1, 2020 07:49

make tests more modular and test different enclave paths

ff3503b

Signed-off-by: Mikael Arguedas <[email protected]>

mikaelarguedas force-pushed the generate_policy branch from 57c194e to ff3503b Compare May 1, 2020 08:48

mikaelarguedas approved these changes May 1, 2020

View reviewed changes

ivanpauno approved these changes May 1, 2020

View reviewed changes

kyrofa reviewed May 1, 2020

View reviewed changes

sros2/sros2/verb/generate_policy.py Outdated Show resolved Hide resolved

sros2/test/sros2/commands/security/verbs/test_generate_policy.py Outdated Show resolved Hide resolved

sros2/test/sros2/commands/security/verbs/test_generate_policy.py Outdated Show resolved Hide resolved

gotta format 'em all

cab6123

Signed-off-by: Mikael Arguedas <[email protected]>

kyrofa approved these changes May 1, 2020

View reviewed changes

jacobperron approved these changes May 1, 2020

View reviewed changes

ivanpauno merged commit 1382be3 into master May 1, 2020

ivanpauno deleted the generate_policy branch May 1, 2020 16:52

ivanpauno mentioned this pull request May 1, 2020

Use security enclaves #182

Open

20 tasks

mikaelarguedas mentioned this pull request May 2, 2020

reenable test_generate_policy_no_policy_file #206

Merged

mikaelarguedas mentioned this pull request Jun 5, 2020

Eloquent backports #222

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update generate_policy verb for enclaves #203

Update generate_policy verb for enclaves #203

ruffsl commented May 1, 2020 •

edited

Loading

ruffsl commented May 1, 2020 •

edited

Loading

mikaelarguedas commented May 1, 2020

ruffsl commented May 1, 2020 •

edited

Loading

mikaelarguedas commented May 1, 2020

ruffsl commented May 1, 2020

mikaelarguedas commented May 1, 2020

ruffsl commented May 1, 2020

mikaelarguedas commented May 1, 2020

codecov bot commented May 1, 2020 •

edited

Loading

mikaelarguedas commented May 1, 2020 •

edited

Loading

mikaelarguedas left a comment

ivanpauno left a comment

ivanpauno commented May 1, 2020

mikaelarguedas commented May 1, 2020

kyrofa left a comment

jacobperron commented May 1, 2020

Update generate_policy verb for enclaves #203

Update generate_policy verb for enclaves #203

Conversation

ruffsl commented May 1, 2020 • edited Loading

ruffsl commented May 1, 2020 • edited Loading

mikaelarguedas commented May 1, 2020

ruffsl commented May 1, 2020 • edited Loading

mikaelarguedas commented May 1, 2020

ruffsl commented May 1, 2020

mikaelarguedas commented May 1, 2020

ruffsl commented May 1, 2020

mikaelarguedas commented May 1, 2020

codecov bot commented May 1, 2020 • edited Loading

Codecov Report

mikaelarguedas commented May 1, 2020 • edited Loading

mikaelarguedas left a comment

Choose a reason for hiding this comment

ivanpauno left a comment

Choose a reason for hiding this comment

ivanpauno commented May 1, 2020

mikaelarguedas commented May 1, 2020

kyrofa left a comment

Choose a reason for hiding this comment

jacobperron commented May 1, 2020

ruffsl commented May 1, 2020 •

edited

Loading

ruffsl commented May 1, 2020 •

edited

Loading

ruffsl commented May 1, 2020 •

edited

Loading

codecov bot commented May 1, 2020 •

edited

Loading

mikaelarguedas commented May 1, 2020 •

edited

Loading