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

Allow ROS2 CLI to support Security #69

Closed
ruffsl opened this issue Sep 12, 2018 · 9 comments
Closed

Allow ROS2 CLI to support Security #69

ruffsl opened this issue Sep 12, 2018 · 9 comments
Assignees
Labels
enhancement New feature or request

Comments

@ruffsl
Copy link
Member

ruffsl commented Sep 12, 2018

The ROS2 CLI utilizes a node_name_suffix to ensure an unique node name by including the process's pid:

https://github.com/ros2/ros2cli/blob/e5f67d0e0362462644a5530094fac43af84ca938/ros2cli/ros2cli/node/direct.py#L36-L38

This makes it difficult to provide any CLI the necessary root dir for its security artifacts at runtime.
I see a few solutions:

  1. passing an argument to the cli to drop the use of its node_name_suffix, however the loss of a unique FQN could bing unintended consequences.
  2. Add support for providing the exact path to the node_secure_root directory. This could be done by pre checking a new security environment variable that would take precedence over ROS_SECURITY_ROOT_DIRECTORY , e.g: ROS_SECURITY_NODE_DIRECTORY?

I like 2) as it would allow users to override the normal root lookup when using any CLI or debugging nodes with separate temporary credentials provisioned with super privileges, e.g "*" permissions for all topics.

@codebot
Copy link
Member

codebot commented Sep 12, 2018

Interesting points. I agree that (2) seems like a more general solution. I don't think there is any loss of security by going that route, since people could already change the node name during invocation anyway on the command line. Ideally it would also be possible to provide some "syntactic sugar" variable and/or command-line switch to handle the common case (namely, removing the PID suffix when looking up the node name in the security artifact directory tree).

@ruffsl
Copy link
Member Author

ruffsl commented Sep 12, 2018

Ideally it would also be possible to provide some "syntactic sugar" variable and/or command-line switch to handle the common case (namely, removing the PID suffix when looking up the node name in the security artifact directory tree).

That would also be a good idea! I did something similar in SROS1 to enable roslaunch/ros-topic/ros-etc, but it was sort of hacky and just searched for PID patterned suffixes and striped them:
https://github.com/ros/ros_comm/blob/f95b4a5de2acb4fb53f0e9a4cff47dcef928eac5/tools/rosgraph/src/rosgraph/security.py#L114-L118

I suppose we could signal and anonymous flag down to rcl, but it seems worth doing as it would simplify the use and organization of CLI utils that use PID suffixes. @codebot, would that be a use case for the cli_args argument?. Are there any other ROS2 examples of intentional anonymous node names?

@ruffsl
Copy link
Member Author

ruffsl commented Sep 12, 2018

Still not sure how to handle the service caller or topic publisher though:
https://github.com/ros2/ros2cli/search?utf8=%E2%9C%93&q=create_node&type=

Perhaps this an anonymous argument could specify the length of the prefix in the node name string to preserve, or some char mask list to normalise the node name string into a canonical form for security root lookup.

@AAlon
Copy link
Contributor

AAlon commented Oct 30, 2018

Hi, I've started a related discussion on Discoure: https://discourse.ros.org/t/ros2-security-cli-tools/6647 - would appreciate your input.

ruffsl added a commit to ros2/rcl that referenced this issue Oct 31, 2018
ruffsl added a commit to ros2/rcl that referenced this issue Nov 5, 2018
ruffsl added a commit to ros2/rcl that referenced this issue Nov 6, 2018
ruffsl added a commit to ros2/rcl that referenced this issue Nov 8, 2018
ruffsl added a commit to ros2/rcl that referenced this issue Nov 10, 2018
ruffsl added a commit to ros2/rcl that referenced this issue Nov 13, 2018
* Expand node_secure_root using local_namespace

* rcutils_normalize_path -> rcutils_to_native_path

* correct comments and docblock

* node_root_norm -> node_root_path

* normalized -> native

* Adding TODOs for function reuse

* Adding Null check node_secure_root

* Adding override for ROS_SECURITY_NODE_DIRECTORY
Addressing ros2/sros2#69

* Change from strcpy to memcpy

Signed-off-by: Ruffin White <roxfoxpox@gmail.org>
@ruffsl
Copy link
Member Author

ruffsl commented Nov 13, 2018

The following PRs addressing this issue have landed:

When using CLI tooling or debugging nodes, user may set ROS_SECURITY_NODE_DIRECTORY environment variable to overridingly specify the exact security artifacts path for the node is to use.

Next step is to document this feature and intended use pattern. Would the README file here be most appropriate, or would github.com/ros2/ros2_documentation more suited to hosting this.

ping @clalancette

@clalancette
Copy link
Contributor

Next step is to document this feature and intended use pattern. Would the README file here be most appropriate, or would github.com/ros2/ros2_documentation more suited to hosting this.

I think putting it in https://github.com/ros2/ros2_documentation would give it the most visibility, so I'd vote for putting it there. @tfoote may have some further advice.

@ruffsl
Copy link
Member Author

ruffsl commented Nov 15, 2018

Looking at github.com/ros2/ros2_documentation just now, it seem like its in a peculiar state. I can't immediately recognize if its supposed to be a ROS2 wide wiki, or just a collection of unorganized docs for core ROS2 packages, or something else. I made a comment about it here:

https://discourse.ros.org/t/ros-2-documentation-home-moving-to-index-ros-org-doc-ros2/6612/8

nburek pushed a commit to nburek/rcl that referenced this issue Nov 26, 2018
* Expand node_secure_root using local_namespace

* rcutils_normalize_path -> rcutils_to_native_path

* correct comments and docblock

* node_root_norm -> node_root_path

* normalized -> native

* Adding TODOs for function reuse

* Adding Null check node_secure_root

* Adding override for ROS_SECURITY_NODE_DIRECTORY
Addressing ros2/sros2#69

* Change from strcpy to memcpy

Signed-off-by: Ruffin White <roxfoxpox@gmail.org>
@ruffsl ruffsl self-assigned this Dec 14, 2018
@mikaelarguedas
Copy link
Member

As the relevant PRs have been merged for both the MATCH_PREFIX behavior and the ROS_SECURITY_NODE_DIRECTORY and the behavior is documented at http://design.ros2.org/articles/ros2_dds_security.html I believe this can be closed.

@ruffsl are there any remaining action items here?

@ruffsl
Copy link
Member Author

ruffsl commented Oct 24, 2019

Mission accomplished 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants