-
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 PKCS12_set_mac #2128
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2128 +/- ##
=======================================
Coverage 78.97% 78.97%
=======================================
Files 611 611
Lines 105552 105632 +80
Branches 14950 14951 +1
=======================================
+ Hits 83361 83425 +64
- Misses 21538 21558 +20
+ Partials 653 649 -4 ☔ View full report in Codecov by Sentry. |
7f55519
to
655d214
Compare
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.
Minor style feedback on tests, non-blocking.
655d214
to
72a0f41
Compare
const uint8_t *orig_authsafe = CBS_data(&authsafe); | ||
size_t orig_authsafe_len = CBS_len(&authsafe); | ||
|
||
// Parse for |authsafes| which is the data that we should be running HMAC on. |
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.
what is "authsafe"? i don't see it mentioned anywhere in the overview
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.
authsafe
is the ASN1 content that we need to run HMAC against. It's a field defined within the PFX syntax for PKCS12: https://datatracker.ietf.org/doc/html/rfc7292#section-4
It has the ContentInfo
type, which is defined in another RFC: https://datatracker.ietf.org/doc/html/rfc2315#section-7.
I didn't go into further explanation here, since we have prior art/documentation in our parsers. I also tried sharing the code here, but both parsers need slightly different information extracted.
// Parse for |authsafes| which is the data that we should be running HMAC on. | ||
if (!CBS_get_asn1(&authsafe, &content_type, CBS_ASN1_OBJECT) || | ||
!CBS_get_asn1(&authsafe, &wrapped_authsafes, | ||
CBS_ASN1_CONTEXT_SPECIFIC | CBS_ASN1_CONSTRUCTED | 0) || |
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.
why | 0
?
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.
Great question, 0 is the explicit tag that corresponds to this field. We use CBS_ASN1_CONSTRUCTED
to OR into a tag to look up the constructed bit.
Other usages of CBS_ASN1_CONSTRUCTED
follow a similar pattern: https://github.com/search?q=repo%3Aaws/aws-lc%20CBS_ASN1_CONSTRUCTED&type=code
5f0b1bf
to
3deaf76
Compare
Issues:
Addresses
CryptoAlg-2821
Description of changes:
Ruby has added a binding for an additional function called
PKCS12_set_mac
in it's master branch: ruby/ruby@c79b435 This was discovered prior to the PR for Ruby's CI integration with the master branch being merged: #2071OpenSSL's implementation of the function directly sets a designated mac field within the
PKCS12
structure. OurPKCS12
structure is folded into a string of bytes along with its length and there aren't any available fields for us to directly set. This means that AWS-LC's implementation ofPKCS12_set_mac
has to parse the proper contents fromPKCS12
and rerun the key and mac generation with the new parameters provided.Call-outs:
PKCS12_get_key_and_certs
, but the logic can't be shared. This is because we need to maintain pointers to certain parts of the CBS parsers inPKCS12_set_mac
, so that we can properly rewrite the contents later.PKCS12_create
andPKCS12_set_mac
to a single function calledpkcs12_gen_and_write_mac
.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.