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 fix suggested in sros2 bug report. #1551

Conversation

jpace121
Copy link

Description

Fixes #1546 by taking the solution from OpenDDS/OpenDDS#3992 (comment). As mentioned in the OpenDDS PR:

"Specifically, use PKCS7_NOINTERN to not accept any signatures in the signed document. This, in turn, requires the use of the certs parameter to PKCS7_verify. PKCS7_NOVERIFY is used since the permissions CA certificate will not be chain verified."

Related to: ros2/sros2#282

Verification

I built ros2 rolling from scratch with version of 0.10.2 cyclonedds and used the script in the sros2 linked above to verify the error was replicate-able. I then cherry-picked the commit in this PR to the 0.10.2 version of cyclonedds and repeated the test and confirmed the issue did not replicate.

As stated in: OpenDDS/OpenDDS#3992 (comment)

"Implement the suggestion in ros2/sros2#282.
Specifically, use PKCS7_NOINTERN to not accept any signatures in the signed document.
This, in turn, requires the use of the certs parameter to PKCS7_verify.
PKCS7_NOVERIFY is used since the permissions CA certificate will not be chain verified."

Fixes: eclipse-cyclonedds#1546
Related to: ros2/sros2#282

Signed-off-by: James Pace <jpace121@gmail.com>
@jpace121 jpace121 marked this pull request as draft January 27, 2023 03:04
@jpace121
Copy link
Author

jpace121 commented Jan 27, 2023

Converted to draft. While this fixes the observed issue, I would like to take a second look in the morning and make sure the API call makes sense still. If someone sees something I missed initially I would appreciate the heads up.

EDIT: For the record, it fixed the bug by rejecting all certs, so at least it was very secure. ;)

@jpace121
Copy link
Author

Nope.... Way jumped the gun here...

@jpace121 jpace121 closed this Jan 27, 2023
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.

Chain of trust issues with a single CA certificate
1 participant