search

ros2 / design

Design documentation for ROS 2.0 effort
http://design.ros2.org/
Apache License 2.0
223 stars 194 forks source link

Add and update security designs for Contexts #274

Closed ruffsl closed 4 years ago

ruffsl commented 4 years ago

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 (https://github.com/ruffsl/design/pull/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.

ros-discourse commented 4 years ago

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

ivanpauno commented 4 years ago

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 commented 4 years ago

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: https://github.com/ros2/design/pull/274#discussion_r385249075

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 commented 4 years ago

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).

ruffsl commented 4 years ago

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 ="".

mikaelarguedas commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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:

That's the best option I could come up.

ivanpauno commented 4 years ago

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).

We could also add in rcl an explicit way to indicate the context name to follow an specific node name.

e.g.:

rclcpp::InitOptions init_options;

init_options.follow_node_name("my_node_namespace", "my_node_name");
rclcpp::init(argc, argv, init_options);

And it should also be possible to specify a context name directly:

rclcpp::InitOptions init_options;

init_options.set_context_name("my_context_name");
rclcpp::init(argc, argv, init_options);
ivanpauno commented 4 years ago

@ruffsl @mikaelarguedas @kyrofa opinions in which path to take?

The fastrtps refactor https://github.com/ros2/rmw_fastrtps/pull/312 and related PRs are ready to merge (wating for green CI).

I wrote here an updated list of remaining tasks: https://github.com/ros2/rmw/issues/183#issue-488869085. My idea is to propose merging the current PRs as-is and disabling security tests for fastrtps, and then restore security to a working state in some follow-up PRs (those PRs will be a blocker for Foxy). My main reason to propose that is that the size of the refactor is already huge (adding the diff of all PRs, about +8000 -3000), and I prefer making it land early so we have at least a month of testing before the release. Of course, restoring sros2 to a working state will be a blocker for Foxy. We still have 1 month until the API freeze and what's needed to be done is not much:

I have bandwidth to take care of the first two items, but we first need to define exactly what do we want. About the third item, do you have enough bandwidth to handle it before the Foxy API freeze (April 8th)? If not, I will ask in a ros2 meeting for additional resources (I can't promise anything).

Hope the plan sounds reasonable, please share your opinions about it.

ruffsl commented 4 years ago

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

Are you saying that before rcl reaches the logic for looking up the context directory, rcl still remains unaware of the namespace of the node that it is creating the context for?

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.

At what stage is this convention done by the user? When hard coding names in the source executable, when writing launch files, or by setting env variables when using ros2 run directly?

Node action uses /namespace/name as context name by default if both the node namespace and name were specified. ...

I've have a few uncertainties about the combination of cases, as I don't think those cover all the corner cases. I'd like to propose the following exhaustive matrix to demonstrate what I had in mind:

Ledgend Example
pushed_ns <push_ros_namespace namespace="foo"/>
node_ns <node namespace="bar"/>
node_name <node name="baz"/>
node_context <node context="spam"/>
hc_name Node("ham");
Symbol Discription
NULL Empty string
* Any/Empty string
. "." string
pushed_ns node_ns node_name node_context hc_name resolved_context
NULL NULL NULL NULL ham /ham
NULL bar NULL NULL ham /bar/ham
foo NULL NULL NULL ham /foo/ham
NULL NULL baz NULL * /baz
NULL bar baz NULL * /bar/baz
foo bar baz NULL * /foo/bar/baz
* /bar baz NULL * /bar/baz
foo bar * spam * /foo/bar/spam
* /bar * spam * /bar/spam
NULL NULL * spam * /spam
* * * /spam * /spam
foo bar * . * /foo/bar
* /bar * . * /bar
NULL NULL * . * /
* * * / * /

@ruffsl @mikaelarguedas @kyrofa opinions in which path to take?

Lets make this a primary purpose for the next security meeting on Tue. We didn't get enough time to sync on this over the last meeting, so I'd like to make sure this gets resolved before discussing any other agendas.

wjwwood commented 4 years ago

remains unaware of the namespace of the node that it is creating the context for?

Sorry, a bit of a drive by comment, but this doesn't make sense to me. What if a context contains two nodes which have different namespaces? This seems to be a misconception on the relationship between the context and the node. The context isn't owned by a node, instead nodes take a reference to a context. In fact a context doesn't even store a list of nodes that use it, afaik.

ruffsl commented 4 years ago

This seems to be a misconception on the relationship between the context and the node.

This question was in relation to the discussion for the common case where the user is only spawning one node per process, an what should the default context path be for that case.

If a context contains two nodes which have different namespaces, then we where thinking of something like this would be used to specify the context to the container process: https://github.com/ros2/design/pull/274#discussion_r384740668

wjwwood commented 4 years ago

I think generating a context path (name?) based on the node's settings is haphazard at best.

Maybe I can answer this other question too:

Are you saying that before rcl reaches the logic for looking up the context directory, rcl still remains unaware of the namespace of the node that it is creating the context for?

Yes, the context does not have access to this information when created.

Specifically this cell in your table:

pushed_ns node_ns node_name node_context hc_name resolved_context
NULL NULL NULL NULL ham /ham

I don't see how that result is technically possible.

Also, what if no node is created? Or what if the node is only conditionally created?

ruffsl commented 4 years ago

I don't see how that result is technically possible.

If no namespaces are pushed, and no namespace and name parameters are set in the <node/> element in the launch file, what name, other than that which is hard coded as the default in the source code, is used to create the node name and namespace?

Also, what if no node is created?

I hadn't thought of using ROS2 to create contexts/participants without any nodes. Is this common/possible with the current rcl API? Would this perhaps happen during the construction of the ComposableNode. If so, could the ComposableNode launch action be used to specify the context path?

If not using ros launch, then I suppose the node'less process could simply check the prioritized ROS_SECURITY_CONTEXT_DIRECTORY environment variable, then perhaps fallback to just "/" as the context path, and look for any security artifacts in the root folder of ROS_SECURITY_ROOT_DIRECTORY.

Or what if the node is only conditionally created?

I'm not sure what that means. A node that is loaded into a container and then later removed?

wjwwood commented 4 years ago

If no namespaces are pushed, and no namespace and name parameters are set in the element in the launch file, what name, other than that which is hard coded as the default in the source code, is used to create the node name and namespace?

There is none other, but that doesn't mean the context has access to the hard coded node name/namespace...

Is this common/possible with the current rcl API?

Common? no, possible? yes.

If not using ros launch, then I suppose the node'less process could simply check the prioritized ROS_SECURITY_CONTEXT_DIRECTORY environment variable, then perhaps fallback to just "/" as the context path, and look for any security artifacts in the root folder of ROS_SECURITY_ROOT_DIRECTORY.

Not sure I have the context needed to comment on whether or not that make sense, but I will say it seems to me the context name should either be set explicitly by the user (programmatically, via command-line, or via a launch file) or it should default to something well known, e.g. "" or "default_context_name".

Or what if the node is only conditionally created?

I'm not sure what that means. A node that is loaded into a container and then later removed?

rclcpp::init(...);

if (condition) {
  auto node = std::make_shared<rclcpp::Node>(...);
}

You need to handle what's possible, not just what is common place.

ruffsl commented 4 years ago

You need to handle what's possible, not just what is common place.

In the example above, is the context first initialized at rclcpp::init(...);, via rcl_get_zero_initialized_context?

I think generating a context path (name?) based on the node's settings is haphazard at best.

This touches back on what are intuitive ways of organizing/compartmentalizing contexts. Namespaces are nice, being familiar concepts to ROS users, but if rcl contexts are ignorant of node settings, like node names/namespaces, then perhaps we shouldn't push this convention beyond ros launch.

For starting processes manually, perhaps a ros command argument, __context could be used to specify the context to the process, and subsequent parsed via args passed to rclcpp::init(...):

$ ros2 run demo talker --ros-args __context:=/spam
$ ros2 run demo listener --ros-args __context:=/baz/spam

For multiple contexts per process, https://github.com/ros2/design/pull/274#discussion_r384737510 , perhaps this would be appropriate:

$ ros2 run composition manual_composition --ros-args \
  -r talker:__context:=/spam -r listener:__context:=/baz/spam
wjwwood commented 4 years ago

In the example above, is the context first initialized at rclcpp::init(...);, via rcl_get_zero_initialized_context?

Yes, and sort of. rcl_get_zero_initialized_context is used first, but the actual setup of the context is done with rcl_init(). Also, there's rmw_context_t to consider as well.

but if rcl contexts are ignorant of node settings, like node names/namespaces

They are for sure.

then perhaps we shouldn't push this convention beyond ros launch.

I can't say if that's the best thing to do, maybe it is, but launch_ros needs to communicate its decision to the node somehow (usually done via command line arguments), and anything you can do with a launch file should be possible to do without it, even if it isn't convenient.

ros2 run demo talker --ros-args __context:=/spam

Being able to change context names without recompiling seems reasonable to me, but it's unclear to me that all the cases can be covered in that way.

This assumes there's one context, it seems, what happens if there's more than one? Do all of them change names?

ros2 run composition manual_composition --ros-args \ -r talker:context:=/spam -r listener:context:=/baz/spam

I don't think this style is technically possible. Once again you're trying to set the name of a context based on a node's name. The context is created before the node name is known. Do you change the context name after creating it? I don't see how it would be implemented successfuly.

How do you know there's a different context for talker and listener (I'm assuming talker and listener are nodes). What if they shared a context, which name will it have?

I went back and read some of the previous conversation and my input is...

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

ruffsl commented 4 years ago

I can't say if that's the best thing to do, maybe it is, but launch_ros needs to communicate its decision to the node somehow (usually done via command line arguments),

you're trying to set the name of a context based on a node's name. The context is created before the node name is known. Do you change the context name after creating it?

Hmm.. How about we roll back to the ideas currently in the design doc, as of 8da5c02b2daef48c7791b3bedd82e63a8e844151 ; such that we avoid the use of node names in the ros launch equation, and instead simply encourage the convention of using matching node and relative context names for node launch actions in launch files:

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.

<node name="baz" context="baz" ... />

Scenarios for composing ros launch files with contexts then simplifies to a smaller table:

pushed_ns node_ns node_context resolved_context
NULL NULL NULL /
NULL bar NULL /bar
foo NULL NULL /foo
foo bar NULL /foo/bar
* /bar NULL /bar
foo bar spam /foo/bar/spam
* /bar spam /bar/spam
* * /spam /spam
NULL NULL spam /spam

and anything you can do with a launch file should be possible to do without it, even if it isn't convenient.

Given the simplified scenarios, to replicate the composition ros launch would automate:

NODE_NS="/bar"
NODE_NAME="baz"
ros2 run demo talker --ros-args \
    __ns:=${NODE_NS} \
    __node:=${NODE_NAME} \
    __context:=${NODE_NS}/${NODE_NAME}

This assumes there's one context, it seems, what happens if there's more than one?

From a security standpoint, I'd actually like to disallow more than one set of security artifacts from being used per process, particularly for information flow control analysis, as mentioned in the design doc.

For use cases using more than one participant per process, e.g. for messaging over two separate DDS domains simultaneously, both DDS participants should be able to use the same security artifacts, e.g. identity and permission files. SROS2 could simply be extended to export the DDS Rules as per required for either relevant DDS DomainIdSet.

https://github.com/ros2/sros2/blob/44479cd28a4ec946efd805a0041b20a2f30bf71e/sros2/sros2/policy/schemas/dds/permissions.xsd#L42

For remaining use cases using more than one participant per process, but all simultaneously on the same DDS domain, given the security boundaries of a shared process space, it wouldn't really serve a purpose to load security permissions from separate context paths that could otherwise be explicitly combined into one context path anyway. We could have SROS2 understand what permissions to collapse via added metadata to the SROS2 policy schema.

Do all of them change names?

In summery, one could still have multiple contexts/participants per process, but all contexts would load from the same context path specified at run time. This would also match the expected behavior of using the ROS_SECURITY_CONTEXT_DIRECTORY env to override what context path is loaded for all contexts in the process.

Automatically changing the context name based on the node namespace is not wise, mostly because it hides important details from the user and it's hard/impossible to implement without launch.

I think the simplification of launch behaviors above could address the later point. For the former: I think we'll want to emphasize how interconnected namespaces and permissions are in ROS2. If one where to change the namespace of a node, it's subsequent security permissions would also necessitate changes. Having the context path by convention also change to reflect this could remind users the necessity to create new permissions for the new context, even when all the user thinks that they've done was pushing an included launch file to a different namespace.

I think defaulting to "/" or "" when a context is created programmatically without any explicit hard coded name, is the best way to do it.

I agree "/" is a clear default. By hard coding, you mean like what @ivanpauno suggested here

rclcpp::InitOptions init_options;

init_options.set_context_name("my_context_name");
rclcpp::init(argc, argv, init_options);

Let say, if all contexts loaded from the same context path, then I'm not sure what use cases there could be for naming the contexts in the same process uniquely. I suppose if we wanted the context name to bubble up to the ros graph API to query such telemetry remotely, then having unique context names per process could help distinguish them, but that seems like something GUIDs in DDS and node names could already do. If all contexts shared security artifacts from a common context path/directory, then that path string would be the most relevant for auditing context permissions.

For example, your chart above @ruffsl includes hc_name but you can also hard code the node's namespace, so there should be a hc_namespace too I guess?

After writing all of the above: "hard code the node's namespace". Gosh dang it...

ivanpauno commented 4 years ago

Thanks @wjwwood and @ruffsl for the discussion.

I will try to go from the simplest thing to the more complex one:

  1. We need a way to modify the context name when running an executable. Considering that the Context doesn't know about nodes, and that it should be possible to specify the context name when manually running the executable, we need an additional argument for that. The simplest thing is to add remapping rules for it:
ros2 run package executable --ros-args -r __context:=my_context_path
ros2 run package executable --ros-args -r original_path:__context:=my_context_path

  1. We could then just not add anything else, and make the context/name path selection completly independent of the node name. I would use the following conventions to name contexts:

    • In single node executables, the context name is the same as the (fq) node name.
    • When running nodes in a component container, the context name is the same as the (fq) component container name.
    • When doing manual composition of multiple nodes, the context name is made up by the author. If only one context is created in the executable, we can have a convention of using a default name, like "" "/" or "default_context_name". This convetions would be enforced just by harcoding the respective context name in the executables.

  2. We then can say that we don't like harcoding stuff, and add some extra batteries to make this conventions simpler (e.g.: without adding anything else, if we want to remap in launch both the context name and the node name in a single node executable, we would need to explicitly do both independently). I agree with @wjwwood that adding magic to launch that can then not be reproduced manually isn't nice, though I have proposed before some ways of doing that (see https://github.com/ros2/design/pull/274#issuecomment-592700819). The other option is to add a way to make the context name "follow" the node name. Here an example:

    rclcpp::InitOptions init_options;
init_options.set_context_name(get_expanded_and_remapped_node_name("original_node_namespace", "original_node_name"));
rclcpp::init(argc, argv, init_options);
// EXECUTABLE THAT CREATE ONE NODE AND OTHER STUFF LATER

    That would be possible by exposing the way a node does name remapping and expansion. Then it will work like:

    ros2 run package executable --ros-args -r __node:=my_new_node_name -r __ns:=my_new_ns
# Context path is also changed to: `/my_new_ns/my_new_node_name
ros2 run package executable --ros-args -r __node:=my_new_node_name -r __ns:=my_new_ns __context:="my_context_path"
# Context is set to `my_context_path`. The specified name is always taken as absolute name, and never like a relative name, neither a private name is allowed.

From the list above, I will start implementing item 1., which doesn't really have any alternative. Item 2. can easily be done in a follow-up (it's just making our current examples to apply the conventions). Item 3. can be discussed, or an alternative to it. But I think that just having an independent way of setting the context name is what we really need.

ruffsl commented 4 years ago

If only one context is created in the executable, we can have a convention of using a default name, like "" "/" or "default_context_name". This convetions would be enforced just by harcoding the respective context name in the executables.

As an aside, @ivanpauno , what is the rational for an rcl context to possess a name?

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

I see how a user who hardcodes different names for separate rcl context's they construct would lend itself to this remapping pattern, but I'm not seeing why we should allow separate rcl contexts in a single process to load security artifacts from different context paths. Thus anything beyond -r __context:= shouldn't be necessary, given -r __context:= could just apply to all contexts in the process equally.

Otherwise, we'd be propagating the same security issues we had from before contexts, where a process composing multiple nodes would load multiple security artifacts, and this relaxation of IFC couldn't be accounted for in the policy model, nor made explicit offline to security policy authors.

ivanpauno commented 4 years ago

As an aside, @ivanpauno , what is the rational for an rcl context to possess a name?

Well, I think it was an idea from one of the meetings we had to discuss this topic really. I also mentioned the possibility of just specifying on path per process, and don't allow using different security artifacts for Contexts created in the same process. The conclusion of that meeting is that that was restrictive for some special cases (particularly, domain bridge applications).

I don't care about dropping the context name and just using a global path for the security artifacts of all contexts, but I think this has been already discussed before.

ruffsl commented 4 years ago

The conclusion of that meeting is that that was restrictive for some special cases (particularly, domain bridge applications).

IIRC, during that meeting was where we first discussed the security ramifications of a shared process space. I think the idea of naming rcl contexts came about due to the notion of overhauling rcl to account for deficiencies in SROS2 policy expressions. What we didn't think of then was to instead just modify SROS2 to accommodate domain bridge applications instead. I'll update the schema to enable special cases for multi domain bridging.

mikaelarguedas commented 4 years ago

For use cases using more than one participant per process, e.g. for messaging over two separate DDS domains simultaneously, both DDS participants should be able to use the same security artifacts, e.g. identity and permission files. SROS2 could simply be extended to export the DDS Rules as per required for either relevant DDS DomainIdSet.

@ruffsl One case this prevents is if you need to bridge 2 different protocols that load their own set of security artifacts. I know this is not a currently tested use case but it is a probable one in the future. But it's fine to not allow that for now and revisit the day such an alternative middleware is supported.

Thanks @ivanpauno for expanding the suggestions.

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

@ivanpauno I understand the first one but don't really get the second one. What is original_path in that case? Is original_path the hardcoded/litteral RCL Context name ?

This convetions would be enforced just by harcoding the respective context name in the executables.

By enforced, do you mean that users writing their nodes should all follow these conventions when creating a node. Is that correct?

Could we clarify that with some launchfiles examples. One thing that is still fuzzy for me is the one that sparked this PR in the first place (disclaimer I'm far from being familiar with the ROS2 XML launch syntax). How does my top level launch file defines contexts for the entire application without manually modifying launch files or code of released packages?

Lets say I have one launchfile that is

<launch>
  <node pkg="demo_nodes_cpp" exec="talker"/>
  <node pkg="demo_nodes_cpp" exec="listener"/>
</launch>

and a higher level launchfile that is:

<launch>
  <group,process,container name="group1">
    <push_ros_namespace namespace="new_namespace">
    <include file="/path/to/launchfile"/>
    <node pkg="demo_nodes_cpp" exec="add_two_ints_server"/>
  </group,process,container>
  <node pkg="demo_nodes_cpp" exec="add_two_ints_client"/>
</launch>

How can I modify this launchfile to have:

  1. all the participants using the same credentials / context path
  2. all the participants in "group1" using "/my/context/path1" and add_two_int_clients to use "/context/path2"
  3. all the participants in "group1" using "/new_namespace"

More SROS2 specific @ruffsl How would we name the profiles to reflect that? Especially, how do we handle the expansion of topics/services ? e.g. I have a topic "~/get_parametersRequest" how would that get expanded to "/node_namespace/node_name/get_parametersRequest"?

wjwwood commented 4 years ago

I think it will be hard to follow the conventions you mentioned @ivanpauno, like matching the context path to the node name/namespace in the single node executable case. Simply because you need to know the node name/namespace before creating the context (likely only launch could do this) or the user would need to hard code the name/namespace in two places, which would be annoying.

Instead, I'd propose a convention that the context path (name) is "", or maybe "/", in a single context per process situation.

If there is more than one context per process (not the common case most likely), then the context path may be set on each context manually so that different security artifacts can be provided for each. We knew that it was not safe to mix security artifacts within a single process, but the use case was to support cases where you must use more than one participant, like when you have a pair, each on a different DDS domain, which is what @mikaelarguedas was speaking about.

So in the common case the context would not have a name and there would only ever be one set of security artifacts and/or settings per process. And only in exceptional cases would you name the contexts (most likely manually or via remapping perhaps) and also have more than one set of security artifacts/settings.

Therefore, I think it would be ok to skip fancy features like remapping the context name via the command line, because presumably the developer knows that if a process has two contexts they need to be named differently. The only exception I can imagine is where some kind of plugins load their own contexts for some reason, then they would not have had a chance to coordinate, but then remapping would not help much either.

You could also avoid the context path/name altogether for simplicity, but we already know of a practical case where that would make configuring two contexts hard (two participants on two different domains), or at least it would be hard from my understanding.

mikaelarguedas commented 4 years ago

So in the common case the context would not have a name and there would only ever be one set of security artifacts and/or settings per process. And only in exceptional cases would you name the contexts (most likely manually or via remapping perhaps) and also have more than one set of security artifacts/settings.

The main use case for being able to have different context names is for different processes to load different credentials based on their context names. Otherwise the only way to specify where to look for credentials is via environment variables, so it applies to all processes started in that shell. Which means If I have a single application top level launch file, all the nodes of my entire system will use the same credentials and permissions. Which is definitely not desired.

One way to avoid that without remapping context could be to override the environment for each process started by launch to point it to the right place. It would simplify significantly the lookup mechanism in RCL but doesn't seem as nice as being able to remap contexts for some reason.

wjwwood commented 4 years ago

The main use case for being able to have different context names is for different processes to load different credentials based on their context names.

Ok, I somehow assumed you'd end up with a different set of credentials (and/or settings) for each context, and in the one context per process case you'd point that process to the exact credentials it needed using the environment variable, rather than pointing them all at the same place and giving each a name programmatically. But either pattern should work in my opinion if you have remapping.

My thought was that you would either:

That would be the "bare" functionality, and then launch could do more sophisticated things using these mechanisms.

If you tend to go with the latter pattern, then having some conventions on what to name them makes sense, but I still think basing those names on the node name is not great because, as I said before, either the user has to hard code the names or you must pass overrides for the node name and namespace, otherwise that information is not available to the context when it is created.

I guess I prefer this (whether run by hand or by launch):

$ ROS_SECURITY_ROOT_DIRECTORY=/path/to/sec/contexts/talkerA \
    ros2 run demo_nodes_cpp talker --ros-args -r __node:=talkerA
# and then in another terminal
$ ROS_SECURITY_ROOT_DIRECTORY=/path/to/sec/contexts/talkerB \
    ros2 run demo_nodes_cpp talker --ros-args -r __node:=talkerB

In the above case, both have a context name of "" or "/", but have separate documents in contexts/talkerA and contexts/talkerB.

Another way to do it, would be to have it like this:

$ ROS_SECURITY_ROOT_DIRECTORY=/path/to/sec \
    ros2 run demo_nodes_cpp talker --ros-args -r __node:=talkerA -r __context:=talkerA
# and then in another terminal
$ ROS_SECURITY_ROOT_DIRECTORY=/path/to/sec \
    ros2 run demo_nodes_cpp talker --ros-args -r __node:=talkerB -r __context:=talkerB

In the above case, each process would have a differently named context, and they would find their separate credentials relatively from the root security directory.

What does not work, in my opinion is trying to do this and expecting it to just work some how:

$ ROS_SECURITY_ROOT_DIRECTORY=/path/to/sec \
    ros2 run demo_nodes_cpp talker --ros-args -r __node:=talkerA
# and then in another terminal
$ ROS_SECURITY_ROOT_DIRECTORY=/path/to/sec \
    ros2 run demo_nodes_cpp talker --ros-args -r __node:=talkerB

Note the context is not remapped, and with the assumption that the context names are the same (running the same executable twice).

Hopefully that's more clear about what I was envisioning. I think the above satisfies the use cases I've heard so far.

ruffsl commented 4 years ago

One case this prevents is if you need to bridge 2 different protocols that load their own set of security artifacts. I know this is not a currently tested use case but it is a probable one in the future. But it's fine to not allow that for now and revisit the day such an alternative middleware is supported.

We knew that it was not safe to mix security artifacts within a single process, but the use case was to support cases where you must use more than one participant, like when you have a pair, each on a different DDS domain, which is what @mikaelarguedas was speaking about.

I've just updated the policy schema to help resolve this case. Users shouldn't be required to compromise fidelity of security to support cross domain/transport use cases, and we probably don't want to ignore this given how security sensitive bridges are in the likes of IIoT, automotive systems, being among the major bastion interconnects between varying security layers. See e387ac1

Example Policy for bridge applications

The following is an example policy that defines a context for an application that bridges across either different DDS domains given multiple DDS participants or even entirely different middleware transport types. SROS2 can then interpret the metadata associated to respective profiles in the common context, such that the application can find all its necessary security artifacts under the common context path in the keystore.

``` xml 3 10 4 2 ```

How would we name the profiles to reflect that?

@mikaelarguedas , I'm not sure I grasp the question. Are you asking how this all ties back to topic mangling of relative paths from ROS2 to DDS? I don't think any lower level topic transformation would have to change, only how we construct allow_rules in the higher level grants, in the DDS use case.

For DDS, the resulting security artifacts would look the same, only the permission.xml would be procedurally generated from the context above to look like this:

``` xml CN=/my/context_path 2013-10-26T00:00:00 2023-10-26T22:45:30 3 10 rq/talker/describe_parametersRequest ... rt/chatter rq/talker/describe_parametersRequest ... 4 2 rq/listener/describe_parametersRequest ... rq/listener/describe_parametersRequest ... rt/chatter DENY ```

The main use case for being able to have different context names is for different processes to load different credentials based on their context names. Otherwise the only way to specify where to look for credentials is via environment variables, so it applies to all processes started in that shell. Which means If I have a single application top level launch file, all the nodes of my entire system will use the same credentials and permissions. Which is definitely not desired.

+1 We'll most definitely want a way for ros launch to start different processes with separate context paths for scalable/modular bring-up of nodes using security.

One way to avoid that without remapping context could be to override the environment for each process started by launch to point it to the right place. It would simplify significantly the lookup mechanism in RCL but doesn't seem as nice as being able to remap contexts for some reason.

-1 As mentioned in https://github.com/ros2/design/pull/274#discussion_r384739409 , that may be a particularly painful ani-pattern for compatibility, and that we should probably discourage.

mikaelarguedas commented 4 years ago

See e387ac1

:+1:

I'm not sure I grasp the question.

I got confused and thought the node ns and name information was not available in the policy anymore. But it will still be in the profile itself regardless of how the context is named: https://github.com/ros2/sros2/blob/44479cd28a4ec946efd805a0041b20a2f30bf71e/sros2/test/policies/talker_listener.xml#L5

As mentioned in #274 (comment) , that may be a particularly painful ani-pattern for compatibility, and that we should probably discourage.

Yeah what I meant is that it gives the same amount of granularity as remapping context on the commandline. And it seems that we wouldn't more granularity as we had before with fancy lookup strategies like matching prefix. I do prefer something decorrelated from the env vars so that we dont lock ourselves in a corner when time comes to support more advanced use cases that don't support/allow environment variables

ivanpauno commented 4 years ago

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

@ivanpauno I understand the first one but don't really get the second one. What is original_path in that case? Is original_path the hardcoded/litteral RCL Context name ?

Exactly. As mentioned earlier, remapping the node name/namespace in an executable with multiple nodes works in the same fashion: https://index.ros.org/doc/ros2/Tutorials/Node-arguments/#passing-remapping-arguments-to-specific-nodes.

@wjwwood I agree with @ruffsl that remapping (or another form of command line argument) should be prefered over making use of environment variables for compatibility reasons.

What does not work, in my opinion is trying to do this and expecting it to just work some how:

$ ROS_SECURITY_ROOT_DIRECTORY=/path/to/sec \
    ros2 run demo_nodes_cpp talker --ros-args -r __node:=talkerA
# and then in another terminal
$ ROS_SECURITY_ROOT_DIRECTORY=/path/to/sec \
   ros2 run demo_nodes_cpp talker --ros-args -r __node:=talkerB

Note the context is not remapped, and with the assumption that the context names are the same > (running the same executable twice).

@wjwwood Yes, agreed.

The following is an example policy that defines a context for an application that bridges across either different DDS domains given multiple DDS participants or even entirely different middleware transport types. SROS2 can then interpret the metadata associated to respective profiles in the common context, such that the application can find all its necessary security artifacts under the common context path in the keystore.

@ruffsl I don't have good background on how DDS security work. My main question is: can cross-domain applications be implemented using the same set of security artifacts in all the Participants? If the answer is yes, I agree in just having one security directory for every context in a process, and not having different context names. It will also make my life as a developer easier (because it's much easier to implement :smile:).

@ruffsl One case this prevents is if you need to bridge 2 different protocols that load their own set of security artifacts.

I repeat this question here. Could bridging between different protocols work with the same security artifacts? Or more generally, is there any possible DDS bridging application limited by using the same set of security artifacts in all Participants? or that doesn't impose any limitation?

mikaelarguedas commented 4 years ago

can cross-domain applications be implemented using the same set of security artifacts in all the Participants?

Yes, both the governance and permission files allow to specify domain iDs. So you can easily say "set of permissions A applies to domain 1 and 2" and "set of permissions B applies to domains 5 to 20" in the same permissions file.

Could bridging between different protocols work with the same security artifacts?

Yeah this was more theoretical as currently DDS is the only officially supported protocol. As DDS invented its' own governance and permission files we should be good on that front (the other protocol would not use those). You could imagine a conflict arising when different protocols do rely on key and certificates but impose different configurations for those (e.g. DDS uses a specific elliptic curve and the other protocol specifies something else). We can always work around it by generating different artifacts for different protocols and point them to their respective artifacts.

ivanpauno commented 4 years ago

@mikaelarguedas Thanks for the answer. Based on all the previous answers, my conclusion is:

Based on that, I suggest the following:

/path/to/ros2/executable --ros-args --security-directory /path/to/security/directory

i.e.: just adding an argument for it.

For rmw implementations that weren't migrated yet (one Participant per Node), we can:

I think that doing option one make sense and will actually make the security directory layout the same for all rmw implementations regardless if one Participant is created per Node or per Context (only one security directory per process will be needed).

wjwwood commented 4 years ago

Sounds good to me, looks like you guys have it under control, sorry if I derailed things a bit.

ivanpauno commented 4 years ago

Sounds good to me, looks like you guys have it under control, sorry if I derailed things a bit.

Thanks for the comments @wjwwood , much appreciated! The whole conversation derailed a bit from the original proposal :rofl:.

@ruffsl @mikaelarguedas I would like to hear if the proposal here sounds reasonable. I'm planning to start with that next Monday.

mikaelarguedas commented 4 years ago

I think that doing option one make sense and will actually make the security directory layout the same for all rmw implementations regardless if one Participant is created per Node or per Context (only one security directory per process will be needed).

:+1: that was one of the goals of this PR, to avoid having two different keystore layouts.

arguments are preferred over environment variables, because of compatibility reasons.

While arguments are preferred, the ability to override using environment variable should still be available though. For example to redirect all nodes of a launchfile to the same security directory without modifying code or launchfile

/path/to/ros2/executable --ros-args --security-directory /path/to/security/directory

I don't feel strongly about the name of the option. The new name makes me wonder though: Does that still change the name of the context or participant ? Will this change be visible on the ROS graph?

Currently SROS2 introspect permissions of a running system as follows:

This results in self contained, fully described permission set with the full name of the participant and all its permissions.

With this proposal 2 things are unclear to me: My understanding is that the ROS graph provides a list of ROS nodes (and not a list of participants/contexts).

ivanpauno commented 4 years ago

While arguments are preferred, the ability to override using environment variable should still be available though. For example to redirect all nodes of a launchfile to the same security directory without modifying code or launchfile

That's reasonable. I think it can work as following:

I don't feel strongly about the name of the option. The new name makes me wonder though: Does that still change the name of the context or participant ? Will this change be visible on the ROS graph?

That's a great question. If we have just one security directory per process, I wouldn't add the concept of context name. Though all contexts will have an associated security directory, that can be used as the "name". An introspection function to get the security path ("context_name") of each node can be added, i.e. a function returning a list of nodes, and a list of security directories. The "context name" would be the security path, without prefix.

P.S.: I prefer talking about "security paths" than "context names", and replace the proposed "contexts" tag with "processes".

ruffsl commented 4 years ago

ROS_SECURITY_ROOT_DIRECTORY is used as prefix, if exists. If not, / is the prefix.

Currently, the ROS_SECURITY_ROOT_DIRECTORY env is always required to use security unless the ROS_SECURITY_CONTEXT_DIRECTORY env is set to override the former. Perhaps we should keep to throwing a run-time error if neither envs are set, to remind the user they are required for security, rather than implicitly defaulting to the root of the OS's file system, which might lead to confusing error messages for users.

The --security-directory option is used directly if it's an absolute path. If it's a relative path, the resulting security directory is ${ROS_SECURITY_ROOT_DIRECTORY}/PROVIDED_ARGUMENT.

As an amendment:

By absolute path, you me an absolute path in the OS's file system, correct? e.g.

--security-directory="/home/bob/.ros/sros_keystore/contexts" I think allowing for absolute file system paths will make it tricky to trace back the context to its policy definition, as mentioned below.

The "context name" would be the security path, without prefix.

Again, by prefix, do you mean the absolute file system path prefix? I'd suggest introspection functions get the context path with respect to keystore root directory env, such that graph interactions from specific participant using a given set of security artifacts can be associated back to a element in the policy file, regardless of the file system location of the keystore root directory.

P.S.: I prefer talking about "security paths" than "context names", and replace the proposed "contexts" tag with "processes".

I agree paths is a better term than name, though I don't think "processes" would be as applicable, given currently a security context could be shared by multiple processes: e.g. say a designer decides to allocate all processes running under a given user group or entire device a common context to conservatively model the IFC of a larger distributed system.

ruffsl commented 4 years ago

For example, how I'd think of explaining these concepts in English:

ivanpauno commented 4 years ago

Currently, the ROS_SECURITY_ROOT_DIRECTORY env is always required to use security unless the ROS_SECURITY_CONTEXT_DIRECTORY env is set to override the former. Perhaps we should keep to throwing a run-time error if neither envs are set, to remind the user they are required for security, rather than implicitly defaulting to the root of the OS's file system, which might lead to confusing error messages for users.

:+1:

As an amendment:

  • unless the ROS_SECURITY_CONTEXT_DIRECTORY is set, that then take precedence over any provided arguments.

:+1:

By absolute path, you me an absolute path in the OS's file system, correct? e.g.

Yes.

Again, by prefix, do you mean the absolute file system path prefix? I'd suggest introspection functions get the context path with respect to keystore root directory env, such that graph interactions from specific participant using a given set of security artifacts can be associated back to a element in the policy file, regardless of the file system location of the keystore root directory.

"I'd suggest introspection functions get the context path with respect to keystore root directory env" That's exactly what I meant :+1: .

I agree paths is a better term than name, though I don't think "processes" would be as applicable, given currently a security context could be shared by multiple processes: e.g. say a designer decides to allocate all processes running under a given user group or entire device a common context to conservatively model the IFC of a larger distributed system.

Yes, I agree, processes isn't a good name. The current contexts is fine.

ivanpauno commented 4 years ago

I'm working in modifying the rcl PR, to adjust to the mentioned changes. I will also adjust the tests in system_tests accordingly. It would be great to update the document, to clarify everything. @ruffsl can you do that?

mikaelarguedas commented 4 years ago

ROS_SECURITY_ROOT_DIRECTORY is used as prefix, if exists. If not, / is the prefix. The --security-directory option is used directly if it's an absolute path. If it's a relative path, the resulting security directory is ${ROS_SECURITY_ROOT_DIRECTORY}/PROVIDED_ARGUMENT.

Sounds reasonable to me.

throwing a run-time error if neither envs are set, to remind the user they are required for security, rather than implicitly defaulting to the root of the OS's file system, which might lead to confusing error messages for users.

You mean only in the case where ROS_SECURITY_ENABLE is set ? otherwise we would be using the mapping described by @ivanpauno above?

An introspection function to get the security path ("context_name") of each node can be added, i.e. a function returning a list of nodes, and a list of security directories.

This same security path is what will be stored in the participant's USER_DATA ? where previously the node namespace and names were stored, is that right ?

ivanpauno commented 4 years ago

This same security path is what will be stored in the participant's USER_DATA ? where previously the node namespace and names were stored, is that right ?

Yes, I will exactly use that.

ruffsl commented 4 years ago

throwing a run-time error if neither envs are set, to remind the user they are required for security,

You mean only in the case where ROS_SECURITY_ENABLE is set ?

Yes

ruffsl commented 4 years ago

It would be great to update the document, to clarify everything. @ruffsl can you do that?

@ivanpauno , yep, I'll try and get this doc updated and pushed this afternoon.

ivanpauno commented 4 years ago

@mikaelarguedas @ruffsl There's another point to be discussed about changes after #250. There is some ros discovery information published in a topic called ros_discovery_info, by all created Contexts. That happens automatically, and the user can't change the topic name (ros_discovery_info is the actual DDS topic name, there's no name mangling in this case). I think that it would make sense to automatically generate permissions to read/write that topic, without the user explicitly adding it in a profile file.

ruffsl commented 4 years ago

That happens automatically, and the user can't change the topic name (ros_discovery_info is the actual DDS topic name, there's no name mangling in this case).

You're saying that there is now another DDS topic that every node will need to be given read and write access to? Uhh, this is just like parameter_events all over again, and it's not even optional for security sensitive users? I'm not liking these standard relaxations of IFC and Access Control.

ivanpauno commented 4 years ago

What type of discovery info is transmitted on that topic ? the topic matching events ?

Before, you could know what node created a publisher/subscription by checking the Participant guid that created it. Now, that information isn't available directly, and there's an extra topic to provide the extra information. The message definition is here: https://github.com/ros2/rmw_dds_common/blob/ivanpauno/first-implementation/rmw_dds_common/msg/ParticipantEntitiesInfo.idl.

I'm not sure what you mean by this, could you please detail or rephrase this part ?

I mean that all Participants need to be able to create a publisher and subscriber in that topic.

You're saying that there is now another DDS topic that every node will need to be given read and write access to?

Yes.

Uhh, this is just like parameter_events all over again, and it's not even optional for security sensitive users? I'm not liking these standard relaxations of IFC and Access Control.

The topic just provides information useful for being able to introspect the graph (i.e.: list publishers of a node, etc). I think that all DDS built-in topics have the same "relaxation" problem.

ruffsl commented 4 years ago

Before, you could know what node created a publisher/subscription by checking the Participant guid that created it. Now, that information isn't available directly, and there's an extra topic to provide the extra information.

The topic just provides information useful for being able to introspect the graph (i.e.: list publishers of a node, etc). I think that all DDS built-in topics have the same "relaxation" problem.

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? It should also be noted that automatic decentralized discovery is optional for DDS.

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

ivanpauno commented 4 years ago

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 commented 4 years ago

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.