add --secure option to launch with encryption #180

Arnatious · 2020-09-28T18:23:42Z

add implementation of --secure flag to have ros2launch handle configuring all launched actions to use secure ros functionality. Currently only handles encryption, access control functionality can be built from this.

Adds two new optional arguments to ros2 launch,

--secure [keystore_path]
Enable security. If a keystore path is provided, create and initialize it if necessary and use it. Otherwise generate an ephemeral keystore for this launch invocation only.
--no-create-keystore
Do not attempt to create/initialize a keystore or create an ephemeral keystore. Require keystore_path to point to an existing keystore.

The launch description is modified with the necessary steps to set environment variables as needed by rcl, and the creation of keys is handled automatically in the Node action type by making a best guess of the fully qualified node name using NoDL.

Adds dependencies on

NoDL: launch_ros
SROS2: launch_ros, ros2launch

kyrofa · 2020-10-23T20:02:11Z

@clalancette can we get some help poking those tests?

clalancette · 2020-10-23T20:59:44Z

@clalancette can we get some help poking those tests?

Do you need us to rerun the PR tests, run on ci.ros2.org, or something else?

kyrofa · 2020-10-23T21:01:12Z

Re-run the PR tests, please. The SROS2 PR on which this PR depended landed, but we can't seem to convince it to run again. We thought force-pushing would do it, but that doesn't seem to be the case. I can kick off the other CI.

clalancette · 2020-10-26T12:39:54Z

@ros-pull-request-builder retest this please

Arnatious · 2020-10-27T18:27:58Z

@clalancette we're getting a ERROR - ModuleNotFoundError: No module named 'sros2.keystore' error in the tests, is this because we need to generate a release of sros2 for rolling?

clalancette · 2020-10-27T18:33:14Z

@clalancette we're getting a ERROR - ModuleNotFoundError: No module named 'sros2.keystore' error in the tests, is this because we need to generate a release of sros2 for rolling?

Yeah, that's correct. You'll need to do a new release into Rolling before the PR job will succeed.

jacobperron

I've left some surface-level comments; haven't looked at the tests yet.

jacobperron · 2020-10-27T22:35:24Z

launch_ros/launch_ros/actions/node.py

+
+        self.__enclave = self.node_name.replace(
+            Node.UNSPECIFIED_NODE_NAME, nodl_node.name
+        ).replace(Node.UNSPECIFIED_NODE_NAMESPACE, '')


An unspecified namespace does not imply no namespace; it could be set within the node and therefore launch doesn't know the fully-qualified name of the node. Does this have any implication on the name of the enclave chosen?

The enclave must be the fully qualified node name. Unfortunately there is no way to get this prior to runtime, so we're using a best-guess here that a node won't namespace itself in code to make the secure launch function work at all.

The enclave doesn't need to be the fully qualified node name, right? It just needs to match up with the keypair that has been generated. Thankfully, here we're doing both-- generating the keypair and then passing the --ros-args necessary to use it by setting the enclave name. There's nothing requiring that enclave name to be anything special.

Note, however, that the security files IN the enclave must match up to the given participant name, which I think is where this becomes important.

ros2launch/ros2launch/api/api.py

ros2launch/ros2launch/command/launch.py

jacobperron · 2020-10-27T22:49:50Z

ros2launch/ros2launch/command/launch.py

+            metavar='keystore',
+            nargs='?',
+            const='',
+            help=('Launch using ROS 2 security features using specified keystore dir.'


nitpick:

Suggested change

help=('Launch using ROS 2 security features using specified keystore dir.'

help=('Launch with ROS 2 security features using the specified keystore directory.'

Signed-off-by: Ted Kern <ted.kern@canonical.com>

clalancette · 2020-11-06T19:38:15Z

launch_ros/launch_ros/actions/node.py

@@ -51,9 +51,13 @@
 from launch_ros.utilities import normalize_remap_rules
 from launch_ros.utilities import prefix_namespace

+import nodl


If I understand the situation correctly (from here), nodl is under a GPLv3 license. Because of that, I don't think we can take this into the launch_ros core. I'm going to double-check on this internally, so I'll get back with a definitive answer, but I just wanted to make you aware that this may be a non-starter.

Close-- It's LGPL, @clalancette, which should be better for integration. That's standing guidance from our legal department. Let us know if that proves to be an issue; we can have a conversation with them if necessary, so don't let that stand in the way of review.

We're re-licensing as Apache 2.0 in ubuntu-robotics/nodl#35

jacobperron · 2020-11-19T02:34:37Z

I've discussed this change briefly with the team, and we think it would make sense to add extensions points to ros2launch (and launch_ros as needed), such that this feature (and future additions) could be maintained separately and we don't have to worry about bringing in additional dependencies to launch_ros. It deserves more thought; I'm not sure exactly what this would look like.

kyrofa · 2020-11-24T17:49:30Z

It deserves more thought; I'm not sure exactly what this would look like.

No argument here, that may very well make sense, but given that such a thing doesn't exist today, what does that mean for this feature?

jacobperron · 2020-12-02T17:19:03Z

No argument here, that may very well make sense, but given that such a thing doesn't exist today, what does that mean for this feature?

I'm not sure. I'd rather not introduce more dependencies. In particular, introducing nodl creates additional maintenance work for the ROS 2 team, e.g. adding it to the ros2.repos files for CI/releases, ensuring it is kept up-to-date, and ensuring it is released at a similar cadence as other core packages. If we go this route, I think it would be better to move it to the ros2 GitHub org so that other members of the ROS 2 team can assist with maintenance if necessary.

Until someone gets around to making launch_ros.Node and/or ros2launch more extensible, perhaps a good compromise would be to make nodl a soft dependency. I.e. only enable the "secure" feature if we detect that nodl and sros2 are installed.

@ros2/team Thoughts?

ros-discourse · 2020-12-02T19:58:18Z

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

https://discourse.ros.org/t/eprosima-presents-visual-ros/17632/5

wjwwood · 2020-12-02T22:02:39Z

I agree with @jacobperron, one of:

move NoDL to ros2 org so it is less likely to disrupt releases of ros2launch (least preferable to me)
delay this until we add the feature making it extensible, eliminating the hard dependency (hopefully with help from you guys)
merge it with a "soft" dependency until the extensions can be made (not ideal, but perhaps the best short term compromise)

The second bullet is ideal in my opinion, but it also takes the most work and incurs the most delays. I really would rather not merge this as-is adding the hard dependency on NoDL for now.

kyrofa · 2021-01-04T19:21:41Z

Until someone gets around to making launch_ros.Node and/or ros2launch more extensible, perhaps a good compromise would be to make nodl a soft dependency. I.e. only enable the "secure" feature if we detect that nodl and sros2 are installed.

I'd say our only strong feeling is "not this". I believe a soft dependency will lead to confusing documentation, "why doesn't this work" bugs, tests that don't run regularly, and overall feature rot.

I think we all agree that making this more extensible is ideal, but that sounds like a fundamental restructuring of the project, which is honestly something we're not qualified to do. If someone wants to propose a design doc we can at least help vet it though, and might be able to help implement it in the future. I wonder if it might fit into @roger-strain's refactor.

gbiggs · 2021-01-26T01:21:12Z

Here's a PR that adds extensibility and here's a repository/PR that adds an extension.

clalancette · 2021-05-13T13:37:45Z

I do believe that we've now done this in a different way through #216 . I'm going to close this out, but please feel free to reopen if I'm wrong.

Arnatious force-pushed the secure_launch branch from e1a59eb to add25f3 Compare October 6, 2020 04:06

clalancette assigned ros-security-wg Oct 22, 2020

Arnatious force-pushed the secure_launch branch from add25f3 to ee23262 Compare October 22, 2020 22:04

jacobperron reviewed Oct 27, 2020

View reviewed changes

Arnatious and others added 3 commits November 6, 2020 19:08

initial secure launch implementation

12c3b59

Signed-off-by: Ted Kern <ted.kern@canonical.com>

update to new sros2 api, add security env flags

4a6bd4c

Signed-off-by: Ted Kern <ted.kern@canonical.com>

add missing sros2 dependencies

984f1cf

Signed-off-by: Ted Kern <ted.kern@canonical.com>

Arnatious force-pushed the secure_launch branch from 1effef7 to 984f1cf Compare November 6, 2020 19:09

clalancette requested changes Nov 6, 2020

View reviewed changes

This was referenced Jan 26, 2021

Add security extensions osrf/ros2launch_security#1

Merged

Add options extensions to ros2launch and extensibility to the node action #216

Merged

clalancette closed this May 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add --secure option to launch with encryption #180

add --secure option to launch with encryption #180

Arnatious commented Sep 28, 2020 •

edited

Loading

kyrofa commented Oct 23, 2020

clalancette commented Oct 23, 2020

kyrofa commented Oct 23, 2020 •

edited

Loading

clalancette commented Oct 26, 2020

Arnatious commented Oct 27, 2020

clalancette commented Oct 27, 2020

jacobperron left a comment

jacobperron Oct 27, 2020

Arnatious Nov 4, 2020

kyrofa Nov 5, 2020

kyrofa Nov 5, 2020

jacobperron Oct 27, 2020

clalancette Nov 6, 2020

kyrofa Nov 6, 2020 •

edited

Loading

Arnatious Nov 19, 2020

jacobperron commented Nov 19, 2020 •

edited

Loading

kyrofa commented Nov 24, 2020

jacobperron commented Dec 2, 2020

ros-discourse commented Dec 2, 2020

wjwwood commented Dec 2, 2020

kyrofa commented Jan 4, 2021

gbiggs commented Jan 26, 2021

clalancette commented May 13, 2021

	help=('Launch using ROS 2 security features using specified keystore dir.'
	help=('Launch with ROS 2 security features using the specified keystore directory.'

add --secure option to launch with encryption #180

add --secure option to launch with encryption #180

Conversation

Arnatious commented Sep 28, 2020 • edited Loading

kyrofa commented Oct 23, 2020

clalancette commented Oct 23, 2020

kyrofa commented Oct 23, 2020 • edited Loading

clalancette commented Oct 26, 2020

Arnatious commented Oct 27, 2020

clalancette commented Oct 27, 2020

jacobperron left a comment

Choose a reason for hiding this comment

jacobperron Oct 27, 2020

Choose a reason for hiding this comment

Arnatious Nov 4, 2020

Choose a reason for hiding this comment

kyrofa Nov 5, 2020

Choose a reason for hiding this comment

kyrofa Nov 5, 2020

Choose a reason for hiding this comment

jacobperron Oct 27, 2020

Choose a reason for hiding this comment

clalancette Nov 6, 2020

Choose a reason for hiding this comment

kyrofa Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

Arnatious Nov 19, 2020

Choose a reason for hiding this comment

jacobperron commented Nov 19, 2020 • edited Loading

kyrofa commented Nov 24, 2020

jacobperron commented Dec 2, 2020

ros-discourse commented Dec 2, 2020

wjwwood commented Dec 2, 2020

kyrofa commented Jan 4, 2021

gbiggs commented Jan 26, 2021

clalancette commented May 13, 2021

Arnatious commented Sep 28, 2020 •

edited

Loading

kyrofa commented Oct 23, 2020 •

edited

Loading

kyrofa Nov 6, 2020 •

edited

Loading

jacobperron commented Nov 19, 2020 •

edited

Loading