Conversation
bhess
left a comment
bhess
left a comment
There was a problem hiding this comment.
Thank you, @Mehrn0ush, for adding AES-GCM-SIV, this looks good to me.
Please check my comment regarding the pattern.
I also noticed some changes duplicated from your other PRs (XChaCha20, TUAK, and bcrypt). You might want to remove those from this PR to ensure a clean merge.
stevespringett
added
the
cap: cryptography-registry
There was a problem hiding this comment.
Pull request overview
This PR adds AES-GCM-SIV support to the Cryptography Registry as requested in issue #758. However, it also includes several unrelated changes that were not mentioned in the PR description.
Changes:
- Adds AES-GCM-SIV (RFC 8452) as an AEAD variant to the AES family
- Adds XChaCha20-Poly1305 to ChaCha20 family variants (not mentioned in PR description)
- Removes BLAKE2b-HMAC variant (not mentioned in PR description)
- Fixes spelling errors in TUAK patterns (TUAC -> TUAK)
- Fixes typo in bcrypt pattern (closing parenthesis -> closing brace)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }, | ||
| { | ||
| "pattern": "XChaCha20-Poly1305", | ||
| "primitive": "ae" |
There was a problem hiding this comment.
The addition of XChaCha20-Poly1305 is not mentioned in the PR description or issue #758. This change should either be removed from this PR and submitted separately with appropriate justification, or the PR description should be updated to explain why this variant is being added.
| }, | |
| { | |
| "pattern": "XChaCha20-Poly1305", | |
| "primitive": "ae" |
| { | ||
| "pattern": "BLAKE2s-(160|256)", | ||
| "primitive": "hash" | ||
| }, | ||
| { | ||
| "pattern": "BLAKE2b-(160|256|384|512)-HMAC", | ||
| "primitive": "mac" | ||
| } |
There was a problem hiding this comment.
The removal of the BLAKE2b-HMAC variant (lines 944-946 in the original) is not mentioned in the PR description or issue #758. This deletion should either be reverted and submitted in a separate PR with appropriate justification, or the PR description should be updated to explain why this variant is being removed.
stevespringett
left a comment
stevespringett
left a comment
There was a problem hiding this comment.
Please update the pattern based on the suggestion from @bhess
stevespringett
left a comment
stevespringett
left a comment
There was a problem hiding this comment.
Thanks for the PR. Could you resolve the issue that @bhess mentioned? Once corrected, we'll merge this in. Thanks again
Mehrn0ush
force-pushed
the
feat/crypto-registry-aes-gcm-siv
branch
2 times, most recently
from
c36a357 to
52a3a59
Compare
| "standard": [ | ||
| { | ||
| "name": "RFC 3686", | ||
| "url": "https://doi.org/10.17487/RFC3686" |
There was a problem hiding this comment.
Removing this entry looks like a artifact after merging, can you please check and update @Mehrn0ush ?
There was a problem hiding this comment.
Thanks @bhess — confirmed. The earlier diff around the AES-CTR-HMAC (RFC3686) entry was an artifact from previous rebases/cleanup attempts.
I’ve now rebuilt the branch cleanly on top of the current master and re-applied only the AES-GCM-SIV (RFC8452) addition with the correct pattern AES[-(128|192|256)]-GCM-SIV[-{tagLength}][-{ivLength}].
The AES-CTR-HMAC-SHA1-96 entry (RFC3686) remains unchanged.
Signed-off-by: Mehrn0ush <mehrnoush.vaseghi@gmail.com>
Mehrn0ush
force-pushed
the
feat/crypto-registry-aes-gcm-siv
branch
from
52a3a59 to
f613597
Compare
3 participants
As discussed in ticket #758, this PR adds AES-GCM-SIV as an AEAD variant to the Cryptography Registry.
Fixes #758
Details
aevariant under the existing AES familyScope
schema/cryptography-defs.json)