feat: add AES key support for KEK bundles#25
Conversation
Compute AES Key Check Value by encrypting a 16-byte zero block using ECB internally and returning the first 3 bytes as hex. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add CreateFromKeyBytes and CreateFromKeyString factory functions, matching the DES package pattern. Existing New() now delegates to CreateFromKeyBytes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add KeyType field, NewWithKeyType constructor, and MergeKey method that supports both 3DES and AES key types. Existing New() and Merge() remain backward compatible for 3DES bundles. Includes KeyCipher interface and comprehensive AES bundle tests. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
/request-review |
|
Success 🎉 The review request was sent to the following teams:
If you see something that doesn't look right, follow this doc to improve our slack channel mapping. Thank you! |
There was a problem hiding this comment.
Pull request overview
Changes: New feature (1), Test improvement (1), Documentation update (1), Maintenance (1)
This PR extends the KEK bundle system to support AES keys (in addition to existing 3DES), adds AES key factory helpers and AES key check value (KCV) support, and updates tests/testdata accordingly.
Changes:
- Add AES factory functions and AES KCV computation/verification on
aes.Cipher. - Extend KEK bundle to support AES components and merging via a
KeyTypeplusNewWithKeyTypeandMergeAESKey. - Refresh PGP test keys/test expectations, update README, and bump library version.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
aes/aes_cipher.go |
Adds AES KCV (CheckValue / VerifyCheckValue) and deprecates New in favor of factory functions. |
aes/aes_cipher_test.go |
Adds coverage for AES KCV behavior. |
aes/aes_factory.go |
Introduces CreateFromKeyBytes / CreateFromKeyString AES cipher factories. |
aes/aes_factory_test.go |
Adds validation tests for AES key factories (string + byte lengths). |
kek/kek_bundle.go |
Adds KeyType, NewWithKeyType, AES-aware AddComponent, and MergeAESKey while keeping 3DES compatibility. |
kek/kek_bundle_test.go |
Adds KEK AES component and merge tests; adds test for MergeTripleDESKey. |
pgp/pgp_factory_test.go |
Updates expected key hash and improves test failure diagnostics. |
pgp/testdata/public.pgp |
Replaces the PGP public test key. |
pgp/testdata/private.pgp |
Replaces the PGP private test key. |
pgp/testdata/key_info.yml |
Updates the PGP key metadata (expiry). |
README.md |
Updates documentation to reflect AES factory + KCV and KEK AES support. |
VERSION.txt |
Bumps version to 1.12.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df4f43141d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Context
Add AES key type support to the KEK (Key Encryption Key) bundle system, which was previously hardcoded to 3DES only.
Changes:
CheckValue()andVerifyCheckValue()toaes.Cipherfor AES Key Check Value computation (ECB encrypt of 16-byte zero block, first 3 bytes)CreateFromKeyBytes/CreateFromKeyStringfactory functions to theaespackage, matching thedesfactory patternKeyTypefield,NewWithKeyTypeconstructor, separate merge methods supporting both 3DES and AESNew()andMerge()remain backward compatible for 3DES bundlesNote
This PR is split into separate commits to simplify review process. Each commit's message explains scope of the PR changes it contains. Reviewing commit-by-commit will improve PR review experience.
Checklist
User prompts that led to this change
"Add AES keys support for KEK"
"Can we keep the check value at 3 bytes / 6 hexadecimal character always?"
"Maybe it can be done internally in the aes_cipher's VerifyCheckValue and CheckValue methods, internally it can create a new ECB cipher"
"Would you be removing the New method in aes_cipher.go and migrate it to the new aes_factory?"
New()constructor"We can make it non-backwards compatible, it's all good"
New(), then changed mind to keep it as a delegate toCreateFromKeyBytes🤖 Generated with Claude Code