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 PKCS7_set/get_detached #2134

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

samuel40791765
Copy link
Contributor

Description of changes:

We’re still missing two PKCS7 symbols: https://github.com/aws/aws-lc/actions/runs/12898691659/job/35966230534#step:4:4012 This also applies to past releases.

  • PKCS7_get_detached is basically an alias to PKCS7_is_detached, so I've done that accordingly.
  • PKCS7_set_detached is used to "detach" data contents from the internal PKCS7 structure. The OpenSSL code uses a detached field in the PKCS7 structure that’s missing in AWS-LC. This field isn't necessarily needed though, we can just detect the functionality accordingly.

Call-outs:

N/A

Testing:

New 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.

@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2025

Codecov Report

Attention: Patch coverage is 78.94737% with 4 lines in your changes missing coverage. Please review.

Project coverage is 78.94%. Comparing base (29be983) to head (a712d2c).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
crypto/pkcs7/pkcs7.c 75.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2134      +/-   ##
==========================================
- Coverage   78.95%   78.94%   -0.01%     
==========================================
  Files         610      610              
  Lines      105293   105312      +19     
  Branches    14911    14924      +13     
==========================================
+ Hits        83129    83141      +12     
- Misses      21511    21517       +6     
- Partials      653      654       +1     

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

ASN1_OCTET_STRING_free(p7->d.sign->contents->d.data);
p7->d.sign->contents->d.data = NULL;
}
return detach;
Copy link
Contributor

Choose a reason for hiding this comment

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

The semantic of the detach parameter is unexpected. The call always "fails" (returns 0) when detach = 0.

Could we detect detach == 0 and return 0 at the top? (The difference would be that calls to OPENSSL_PUT_ERROR in certain situations would not be made.)

if (detach != 1) {
    return 0;
}

If the calls to OPENSSL_PUT_ERROR are important, we should have test coverage to confirm they are made.

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 detach parameter is expected to be consumed as a boolean. Ruby has a good example of the usage here: https://github.com/ruby/ruby/blob/dd863714bf377b044645ea12b4db48920d49694e/ext/openssl/ossl_pkcs7.c#L497-L500

I completely agree with the weird semantic of the detach parameter, especially with 0. OpenSSL's implementation allows for other numbers to be used, but seeing that it doesn't align with how the API was meant to be used, I decided to restrict the values to only 0 and 1.
Since the parameter value was meant to take a boolean integer, I do think 0 should be allowed as a value though. I'll add coverage for the unsupported operations.

crypto/pkcs7/pkcs7_test.cc Show resolved Hide resolved
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.

3 participants