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

ROS2 DDS Security PKCS#11 URI support #332

Closed
wants to merge 6 commits into from

Conversation

MiguelCompany
Copy link

This is a rework of #319, addressing the comments there, since I don't have write access on the original contributor repository.

IkerLuengo and others added 5 commits July 28, 2021 13:42
 The DDS-Security specification defines the use of Hardware Security Modules (HSM) and PKCS#11 URIs as an alternative to private keys and certificates stored in the file system. Current implementation only supports these tokens to be directly stored in the file system as `.pem` files. This is a design proposal to support PKCS#11 URIs.

The changes affect to the RMW implementations, as these are filling the DDS security attributes for the participant. However, it also affects the contents of the enclave directories in the keystore. Although the proposed changes are totally backwards compatible (meaning that current RMW implementations will continue working if no PKCS#11 URIS are used), description of the new enclave contents and the expected RMW behavior seems appropriate.
Co-authored-by: Miguel Company <miguelcompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@kallekoivisto
Copy link

This is good

@maseabunikie
Copy link

👍

Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

looks great thanks for the update 🙏

articles/ros2_dds_security_pkcs11_support.md Outdated Show resolved Hide resolved
articles/ros2_dds_security_pkcs11_support.md Outdated Show resolved Hide resolved
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

Co-authored-by: Mikael Arguedas <mikael.arguedas@gmail.com>
@MiguelCompany
Copy link
Author

@mikaelarguedas Thanks for the review. Just applied your fixes.

Copy link
Member

@ruffsl ruffsl left a comment

Choose a reason for hiding this comment

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

As discussed over today's the Security WG meeting, this LGTM.

@ruffsl
Copy link
Member

ruffsl commented Feb 14, 2023

Ping @clalancette or @tfoote ? Good to merge?

@tfoote
Copy link
Contributor

tfoote commented Feb 14, 2023

In general this sounds good. However this site overall is deprecated and we would much prefer to land this as a REP instead so that we can have all of our reference materials in one place. Would you mind submitting it over there as a new informational REP instead?

@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-tsc-meeting-minutes-2023-02-16/29927/1

@EduPonz
Copy link

EduPonz commented Mar 31, 2023

Hi @tfoote, we have gone ahead and moved this to a REP in ros-infrastructure/rep#375

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

a few comments and questions.

@@ -0,0 +1,86 @@
---
layout: default
title: ROS2 DDS security PKCS#11 URI support
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
title: ROS2 DDS security PKCS#11 URI support
title: ROS 2 DDS security PKCS#11 URI support

published: false
---

- This will become a table of contents (this text will be scraped).
Copy link
Collaborator

Choose a reason for hiding this comment

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

we need to remove this line?


Original Author: {{ page.author }}

## Scope
Copy link
Collaborator

Choose a reason for hiding this comment

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

I take this into more like Motivation section? especially the end of this section would look like problem statement.

Then, the RMW can be aware of the file extension and set the security property accordingly.
Taking the private key in the authentication plugin as an example:

1. The RMW will look for a file with name `key.pem` or `key.p11` in the enclave.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens if both files are in the enclaves? key.p11 prevails to key.pem and if rmw implementation does not support key.p11, then it falls back to use key.pem?

@MiguelCompany
Copy link
Author

Closing in favor of ros-infrastructure/rep#375

@MiguelCompany MiguelCompany deleted the pkcs11-support branch April 4, 2023 09:40
@ros-discourse
Copy link

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

https://discourse.ros.org/t/announcing-rep-2015-ros-2-dds-security-pkcs-11-support/31357/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

Successfully merging this pull request may close these issues.

None yet

10 participants