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 support for OCSP_copy_nonce #1711

Merged
merged 4 commits into from
Aug 5, 2024

Conversation

samuel40791765
Copy link
Contributor

@samuel40791765 samuel40791765 commented Jul 17, 2024

Issues:

Addresses CryptoAlg-2420

Description of changes:

Ruby consumes OCSP_copy_nonce, which copies the nonce from the request to the OCSP response.
OpenSSL's OCSP_copy_nonce directly appends the new OCSP nonce to the list of extensions in the response. This allows multiple OCSP nonces to exist the same response since the existing nonces are not deleted. It's a bit strange to allow multiple nonces to exist in a single response. To make things stranger, OCSP_check_nonce directly takes the first OCSP nonce in the list to compare, so any newer nonces are ignored.
We ultimately decided to follow OpenSSL's faulty implementation and document/test the behavior, there doesn't seem to be much value in diverging from the original and will only introduce more unanticipated behavioral changes.

Call-outs:

N/A

Testing:

Extend upon original OCSP nonce tests

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@samuel40791765 samuel40791765 requested a review from a team as a code owner July 17, 2024 00:25
@codecov-commenter
Copy link

codecov-commenter commented Jul 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.31%. Comparing base (b32e641) to head (0632214).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1711   +/-   ##
=======================================
  Coverage   78.31%   78.31%           
=======================================
  Files         580      580           
  Lines       96619    96639   +20     
  Branches    13850    13859    +9     
=======================================
+ Hits        75665    75686   +21     
+ Misses      20340    20338    -2     
- Partials      614      615    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@samuel40791765 samuel40791765 force-pushed the ocsp-copy-nonce branch 3 times, most recently from 06fb1a5 to cd660a2 Compare July 30, 2024 19:50
// OCSP_copy_nonce copies the nonce value (if any) from |req| to |resp|. Returns
// 1 on success and 0 on failure. If the optional nonce value does not exist in
// |req|, we return 2 instead.
OPENSSL_EXPORT int OCSP_copy_nonce(OCSP_BASICRESP *resp, OCSP_REQUEST *req);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should we document that the nonce extension is prepended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes we should

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The more I think about it, I think the correct behavior should be replacing any existing nonce in resp instead of prepending/appending it to the list of extensions. OpenSSL appends the nonce to the list, but it doesn't really make sense for a resp to have two nonces.
My initial concern was that someone may have noticed OpenSSL's double nonce behavior and they're removing the old extra nonce themselves after the call to OCSP_copy_nonce. This might cause our enforced singular nonce to be removed unintentionally. But it's an edge case trying to get around OpenSSL's buggy behavior, enforcing the right behavior might be the better call here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussing with @WillChilds-Klein, ultimately decided to align with OpenSSL's implementation. There's not too much benefit in misaligning here to be fair and creates more unanticipated behavioral changes.

skmcgrail
skmcgrail previously approved these changes Aug 1, 2024
@samuel40791765 samuel40791765 merged commit 953f643 into aws:main Aug 5, 2024
102 of 106 checks passed
@samuel40791765 samuel40791765 deleted the ocsp-copy-nonce branch August 5, 2024 17:11
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.

4 participants