-
Notifications
You must be signed in to change notification settings - Fork 155
Add EVP_AEAD API for XAES-256-GCM with Key Commitment #2862
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## xaes-256-gcm #2862 +/- ##
=============================================
Coverage 78.32% 78.33%
=============================================
Files 683 683
Lines 117745 117822 +77
Branches 16516 16527 +11
=============================================
+ Hits 92226 92293 +67
- Misses 24633 24642 +9
- Partials 886 887 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Do we need to test with a truncated key commitment for both EVP and AEAD?
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.
I have added tests for truncated key commitment of EVP_CIPHER:
https://github.com/ttunglee/aws-lc/blob/xaes-256-gcm/crypto/cipher_extra/cipher_test.cc#L1704
https://github.com/ttunglee/aws-lc/blob/xaes-256-gcm/crypto/cipher_extra/cipher_test.cc#L1748
https://github.com/ttunglee/aws-lc/blob/xaes-256-gcm/crypto/cipher_extra/cipher_test.cc#L1788
https://github.com/ttunglee/aws-lc/blob/xaes-256-gcm/crypto/cipher_extra/cipher_test.cc#L1796
| * Therefore, truncating tag happens to key commitment first | ||
| * We consider the following possible cases of tag_len: | ||
| * tag_len > 16 : tag includes 16-byte MAC tag and (tag_len - 16)-byte key commitment | ||
| * tag_len <= 16: tag includes only (tag_len)-byte MAC tag and 0-byte key commitment */ |
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.
Did we decide in the design that we want to support this case or we error out as this would not be a key commitment cipher so someone calling it doesn't think key commitment exists in the tag?
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.
Not yet. I just realized this issue when putting EVP_aead_xaes_256_gcm_kc into aead_test: https://github.com/ttunglee/aws-lc/blob/xaes-256-gcm/crypto/cipher_extra/aead_test.cc#L100
With flag kCanTruncateTags enabled, it will execute this test, which needs to process truncated MAC+KC properly:
https://github.com/ttunglee/aws-lc/blob/xaes-256-gcm/crypto/cipher_extra/aead_test.cc#L703
To process the case you mentioned, it is needed to remove the kCanTruncateTags for EVP_aead_xaes_256_gcm_kc, otherwise the test will fail.
crypto/fipsmodule/cipher/e_aes.c
Outdated
| return 1; | ||
| } | ||
|
|
||
| static int aead_xaes_256_gcm_key_commit_seal_scatter( |
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.
Here and in open_gather, we can replace key_commit with kc or the other way around in init function.
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.
Thank you! I'll change to make it consistent.
nebeid
left a comment
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.
Before integrating into main, we need to consider:
- Do we want to keep the current design which allows the case of truncated tags to 16 bytes or less? This will not contain key commitment despite that the caller may be thinking that the API will still produce some key commitment.
- it's tricky to know whether the caller wants to truncate the GCM tag or the key commitment tag in this case.
- if we don't allow it, as Tung mentioned in #2862 (comment), we will need to test it with a new flag other than
kCanTruncateTags, which starts at tag length of 0, or just create a separate test for it.
- We should test the correctness of the key commitment using the CMAC API to calculate it separately since we don't have a source for test vectors.
Description of Changes
extra_indata to support AWS-LC-RS: https://github.com/ttunglee/aws-lc/blob/xaes-256-gcm/crypto/fipsmodule/cipher/e_aes.c#L2049, https://github.com/ttunglee/aws-lc/blob/xaes-256-gcm/crypto/fipsmodule/cipher/e_aes.c#L2326.Testing
Reuse the test vectors in #2809 with our own added key commitment values, similar to #2826.
./crypto_test --gtest_filter='All/PerAEADTest.*'./crypto_test --gtest_filter='CipherTest.*'Modifications compared with #2652
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.