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 #319

Closed
wants to merge 3 commits into from

Conversation

IkerLuengo
Copy link

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.

 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.
@clalancette
Copy link
Contributor

@ros2/security_working_group Thoughts about this proposal?

@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-2021-08-19/22008/1

@IkerLuengo
Copy link
Author

To give some context, this proposal was discussed an approved on the @ros2/security_working_group on June (minutes here, although with Sid gone, they are still unmerged).

Copy link

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

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

I found a typo on the DDS property name.

articles/ros2_dds_security_pkcs11_support.md Outdated Show resolved Hide resolved
Co-authored-by: Miguel Company <miguelcompany@eprosima.com>
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.

Sorry i was looking at only specific commit, i will take a look at the whole fix.

layout: default
title: ROS2 DDS security PKCS#11 URI support
abstract:
The [DDS-Security specification][dds_security] 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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

one sentence per line, and so is everywhere else. (if i am not mistaken, this is required format for doc.)

Suggested change
The [DDS-Security specification][dds_security] 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 [DDS-Security specification][dds_security] 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.

Copy link

@asorbini asorbini left a comment

Choose a reason for hiding this comment

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

I had been asked to review this PR during a recent RMW WG meeting, and I'm doing so with a bit of delay. Apologies for that.

Anyway, I think the proposal is sound from a technical point of view, and I only had a minor comment on the loading order of the security material.

Comment on lines +61 to +63
1. The RMW will look for a file with name `key.pem` or `key.p11` in the enclave.
1. If there is a `key.pem` file, it keeps the current behavior, and sets the value of the property to the path of the file.
1. Otherwise, if there is a `key.p11` file, it must load the content of the file, and set this content as the value of the property. Some check can be done at this point, e.g., assert that the file contains a PKCS#11 URI (i.e., starts with `pkcs11:`).

Choose a reason for hiding this comment

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

Maybe I'm misreading the description, but I think that the algorithm should have the key.p11 file have higher priority (i.e. be checked first by the RMW), and then have key.pem be the default, "fallback" selection.

Are there reasons for forcing the use of key.pem over key.p11 if both files are present? Maybe this approach will offer better "backwards compatibility" and avoid introducing sudden changes if the enclave material now contains a .p11 file (since the .p11 file will only be used if the user explicitly deletes key.pem)?

Choose a reason for hiding this comment

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

I think this would make sense. I was not on the WG meetings where this was discussed. Perhaps @Quinz or @ruffsl could bring us some light

Choose a reason for hiding this comment

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

Yep, I believe the main idea was to keep the "backwards compatibility" for existing enclaves. But on the other hand there wasn't much discussion on the loading order for pem vs p11 files. I agree with @asorbini that it would be more intuitive for me to have p11 have higher priority as it can be considered also as more secure option in most of the configurations.

@MiguelCompany
Copy link

@Quinz @fujitatomoya @asorbini I've open #332 since I don't have write access on the repo of the original contributor.

This PR can be closed.

@clalancette
Copy link
Contributor

Closing based on the last comment.

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

7 participants