Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add and update security designs for Contexts #274

Open
wants to merge 27 commits into
base: gh-pages
from
Open

Conversation

@ruffsl
Copy link
Member

ruffsl commented Feb 25, 2020

With the migration to contexts in ROS2 underway, I’ve been iterating with @mikaelarguedas on adjustments to SROS2 tooling to accommodate this mapping of nodes to participants (ruffsl#1). This PR proposes relevant changes to the keystore layout, security directory lookup, and policy schema.

Related tickets:

Feel free to ping any interested parties I've missed from scraping the above ticket discussions.

ruffsl and others added 3 commits Feb 22, 2020
Signed-off-by: ruffsl <roxfoxpox@gmail.com>
Co-Authored-By: Mikael Arguedas <mikael.arguedas@gmail.com>
@ros-discourse

This comment has been minimized.

Copy link

ros-discourse commented Feb 25, 2020

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-security-working-group-meeting-02-25-2020/12958/2

@dirk-thomas dirk-thomas removed their request for review Feb 25, 2020
Copy link
Member

ivanpauno left a comment

Some comments and questions, but it's looking good.

articles/ros2_access_control_policies.md Outdated Show resolved Hide resolved
articles/ros2_security_contexts.md Outdated Show resolved Hide resolved
articles/ros2_security_contexts.md Outdated Show resolved Hide resolved
articles/ros2_security_contexts.md Show resolved Hide resolved
articles/ros2_security_contexts.md Outdated Show resolved Hide resolved
articles/ros2_security_contexts.md Show resolved Hide resolved
articles/ros2_security_contexts.md Show resolved Hide resolved
articles/ros2_security_contexts.md Show resolved Hide resolved
articles/ros2_security_contexts.md Outdated Show resolved Hide resolved
articles/ros2_security_contexts.md Show resolved Hide resolved
ruffsl added 3 commits Feb 27, 2020
@ivanpauno

This comment was marked as resolved.

Copy link
Member

ivanpauno commented Feb 27, 2020

After some more thinking, I think that in general the mechanism for specifying the context name/path should be independent of the node namespace.
In the general case, a context can group more than one node, each node with a different namespace. So, namespacing the context name/path with a node namespace would be confusing.
In the particular case of the launch Node action, where you only have one node in one context (in one process), the node namespace (or the fully quallyfied node name) could be used as the context name/path too.
But IMO, there should be a way to override it.
In the case of a component container, the same applies: the container namespace could be used to push the context directory (or the fully qualified container name could be directly used as the context name/path).

The current proposal is fine, as there's a way to override the context path:

<launch>
  <node pkg="demo_nodes_cpp" exec="talker"/>
  <node pkg="demo_nodes_cpp" exec="listener" namespace="foo"/>
  <node pkg="demo_nodes_cpp" exec="talker" name="talker2" namespace="foo" context="bar"/>
  <node pkg="demo_nodes_cpp" exec="listener" name="listener2" namespace="foo" context="/bar"/>
</launch>

The first node uses artifacts in contexts/.
The 2nd in contexts/foo.
The 3rd in contexts/foo/bar.
The 4th in contexts/bar.

But I think that that mechanism should be implemented in launch, and it shouldn't be any assumption in rcl/rclcpp/rclpy about how the context path is named.
rcl should only provide a way of remapping the context name:

ros2 run package executable --ros-args -r __context:=my_context_path

and there should be a way to target an specific name, like:

ros2 run package executable --ros-args -r original_path:__context:=my_context_path

In the case we want to add a mechanism in launch to directly use the fully qualified node name by default as context name/path, it would look like:

<launch>
  <node pkg="demo_nodes_cpp" exec="talker"/>
  <node pkg="demo_nodes_cpp" exec="listener" namespace="foo"/>
  <node pkg="demo_nodes_cpp" exec="talker" name="talker2" namespace="foo" context="bar"/>
  <node pkg="demo_nodes_cpp" exec="listener" name="listener2" namespace="foo" context="/bar"/>
</launch>

The first node uses artifacts in contexts/talker.
The 2nd in contexts/foo/listener.
The 3rd in contexts/foo/bar (overrides just the node name in the context path)
The 4th in contexts/bar (completly overrides the context path)

@ruffsl

This comment was marked as resolved.

Copy link
Member Author

ruffsl commented Feb 27, 2020

and there should be a way to target an specific name, like:

ros2 run package executable --ros-args -r original_path:__context:=my_context_path

I'm not sure what this means, is this a remapping syntax, like with topics, but for context paths?

The 3rd in contexts/foo/bar (overrides just the node name in the context path)

FYI, for others, originating discussion here: #274 (comment)

Then perhaps the use of dot and slash could be used to denote the following example:

<launch>
  <node pkg="demo_nodes_cpp" exec="talker" name="talker2" namespace="foo" context="."/>
  <node pkg="demo_nodes_cpp" exec="listener" name="listener2" namespace="foo" context="/"/>
</launch>

The 1st in contexts/foo (overrides just the node name in the context path)
The 2nd in contexts/ (completly overrides the context path)

I was also thinking of using and empty string instead of dot to denote this, but I'm not sure if XML/Python parser can distinguish between None and empty strings.

@ivanpauno

This comment was marked as resolved.

Copy link
Member

ivanpauno commented Feb 27, 2020

I'm not sure what this means, is this a remapping syntax, like with topics, but for context paths?

Yes. If launch is going to be able to change the context path, rcl should have an argument to change that. I think that the simplest thing is to take advantage of remapping.

My suggestion just follows how node name and namespace remapping works:
https://index.ros.org/doc/ros2/Tutorials/Node-arguments/#passing-remapping-arguments-to-specific-nodes.

The 1st in contexts/foo (overrides just the node name in the context path)
The 2nd in contexts/ (completly overrides the context path)

I was also thinking of using and empty string instead of dot to denote this, but I'm not sure if XML/Python parser can distinguish between None and empty strings.

Maybe better, just follow how it currently work for topics:
~/context_path is translated to /namespace/name/context_path
context_path to /namespace/context_path
/context_path to /context_path
The default is ~ (i.e.: /namespace/name).

Copy link
Member

kyrofa left a comment

I haven't quite made it through the whole thing, but a few quick comments from me.

articles/ros2_access_control_policies.md Show resolved Hide resolved
articles/ros2_dds_security.md Outdated Show resolved Hide resolved
articles/ros2_security_contexts.md Outdated Show resolved Hide resolved
articles/ros2_security_contexts.md Outdated Show resolved Hide resolved
articles/ros2_security_contexts.md Outdated Show resolved Hide resolved
articles/ros2_security_contexts.md Outdated Show resolved Hide resolved
@ruffsl

This comment was marked as resolved.

Copy link
Member Author

ruffsl commented Feb 27, 2020

The default is ~ (i.e.: /namespace/name).

I like this idea of replicating the patter by using ~.

Could you still show how one would convey just /namespace?
I think this would still be a case for using dot ="." or the empty string ="".

ruffsl and others added 4 commits Feb 27, 2020
Co-Authored-By: Kyle Fazzari <kyle@canonical.com>
Fix typo
Co-Authored-By: Kyle Fazzari <kyle@canonical.com>
Co-Authored-By: Kyle Fazzari <kyle@canonical.com>
@mikaelarguedas

This comment was marked as resolved.

Copy link
Member

mikaelarguedas commented Feb 28, 2020

Maybe better, just follow how it currently work for topics:
~/context_path is translated to /namespace/name/context_path
context_path to /namespace/context_path
/context_path to /context_path
The default is ~ (i.e.: /namespace/name).

I like this idea of replicating the patter by using ~.

This means that 2 different nodes will have different default context name and security artifact directory, is that correct?
@kyrofa FYI as this may impact your plan to "turn on security by default"

@ruffsl

This comment was marked as resolved.

Copy link
Member Author

ruffsl commented Feb 28, 2020

this may impact your plan to "turn on security by default"

With the design doc as it is currently, there would already have to be a context for every namespace for every standalone node process. Perhaps a encryption with access control disabled by default could be achieved by crating a single root context with stuff like rt/* in the mangled permission file, and documenting to users to just set ROS_SECURITY_CONTEXT_DIRECTORY to that root same directory.

@ivanpauno

This comment was marked as resolved.

Copy link
Member

ivanpauno commented Feb 28, 2020

I like this idea of replicating the patter by using ~.

Could you still show how one would convey just /namespace?
I think this would still be a case for using dot ="." or the empty string ="".

I think that the empty string should be fine.

Another comment related to this:
node_name is an optional attribute in launch (same applies to namespace).
When they are not specified, the name/namspace "harcoded" in the executable are used.

In the case the user doesn't specify the node name/namespace in the launch file, I don't see a way of making context="~" work. That's because there's no way to query the name/namespace that the executable is using.

I also don't want to add any code in rcl to magically deduct the context name from the node name, as in general it doesn't work (that can only be done in single node executables).
So I would use the following set of rules to name contexts:

  • Executables with a single node use by convention the same name for the context and the node. It's up to the user to use the convention or not.
  • Luanch uses the following rules:
    • Node action uses /namespace/name as context name by default if both the node namespace and name were specified.
    • Node action doesn't remap the context name if the node namespace or name weren't specified in the launch file, and context attribute wasn't explicitly provided.
    • Node action context attribute can be used to override the context path if an absolute path is provided (i.e. context="/my/context/path/").
    • Node action context attribute can be a relative name, if namespace was exlicitly used in the launch file. It raises an error if namespace wasn't provided.
        <node pkg="demo_nodes_cpp" exec="talker" namespace="foo" context="asd"/>  # works fine
        <node pkg="demo_nodes_cpp" exec="talker" context="asd"/>  # raises an error
    • Node action context attribute can be a private name, if name and namespace were exlicitly provided in the launch file. It raises an error if they weren't provided.
    • The same set of rules apply for a component container, but the component container name/namespace is used.

That's the best option I could come up.

@ivanpauno

This comment has been minimized.

Copy link
Member

ivanpauno commented Mar 10, 2020

But couldn't this discovery information be disseminated via the USER_DATA payload in the DDS discovery RTPS frame, rather than having to make a separate mandatory DDS topic?

Maybe yes, it's not working like that right now.
The design document considered both alternatives.

It should also be noted that automatic decentralized discovery is optional for DDS.

Decentralized or not, discovery data has still to be shared and every participant will need permissions to share it, right?

@ivanpauno

This comment has been minimized.

Copy link
Member

ivanpauno commented Mar 10, 2020

Are there any other non-vendor specific built-in topics your aware of? I haven't audited the DDS topic graph sense crystal release.

Do you mean ROS topics? /rosout can be included, though it can be disabled. Then the parameters events topic you already mentioned. I don't have anything else in mind.

@ruffsl

This comment has been minimized.

Copy link
Member Author

ruffsl commented Mar 10, 2020

The design document considered both alternatives.

Thanks for pointing that out, not sure I saw that Using USER_DATA and GROUP_DATA QoSPolicy section when I first read the PR a while back.

Decentralized or not, discovery data has still to be shared and every participant will need permissions to share it, right?

If the network is static, say in an automotive/aerospace or other controlled system setting, then discovery info can be embedded and loaded from the vendor QoS settings. If there was a similar method for serializing and loading such discover info from disk for ROS2, then dynamic discovery could still remain optional for ROS2. But we can defer this to the design PR dedicated for contexts.

Do you mean ROS topics? /rosout can be included in the list, though it can be disabled. Then the parameters events topic you already mentioned. I don't have anything else in the list.

The use of /rosout is tolerable, given that nodes only need write and not read permissions to such global topics. Users can then choose to include/trust a ros log aggregator node to subscribe to such sensitive logs.

For the parameters, user could still choose to just use a custom node interface without a parameter server, if they don't want every node to be able to publish data to every other node.

But requiring a pub/sub access to a unified topic for side channel discovery doesn't lend itself well to the separation principle. I'm thinking of an example where the DDS library itself may be controlled by a separate certified hardware security layer, and the rmw layers above could only transmit authorized data out of the network interface to authorized recipients.

@wjwwood

This comment has been minimized.

Copy link
Member

wjwwood commented Mar 10, 2020

Do you mean ROS topics? /rosout can be included

I think there should be a distinction between this discovery topic and topics like /rosout. The discovery topic is an implementation detail, whereas /rosout is part of the standard conventional ROS interface for a node, i.e. anyone can create a subscription to /rosout, but you cannot portably subscribe to this discovery topic as a user.

A different rmw implement could use a static file containing discovery info, or it could have a centralized service that communicated with XMLRPC (like ROS 1) or it could also use a DDS topic but with a different message than the one @ivanpauno made in the dds_common package.

In each case, if needed the rmw implementation has to adjust the security document that is generated to allow for these implementation details.

This is not the case for /rosout, that should be handled higher in the stack, it should be before generation for a particular rmw implementation, and could either be done automatically for the user or explicitly by the user.

But requiring a pub/sub access to a unified topic for side channel discovery doesn't lend itself well to the separation principle. I'm thinking of an example where the DDS library itself may be controlled by a separate certified hardware security layer, and the rmw layers above could only transmit authorized data out of the network interface to authorized recipients.

How does that prevent you from having a topic that every participant writes and reads to? Isn't something like /tf_static likely to be like this, at least similar (many writers, many readers, some reader/writers)?

@ivanpauno

This comment has been minimized.

Copy link
Member

ivanpauno commented Mar 10, 2020

But requiring a pub/sub access to a unified topic for side channel discovery doesn't lend itself well to the separation principle. I'm thinking of an example where the DDS library itself may be controlled by a separate certified hardware security layer, and the rmw layers above could only transmit authorized data out of the network interface to authorized recipients.

Though that might be true, and a different mechanism to share this information could be used or a way to bypass it could be added, it's not realistic to do it before the Foxy API freeze (less than a month). Considering that the shared information in that topic is just used for introspection (debugging) tools, and that not other behavior changes, I wouldn't block on this. We could add explicitly an allow rule in the ROS default profile file for this topic (we should need a new tag for DDS bare topic names), or implicitly add it when creating the permission file. In the first case, old security profiles file should be updated.

@wjwwood

This comment has been minimized.

Copy link
Member

wjwwood commented Mar 10, 2020

We could add explicitly an allow rule in the ROS default profile file for this topic

This seems wrong to me, it needs to be possible for DDS based rmw implementations to contribute to the security specification that is generated. Adding to the ROS part of the security API and therefore trying to find a way to specify a plain DDS topic in that interface is the wrong approach I think. Even if you could add it, this allow rule would not make sense for cycloneDDS (until they update their implementation) nor would it make sense for things like iceoryx...

@mikaelarguedas

This comment has been minimized.

Copy link
Member

mikaelarguedas commented Mar 10, 2020

Even if you could add it, this allow rule would not make sense for cycloneDDS (until they update their implementation) nor would it make sense for things like iceoryx...

It is true that if the topic was added blindly to all generated permissions, implementation not needing it will have this extra unnecessary access and it should be avoided

This seems wrong to me, it needs to be possible for DDS based rmw implementations to contribute to the security specification that is generated. Adding to the ROS part of the security API and therefore trying to find a way to specify a plain DDS topic in that interface is the wrong approach I think.

One assumption that seem to be added here is a coupling between the tool and the rmw implementation in use. Currently there is none.
Generation and signing would ideally happen before and elsewhere than where the application is deployed. Commonly on a computer with no ROS installed.
In order to account for these "implementation detail topics", the tool would need to have this information baked-in. It also means you'd need to tell the tool what impl are the artifact generated for to avoid adding unnecessary permissions to the access control list.

@wjwwood

This comment has been minimized.

Copy link
Member

wjwwood commented Mar 10, 2020

One assumption that seem to be added here is a coupling between the tool and the rmw implementation in use. Currently there is none.

I thought we had discussed that before and concluded that in order to support one participant per context we would need to have this coupling (adding it if needed), because one rmw implementation might use one participant per context and another might use one participant per node and another might not even be DDS based or it might be DDS based but configured differently than the ones we have now.

@wjwwood

This comment has been minimized.

Copy link
Member

wjwwood commented Mar 10, 2020

Generation and signing would ideally happen before and elsewhere than where the application is deployed. Commonly on a computer with no ROS installed.

I think this would still be possible but you'd need some information about the target rmw implementations, however this could just be like a config file or "profile" that you could get separately from the rmw implementation code, which would be an input to the generation process and could add things like this discovery topic.

@mikaelarguedas

This comment has been minimized.

Copy link
Member

mikaelarguedas commented Mar 10, 2020

I thought we had discussed that before and concluded that in order to support one participant per context we would need to have this coupling (adding it if needed),

This was one of the motivations for this PR: remove the need for adding such complexity to both RCL and the keystore generation. Contexts will be used everywhere instead.

I think this would still be possible but you'd need some information about the target rmw implementations, however this could just be like a config file or "profile" that you could get separately from the rmw implementation code, which would be an input to the generation process and could add things like this discovery topic.

👍

@wjwwood

This comment has been minimized.

Copy link
Member

wjwwood commented Mar 10, 2020

I thought we had discussed that before and concluded that in order to support one participant per context we would need to have this coupling (adding it if needed),

This was one of the motivations for this PR: remove the need for adding such complexity to both RCL and the keystore generation. Contexts will be used everywhere instead.

I'm so confused 🤣, how does this pr avoid that coupling? How does "using contexts everywhere" address the case that we currently have where fast-rtps will be merged with one participant per context and cyclone/connext will continue to be one participant per node? @ivanpauno is this not the current plan or did it change?

@mikaelarguedas

This comment has been minimized.

Copy link
Member

mikaelarguedas commented Mar 10, 2020

Haha my bad poor choice of words.

With this PR the goal is to scope permissions at the process level. Regardless of how many contexts in the process or how many participants per context. Only one set of security files is used per process.
As the end goal for all rmw implementations to converge to 1 participant per context, giving up the granularity in the meantime doesn't cost much and allow a single keystore and lookup mechanism to be used for all.

@wjwwood

This comment has been minimized.

Copy link
Member

wjwwood commented Mar 10, 2020

But that's only one reason to have generated artifacts influenced by the rmw implementation. In my opinion it is still needed to handle things like this discovery topic and to handle future rmw implementations which deal with security differently.

My conclusion is that if you continue forward without coupling artifact generation to the target rmw implementations that you will have to "over" allow the discovery topic, even if it is unused. This doesn't seem like a good thing to me, and in the future we will likely need to have some rmw implementation specific details in the security generation, no?

@wjwwood

This comment has been minimized.

Copy link
Member

wjwwood commented Mar 10, 2020

I mean you guys should do what you think you have time to do for Foxy, but I feel we will regret not having some way to influence security artifact generation based on the rmw implementations that will be used.

ruffsl added 2 commits Mar 11, 2020
It's now redundan given context paths can be independent of namespace
#274 (comment)
@ruffsl

This comment has been minimized.

Copy link
Member Author

ruffsl commented Mar 11, 2020

I think there should be a distinction between this discovery topic and topics like /rosout.

How does that prevent you from having a topic that every participant writes and reads to? Isn't something like /tf_static likely to be like this, at least similar (many writers, many readers, some reader/writers)?

Like you mentioned, I think a similar distinction between /tf_static and ros_discovery_info should also be made. /tf_static is a data topic created for the application and not the middle ware; thus users can easily control if the node needs to publish or subscribe to that topic depending on if/how they elect to use tf listeners. For example, users with multiple robots per domain could remap the tf topics to avoid leaking/cross-talk of pose information of one robot to others. These robots may still communicate over other designated DDS topics, but the nodes with access to such permissive topics could be limited and stringently accounted for.

If every DDS participant now requires mandatory read-&-write access to an application level DDS topic, then there isn't much DDS access control plugins can do to limit who can talk to who, or what data could be passed along that application topic, as it would then be a fully connected graph.


Though that might be true, and a different mechanism to share this information could be used or a way to bypass it could be added, it's not realistic to do it before the Foxy API freeze (less than a month).

Understood. I just hope we aren't committing future releases of ROS2 to an eroded separation of privilege as we forgo security properties for side channel discovery mechanisms, as it's a lot harder to sanitize our own side channel discovery than it is to rely on the DDS standards.


In response to the discussion between @mikaelarguedas and @wjwwood , I'll just add that I think we could easily allow the user to hint about the rmw vendor in the <metadata> element under the the <profiles> tag, as we've already established the need for users to hint about the rmw type. This could be used to by SROS2 to simply switch between the mangling transform that could include some content permissions applicable to the quirks of the rmw vendor. Though I don't like the trend of adding out of band topic flows not explicitly captured by the top level security policy.

@ivanpauno

This comment has been minimized.

Copy link
Member

ivanpauno commented Mar 11, 2020

In response to the discussion between @mikaelarguedas and @wjwwood , I'll just add that I think we could easily allow the user to hint about the rmw vendor in the element under the the tag, as we've already established the need for users to hint about the rmw type. This could be used to by SROS2 to simply switch between the mangling transform that could include some content permissions applicable to the quirks of the rmw vendor. Though I don't like the trend of adding out of band topic flows not explicitly captured by the top level security policy.

I think we don't need a hint in the policy file, because if not the same file won't be useful for another DDS vendor. IMO, the policy file shouldn't change, just the generated artifacts.
The easiest thing to do, is to get the current rmw implementation, and generate different artifacts depending on it. I see that sros2 was already thought to use different schemas/templates for generating artifacts, though dds schemas/templates are automatically being used currently.
It would be great to extend sros2 in order to be extensible from external packages, i.e.: use entry points extensively to be able to modify how artifacts are generated, and take a decision based on the rmw implementation being used.
To make things simpler now, we can just detect rmw_fastrtps_* implementations and tweak the generation a bit.

Understood. I just hope we aren't committing future releases of ROS2 to an eroded separation of privilege as we forgo security properties for side channel discovery mechanisms, as it's a lot harder to sanitize our own side channel discovery than it is to rely on the DDS standards.

This definitely could be addressed in the future (though as mentioned before, not for Foxy). I will create a ticket specifying what would be needed.

This was one of the motivations for this PR: remove the need for adding such complexity to both RCL and the keystore generation. Contexts will be used everywhere instead.

Though I agree this proposal deletes the necessity of different security directory discovery logic for different implementations (i.e.: always use the security path specified by the context name, regardless if one Participant is created per Node or Context), I don't think that the need of generating different artifacts can be avoided. The currently generated artifacts are useful for DDS, and others rmw implementations will need different artifacts.

Generation and signing would ideally happen before and elsewhere than where the application is deployed. Commonly on a computer with no ROS installed.

sros2 depends on rclpy, so I don't know how that would be possible.

In order to account for these "implementation detail topics", the tool would need to have this information baked-in. It also means you'd need to tell the tool what impl are the artifact generated for to avoid adding unnecessary permissions to the access control list.

As I commented above, other rmw implementations could need different artifacts.
It's in general, a good idea to be able to tweak how the artifacts are generated.
If ROS is not present in the machine generating the artifacts (supposing that sros2 requirement of rclpy is deleted), it would also be possible to add an explicit mechanism to tell sros2 what artifacts to generate. This mechanism can be an environment variable, or an argument to specify it.

@ruffsl

This comment has been minimized.

Copy link
Member Author

ruffsl commented Mar 11, 2020

This definitely could be addressed in the future (though as mentioned before, not for Foxy). I will create a ticket specifying what would be needed.

Ok, I've added a relevant comment that the issue could investigate as well here:
https://github.com/ros2/design/pull/250/files#r391160903


I think we don't need a hint in the policy file, because if not the same file won't be useful for another DDS vendor. IMO, the policy file shouldn't change, just the generated artifacts.

This mechanism can be an environment variable, or an argument to specify it.

Yeah, I guess given that we've been able to keep the <profile> tags agnostic of rwm, having to bind the <context> to particular rmw vendors would make entire profile less portable. We could have sros2 cli pick up the rwm vendor hint from the environment variable or runtime argument. I guess I was thinking of if multiple rmw vendors could be used simultaneously, but now that I think about it, I don't think that's possible yet, and when it is, we can optimize later.

It would be great to extend sros2 in order to be extensible from external packages, i.e.: use entry points extensively to be able to modify how artifacts are generated, and take a decision based on the rmw implementation being used.

As @mikaelarguedas mentioned, we had a goal for allowing for SROS2 cli to run without all of ros installed, but this seems harder and harder keep with given the rospy graph introspection, and use of entry points from other ros packages. This sort of ties back to the idea we had with registering common policies with the ament index: ros2/sros2#147

@mikaelarguedas

This comment has been minimized.

Copy link
Member

mikaelarguedas commented Mar 11, 2020

My conclusion is that if you continue forward without coupling artifact generation to the target rmw implementations that you will have to "over" allow the discovery topic, even if it is unused. This doesn't seem like a good thing

Agreed

in the future we will likely need to have some rmw implementation specific details in the security generation, no?

Yeah we expected that in the future, as more implementations get supported, some specific rules would have to be added. We just didn't expect anything new now.

I think we don't need a hint in the policy file, because if not the same file won't be useful for another DDS vendor. IMO, the policy file shouldn't change, just the generated artifacts.

+1

It would be great to extend sros2 in order to be extensible from external packages, i.e.: use entry points extensively to be able to modify how artifacts are generated, and take a decision based on the rmw implementation being used.

I'm not sure I understand how this would work. Does this mean that various RMW implementations would provide an additional package depending on sros2 to influence its' generation method?

The easiest thing to do, is to get the current rmw implementation, and generate different artifacts depending on it. I see that sros2 was already thought to use different schemas/templates for generating artifacts, though dds schemas/templates are automatically being used currently.

sros2 depends on rclpy, so I don't know how that would be possible.

It used to depend on rclpy only for the "analyse a running ROS system and infer permissions", but not for any of the rest (creating keystores, generating keys, certificates, DDS files, signing etc). But it looks like more rclpy creeped in over time..

we had a goal for allowing for SROS2 cli to run without all of ros installed, but this seems harder and harder keep with given the rospy graph introspection, and use of entry points from other ros packages. This sort of ties back to the idea we had with registering common policies with the ament index: ros2/sros2#147

Yeah it looks like it would be harder to ever go back to a point where it can be used standalone.
We could for now provide the rmw_implementation detection hack suggested by @ivanpauno and we can see if a way to provide rules externally can be added on top by Foxy freeze or not.

@ivanpauno

This comment has been minimized.

Copy link
Member

ivanpauno commented Mar 12, 2020

I'm not sure I understand how this would work. Does this mean that various RMW implementations would provide an additional package depending on sros2 to influence its' generation method?

I mean, that it would be good to be able to extend sros2 functionalities from external packages, so a different rmw implementation requiring different artifacts can extend sros2 functionality without modifying its code. The external package could provide the templates/schemas used to generate the artifacts, or could provide some functions to generate the artifacts from the ros policies/etc (I haven't evaluated how exactly).

Yeah it looks like it would be harder to ever go back to a point where it can be used standalone.
We could for now provide the rmw_implementation detection hack suggested by @ivanpauno and we can see if a way to provide rules externally can be added on top by Foxy freeze or not.

👍

@ivanpauno

This comment has been minimized.

Copy link
Member

ivanpauno commented Mar 12, 2020

Encapsulates a collection of subject privileges.
This is specific to a unique node instance as determined by associative attributes.
Attributes:

  • ns: Namespace of the node
  • node: Name of the node

Why do we still need the node name and namespace in the profile file?
Are they still necessary?

Considering that we mentioned that we are going to use the same set of security artifacts for all the Participants in a process, both when one is created per Node or per Context, I don't think that the node name/namespace is needed in the policy file.
Removing them will avoid having more nesting of tags, i.e.: contexts and context tags won't be needed, and a profile can be identified from the context name (instead of ns node).

@ruffsl

This comment has been minimized.

Copy link
Member Author

ruffsl commented Mar 12, 2020

Why do we still need the node name and namespace in the profile file?
Are they still necessary?

Those are a convenience for writing permissions in profiles, such that users can write composable and reusable permission blocks. Please look closely on how they are used in conjunction with xml includes to flesh out common node permissions, without having to be tremendously verbose.

https://github.com/ros2/sros2/tree/master/sros2/test/policies
https://github.com/ros2/sros2/blob/master/sros2/test/policies/common/node.xml

Without those attributes, users would have to hardcode the FQN of every relative/private topic/service/action rule. You may be conflating what those ns and node attributes in the profile tag are for, and why we've added the path in the context tag.

@ivanpauno

This comment has been minimized.

Copy link
Member

ivanpauno commented Mar 13, 2020

Those are a convenience for writing permissions in profiles, such that users can write composable and reusable permission blocks. Please look closely on how they are used in conjunction with xml includes to flesh out common node permissions, without having to be tremendously verbose.

https://github.com/ros2/sros2/tree/master/sros2/test/policies
https://github.com/ros2/sros2/blob/master/sros2/test/policies/common/node.xml

Without those attributes, users would have to hardcode the FQN of every relative/private > topic/service/action rule. You may be conflating what those ns and node attributes in the profile tag > are for, and why we've added the path in the context tag.

I see, I think I didn't explain myself correctly.
What I meant is: why don't replacing the old namespace/node_name with the context_name, and avoid adding the two new tags (contexts/context)?
Examples resulting files here: https://github.com/ivanpauno/sros2/tree/ivanpauno/new_policy_file_format/sros2/test/policies.


I've updated the rcl PR, relevant commits:

I've also updated rmw_fastrtps and rmw_connext PRs accordingly (I've to update the rmw_cyclonedds PR too, including adding a graph function to introspect the context name corresponding to a node:

(edit) Opened PR updating test_security.

@mikaelarguedas @ruffsl can you take a look?

@ivanpauno

This comment has been minimized.

Copy link
Member

ivanpauno commented Mar 13, 2020

@ruffsl @mikaelarguedas I can start helping in sros2 changes next week. Is there any ongoing/planned progress?

@ruffsl

This comment has been minimized.

Copy link
Member Author

ruffsl commented Mar 13, 2020

What I meant is: why don't replacing the old namespace/node_name with the context_name, and avoid adding the two new tags (contexts/context)?
Examples resulting files here:

From your commit here, that still wouldn't provide the XML template transforms the wherewithal to resolve the relative/private namespaces of resources I mentioned earlier, e.g.

Additionally, we still need to consider the cases where the context path may be orthogonal to the node namespace, like with ros2 containers collecting nodes of non overlapping namespaces.

The current design of the policy hierarchy is:

  • a <profile> tag is specific to a ROS2 node
  • a sequence of <profile> tags form a <profiles> tag
  • a <profiles> tag has some respective <metadata>
    • e.g specific to a DDS domain/participant, etc.
  • a sequence of <profiles> tags form a <context> tag
    • e.g specific to a process/user/robot, etc.
  • a sequence of <context> tags form a <contexts> tag
  • a <contexts> tag is a child of the root <policy> tag

There could be other ways to flatten the nested hierarchy here, but (IMO) not without incurring info duplication, leading to a less DRY security policy.


I've updated the rcl PR, relevant commits:

I've left a few comments:


Is there any ongoing/planned progress?

I'll try and push a sors2 PR this weekend, so we can iterate next week.

@ivanpauno

This comment has been minimized.

Copy link
Member

ivanpauno commented Mar 13, 2020

From your commit here, that still wouldn't provide the XML template transforms the wherewithal to resolve the relative/private namespaces of resources I mentioned earlier, e.g.

Sorry, I missed that part. Thanks for clarifying.

I'll try and push a sors2 PR this weekend, so we can iterate next week.

Thanks!

@ruffsl ruffsl mentioned this pull request Mar 15, 2020
4 of 7 tasks complete
@ros-discourse

This comment has been minimized.

Copy link

ros-discourse commented Mar 20, 2020

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2020-03-18/13313/1

@ivanpauno ivanpauno mentioned this pull request Mar 26, 2020
0 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.