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

Implement PKCS7_verify, update PKCS7_sign #1993

Merged
merged 8 commits into from
Dec 9, 2024

Conversation

WillChilds-Klein
Copy link
Contributor

@WillChilds-Klein WillChilds-Klein commented Nov 14, 2024

Notes

This PR implements PKCS7_verify for verifying signedData-type PKCS7 messages. It also adds another mode of operation to PKCS7_sign for compatibility with Ruby. Flags for these functions are documented in header comments.

We also remove most support for signedAndEnveloped-type PKCS7 messges, as they're not required by Ruby's tests beyond a few getters/setters. OpenSSL supports this type in PKCS7_decrypt but not PKCS7_verify, PKCS7_sign, nor PKCS7_encrypt, so it's unclear what (if any) role signedAndEnveloped types have with respect to these functions. If we wish to revisit support for this type in the future, I have a branch here implementing encrypt/decrypt/sign/verify support and a test.

Lastly, we externalize some of the previously internal PKCS7 ASN.1/struct definitions as ruby's source accesses some of their members directly.

Testing

  • unit tests
  • with ruby source patch and integration test from this branch, ran ruby's PKCS7 test suite against this PR's contents
$ time ./tests/ci/integration/run_ruby_integration.sh ruby_3_1
...
# Running tests:

Finished tests in 0.160404s, 81.0451 tests/s, 579.7843 assertions/s.
13 tests, 93 assertions, 0 failures, 0 errors, 1 skips

ruby -v: ruby 3.1.6p260 (2024-12-03 revision 326a6cbf9b) [x86_64-linux]
+ popd
~/aws-lc/RUBY_BUILD_ROOT/ruby-src ~/aws-lc/RUBY_BUILD_ROOT
+ popd
~/aws-lc/RUBY_BUILD_ROOT

real    4m13.491s
user    6m35.828s
sys     1m3.948s

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.

@codecov-commenter
Copy link

codecov-commenter commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 79.74277% with 63 lines in your changes missing coverage. Please review.

Project coverage is 78.76%. Comparing base (2a72226) to head (f66e6ef).
Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
crypto/pkcs7/pkcs7.c 75.15% 39 Missing ⚠️
crypto/pkcs7/pkcs7_x509.c 72.13% 17 Missing ⚠️
crypto/test/test_util.cc 66.66% 4 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1993      +/-   ##
==========================================
+ Coverage   78.68%   78.76%   +0.07%     
==========================================
  Files         598      598              
  Lines      103323   103620     +297     
  Branches    14686    14738      +52     
==========================================
+ Hits        81301    81617     +316     
+ Misses      21372    21351      -21     
- Partials      650      652       +2     

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

@WillChilds-Klein WillChilds-Klein force-pushed the pkcs7-ruby branch 2 times, most recently from ebb86d2 to a94835a Compare November 18, 2024 18:17
@WillChilds-Klein WillChilds-Klein changed the title [DO NOT MERGE] Ruby/PKCS7 integration CI checks [DRAFT] Implement PKCS7_verify, update PKCS7_sign Nov 18, 2024
@WillChilds-Klein WillChilds-Klein force-pushed the pkcs7-ruby branch 3 times, most recently from 59a15a7 to b18f29c Compare November 19, 2024 13:15
@WillChilds-Klein WillChilds-Klein force-pushed the pkcs7-ruby branch 8 times, most recently from e94fa2a to 576945c Compare November 29, 2024 18:17
@WillChilds-Klein WillChilds-Klein changed the title [DRAFT] Implement PKCS7_verify, update PKCS7_sign Implement PKCS7_verify, update PKCS7_sign Nov 29, 2024
@WillChilds-Klein WillChilds-Klein marked this pull request as ready for review December 3, 2024 16:26
@WillChilds-Klein WillChilds-Klein requested a review from a team as a code owner December 3, 2024 16:26
include/openssl/base.h Outdated Show resolved Hide resolved
crypto/pkcs7/internal.h Outdated Show resolved Hide resolved
include/openssl/pkcs7.h Outdated Show resolved Hide resolved
include/openssl/pkcs7.h Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7_x509.c Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7_x509.c Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7_x509.c Show resolved Hide resolved
crypto/pkcs7/pkcs7_x509.c Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7_x509.c Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Show resolved Hide resolved
include/openssl/pkcs7.h Outdated Show resolved Hide resolved
include/openssl/pkcs7.h Outdated Show resolved Hide resolved
include/openssl/pkcs7.h Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7_x509.c Show resolved Hide resolved
crypto/pkcs7/pkcs7_x509.c Show resolved Hide resolved
@WillChilds-Klein WillChilds-Klein merged commit 96de127 into aws:main Dec 9, 2024
119 of 124 checks passed
@WillChilds-Klein WillChilds-Klein deleted the pkcs7-ruby branch December 9, 2024 18:16
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.

5 participants