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

Add and update security designs for Contexts #274

Merged
merged 49 commits into from
May 20, 2020
Merged

Conversation

ruffsl
Copy link
Member

@ruffsl ruffsl commented Feb 25, 2020

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 (ruffsl#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.

ruffsl and others added 3 commits February 24, 2020 20:24
Signed-off-by: ruffsl <roxfoxpox@gmail.com>
Co-Authored-By: Mikael Arguedas <mikael.arguedas@gmail.com>
@ros-discourse
Copy link

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

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments and questions, but it's looking good.

articles/ros2_access_control_policies.md Outdated Show resolved Hide resolved
articles/ros2_security_contexts.md Outdated Show resolved Hide resolved
articles/ros2_security_contexts.md Outdated Show resolved Hide resolved
articles/ros2_security_contexts.md Outdated Show resolved Hide resolved
articles/ros2_security_contexts.md Outdated Show resolved Hide resolved
articles/ros2_security_contexts.md Outdated Show resolved Hide resolved
articles/ros2_security_contexts.md Outdated Show resolved Hide resolved
articles/ros2_security_contexts.md Outdated Show resolved Hide resolved
articles/ros2_security_contexts.md Outdated Show resolved Hide resolved
articles/ros2_security_contexts.md Outdated Show resolved Hide resolved
@ivanpauno

This comment has been minimized.

@ruffsl

This comment has been minimized.

@ivanpauno

This comment has been minimized.

Copy link
Member

@kyrofa kyrofa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't quite made it through the whole thing, but a few quick comments from me.

articles/ros2_access_control_policies.md Outdated Show resolved Hide resolved
articles/ros2_dds_security.md Outdated Show resolved Hide resolved
articles/ros2_security_contexts.md Outdated Show resolved Hide resolved
articles/ros2_security_contexts.md Outdated Show resolved Hide resolved
articles/ros2_security_contexts.md Outdated Show resolved Hide resolved
articles/ros2_security_contexts.md Outdated Show resolved Hide resolved
@ruffsl

This comment has been minimized.

ruffsl and others added 4 commits February 27, 2020 14:29
Co-Authored-By: Kyle Fazzari <kyle@canonical.com>
Co-Authored-By: Kyle Fazzari <kyle@canonical.com>
Co-Authored-By: Kyle Fazzari <kyle@canonical.com>
@mikaelarguedas

This comment has been minimized.

@ruffsl

This comment has been minimized.

@ivanpauno

This comment has been minimized.

@ivanpauno
Copy link
Member

To modify the names used in the API, changes in rcl, sros2 and system_tests are needed.
To completely delete the security_context word from implementation details, PRs to rmw, rmw_implementation and the specific rmw implementations will also be needed.

@ruffsl @mikaelarguedas Let me know if you can open PRs for the re-naming.

@mikaelarguedas
Copy link
Member

mikaelarguedas commented Apr 9, 2020

@ruffsl
Copy link
Member Author

ruffsl commented Apr 10, 2020

#274 (comment)

I think we can do the aircraft control to track these and other PRs in flight over on ros2/sros2#182 .

@dirk-thomas
Copy link
Member

@mikaelarguedas I aborted the Windows build since it was stuck for 1+h on ros2topic tests.

@ivanpauno
Copy link
Member

@mikaelarguedas @ruffsl Do you think there is anything else missing to merge this?

@ruffsl
Copy link
Member Author

ruffsl commented Apr 14, 2020

@ivanpauno, one last bit. I've added two commits to update the environment variables.
Related:

@ivanpauno
Copy link
Member

@ivanpauno, one last bit. I've added two commits to update the environment variables.
Related:

The renaming sounds good to me.
If the rest of the security working group agrees, I will go ahead an run CI/merge.

@mikaelarguedas
Copy link
Member

mikaelarguedas commented Apr 14, 2020

one last bit. I've added two commits to update the environment variables.

As discussed on Matrix, if we rename the variables associated to the keystore, I believe we should rename it everywhere and remove any mention to "secure_root". Currently this only renames the env variables and not the associated datastructures and functions

Do you think there is anything else missing to merge this?

I think we would benefit from an "Enclaves" sub-section in the "Concepts" section, however I won't have time to work on that today

@ruffsl
Copy link
Member Author

ruffsl commented Apr 14, 2020

I believe we should rename it everywhere and remove any mention to "secure_root".

I left the associated data structures and functions alone for the time being as file system paths are at present the only current supported keystore URI. I also couldn't think of a better substitute term given most of the functions and tests where file system oriented.

@ivanpauno
Copy link
Member

As discussed on Matrix, if we rename the variables associated to the keystore, I believe we should rename it everywhere and remove any mention to "secure_root".

Sounds good.
I will run CI, when the current failures are solved.

Currently this only renames the env variables and not the associated datastructures and functions

Though I agree an update would be good, that API is not directly used by the user, and the only thing that the user get access is the environment variable names.
So, I'm ok with only updating the env variable names for now.

I think we would benefit from an "Enclaves" sub-section in the "Concepts" section, however I won't have time to work on that today

Sounds good.

@ruffsl
Copy link
Member Author

ruffsl commented May 20, 2020

one last bit. I've added two commits to update the environment variables

Now that those have been merged, I'll merge here as well so that the security design is up to date.

@ruffsl ruffsl merged commit fca6752 into gh-pages May 20, 2020
@delete-merged-branch delete-merged-branch bot deleted the security-contexts branch May 20, 2020 18:55
mlanting pushed a commit to mlanting/design that referenced this pull request Aug 17, 2020
* Add initial draft for ROS 2 Security Contexts

Signed-off-by: ruffsl <roxfoxpox@gmail.com>
Co-Authored-By: Mikael Arguedas <mikael.arguedas@gmail.com>

* Update policy schema for contexts

* Update DDS-Security integration for contexts

* Allow contexts to formalize cross domain bridging

* Update context vocab

* Update design for runtime argument

* Simplify context arg

* Removing shell specific $ from variables

* Restore notes on synlinks

* Remove the use of alternate lookup methods
It's now redundan given context paths can be independent of namespace
ros2#274 (comment)

* Update policy schema to match that currently used

* Update key argument name for security context

* Publish Security Contexts document

* Update context directory overide env name

* Update story for ROS_SECURITY_ROOT_DIRECTORY

* Context -> Enclave

* Rename for enclave terminology change

* Update runtime argument for enclave assignment

* Relegate lunch interrogation to future work

* Disassociate context paths from namespaces

* Update override example of root directory and CLA

* Rename security environment variables

* Update behavor of ROS_SECURITY_ENCLAVE_OVERRIDE

Co-authored-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Co-authored-by: Kyle Fazzari <kyle@canonical.com>
mlanting pushed a commit to mlanting/design that referenced this pull request Aug 17, 2020
* Add initial draft for ROS 2 Security Contexts

Signed-off-by: ruffsl <roxfoxpox@gmail.com>
Co-Authored-By: Mikael Arguedas <mikael.arguedas@gmail.com>

* Update policy schema for contexts

* Update DDS-Security integration for contexts

* Allow contexts to formalize cross domain bridging

* Update context vocab

* Update design for runtime argument

* Simplify context arg

* Removing shell specific $ from variables

* Restore notes on synlinks

* Remove the use of alternate lookup methods
It's now redundan given context paths can be independent of namespace
ros2#274 (comment)

* Update policy schema to match that currently used

* Update key argument name for security context

* Publish Security Contexts document

* Update context directory overide env name

* Update story for ROS_SECURITY_ROOT_DIRECTORY

* Context -> Enclave

* Rename for enclave terminology change

* Update runtime argument for enclave assignment

* Relegate lunch interrogation to future work

* Disassociate context paths from namespaces

* Update override example of root directory and CLA

* Rename security environment variables

* Update behavor of ROS_SECURITY_ENCLAVE_OVERRIDE

Co-authored-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Co-authored-by: Kyle Fazzari <kyle@canonical.com>
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

Successfully merging this pull request may close these issues.

8 participants