-
Notifications
You must be signed in to change notification settings - Fork 121
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
Conversation
Codecov ReportAttention: Patch coverage is
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. |
ASN1_OCTET_STRING_free(p7->d.sign->contents->d.data); | ||
p7->d.sign->contents->d.data = NULL; | ||
} | ||
return detach; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 toPKCS7_is_detached
, so I've done that accordingly.PKCS7_set_detached
is used to "detach" data contents from the internalPKCS7
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.