Security directory lookup improvements

[AAlon]

Summary

This is part of the improvements suggested here mainly intended for CLI tools' (and dynamic node names in general) usability with security (also see ros2/sros2#69).
These changes will make it possible to create a security directory for the nodes launched by the ros2cli tool (including rosbag2), overcoming the issue of unknown node names, and without having to specify a directory override.

Changes overview

• Introduce prefix matching for the security directory, only for the case where an override was not specified. Thus a node called ros2cli_node_814918 will be able to load directory named ros2cli_node (assuming it has the necessary system permissions, of course).
  • This utilizes tinydir for lightweight cross-platform file lookup.
• Move rcl_get_secure_root out of node into a new security_directory module and add unit tests to cover (hopefully) all cases.
• Add logging notifying which security directory is going to be loaded
• Small bugfix on line 174 where we previously called deallocate even if node_secure_root is NULL.

Related changes

• ros2cli and rosbag2 to launch all nodes with a consistent ros2cli_node prefix.
  • Consistent node naming ros2cli#158
  • Consistent node naming across ros2cli tools rosbag2#60

Refer to

ros2/sros2#69
https://discourse.ros.org/t/ros2-security-cli-tools/6647/

[AAlon]

Note about the lint tests - they failed mainly because multiple issues in the imported tinydir header. What should we typically do in such cases?

[ruffsl]

As from the discourse thread, I commented on my personal aversion to more involved resource location resolution, in part by how the added complexity may result in unintened consquenses, but didn't give an example. For a longest matching prefix approach, I'll formulate such a vulnerability below:

A user is migrating a ROS2 system to use security and have a nested set of structure namespaces to organize and segment the computation graph. The use has two nodes with the FQN: /foo/bar and /foo/bar/baz, each with their own security set of permissions necessary for the capabilities each requires. The permission for /foo/bar/baz is made restrictive and is a subset of the grater permissions granted to /foo/bar.

However, upon deployment, the secure_node_root directory /foo/bar/baz is somehow deleted or omitted from disk, and so upon runtime the longest matching prefix approach would silently resolve the secure_node_root directory, and fallback to /foo/bar, thus unintentionally (or maliciously if the case may be) escalating the permissions of the /foo/bar/baz node.

I release this might be a contrived case, but given the FQN hierarchy or ROS applications, and a shared security root path, I think this case of silent upgrading/downgrading/alternate of permissions/credentials from resolving the wrong or unintended directory would be possible or common.

Arguably, you might suggest using something like linux security modules to restrict file system access for separate nodes; but this would complicate the use of tinydir to traverse the secure root directory. Alternately you could restrict FQN expansion to the node name, not the namespace prefix, yet /foo/bar and /foo/bar_baz would fall prey to this as well.

I think being explicit in setting the FQN of the node or explicitly setting the secure_node_root directory, rather than having any interpreter operate on uncontrolled data input, would be safer. Better still would be to refactor or extend the fetching/referencing of security artifacts as a plugable system, as I commented down in this review.

[ruffsl]

+1 for breaking out security related functions, and organizing them under their own source file.

[ruffsl]

Nice catch, line 174 was a bug.

[AAlon]

Hey, I was wondering whether you had any thoughts about my latest replies. It would be great to get this in to Crystal. I can improve the logging to include the actual resolved location but I'm waiting to hear back about the other comments.

[ruffsl]

One thing I guess I'm not seeing is when would be the case where the development CLI tools in the same shell environment require different credentials and permissions? If I was developing, I'd at least want all my CLI tools to have developer level privileges so I could introspect the entire ROS system, and so they would have equivalent credentials, thus I might as well set the secure node environment to control what they use exactly, regardless of what CLI tool I trust to use it. And if I did need to use two tools at the same time with different credentials, wouldn't one use multiple shells?

I added some quoted citations to give a better background to my other comment above. I'll list the title below.

Saltzer, Jerome H., and Michael D. Schroeder. "The protection of information in computer systems." Proceedings of the IEEE 63.9 (1975): 1278-1308.

Given I already let a null error slip my previous review in line 174, I don't think I'm the best to ask in auditing all of tinydir. Perhaps another contributor provide more thorough feedback.

I can improve the logging to include the actual resolved location but I'm waiting to hear back about the other comments.

I'd still like to see this logging make it into crystal, great for debugging and given new users better hints.

[AAlon]

One thing I guess I'm not seeing is when would be the case where the development CLI tools in the same shell environment require different credentials and permissions? If I was developing, I'd at least want all my CLI tools to have developer level privileges so I could introspect the entire ROS system, and so they would have equivalent credentials, thus I might as well set the secure node environment to control what they use exactly, regardless of what CLI tool I trust to use it. And if I did need to use two tools at the same time with different credentials, wouldn't one use multiple shells?

You'd want the CLI tools to have elevated permissions (or, developer level privileges), but you wouldn't want other nodes to have those. That's relevant both for a development environment and a production system. For a development environment, you'd be launching nodes "manually" from a shell, and you'd also be using CLI tools, either via the same shell you launched a node or in a different one - both workflows are possible.

The thing is, if developers are limited to using environment variables, one of two things would happen:

1. During development they would deviate from the intended setup for the real world and would set up elevated privileges for essentially all nodes (e.g. by setting the node securiy dir env var in .bashrc). So throughout development they would have different conditions to what they would have in the production system. This is not ideal, any discrepancies between the dev/testing environment and the production system should be minimized.
2. Or, if they wouldn't want to deviate from the final setup - they would need to make sure that the overriding environment variable is correctly set whenever they use the CLI tool, open new shells, or go and launch new nodes. This would be tedious and cumbersome. Note that this could be avoided if we swap the preference so that a matching directory would be preferred over an environment variable (right now the env var has priority).

[AAlon]

We really have to close on this, as I see it we have three options, from best to worst:

• Option 1: If I managed to convince you - I can make the CMake change to pull in tinydir, fix any remaining issues (like linter), and we merge this in. If you're on the fence I'd be happy to jump on a quick call with you to further discuss this.
• Option 2: Close the PRs but open a new one to inverse the preference between environment variable and directory lookup (as explained in my previous comment, I expect this would be the main annoyance with the current override implementation).
• Option 3: Close the PRs.

[ruffsl]

We really have to close on this

I don't think I have merge permissions for repo (just voltering my spare time during grad school for reviewing), so I'm probably not the one you'd have to ultimately convince here. That said, you've swayed me and I do think this is a worthwhile feature, and I'd approve such a PR provided it incorporate the following:

• Simple Defaults
  • Let the preexisting basic loopup behavior be the default: exact name matching
  • I.e. load exactly what the application layer tells you to: nothing more, nothing less
  • raise an informative error to the user if you can't find what they told you to load
• Advanced configuration
  • Introduce a new environment variable to override simple default: perhaps ROS_SECURITY_LOOKUP, or ROS_SECURITY_LOAD?
  • If this env is left unset, rcl falls back to default as above; DEFAULT or MATCH_EXACT?
  • User can set this env to enable longest matching prefix; PREFIX, or MATCH_PREFIX?
  • This leaves us room for use to add more load behaviors, e.g. FROM_TPM, SGX, TRUSTZONE etc
  • Users can set and forget this env with ROS_SECURITY_ENABLE should they decide to opt-in
• Optional build dependency
  • What happens if I don't have the external dependency required for a given load behavior
  • rcl should still compile with the load behavior plugins whos dependencies are satisfied
• Compatible with linux security modules
  • Given the runtime process may be guarded or quarantined using AppArmor, SELinux, SMACK, etc
  • Used to protect file or device IO: e.g. restricting read access for certain names in the keystore
  • A load behavior such as PREFIX should avoid inhibiting the use of such security modules
  • E.g. tinydir should not crash the node just because of a denied access while walking the file tree

[AAlon]

We really have to close on this

I don't think I have merge permissions for repo (just voltering my spare time during grad school for reviewing), so I'm probably not the one you'd have to ultimately convince here. That said, you've swayed me and I do think this is a worthwhile feature, and I'd approve such a PR provided it incorporate the following:

• Simple Defaults

  • Let the preexisting basic loopup behavior be the default: exact name matching
  • I.e. load exactly what the application layer tells you to: nothing more, nothing less
  • raise an informative error to the user if you can't find what they told you to load

• Advanced configuration

  • Introduce a new environment variable to override simple default: perhaps ROS_SECURITY_LOOKUP, or ROS_SECURITY_LOAD?
  • If this env is left unset, rcl falls back to default as above; DEFAULT or MATCH_EXACT?
  • User can set this env to enable longest matching prefix; PREFIX, or MATCH_PREFIX?
  • This leaves us room for use to add more load behaviors, e.g. FROM_TPM, SGX, TRUSTZONE etc
  • Users can set and forget this env with ROS_SECURITY_ENABLE should they decide to opt-in

• Optional build dependency

  • What happens if I don't have the external dependency required for a given load behavior
  • rcl should still compile with the load behavior plugins whos dependencies are satisfied

• Compatible with linux security modules

  • Given the runtime process may be guarded or quarantined using AppArmor, SELinux, SMACK, etc
  • Used to protect file or device IO: e.g. restricting read access for certain names in the keystore
  • A load behavior such as PREFIX should avoid inhibiting the use of such security modules
  • E.g. tinydir should not crash the node just because of a denied access while walking the file tree

Sounds good to me. I'll have an updated version later today.
Thanks!

[AAlon]

Updated. Introduced ROS_SECURITY_LOOKUP_TYPE env var, defaults to MATCH_EXACT, supports MATCH_PREFIX.
Improved error messages:

$ ros2 topic echo /hi
Unknown error creating node: SECURITY ERROR: directory /home/avishaya/sros2_demo/demo_keys2/ros2cli_node_9896 does not exist. Lookup strategy: MATCH_EXACT, at /home/avishaya/ros2_ws/src/ros2/rcl/rcl/src/rcl/security_directory.c:231

With export ROS_SECURITY_LOOKUP_TYPE=MATCH_PREFIX:

$ ros2 topic echo /hi
Unknown error creating node: SECURITY ERROR: unable to find a folder matching the node name in /home/avishaya/sros2_demo/demo_keys2/. Lookup strategy: MATCH_PREFIX, at /home/avishaya/ros2_ws/src/ros2/rcl/rcl/src/rcl/security_directory.c:229

With export ROS_SECURITY_NODE_DIRECTORY=/hi:

$ ros2 topic echo /hi
Unknown error creating node: SECURITY ERROR: directory /hi does not exist. Lookup strategy: NODE_OVERRIDE, at /home/avishaya/ros2_ws/src/ros2/rcl/rcl/src/rcl/security_directory.c:231

[ruffsl]

I couldn't really view the PR file change at first (93 files changed), as the PR commit history seemed jumbled. So I rebased the PR branch wrt to the current master branch. Please check I didn't botch anything.

I tried testing this locally:

• I can reproduce the new error messages just fine by attempting an imposable node path
• I can successfully use the default MATCH_EXACT
• I can use ROS_SECURITY_NODE_DIRECTORY to override ROS_SECURITY_LOOKUP_TYPE

but encountered a snag:

• I can't successfully start a node using MATCH_PREFIX using the talker node from demo_nodes_cpp

Check if you can reproduce my test temporarily in a single environment session:

export ROS_SECURITY_ROOT_DIRECTORY=~/sros2_demo/demo_keys
export ROS_SECURITY_ENABLE=true
export ROS_SECURITY_STRATEGY=Enforce

# test talker
ros2 run demo_nodes_cpp talker[INFO] [rcl]: Found security directory: /home/ubuntu/sros2_demo/demo_keys/talker                                                               
[INFO] [talker]: Publishing: 'Hello World: 1'                                                                                                
[INFO] [talker]: Publishing: 'Hello World: 2'
...
# works fine, and stop it
^C

# move node folder and set use prefix matching
mv $ROS_SECURITY_ROOT_DIRECTORY/talker $ROS_SECURITY_ROOT_DIRECTORY/talk
export ROS_SECURITY_LOOKUP_TYPE=MATCH_PREFIX

# test talker again
ros2 run demo_nodes_cpp talker
...
^C[INFO] [rclcpp]: signal_handler(signal_value=2)
^C[INFO] [rclcpp]: signal_handler(signal_value=2)
# talker will hang with no stdout, force kill it
killall -9 talker

# now switch to using override 
export ROS_SECURITY_NODE_DIRECTORY=$ROS_SECURITY_ROOT_DIRECTORY/talk

# test talker for the third time
ros2 run demo_nodes_cpp talker[INFO] [rcl]: Found security directory: /home/ubuntu/sros2_demo/demo_keys/talker                                                               
[INFO] [talker]: Publishing: 'Hello World: 1'                                                                                                
[INFO] [talker]: Publishing: 'Hello World: 2'
...
# works fine, and stop it
^C

[AAlon]

I couldn't really view the PR file change at first (93 files changed), as the PR commit history seemed jumbled. So I rebased the PR branch wrt to the current master branch. Please check I didn't botch anything.

I tried testing this locally:

• I can reproduce the new error messages just fine by attempting an imposable node path
• I can successfully use the default MATCH_EXACT
• I can use ROS_SECURITY_NODE_DIRECTORY to override ROS_SECURITY_LOOKUP_TYPE

but encountered a snag:

• I can't successfully start a node using MATCH_PREFIX using the talker node from demo_nodes_cpp

Check if you can reproduce my test temporarily in a single environment session:

export ROS_SECURITY_ROOT_DIRECTORY=~/sros2_demo/demo_keys
export ROS_SECURITY_ENABLE=true
export ROS_SECURITY_STRATEGY=Enforce

# test talker
ros2 run demo_nodes_cpp talker[INFO] [rcl]: Found security directory: /home/ubuntu/sros2_demo/demo_keys/talker                                                               
[INFO] [talker]: Publishing: 'Hello World: 1'                                                                                                
[INFO] [talker]: Publishing: 'Hello World: 2'
...
# works fine, and stop it
^C

# move node folder and set use prefix matching
mv $ROS_SECURITY_ROOT_DIRECTORY/talker $ROS_SECURITY_ROOT_DIRECTORY/talk
export ROS_SECURITY_LOOKUP_TYPE=MATCH_PREFIX

# test talker again
ros2 run demo_nodes_cpp talker
...
^C[INFO] [rclcpp]: signal_handler(signal_value=2)
^C[INFO] [rclcpp]: signal_handler(signal_value=2)
# talker will hang with no stdout, force kill it
killall -9 talker

# now switch to using override 
export ROS_SECURITY_NODE_DIRECTORY=$ROS_SECURITY_ROOT_DIRECTORY/talk

# test talker for the third time
ros2 run demo_nodes_cpp talker[INFO] [rcl]: Found security directory: /home/ubuntu/sros2_demo/demo_keys/talker                                                               
[INFO] [talker]: Publishing: 'Hello World: 1'                                                                                                
[INFO] [talker]: Publishing: 'Hello World: 2'
...
# works fine, and stop it
^C

Good catch! I suspect there was a bug where, if the file is not a directory (!file.is_dir) the loop in get_best_matching_directory simply continues without fetching the next one. That part of the code hasn't been changed since the first version so it's a bit odd we only stumbled upon it now.
Anyway, should be fixed now.

[ruffsl]

Awsome! Just tested locally again as well, and the prefix is now working for the case above. Perhaps you may also want to add some tests for these new features just to better asure coverage for any other corner cases we might have missed manualy running it. Some of the other security tests are located here:
https://github.com/ros2/system_tests/tree/master/test_security

Ask any of the OSRF folks to trigger a jinkines CI build to test this PR against the rest of the release.

[AAlon]

Cool, thank you! will add system tests as well. If there are no outstanding comments here, would you be able to mark your review as approved?
As for documentation, is there an existing page under https://github.com/ros2/ros2_documentation this should be added to, or maybe it'd be better to create a new page dedicated to loading security credentials?

@tfoote could you please help trigger a build against Crystal? Thanks!

[ruffsl]

As for documentation, is there an existing page under https://github.com/ros2/ros2_documentation this should be added to

I don't know of any security documentation other than the current READMEs on the sros2 repo.

, or maybe it'd be better to create a new page dedicated to loading security credentials?

I think porting the old readmes I mentioned above from the sros2 repo into an security section section on the official docs repo would be fine idea.

[tfoote]

I ran a build up to rcl and testing rcl is there specific packages that should be tested in the ci? Also this looks to be in conflict now so I'm not sure how out of date the content is for a rebase/merge.

https://gist.github.com/tfoote/5bf239ac5bd653a9aaba5f2ee3111898

• Linux
• Linux-aarch64
• macOS
• Windows

[jacobperron]

A few minor comments.

Also, there some linter issues (not just in tinydir.h).

Regarding the tinydir dependency, I think copying it here is fine and we could add a linter exception for it.
Alternatively, it could be copied to a separate vendor package for better isolation/reuse (e.g. tinyxml_vendor). But I'm interested to hear what others think.

[jacobperron]

Why should we install the test resources?

[AAlon]

We copy those to the same directory as the test binaries as they're required for running the tests (it's not strictly necessary, I suppose I could've propagated ${test_resources_dir_name} to the test code, but I find it better to separate source from binaries / run-time resources).

[wjwwood]

Regarding the tinydir dependency, I think copying it here is fine and we could add a linter exception for it.

I think it is acceptable to have tinydir.h in this repository so long as you extend the linters so they can ignore the tinydir files and so long as tinydir remains private (not installed or included in the public headers).

I think a better solution would be to extend the existing rcutils filesystem API to suit the needs of this pr, implementing that within rcutils using tinydir if desired. This is simply because filesystem operations shouldn't be implemented in rcl in order to keep rcl "about the client library" and also because having those functions in rcutils may be useful in the future.

Finally, I think the best solution (long term solution) would be to remove all of the use of filesystem and environment variable usage related to security from rcl itself, instead passing in the functions to do this from the client libraries or by using plugins (similar to how Connext's security works). The reason for this would be to make rcl more flexible, so that on systems which use a different way of configuring security or storing keys can customize their solution with a plugin or in the user code rather than having to modify rcl itself. Also for users who are not using security but are on machines which do not have things like filesystems.

Again, for now I think the tinydir in rcl is acceptable given the linter issues are resolved, but I would see it as additional technical debt.

[jacobperron]

+1 for extending the filesystem API in rcutils.

[AAlon]

Again, for now I think the tinydir in rcl is acceptable given the linter issues are resolved, but I would see it as additional technical debt.

Yep, makes sense. We've fixed the linter issues in all but tinydir, as it is an external project it would be easier to upgrade to newer versions and keep track of changes if we keep the original formatting.

I ran a build up to rcl and testing rcl is there specific packages that should be tested in the ci? Also this looks to be in conflict now so I'm not sure how out of date the content is for a rebase/merge.

https://gist.github.com/tfoote/5bf239ac5bd653a9aaba5f2ee3111898

• Linux
• Linux-aarch64
• macOS
• Windows

The Windows build issue should be fixed now, could you please kick off new builds with the latest? @tfoote ^^

[chapulina]

Triggered CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[chapulina]

@AAlon , it now builds on Windows, but there are still lots of linter issues.

[AAlon]

@AAlon , it now builds on Windows, but there are still lots of linter issues.

From what I can see the linter issues are all in tinydir. How can we add a linter exception?

[chapulina]

The linter is not happy about this, I think it needs to be in one line to match the template.

[jacobperron]

From what I can see the linter issues are all in tinydir. How can we add a linter exception?

For cpplint, I think we can add a file CPPLINT.cfg to the tinydir directory with the following line:

exclude_files=tinydir.h

As for the other linters, I'm not sure exactly. I suspect we may have to update CMakeLists.txt to invoke them each separately (instead of relying on ament_lint_auto). And pass them flags to ignore tinydir.h. This would require extending the ament_lint packages to accept said flags, specifically:

• ament_cmake_copyright
• ament_cmake_cppcheck
• ament_cmake_uncrustify

would have to be updated to accept "exclusion" lists.

[emersonknapp]

From the Windows build:
13:52:30 Summary: 560 tests, 0 errors, 0 failures, 2 skipped
13:52:30
13:52:30 II> colcon test-result returned: '0'

Not sure what's causing it to be marked unstable now

[clalancette]

Not sure what's causing it to be marked unstable now

There's two warnings from the compile step; look here: https://ci.ros2.org/job/ci_windows/6034/warnings43Result/new/

[Karsten1987]

@emersonknapp I believe the unstable comes from the warnings here:

https://ci.ros2.org/job/ci_windows/6034/warnings43Result/new/NORMAL/package.1954841715/source.-7982181150217390836/#93
and
https://ci.ros2.org/job/ci_windows/6034/warnings43Result/new/NORMAL/source.-7982181150217390835/#79

'strncpy': This function or variable may be unsafe. Consider using strncpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS

[emersonknapp]

Ah - thanks for finding that (I am still learning the Jenkins interface)! I will update to use strncpy_s

[emersonknapp]

Hey all, I have learned strncpy_s is not supported in glbc. Would it be acceptable to add the _CRT_SECURE_NO_WARNINGS flag for the Windows build, instead?

[emersonknapp]

In the interest of moving forward, I've put up a version disabling that warning on windows - let me know if we think this is an issue. Hopefully this greens all the builds.

[clalancette]

Hey all, I have learned strncpy_s is not supported in glbc. Would it be acceptable to add the _CRT_SECURE_NO_WARNINGS flag for the Windows build, instead?

Looking through the rest of ROS 2 (and gtest, for that matter), it looks like we generally work around this by using memcpy instead of strncpy. That's not great, but disabling CRT_SECURE_NO_WARNINGS may also disable some other warnings that we might want, so I'd be inclined to stick with the status quo. An example of what I'm talking about is here: https://github.com/ros2/urdfdom/blob/ros2/urdf_parser/test/gtest/src/gtest-port.cc#L855

[emersonknapp]

👍 will note to use memcpy going forward - updated and removed the windows warning supression

[chapulina]

New CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[rutvih]

@chapulina is there anything pending on this PR to get it merged and released as part of next patch?

[jacobperron]

A couple minor comments and one concern regarding the tests.

[emersonknapp]

Updated to address @jacobperron 's comments

[jacobperron]

Thanks @emersonknapp 👍

Just two more things and then I'll trigger CI.

[jacobperron]

We should update the all of the TEST macros to TEST_F so they make use of the fixture.

[emersonknapp]

Whoops! Silly oversight on my part

[jacobperron]

nitpick: Make the class name CamelCase (e.g. TestGetSecureRoot), similar to our other tests that use fixtures. And update the test macros accordingly

[emersonknapp]

Ok - updated!

[jacobperron]

LGTM

• Linux
• Linux-aarch64
• macOS
• Windows

[emersonknapp]

Looks like test failures from timeout in unrelated code... first guess overloaded build servers. Is there a standard way to deal with that? Run again when it's less busy?

[sloretz]

@emersonknapp @AAlon There is also a warning on the windows build https://ci.ros2.org/job/ci_windows/6088/warnings43Result/new/

No idea why the other tests failed.

[emersonknapp]

Oof - I clobbered one of my previous changes (removing that strncpy) in switching between Ubuntu and Windows workspaces

[jacobperron]

I can reproduce the rclcpp_action and test_cli test failures locally without this change, so I think we can ignore them for the purpose of this PR.

• Linux
• Linux-aarch64
• macOS
• Windows