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

Update security environment variables #199

Closed
ruffsl opened this issue Apr 14, 2020 · 15 comments
Closed

Update security environment variables #199

ruffsl opened this issue Apr 14, 2020 · 15 comments

Comments

@ruffsl
Copy link
Member

ruffsl commented Apr 14, 2020

Feature request

With the update to enclaves, the security environment variables could also use some updates. To future proof variables. allowing for more exotic keystore URIs, like remote URL (https://...$TOKEN) or TPM devices (tpm://...$KEY), it would be advantageous to take the opportunity to name them more appropriately while generalizing across keystore types. E.g. integrating with more advanced secret management: https://www.vaultproject.io

The following renaming is proposed:

  • ROS_SECURITY_ROOT_DIRECTORY -> ROS_SECURITY_KEYSTORE
  • ROS_SECURITY_DIRECTORY_OVERRIDE -> ROS_SECURITY_ENCLAVE_OVERRIDE

Additionally, to abstract above directories or file paths, I'd also like to sagest that ROS_SECURITY_ENCLAVE_OVERRIDE takes instead a fully qualified enclave name, instead of a absolute file path to an enclave directory. The use of the env ROS_SECURITY_KEYSTORE would then consistently control keystore URI (currently a file system path) and always be used to resolve the context location, regardless if context name is overridden or not.

This could help guide users that secure ros2 nodes will only communicate with nodes that share a common source of trust, or common CA. If the user has multiple keystores on disk, they may be likely to use ROS_SECURITY_DIRECTORY_OVERRIDE to point to an entirely different keystore than the rest of the launched nodes, rendering potentially cryptic looking security handshake errors from Secure DDS vendor plugins given the empty overlap of trusted certificate authorities.

For users who know what they are doing, they can still set the ROS_SECURITY_KEYSTORE uniquely for each process if they do intend on separate processes having separate keystores.


https://github.com/ruffsl/ros2/blob/security_env/ros2.repos

@ivanpauno
Copy link
Member

Testing rcl, test_security, sros2.
Only Fast-RTPS.

  • Focal Build Status
  • AArch64 Bionic Build Status
  • macOS Build Status
  • Windows Build Status

@mikaelarguedas
Copy link
Member

DIdnt get a chance to review everything but as there are several approvals 👍

Testing rcl, test_security, sros2.
Only Fast-RTPS.

Is it possible to run CI with Connext instead? security tests are currently disabled for Fast-RTPS (ros2/system_tests#413)

@ivanpauno
Copy link
Member

ivanpauno commented Apr 15, 2020

Is it possible to run CI with Connext instead? security tests are currently disabled for Fast-RTPS (ros2/system_tests#413)

Good call, doing that in linux, where the tests are disabled:

  • Focal Build Status

@mikaelarguedas
Copy link
Member

Can you please un them on all platforms? the previous CI stopped at sros2 so didn't run any tests from the test_security package. Also the test_security Fast-RTPS tests are disabled on all platforms, not just linux

@ivanpauno
Copy link
Member

  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member

@ruffsl there seem to be failures on macOS and Windows.

@ruffsl
Copy link
Member Author

ruffsl commented Apr 15, 2020

@ivanpauno I just pushed a fix for rcl.TestGetSecureRoot.test_get_security_options for windows ros2/rcl@3e067b4 . Not sure how these PRs would effected the timeout or shutdown tests.
I don't have a mac or windows licence to debug those. Where those failures introduced here?

@mikaelarguedas
Copy link
Member

mikaelarguedas commented Apr 16, 2020

Another round of CI with fixes from ros2/rcl@68a356e

  • Linux Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member

I don't have a mac or windows licence to debug those. Where those failures introduced here?

The macOS ones are also happening in nightlies and ros2/system_tests#409 is needed to fix them.

@ivanpauno
Copy link
Member

@ruffsl can you check linter failures?

@mikaelarguedas
Copy link
Member

I submitted a suggestion at ros2/rcl#617 (review) that should hopefully address them

@ruffsl
Copy link
Member Author

ruffsl commented Apr 16, 2020

I submitted a suggestion

Thanks, merged.

@ivanpauno
Copy link
Member

Final run, checking that linter failures are gone:

  • Linux Build Status

@ivanpauno
Copy link
Member

I think everything was merged, thanks @ruffsl !

@ros-discourse
Copy link

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

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2020-04-16/13709/1

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

No branches or pull requests

4 participants