feat(crypto): add hybrid NIST EC + ML-KEM key wrapping support#3276
Open
feat(crypto): add hybrid NIST EC + ML-KEM key wrapping support#3276
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds three hybrid post‑quantum key algorithms (X‑Wing, P‑256+ML‑KEM‑768, P‑384+ML‑KEM‑1024) across protobuf/OpenAPI/docs, implements X‑Wing and hybrid NIST crypto in ocrypto, integrates hybrid wrapping/unwrapping into SDK and service layers, and updates configs and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SDK as SDK<br/>(tdf.go)
participant Crypto as Cryptography<br/>(ocrypto)
participant KAS as KAS Service<br/>(security)
participant DB as Database
rect rgba(100, 150, 200, 0.5)
Note over Client,SDK: Client creates TDF using hybrid KAS public key
Client->>SDK: CreateTDF(plaintext, policy, KASInfo)
SDK->>SDK: createKeyAccess(hybrid algorithm, kasPublicKey)
SDK->>Crypto: Encapsulate(publicKey)
Crypto-->>SDK: sharedSecret, ciphertext
SDK->>Crypto: CalculateHKDF(tdfSalt, sharedSecret)
Crypto-->>SDK: wrapKey
SDK->>Crypto: AES‑GCM encrypt(wrapKey, DEK)
Crypto-->>SDK: encryptedDEK
SDK->>SDK: ASN.1 marshal(HybridWrapped)
SDK-->>Client: TDF(..., KeyAccess{keyType:"hybrid-wrapped", wrappedKey})
end
rect rgba(150, 100, 200, 0.5)
Note over KAS,DB: KAS unwraps wrapped DEK
KAS->>KAS: Decrypt(keyDetails, wrappedKey)
KAS->>Crypto: Decapsulate(ciphertext, privKey)
Crypto-->>KAS: sharedSecret
KAS->>Crypto: CalculateHKDF(tdfSalt, sharedSecret) → wrapKey
KAS->>Crypto: AES‑GCM decrypt(wrapKey, encryptedDEK)
Crypto-->>KAS: DEK
KAS-->>Client: return DecryptedKey(DEK)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces support for hybrid post-quantum cryptographic key wrapping within the TDF ecosystem. By combining classical elliptic curve cryptography with NIST-standardized ML-KEM algorithms, the changes enhance the platform's security posture against future quantum threats. The implementation includes new cryptographic primitives, updated key management logic, and necessary schema adjustments to support these hybrid schemes across the SDK and KAS services. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Quantum threats are on the way, Hybrid keys will save the day. EC and KEM now intertwined, Security for all mankind. Footnotes
|
github-actions
bot
added
comp:db
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces support for hybrid post-quantum cryptography (PQC) schemes, specifically X-Wing and NIST hybrid (EC + ML-KEM) algorithms, across the ocrypto library, SDK, and KAS service. The changes include new key generation and wrapping logic, protocol buffer updates, and service-level integration for the hybrid-wrapped key type. Review feedback identifies a critical bug in the experimental TDF implementation where all hybrid keys are incorrectly processed as X-Wing, along with a similar issue in the test environment's fake KAS. Additionally, improvements are suggested for documentation accuracy regarding supported hybrid schemes and the modularity of shared cryptographic helper functions.
| <td><p>Type of key wrapping used for the data encryption key | ||
| Required: Always | ||
| Values: 'wrapped' (RSA-wrapped for ZTDF), 'ec-wrapped' (experimental ECDH-wrapped) </p></td> | ||
| Values: 'wrapped' (RSA-wrapped for ZTDF), 'ec-wrapped' (experimental ECDH-wrapped), 'hybrid-wrapped' (experimental X-Wing-wrapped) </p></td> |
Contributor
There was a problem hiding this comment.
The description for 'hybrid-wrapped' is misleading as it only mentions "X-Wing-wrapped". This pull request introduces support for multiple hybrid schemes (X-Wing and NIST EC + ML-KEM). To avoid confusion and improve documentation accuracy, please consider making the description more generic to cover all supported hybrid algorithms.
Values: 'wrapped' (RSA-wrapped for ZTDF), 'ec-wrapped' (experimental ECDH-wrapped), 'hybrid-wrapped' (experimental hybrid key wrapping, e.g., X-Wing, NIST EC + ML-KEM) </p></td>| // --- Decryptor --- | ||
|
|
||
| func NewP256MLKEM768Decryptor(privateKey []byte) (*HybridNISTDecryptor, error) { | ||
| return NewSaltedP256MLKEM768Decryptor(privateKey, defaultXWingSalt(), nil) |
Contributor
There was a problem hiding this comment.
This function uses defaultXWingSalt(), which is defined in xwing.go. This creates a dependency on the X-Wing implementation for a generic NIST hybrid scheme and is confusingly named. A similar issue exists for other helper functions like xwingRawToPEM and cloneOrNil that are defined in xwing.go but used here.
To improve modularity and clarity, I recommend moving these generic helper functions to a new shared file (e.g., hybrid_common.go) and renaming them to be algorithm-agnostic (e.g., defaultHybridSalt()).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
service/pkg/db/marshalHelpers.go (1)
160-165:⚠️ Potential issue | 🟠 Major
FormatAlgstill misses the two ML-KEM algorithm cases.Line 160 introduces X-Wing, but
policy.Algorithm_ALGORITHM_HPQT_SECP256R1_MLKEM768andpolicy.Algorithm_ALGORITHM_HPQT_SECP384R1_MLKEM1024still hit the unsupported path.Proposed fix
case policy.Algorithm_ALGORITHM_HPQT_XWING: return "hpqt:xwing", nil + case policy.Algorithm_ALGORITHM_HPQT_SECP256R1_MLKEM768: + return "hpqt:secp256r1-mlkem768", nil + case policy.Algorithm_ALGORITHM_HPQT_SECP384R1_MLKEM1024: + return "hpqt:secp384r1-mlkem1024", nil case policy.Algorithm_ALGORITHM_UNSPECIFIED: fallthrough🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/pkg/db/marshalHelpers.go` around lines 160 - 165, FormatAlg is missing cases for the two ML-KEM algorithms so those enums fall through to the default unsupported path; add explicit case branches for policy.Algorithm_ALGORITHM_HPQT_SECP256R1_MLKEM768 and policy.Algorithm_ALGORITHM_HPQT_SECP384R1_MLKEM1024 in the switch (alongside ALGORITHM_HPQT_XWING) and return the correct canonical string identifiers (e.g., the ML-KEM names) with nil error so they no longer trigger the default unsupported-algorithm error in FormatAlg.service/kas/kas.go (1)
195-201:⚠️ Potential issue | 🟠 MajorFallback key discovery is incomplete for hybrid ML-KEM variants.
Line 195 only adds
security.AlgorithmHPQTXWing. If deployment keys are onlyhpqt:secp256r1-mlkem768orhpqt:secp384r1-mlkem1024, this fallback path may fail to seed defaults.Proposed fix
- for _, alg := range []string{security.AlgorithmECP256R1, security.AlgorithmRSA2048, security.AlgorithmHPQTXWing} { + for _, alg := range []string{ + security.AlgorithmECP256R1, + security.AlgorithmRSA2048, + security.AlgorithmHPQTXWing, + security.AlgorithmHPQTSecp256r1MLKEM768, + security.AlgorithmHPQTSecp384r1MLKEM1024, + } {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/kas/kas.go` around lines 195 - 201, The fallback loop only checks security.AlgorithmHPQTXWing and misses hybrid ML-KEM variants, causing defaults to be empty when only hybrid keys like hpqt:secp256r1-mlkem768 or hpqt:secp384r1-mlkem1024 exist; update the loop around cryptoProvider.FindKID to also try the hybrid variant identifiers (e.g., the hpqt ML-KEM names used in your deployment) in addition to security.AlgorithmHPQTXWing, appending any found KIDs to defaults and retaining the existing l.Warn for misses so cryptoProvider.FindKID, defaults, and l.Warn continue to be used consistently.service/policy/db/grant_mappings.go (1)
25-30:⚠️ Potential issue | 🟠 MajorAdd missing ML-KEM algorithm mappings in the switch.
Line 25 adds X-Wing, but algorithms for the NIST EC + ML-KEM variants still fall through to
...UNSPECIFIED, which can silently degrade grant key metadata.Proposed fix
case policy.Algorithm_ALGORITHM_HPQT_XWING: return policy.KasPublicKeyAlgEnum_KAS_PUBLIC_KEY_ALG_ENUM_HPQT_XWING + case policy.Algorithm_ALGORITHM_HPQT_SECP256R1_MLKEM768: + return policy.KasPublicKeyAlgEnum_KAS_PUBLIC_KEY_ALG_ENUM_HPQT_SECP256R1_MLKEM768 + case policy.Algorithm_ALGORITHM_HPQT_SECP384R1_MLKEM1024: + return policy.KasPublicKeyAlgEnum_KAS_PUBLIC_KEY_ALG_ENUM_HPQT_SECP384R1_MLKEM1024 case policy.Algorithm_ALGORITHM_UNSPECIFIED: return policy.KasPublicKeyAlgEnum_KAS_PUBLIC_KEY_ALG_ENUM_UNSPECIFIED🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/policy/db/grant_mappings.go` around lines 25 - 30, The switch mapping on policy.Algorithm (seen handling policy.Algorithm_ALGORITHM_HPQT_XWING -> policy.KasPublicKeyAlgEnum_KAS_PUBLIC_KEY_ALG_ENUM_HPQT_XWING) is missing cases for the NIST EC + ML-KEM algorithm variants, causing them to fall through to KAS_PUBLIC_KEY_ALG_ENUM_UNSPECIFIED; add explicit case branches for each ML-KEM algorithm constant (the policy.Algorithm_... ML-KEM variants) and return the corresponding policy.KasPublicKeyAlgEnum_KAS_PUBLIC_KEY_ALG_ENUM_... ML-KEM enum values instead of letting them default to UNSPECIFIED so grant key metadata is preserved. Ensure each new case mirrors the existing case style used for ALGORITHM_HPQT_XWING and the default remains unchanged.docs/openapi/policy/obligations/obligations.openapi.yaml (1)
511-522:⚠️ Potential issue | 🟠 MajorOpenAPI enum is missing the two ML‑KEM hybrid values
policy.Algorithmandpolicy.KasPublicKeyAlgEnuminclude...HPQT_XWINGbut still omit...HPQT_SECP256R1_MLKEM768and...HPQT_SECP384R1_MLKEM1024. That leaves this API contract out of sync with the supported algorithm set and can break generated clients.Suggested patch
policy.Algorithm: type: string title: Algorithm enum: - ALGORITHM_UNSPECIFIED - ALGORITHM_RSA_2048 - ALGORITHM_RSA_4096 - ALGORITHM_EC_P256 - ALGORITHM_EC_P384 - ALGORITHM_EC_P521 - ALGORITHM_HPQT_XWING + - ALGORITHM_HPQT_SECP256R1_MLKEM768 + - ALGORITHM_HPQT_SECP384R1_MLKEM1024 @@ policy.KasPublicKeyAlgEnum: type: string title: KasPublicKeyAlgEnum enum: - KAS_PUBLIC_KEY_ALG_ENUM_UNSPECIFIED - KAS_PUBLIC_KEY_ALG_ENUM_RSA_2048 - KAS_PUBLIC_KEY_ALG_ENUM_RSA_4096 - KAS_PUBLIC_KEY_ALG_ENUM_EC_SECP256R1 - KAS_PUBLIC_KEY_ALG_ENUM_EC_SECP384R1 - KAS_PUBLIC_KEY_ALG_ENUM_EC_SECP521R1 - KAS_PUBLIC_KEY_ALG_ENUM_HPQT_XWING + - KAS_PUBLIC_KEY_ALG_ENUM_HPQT_SECP256R1_MLKEM768 + - KAS_PUBLIC_KEY_ALG_ENUM_HPQT_SECP384R1_MLKEM1024Also applies to: 538-549
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/policy/obligations/obligations.openapi.yaml` around lines 511 - 522, The OpenAPI enums policy.Algorithm and policy.KasPublicKeyAlgEnum are missing the two ML‑KEM hybrid values; update both enum definitions to include the entries ...HPQT_SECP256R1_MLKEM768 and ...HPQT_SECP384R1_MLKEM1024 (match casing/style of existing entries like ALGORITHM_HPQT_XWING) so the API contract reflects the full supported algorithm set and stays in sync with generated clients.docs/openapi/policy/actions/actions.openapi.yaml (1)
196-207:⚠️ Potential issue | 🟠 MajorAdd the two ML-KEM enum values here as well.
policy.Algorithmandpolicy.KasPublicKeyAlgEnumstill stop at X-Wing, but this PR addsALGORITHM_HPQT_SECP256R1_MLKEM768andALGORITHM_HPQT_SECP384R1_MLKEM1024elsewhere. That leaves this spec unable to describe validkasKeys[].publicKey.algorithm/KasPublicKey.algvalues, and generated clients may reject legitimate server responses.Also applies to: 223-234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/policy/actions/actions.openapi.yaml` around lines 196 - 207, The OpenAPI enum definitions for policy.Algorithm and policy.KasPublicKeyAlgEnum are missing the two new ML-KEM values; update both enums to include ALGORITHM_HPQT_SECP256R1_MLKEM768 and ALGORITHM_HPQT_SECP384R1_MLKEM1024 so that kasKeys[].publicKey.algorithm / KasPublicKey.alg can represent the new algorithms; locate the enum blocks named policy.Algorithm and policy.KasPublicKeyAlgEnum in the YAML and append those two identifiers to their enum lists (also mirror the same changes around the second block noted at lines 223-234).docs/openapi/policy/resourcemapping/resource_mapping.openapi.yaml (1)
406-417:⚠️ Potential issue | 🟠 MajorThese shared policy enums are still missing the two ML-KEM variants.
Only X-Wing was added here, but the shared
policy.Algorithm/policy.KasPublicKeyAlgEnumdefinitions also needALGORITHM_HPQT_SECP256R1_MLKEM768andALGORITHM_HPQT_SECP384R1_MLKEM1024. Otherwise this OpenAPI document still understates the supported key algorithms for embeddedkasKeys/public-key payloads.Also applies to: 433-444
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/policy/resourcemapping/resource_mapping.openapi.yaml` around lines 406 - 417, The shared policy enum definitions are missing two ML-KEM variants; update the policy.Algorithm and policy.KasPublicKeyAlgEnum enum lists to include ALGORITHM_HPQT_SECP256R1_MLKEM768 and ALGORITHM_HPQT_SECP384R1_MLKEM1024 so the OpenAPI file accurately reflects supported key algorithms for kasKeys/public-key payloads, and mirror the same additions in the other identical enum block referenced (the second occurrence around the kas public-key enums).docs/openapi/policy/kasregistry/key_access_server_registry.openapi.yaml (1)
527-549:⚠️ Potential issue | 🟠 MajorKAS registry OpenAPI still blocks the two new NIST hybrid algorithms.
The schema enums stop at X-Wing, and the
CreateKey,ListKeys, andRotateKey.NewKeyCEL snippets still only admit up to6. Since the authoritativepolicy.Algorithmenum now includes values7and8for the ML-KEM variants, this spec still prevents clients from describing or submitting those new key types.Also applies to: 1146-1155, 1669-1678, 1931-1940
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/policy/kasregistry/key_access_server_registry.openapi.yaml` around lines 527 - 549, The OpenAPI spec and CEL validation are missing the two new ML-KEM algorithm values (numeric values 7 and 8) causing clients to be blocked; update the policy.Algorithm and policy.KasPublicKeyAlgEnum enums to include the two ML-KEM entries (matching the authoritative enum names for the new ML-KEM variants) and update any CEL expressions used in CreateKey, ListKeys, and RotateKey.NewKey (and the corresponding enum definitions at the other locations noted) to allow values 7 and 8 instead of stopping at 6 so the new key types are accepted.service/kas/access/publicKey.go (1)
79-86: 🧹 Nitpick | 🔵 TrivialMisleading comment: "For RSA keys" now also handles HPQT algorithms.
The comment on line 81 states "For RSA keys, return the public key in PKCS8 format" but this case now handles HPQT hybrid algorithms as well. Consider updating the comment to reflect this.
📝 Suggested comment update
case security.AlgorithmRSA2048, security.AlgorithmHPQTXWing, security.AlgorithmHPQTSecp256r1MLKEM768, security.AlgorithmHPQTSecp384r1MLKEM1024, "": - // For RSA keys, return the public key in PKCS8 format + // For RSA and HPQT keys, return the public key in PKCS8 format pem, err = keyDetails.ExportPublicKey(ctx, trust.KeyTypePKCS8)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/kas/access/publicKey.go` around lines 79 - 86, Update the misleading comment in the switch case that handles security.AlgorithmRSA2048, security.AlgorithmHPQTXWing, security.AlgorithmHPQTSecp256r1MLKEM768, security.AlgorithmHPQTSecp384r1MLKEM1024 and "" to accurately reflect that both RSA and HPQT/hybrid algorithms are covered; edit the comment above the case (near keyDetails.ExportPublicKey usage) to say something like "For RSA and HPQT/hybrid keys, return the public key in PKCS8 format" so the comment matches the handled algorithms and refers to keyDetails.ExportPublicKey for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/grpc/index.html`:
- Line 6291: Update the description of the 'hybrid-wrapped' value to be
algorithm-agnostic (or list all supported hybrid variants) instead of implying
X-Wing-only; locate the occurrences of the literal 'hybrid-wrapped' in the docs
(e.g., the HTML lines that currently read "hybrid-wrapped (experimental
X-Wing-wrapped)") and replace that phrasing with either a generic description
like "hybrid-wrapped (experimental hybrid KEMs combining classical and
post-quantum/ECDH schemes; algorithm-specific variants may include NIST EC +
ML-KEM, X-Wing, etc.)" or enumerate the supported hybrid variants so integrators
are not misled.
- Around line 4147-4151: The generated docs are missing the ML-KEM enum entries
for KasPublicKeyAlgEnum; add entries for KAS_PUBLIC_KEY_ALG_ENUM_MLKEM768 = 11
and KAS_PUBLIC_KEY_ALG_ENUM_MLKEM1024 = 12 alongside the existing
KAS_PUBLIC_KEY_ALG_ENUM_HPQT_XWING (10) in the KasPublicKeyAlgEnum section so
the documentation matches service/policy/objects.proto (symbols:
KasPublicKeyAlgEnum, KAS_PUBLIC_KEY_ALG_ENUM_HPQT_XWING,
KAS_PUBLIC_KEY_ALG_ENUM_MLKEM768, KAS_PUBLIC_KEY_ALG_ENUM_MLKEM1024).
In `@docs/openapi/authorization/v2/authorization.openapi.yaml`:
- Line 178: The OpenAPI enums that currently list ALGORITHM_HPQT_XWING need the
two ML-KEM variants added so API contracts cover the full hybrid family; update
both enums that contain ALGORITHM_HPQT_XWING to also include
ALGORITHM_HPQT_MLKEM and ALGORITHM_HPQT_MLKEM_AES (or your repository’s exact
ML-KEM symbol names) so NIST EC + ML-KEM variants are present alongside
ALGORITHM_HPQT_XWING.
In `@sdk/basekey_test.go`:
- Around line 96-100: Add test cases for the two new ML-KEM mappings by updating
the table-driven tests in TestGetKasKeyAlg and TestFormatAlg and the round-trip
cases so they include entries for HybridSecp256r1MLKEM768Key and
HybridSecp384r1MLKEM1024Key; specifically, add rows mapping
string(HybridSecp256r1MLKEM768Key) ->
policy.Algorithm_ALGORITHM_ML_KEM_SECP256R1_768 (or the exact enum used) and
string(HybridSecp384r1MLKEM1024Key) ->
policy.Algorithm_ALGORITHM_ML_KEM_SECP384R1_1024 in the same style as the
existing "hybrid xwing" entry, and include them in the round-trip assertion
table so TestGetKasKeyAlg, TestFormatAlg and the round-trip checks validate the
new conversions.
In `@sdk/granter.go`:
- Around line 505-510: FormatAlg is missing cases for
ALGORITHM_HPQT_SECP256R1_MLKEM768 and ALGORITHM_HPQT_SECP384R1_MLKEM1024,
causing those algs to fall through to the default error; add explicit switch
cases in FormatAlg that return the corresponding ocrypto constants
(ocrypto.HybridSecp256r1MLKEM768Key and ocrypto.HybridSecp384r1MLKEM1024Key)
similar to the existing HPQT_XWING case, and update reverseAlgMap in
service/pkg/db/marshalHelpers_test.go to include mappings from
ALGORITHM_HPQT_SECP256R1_MLKEM768 -> ocrypto.HybridSecp256r1MLKEM768Key and
ALGORITHM_HPQT_SECP384R1_MLKEM1024 -> ocrypto.HybridSecp384r1MLKEM1024Key so
tests cover these new variants.
In `@sdk/schema/manifest-lax.schema.json`:
- Line 55: The enum that currently lists ["wrapped", "hybrid-wrapped", "remote"]
must include "ec-wrapped" to preserve compatibility with existing EC manifests;
update the enum array in sdk/schema/manifest-lax.schema.json (the enum
containing "wrapped" / "hybrid-wrapped" / "remote") to also include "ec-wrapped"
so the schema accepts EC-style manifests used by SDK key access generation.
In `@sdk/tdf_test.go`:
- Around line 2919-2920: The two KAS fixture entries for "https://d.kas/" and
"https://e.kas/" advertise "rsa:2048" while their key material variables
(mockECPrivateKey1/mockECPublicKey1 and mockECPrivateKey2/mockECPublicKey2) are
EC keys; update those two entries in sdk/tdf_test.go to the correct EC algorithm
identifier (e.g., "ec:secp256r1" or the project's canonical EC string) so the
advertised algorithm matches the PEM, and re-run tests to ensure no other
fixtures rely on "rsa:2048"; this keeps KASInfo.Algorithm consistent with the
key material.
- Around line 3198-3230: The hybrid rewrap branch creates an asymmetric
encryptor via ocrypto.FromPublicPEM(bodyData.GetClientPublicKey()) and encrypts
the symmetricKey into entityWrappedKey but never sets resp.SessionPublicKey;
update this branch to mirror the ec-wrapped path by extracting the
ephemeral/session public key from the asymmetric encryptor (the same value the
ec-wrapped branch sets) and assign it to resp.SessionPublicKey after creating
asymEncrypt and before returning, so EC client keys receive the session public
key needed to unwrap the returned DEK.
In `@sdk/tdf.go`:
- Line 46: The comment notes duplicated hybrid wrap logic between kHybridWrapped
handling in createKeyAccess/generateWrapKeyWithHybrid and wrapKeyWithHybrid in
sdk/experimental/tdf/key_access.go; refactor by extracting the shared hybrid
key-wrap logic into a single helper function (e.g., NewHybridWrapKey or
wrapKeyHybridShared) placed in a common package accessible to both stable and
experimental code, update createKeyAccess to call that new helper instead of
generateWrapKeyWithHybrid, remove or forward the duplicate wrapKeyWithHybrid to
the shared helper, and ensure kHybridWrapped constant and error handling are
preserved so behavior remains unchanged.
In `@service/internal/security/standard_crypto.go`:
- Around line 502-543: The cases for StandardXWingCrypto and
StandardHybridCrypto risk confusing variable shadowing around err and
privateKey; extract each decryption path into small helper methods (e.g.,
decryptXWing using XWingPrivateKeyFromPem + XWingUnwrapDEK, decryptP256MLKEM768
using P256MLKEM768PrivateKeyFromPem + P256MLKEM768UnwrapDEK,
decryptP384MLKEM1024 using P384MLKEM1024PrivateKeyFromPem +
P384MLKEM1024UnwrapDEK) and call those from the switch, so parsing and unwrap
happen in isolated functions and no local err shadowing occurs in
StandardXWingCrypto / StandardHybridCrypto.
In `@service/policy/kasregistry/key_access_server_registry.proto`:
- Around line 403-407: The CEL validators for the key_algorithm field (validator
id "key_algorithm_defined") restrict allowed values to [1..6], but the Algorithm
enum now includes values 7 and 8 (ALGORITHM_HPQT_SECP256R1_MLKEM768 and
ALGORITHM_HPQT_SECP384R1_MLKEM1024), causing Create/List/Rotate key requests
with ML‑KEM algorithms to fail; update the expression in the key_algorithm
validators (the field named key_algorithm in key_access_server_registry.proto)
to include 7 and 8 (e.g., change "this in [1, 2, 3, 4, 5, 6]" to "this in [1, 2,
3, 4, 5, 6, 7, 8]") for all occurrences (including the other two validator
instances referenced in the comment) so the new enum values pass validation.
---
Outside diff comments:
In `@docs/openapi/policy/actions/actions.openapi.yaml`:
- Around line 196-207: The OpenAPI enum definitions for policy.Algorithm and
policy.KasPublicKeyAlgEnum are missing the two new ML-KEM values; update both
enums to include ALGORITHM_HPQT_SECP256R1_MLKEM768 and
ALGORITHM_HPQT_SECP384R1_MLKEM1024 so that kasKeys[].publicKey.algorithm /
KasPublicKey.alg can represent the new algorithms; locate the enum blocks named
policy.Algorithm and policy.KasPublicKeyAlgEnum in the YAML and append those two
identifiers to their enum lists (also mirror the same changes around the second
block noted at lines 223-234).
In `@docs/openapi/policy/kasregistry/key_access_server_registry.openapi.yaml`:
- Around line 527-549: The OpenAPI spec and CEL validation are missing the two
new ML-KEM algorithm values (numeric values 7 and 8) causing clients to be
blocked; update the policy.Algorithm and policy.KasPublicKeyAlgEnum enums to
include the two ML-KEM entries (matching the authoritative enum names for the
new ML-KEM variants) and update any CEL expressions used in CreateKey, ListKeys,
and RotateKey.NewKey (and the corresponding enum definitions at the other
locations noted) to allow values 7 and 8 instead of stopping at 6 so the new key
types are accepted.
In `@docs/openapi/policy/obligations/obligations.openapi.yaml`:
- Around line 511-522: The OpenAPI enums policy.Algorithm and
policy.KasPublicKeyAlgEnum are missing the two ML‑KEM hybrid values; update both
enum definitions to include the entries ...HPQT_SECP256R1_MLKEM768 and
...HPQT_SECP384R1_MLKEM1024 (match casing/style of existing entries like
ALGORITHM_HPQT_XWING) so the API contract reflects the full supported algorithm
set and stays in sync with generated clients.
In `@docs/openapi/policy/resourcemapping/resource_mapping.openapi.yaml`:
- Around line 406-417: The shared policy enum definitions are missing two ML-KEM
variants; update the policy.Algorithm and policy.KasPublicKeyAlgEnum enum lists
to include ALGORITHM_HPQT_SECP256R1_MLKEM768 and
ALGORITHM_HPQT_SECP384R1_MLKEM1024 so the OpenAPI file accurately reflects
supported key algorithms for kasKeys/public-key payloads, and mirror the same
additions in the other identical enum block referenced (the second occurrence
around the kas public-key enums).
In `@service/kas/access/publicKey.go`:
- Around line 79-86: Update the misleading comment in the switch case that
handles security.AlgorithmRSA2048, security.AlgorithmHPQTXWing,
security.AlgorithmHPQTSecp256r1MLKEM768,
security.AlgorithmHPQTSecp384r1MLKEM1024 and "" to accurately reflect that both
RSA and HPQT/hybrid algorithms are covered; edit the comment above the case
(near keyDetails.ExportPublicKey usage) to say something like "For RSA and
HPQT/hybrid keys, return the public key in PKCS8 format" so the comment matches
the handled algorithms and refers to keyDetails.ExportPublicKey for clarity.
In `@service/kas/kas.go`:
- Around line 195-201: The fallback loop only checks security.AlgorithmHPQTXWing
and misses hybrid ML-KEM variants, causing defaults to be empty when only hybrid
keys like hpqt:secp256r1-mlkem768 or hpqt:secp384r1-mlkem1024 exist; update the
loop around cryptoProvider.FindKID to also try the hybrid variant identifiers
(e.g., the hpqt ML-KEM names used in your deployment) in addition to
security.AlgorithmHPQTXWing, appending any found KIDs to defaults and retaining
the existing l.Warn for misses so cryptoProvider.FindKID, defaults, and l.Warn
continue to be used consistently.
In `@service/pkg/db/marshalHelpers.go`:
- Around line 160-165: FormatAlg is missing cases for the two ML-KEM algorithms
so those enums fall through to the default unsupported path; add explicit case
branches for policy.Algorithm_ALGORITHM_HPQT_SECP256R1_MLKEM768 and
policy.Algorithm_ALGORITHM_HPQT_SECP384R1_MLKEM1024 in the switch (alongside
ALGORITHM_HPQT_XWING) and return the correct canonical string identifiers (e.g.,
the ML-KEM names) with nil error so they no longer trigger the default
unsupported-algorithm error in FormatAlg.
In `@service/policy/db/grant_mappings.go`:
- Around line 25-30: The switch mapping on policy.Algorithm (seen handling
policy.Algorithm_ALGORITHM_HPQT_XWING ->
policy.KasPublicKeyAlgEnum_KAS_PUBLIC_KEY_ALG_ENUM_HPQT_XWING) is missing cases
for the NIST EC + ML-KEM algorithm variants, causing them to fall through to
KAS_PUBLIC_KEY_ALG_ENUM_UNSPECIFIED; add explicit case branches for each ML-KEM
algorithm constant (the policy.Algorithm_... ML-KEM variants) and return the
corresponding policy.KasPublicKeyAlgEnum_KAS_PUBLIC_KEY_ALG_ENUM_... ML-KEM enum
values instead of letting them default to UNSPECIFIED so grant key metadata is
preserved. Ensure each new case mirrors the existing case style used for
ALGORITHM_HPQT_XWING and the default remains unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 19596a14-4638-47ad-be5a-be8062ddd675
⛔ Files ignored due to path filters (4)
lib/ocrypto/go.sumis excluded by!**/*.sumprotocol/go/kas/kas.pb.gois excluded by!**/*.pb.goprotocol/go/policy/kasregistry/key_access_server_registry.pb.gois excluded by!**/*.pb.goprotocol/go/policy/objects.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (50)
docs/grpc/index.htmldocs/openapi/authorization/authorization.openapi.yamldocs/openapi/authorization/v2/authorization.openapi.yamldocs/openapi/kas/kas.openapi.yamldocs/openapi/policy/actions/actions.openapi.yamldocs/openapi/policy/attributes/attributes.openapi.yamldocs/openapi/policy/kasregistry/key_access_server_registry.openapi.yamldocs/openapi/policy/namespaces/namespaces.openapi.yamldocs/openapi/policy/objects.openapi.yamldocs/openapi/policy/obligations/obligations.openapi.yamldocs/openapi/policy/registeredresources/registered_resources.openapi.yamldocs/openapi/policy/resourcemapping/resource_mapping.openapi.yamldocs/openapi/policy/subjectmapping/subject_mapping.openapi.yamldocs/openapi/policy/unsafe/unsafe.openapi.yamllib/ocrypto/asym_decryption.golib/ocrypto/asym_encryption.golib/ocrypto/ec_key_pair.golib/ocrypto/go.modlib/ocrypto/hybrid_nist.golib/ocrypto/hybrid_nist_test.golib/ocrypto/xwing.golib/ocrypto/xwing_test.gosdk/basekey.gosdk/basekey_test.gosdk/experimental/tdf/key_access.gosdk/experimental/tdf/key_access_test.gosdk/experimental/tdf/keysplit/attributes.gosdk/experimental/tdf/keysplit/attributes_test.gosdk/granter.gosdk/schema/manifest-lax.schema.jsonsdk/schema/manifest.schema.jsonsdk/tdf.gosdk/tdf_hybrid_test.gosdk/tdf_test.goservice/internal/security/basic_manager.goservice/internal/security/crypto_provider.goservice/internal/security/in_process_provider.goservice/internal/security/standard_crypto.goservice/kas/access/provider.goservice/kas/access/publicKey.goservice/kas/access/rewrap.goservice/kas/kas.goservice/kas/kas.protoservice/kas/key_indexer.goservice/kas/key_indexer_test.goservice/pkg/db/marshalHelpers.goservice/pkg/db/marshalHelpers_test.goservice/policy/db/grant_mappings.goservice/policy/kasregistry/key_access_server_registry.protoservice/policy/objects.proto
👮 Files not reviewed due to content moderation or server errors (8)
- service/kas/access/rewrap.go
- docs/openapi/policy/unsafe/unsafe.openapi.yaml
- service/kas/key_indexer_test.go
- docs/openapi/policy/namespaces/namespaces.openapi.yaml
- service/kas/key_indexer.go
- docs/openapi/policy/subjectmapping/subject_mapping.openapi.yaml
- service/internal/security/crypto_provider.go
- docs/openapi/authorization/authorization.openapi.yaml
| <td><p>Type of key wrapping used for the data encryption key | ||
| Required: Always | ||
| Values: 'wrapped' (RSA-wrapped for ZTDF), 'ec-wrapped' (experimental ECDH-wrapped) </p></td> | ||
| Values: 'wrapped' (RSA-wrapped for ZTDF), 'ec-wrapped' (experimental ECDH-wrapped), 'hybrid-wrapped' (experimental X-Wing-wrapped) </p></td> |
There was a problem hiding this comment.
Broaden hybrid wrapping wording beyond X-Wing-only phrasing.
Line 6291 and Line 6676 describe hybrid-wrapped as X-Wing-based only. Given this PR’s hybrid NIST EC + ML-KEM scope, these descriptions should be algorithm-agnostic (or enumerate all supported hybrid variants) to avoid misleading integrators.
📝 Suggested wording update
-Values: 'wrapped' (RSA-wrapped for ZTDF), 'ec-wrapped' (experimental ECDH-wrapped), 'hybrid-wrapped' (experimental X-Wing-wrapped)
+Values: 'wrapped' (RSA-wrapped for ZTDF), 'ec-wrapped' (experimental ECDH-wrapped), 'hybrid-wrapped' (experimental hybrid PQ-wrapped, e.g., X-Wing and NIST EC+ML-KEM variants)-Optional: Empty for RSA-based or X-Wing-based ZTDF (key_type="wrapped" or key_type="hybrid-wrapped")
+Optional: Empty for RSA-based or hybrid PQ-wrapped ZTDF (key_type="wrapped" or key_type="hybrid-wrapped")Also applies to: 6676-6676
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/grpc/index.html` at line 6291, Update the description of the
'hybrid-wrapped' value to be algorithm-agnostic (or list all supported hybrid
variants) instead of implying X-Wing-only; locate the occurrences of the literal
'hybrid-wrapped' in the docs (e.g., the HTML lines that currently read
"hybrid-wrapped (experimental X-Wing-wrapped)") and replace that phrasing with
either a generic description like "hybrid-wrapped (experimental hybrid KEMs
combining classical and post-quantum/ECDH schemes; algorithm-specific variants
may include NIST EC + ML-KEM, X-Wing, etc.)" or enumerate the supported hybrid
variants so integrators are not misled.
Comment on lines
+2919
to
+2920
| {"https://d.kas/", mockECPrivateKey1, mockECPublicKey1, "e1", "rsa:2048"}, | ||
| {"https://e.kas/", mockECPrivateKey2, mockECPublicKey2, defaultKID, "rsa:2048"}, |
There was a problem hiding this comment.
Fix the advertised algorithm for the EC KAS fixtures.
https://d.kas/ and https://e.kas/ serve EC key material, but they are still registered as "rsa:2048". That makes the test harness internally inconsistent and can hide regressions in any path that starts trusting KASInfo.Algorithm instead of inferring the key type from the PEM.
Suggested fix
- {"https://d.kas/", mockECPrivateKey1, mockECPublicKey1, "e1", "rsa:2048"},
- {"https://e.kas/", mockECPrivateKey2, mockECPublicKey2, defaultKID, "rsa:2048"},
+ {"https://d.kas/", mockECPrivateKey1, mockECPublicKey1, "e1", string(ocrypto.EC256Key)},
+ {"https://e.kas/", mockECPrivateKey2, mockECPublicKey2, defaultKID, string(ocrypto.EC256Key)},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {"https://d.kas/", mockECPrivateKey1, mockECPublicKey1, "e1", "rsa:2048"}, | |
| {"https://e.kas/", mockECPrivateKey2, mockECPublicKey2, defaultKID, "rsa:2048"}, | |
| {"https://d.kas/", mockECPrivateKey1, mockECPublicKey1, "e1", string(ocrypto.EC256Key)}, | |
| {"https://e.kas/", mockECPrivateKey2, mockECPublicKey2, defaultKID, string(ocrypto.EC256Key)}, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/tdf_test.go` around lines 2919 - 2920, The two KAS fixture entries for
"https://d.kas/" and "https://e.kas/" advertise "rsa:2048" while their key
material variables (mockECPrivateKey1/mockECPublicKey1 and
mockECPrivateKey2/mockECPublicKey2) are EC keys; update those two entries in
sdk/tdf_test.go to the correct EC algorithm identifier (e.g., "ec:secp256r1" or
the project's canonical EC string) so the advertised algorithm matches the PEM,
and re-run tests to ensure no other fixtures rely on "rsa:2048"; this keeps
KASInfo.Algorithm consistent with the key material.
Comment on lines
+3198
to
+3230
| case "hybrid-wrapped": | ||
| kasPrivateKey := strings.ReplaceAll(f.privateKey, "\n\t", "\n") | ||
| if kao.GetKid() != "" && kao.GetKid() != f.KID { | ||
| lk, ok := f.legakeys[kaoReq.GetKeyAccessObject().GetKid()] | ||
| f.s.Require().True(ok, "unable to find key [%s]", kao.GetKid()) | ||
| kasPrivateKey = strings.ReplaceAll(lk.private, "\n\t", "\n") | ||
| } | ||
|
|
||
| var symmetricKey []byte | ||
| switch ocrypto.KeyType(f.Algorithm) { //nolint:exhaustive // only handle hybrid types | ||
| case ocrypto.HybridSecp256r1MLKEM768Key: | ||
| privateKey, err := ocrypto.P256MLKEM768PrivateKeyFromPem([]byte(kasPrivateKey)) | ||
| f.s.Require().NoError(err, "failed to extract P256+ML-KEM-768 private key from PEM") | ||
| symmetricKey, err = ocrypto.P256MLKEM768UnwrapDEK(privateKey, wrappedKey) | ||
| f.s.Require().NoError(err, "failed to unwrap P256+ML-KEM-768 wrapped key") | ||
| case ocrypto.HybridSecp384r1MLKEM1024Key: | ||
| privateKey, err := ocrypto.P384MLKEM1024PrivateKeyFromPem([]byte(kasPrivateKey)) | ||
| f.s.Require().NoError(err, "failed to extract P384+ML-KEM-1024 private key from PEM") | ||
| symmetricKey, err = ocrypto.P384MLKEM1024UnwrapDEK(privateKey, wrappedKey) | ||
| f.s.Require().NoError(err, "failed to unwrap P384+ML-KEM-1024 wrapped key") | ||
| case ocrypto.HybridXWingKey: | ||
| privateKey, err := ocrypto.XWingPrivateKeyFromPem([]byte(kasPrivateKey)) | ||
| f.s.Require().NoError(err, "failed to extract X-Wing private key from PEM") | ||
| symmetricKey, err = ocrypto.XWingUnwrapDEK(privateKey, wrappedKey) | ||
| f.s.Require().NoError(err, "failed to unwrap X-Wing wrapped key") | ||
| default: | ||
| f.s.Require().Failf("unsupported hybrid algorithm", "algorithm: %s", f.Algorithm) | ||
| } | ||
|
|
||
| asymEncrypt, err := ocrypto.FromPublicPEM(bodyData.GetClientPublicKey()) | ||
| f.s.Require().NoError(err, "ocrypto.FromPublicPEM failed") | ||
| entityWrappedKey, err = asymEncrypt.Encrypt(symmetricKey) | ||
| f.s.Require().NoError(err, "ocrypto.encrypt failed") |
There was a problem hiding this comment.
Populate SessionPublicKey when hybrid rewrap targets an EC client key.
This branch re-encrypts with FromPublicPEM(...) and never sets resp.SessionPublicKey. If the client public key in the rewrap request is EC, the reader won't have the ephemeral session key it needs to unwrap the returned DEK. The ec-wrapped path in the same method already handles this correctly.
Suggested fix
- asymEncrypt, err := ocrypto.FromPublicPEM(bodyData.GetClientPublicKey())
- f.s.Require().NoError(err, "ocrypto.FromPublicPEM failed")
- entityWrappedKey, err = asymEncrypt.Encrypt(symmetricKey)
- f.s.Require().NoError(err, "ocrypto.encrypt failed")
+ asymEncrypt, err := ocrypto.FromPublicPEMWithSalt(bodyData.GetClientPublicKey(), tdfSalt(), nil)
+ f.s.Require().NoError(err, "ocrypto.FromPublicPEMWithSalt failed")
+ if e, ok := asymEncrypt.(ocrypto.ECEncryptor); ok {
+ resp.SessionPublicKey, err = e.PublicKeyInPemFormat()
+ f.s.Require().NoError(err, "unable to serialize ephemeral key")
+ }
+ entityWrappedKey, err = asymEncrypt.Encrypt(symmetricKey)
+ f.s.Require().NoError(err, "ocrypto.encrypt failed")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case "hybrid-wrapped": | |
| kasPrivateKey := strings.ReplaceAll(f.privateKey, "\n\t", "\n") | |
| if kao.GetKid() != "" && kao.GetKid() != f.KID { | |
| lk, ok := f.legakeys[kaoReq.GetKeyAccessObject().GetKid()] | |
| f.s.Require().True(ok, "unable to find key [%s]", kao.GetKid()) | |
| kasPrivateKey = strings.ReplaceAll(lk.private, "\n\t", "\n") | |
| } | |
| var symmetricKey []byte | |
| switch ocrypto.KeyType(f.Algorithm) { //nolint:exhaustive // only handle hybrid types | |
| case ocrypto.HybridSecp256r1MLKEM768Key: | |
| privateKey, err := ocrypto.P256MLKEM768PrivateKeyFromPem([]byte(kasPrivateKey)) | |
| f.s.Require().NoError(err, "failed to extract P256+ML-KEM-768 private key from PEM") | |
| symmetricKey, err = ocrypto.P256MLKEM768UnwrapDEK(privateKey, wrappedKey) | |
| f.s.Require().NoError(err, "failed to unwrap P256+ML-KEM-768 wrapped key") | |
| case ocrypto.HybridSecp384r1MLKEM1024Key: | |
| privateKey, err := ocrypto.P384MLKEM1024PrivateKeyFromPem([]byte(kasPrivateKey)) | |
| f.s.Require().NoError(err, "failed to extract P384+ML-KEM-1024 private key from PEM") | |
| symmetricKey, err = ocrypto.P384MLKEM1024UnwrapDEK(privateKey, wrappedKey) | |
| f.s.Require().NoError(err, "failed to unwrap P384+ML-KEM-1024 wrapped key") | |
| case ocrypto.HybridXWingKey: | |
| privateKey, err := ocrypto.XWingPrivateKeyFromPem([]byte(kasPrivateKey)) | |
| f.s.Require().NoError(err, "failed to extract X-Wing private key from PEM") | |
| symmetricKey, err = ocrypto.XWingUnwrapDEK(privateKey, wrappedKey) | |
| f.s.Require().NoError(err, "failed to unwrap X-Wing wrapped key") | |
| default: | |
| f.s.Require().Failf("unsupported hybrid algorithm", "algorithm: %s", f.Algorithm) | |
| } | |
| asymEncrypt, err := ocrypto.FromPublicPEM(bodyData.GetClientPublicKey()) | |
| f.s.Require().NoError(err, "ocrypto.FromPublicPEM failed") | |
| entityWrappedKey, err = asymEncrypt.Encrypt(symmetricKey) | |
| f.s.Require().NoError(err, "ocrypto.encrypt failed") | |
| case "hybrid-wrapped": | |
| kasPrivateKey := strings.ReplaceAll(f.privateKey, "\n\t", "\n") | |
| if kao.GetKid() != "" && kao.GetKid() != f.KID { | |
| lk, ok := f.legakeys[kaoReq.GetKeyAccessObject().GetKid()] | |
| f.s.Require().True(ok, "unable to find key [%s]", kao.GetKid()) | |
| kasPrivateKey = strings.ReplaceAll(lk.private, "\n\t", "\n") | |
| } | |
| var symmetricKey []byte | |
| switch ocrypto.KeyType(f.Algorithm) { //nolint:exhaustive // only handle hybrid types | |
| case ocrypto.HybridSecp256r1MLKEM768Key: | |
| privateKey, err := ocrypto.P256MLKEM768PrivateKeyFromPem([]byte(kasPrivateKey)) | |
| f.s.Require().NoError(err, "failed to extract P256+ML-KEM-768 private key from PEM") | |
| symmetricKey, err = ocrypto.P256MLKEM768UnwrapDEK(privateKey, wrappedKey) | |
| f.s.Require().NoError(err, "failed to unwrap P256+ML-KEM-768 wrapped key") | |
| case ocrypto.HybridSecp384r1MLKEM1024Key: | |
| privateKey, err := ocrypto.P384MLKEM1024PrivateKeyFromPem([]byte(kasPrivateKey)) | |
| f.s.Require().NoError(err, "failed to extract P384+ML-KEM-1024 private key from PEM") | |
| symmetricKey, err = ocrypto.P384MLKEM1024UnwrapDEK(privateKey, wrappedKey) | |
| f.s.Require().NoError(err, "failed to unwrap P384+ML-KEM-1024 wrapped key") | |
| case ocrypto.HybridXWingKey: | |
| privateKey, err := ocrypto.XWingPrivateKeyFromPem([]byte(kasPrivateKey)) | |
| f.s.Require().NoError(err, "failed to extract X-Wing private key from PEM") | |
| symmetricKey, err = ocrypto.XWingUnwrapDEK(privateKey, wrappedKey) | |
| f.s.Require().NoError(err, "failed to unwrap X-Wing wrapped key") | |
| default: | |
| f.s.Require().Failf("unsupported hybrid algorithm", "algorithm: %s", f.Algorithm) | |
| } | |
| asymEncrypt, err := ocrypto.FromPublicPEMWithSalt(bodyData.GetClientPublicKey(), tdfSalt(), nil) | |
| f.s.Require().NoError(err, "ocrypto.FromPublicPEMWithSalt failed") | |
| if e, ok := asymEncrypt.(ocrypto.ECEncryptor); ok { | |
| resp.SessionPublicKey, err = e.PublicKeyInPemFormat() | |
| f.s.Require().NoError(err, "unable to serialize ephemeral key") | |
| } | |
| entityWrappedKey, err = asymEncrypt.Encrypt(symmetricKey) | |
| f.s.Require().NoError(err, "ocrypto.encrypt failed") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/tdf_test.go` around lines 3198 - 3230, The hybrid rewrap branch creates
an asymmetric encryptor via ocrypto.FromPublicPEM(bodyData.GetClientPublicKey())
and encrypts the symmetricKey into entityWrappedKey but never sets
resp.SessionPublicKey; update this branch to mirror the ec-wrapped path by
extracting the ephemeral/session public key from the asymmetric encryptor (the
same value the ec-wrapped branch sets) and assign it to resp.SessionPublicKey
after creating asymEncrypt and before returning, so EC client keys receive the
session public key needed to unwrap the returned DEK.
| kKeySize = 32 | ||
| kWrapped = "wrapped" | ||
| kECWrapped = "ec-wrapped" | ||
| kHybridWrapped = "hybrid-wrapped" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM! Hybrid key wrapping integration in TDF creation.
The implementation correctly:
- Adds the
kHybridWrappedconstant - Extends
createKeyAccessto handle hybrid key types before EC/RSA - Provides
generateWrapKeyWithXWingandgenerateWrapKeyWithHybridhelpers with proper error handling
There's structural similarity between generateWrapKeyWithHybrid here and wrapKeyWithHybrid in sdk/experimental/tdf/key_access.go. If the experimental package becomes stable, consider consolidating these into a shared helper to reduce duplication.
Also applies to: 678-686, 773-814
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/tdf.go` at line 46, The comment notes duplicated hybrid wrap logic
between kHybridWrapped handling in createKeyAccess/generateWrapKeyWithHybrid and
wrapKeyWithHybrid in sdk/experimental/tdf/key_access.go; refactor by extracting
the shared hybrid key-wrap logic into a single helper function (e.g.,
NewHybridWrapKey or wrapKeyHybridShared) placed in a common package accessible
to both stable and experimental code, update createKeyAccess to call that new
helper instead of generateWrapKeyWithHybrid, remove or forward the duplicate
wrapKeyWithHybrid to the shared helper, and ensure kHybridWrapped constant and
error handling are preserved so behavior remains unchanged.
Comment on lines
+502
to
+543
| case StandardXWingCrypto: | ||
| if len(ephemeralPublicKey) > 0 { | ||
| return nil, errors.New("ephemeral public key should not be provided for X-Wing decryption") | ||
| } | ||
|
|
||
| privateKey, err := ocrypto.XWingPrivateKeyFromPem([]byte(key.xwingPrivateKeyPem)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse X-Wing private key: %w", err) | ||
| } | ||
|
|
||
| rawKey, err = ocrypto.XWingUnwrapDEK(privateKey, ciphertext) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to decrypt with X-Wing: %w", err) | ||
| } | ||
|
|
||
| case StandardHybridCrypto: | ||
| if len(ephemeralPublicKey) > 0 { | ||
| return nil, errors.New("ephemeral public key should not be provided for hybrid decryption") | ||
| } | ||
|
|
||
| switch key.Algorithm { | ||
| case AlgorithmHPQTSecp256r1MLKEM768: | ||
| privateKey, err := ocrypto.P256MLKEM768PrivateKeyFromPem([]byte(key.hybridPrivateKeyPem)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse P256-MLKEM768 private key: %w", err) | ||
| } | ||
| rawKey, err = ocrypto.P256MLKEM768UnwrapDEK(privateKey, ciphertext) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to decrypt with P256-MLKEM768: %w", err) | ||
| } | ||
| case AlgorithmHPQTSecp384r1MLKEM1024: | ||
| privateKey, err := ocrypto.P384MLKEM1024PrivateKeyFromPem([]byte(key.hybridPrivateKeyPem)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse P384-MLKEM1024 private key: %w", err) | ||
| } | ||
| rawKey, err = ocrypto.P384MLKEM1024UnwrapDEK(privateKey, ciphertext) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to decrypt with P384-MLKEM1024: %w", err) | ||
| } | ||
| default: | ||
| return nil, fmt.Errorf("unsupported hybrid algorithm [%s]", key.Algorithm) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Variable shadowing may hide errors in hybrid decryption paths.
In the StandardXWingCrypto and StandardHybridCrypto cases, there's a potential issue with variable shadowing:
rawKey, err = ocrypto.XWingUnwrapDEK(privateKey, ciphertext)The err variable on line 512 shadows the err declared on line 507, which means if XWingPrivateKeyFromPem fails but err is reassigned, the original error could be lost. However, looking more closely, the current flow returns early on error, so this is actually safe.
The code flow is correct because:
privateKey, err := ...- if this errors, it returns immediatelyrawKey, err = ...- this uses assignment, not declaration
Recommendation: Consider using more explicit error variable names or extracting each hybrid case into a helper function for clarity.
♻️ Optional refactor for clarity
Consider extracting each hybrid decrypt case into a helper method:
func (s *StandardCrypto) decryptXWing(key StandardXWingCrypto, ciphertext []byte) ([]byte, error) {
privateKey, err := ocrypto.XWingPrivateKeyFromPem([]byte(key.xwingPrivateKeyPem))
if err != nil {
return nil, fmt.Errorf("failed to parse X-Wing private key: %w", err)
}
return ocrypto.XWingUnwrapDEK(privateKey, ciphertext)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/internal/security/standard_crypto.go` around lines 502 - 543, The
cases for StandardXWingCrypto and StandardHybridCrypto risk confusing variable
shadowing around err and privateKey; extract each decryption path into small
helper methods (e.g., decryptXWing using XWingPrivateKeyFromPem +
XWingUnwrapDEK, decryptP256MLKEM768 using P256MLKEM768PrivateKeyFromPem +
P256MLKEM768UnwrapDEK, decryptP384MLKEM1024 using P384MLKEM1024PrivateKeyFromPem
+ P384MLKEM1024UnwrapDEK) and call those from the switch, so parsing and unwrap
happen in isolated functions and no local err shadowing occurs in
StandardXWingCrypto / StandardHybridCrypto.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/experimental/tdf/key_access.go (1)
221-233:⚠️ Potential issue | 🟠 MajorEC key wrapping uses XOR instead of AES-GCM - breaks interoperability with stable SDK.
The experimental
wrapKeyWithECfunction uses XOR-based key wrapping (line 230:wrapped[i] = symKey[i] ^ wrapKey[i]), while the stable SDK'sgenerateWrapKeyWithECuses AES-GCM encryption. TDFs created with the experimental SDK using EC keys will not be decryptable by the stable SDK, and vice versa.The hybrid key wrapping implementation correctly uses AES-GCM (line 298), matching the stable SDK pattern. Align
wrapKeyWithECto use AES-GCM for consistency and interoperability:Expected change
Replace the XOR wrapping logic with AES-GCM:
gcm, err := ocrypto.NewAESGcm(wrapKey) if err != nil { return "", "", "", fmt.Errorf("failed to create AES-GCM: %w", err) } wrapped, err := gcm.Encrypt(symKey) if err != nil { return "", "", "", fmt.Errorf("failed to encrypt: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/experimental/tdf/key_access.go` around lines 221 - 233, The wrapKeyWithEC function currently XORs symKey with wrapKey (wrapped[i] = symKey[i] ^ wrapKey[i]) which breaks interoperability with the stable SDK; replace that XOR logic in wrapKeyWithEC with AES-GCM encryption like the hybrid path and the stable generateWrapKeyWithEC uses: create an AES-GCM instance from wrapKey (using ocrypto.NewAESGcm or equivalent), call its Encrypt on symKey to produce wrapped, and propagate any errors with wrapped/ephemeralPubKey error returns (e.g., wrap failure -> return "", "", "", fmt.Errorf("failed to create AES-GCM: %w", err) or "failed to encrypt: %w"). Ensure the function still returns Base64Encode(wrapped), the same wrapper type "eccWrapped", and the ephemeralPubKey on success.
♻️ Duplicate comments (1)
sdk/tdf.go (1)
774-836: 🧹 Nitpick | 🔵 TrivialImplementation is correct; consider consolidating with experimental package.
The hybrid wrapping logic follows sound cryptographic practices:
- KEM encapsulation → HKDF key derivation → AES-GCM encryption → ASN.1 marshaling
However, this function is structurally identical to
wrapKeyWithHybridinsdk/experimental/tdf/key_access.go. When the experimental package stabilizes, extract the shared logic into a common helper to reduce maintenance burden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/tdf.go` around lines 774 - 836, The generateWrapKeyWithHybrid function duplicates logic found in wrapKeyWithHybrid (sdk/experimental/tdf/key_access.go); consolidate by extracting the common KEM→HKDF→AES-GCM→ASN.1 flow into a shared helper (e.g., shared package or util function) that both generateWrapKeyWithHybrid and wrapKeyWithHybrid call, keeping unique parsing/algorithm-dispatching in each caller (referencing generateWrapKeyWithHybrid, wrapKeyWithHybrid, and the ocrypto encapsulate/KDF/AESGcm/ASN.1 steps) so maintenance happens in one place once the experimental package stabilizes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ocrypto/hybrid_nist.go`:
- Around line 97-105: Add a short clarifying comment around the mlkemDecapsulate
implementation (the anonymous function assigned to mlkemDecapsulate that calls
sk.DecapsulateTo) stating that CIRCL's DecapsulateTo implements "implicit
rejection" and therefore does not return an error on incorrect ciphertext, and
that ciphertext authenticity is provided by the subsequent AES-GCM decryption
step rather than by ML-KEM; place the comment next to the DecapsulateTo call so
future readers understand the design choice and where tampering is detected.
---
Outside diff comments:
In `@sdk/experimental/tdf/key_access.go`:
- Around line 221-233: The wrapKeyWithEC function currently XORs symKey with
wrapKey (wrapped[i] = symKey[i] ^ wrapKey[i]) which breaks interoperability with
the stable SDK; replace that XOR logic in wrapKeyWithEC with AES-GCM encryption
like the hybrid path and the stable generateWrapKeyWithEC uses: create an
AES-GCM instance from wrapKey (using ocrypto.NewAESGcm or equivalent), call its
Encrypt on symKey to produce wrapped, and propagate any errors with
wrapped/ephemeralPubKey error returns (e.g., wrap failure -> return "", "", "",
fmt.Errorf("failed to create AES-GCM: %w", err) or "failed to encrypt: %w").
Ensure the function still returns Base64Encode(wrapped), the same wrapper type
"eccWrapped", and the ephemeralPubKey on success.
---
Duplicate comments:
In `@sdk/tdf.go`:
- Around line 774-836: The generateWrapKeyWithHybrid function duplicates logic
found in wrapKeyWithHybrid (sdk/experimental/tdf/key_access.go); consolidate by
extracting the common KEM→HKDF→AES-GCM→ASN.1 flow into a shared helper (e.g.,
shared package or util function) that both generateWrapKeyWithHybrid and
wrapKeyWithHybrid call, keeping unique parsing/algorithm-dispatching in each
caller (referencing generateWrapKeyWithHybrid, wrapKeyWithHybrid, and the
ocrypto encapsulate/KDF/AESGcm/ASN.1 steps) so maintenance happens in one place
once the experimental package stabilizes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7e652f77-7128-4434-a346-54a1bb0cd243
📒 Files selected for processing (7)
lib/ocrypto/ec_key_pair.golib/ocrypto/hybrid_nist.golib/ocrypto/hybrid_nist_test.golib/ocrypto/xwing.golib/ocrypto/xwing_test.gosdk/experimental/tdf/key_access.gosdk/tdf.go
Comment on lines
+97
to
+105
| mlkemDecapsulate: func(privKey, ciphertext []byte) ([]byte, error) { | ||
| var sk mlkem768.PrivateKey | ||
| if err := sk.Unpack(privKey); err != nil { | ||
| return nil, fmt.Errorf("mlkem768 private key unpack: %w", err) | ||
| } | ||
| ss := make([]byte, mlkem768.SharedKeySize) | ||
| sk.DecapsulateTo(ss, ciphertext) | ||
| return ss, nil | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
ML-KEM DecapsulateTo does not indicate decapsulation failure.
CIRCL's DecapsulateTo (lines 103, 134) doesn't return an error—it implements "implicit rejection" per the ML-KEM spec, meaning wrong ciphertext produces a pseudorandom output rather than failing. This is by design for IND-CCA2 security, but the calling code relies on subsequent AES-GCM decryption to detect tampering.
This is correct cryptographic behavior, but consider adding a comment explaining that authentication is provided by AES-GCM, not ML-KEM decapsulation.
📝 Suggested documentation
mlkemDecapsulate: func(privKey, ciphertext []byte) ([]byte, error) {
var sk mlkem768.PrivateKey
if err := sk.Unpack(privKey); err != nil {
return nil, fmt.Errorf("mlkem768 private key unpack: %w", err)
}
ss := make([]byte, mlkem768.SharedKeySize)
+ // DecapsulateTo implements implicit rejection per ML-KEM spec.
+ // Authentication is provided by subsequent AES-GCM decryption.
sk.DecapsulateTo(ss, ciphertext)
return ss, nil
},Also applies to: 128-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ocrypto/hybrid_nist.go` around lines 97 - 105, Add a short clarifying
comment around the mlkemDecapsulate implementation (the anonymous function
assigned to mlkemDecapsulate that calls sk.DecapsulateTo) stating that CIRCL's
DecapsulateTo implements "implicit rejection" and therefore does not return an
error on incorrect ciphertext, and that ciphertext authenticity is provided by
the subsequent AES-GCM decryption step rather than by ML-KEM; place the comment
next to the DecapsulateTo call so future readers understand the design choice
and where tampering is detected.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ocrypto/hybrid_nist_test.go`:
- Around line 197-227: The tests TestP256MLKEM768Encapsulate and
TestP384MLKEM1024Encapsulate currently only check hybridCt is non-empty; change
those assertions to validate the exact ciphertext length using the protocol
constants (P256MLKEM768CiphertextSize and P384MLKEM1024CiphertextSize) against
the hybridCt slice (and prefer require/ assert that checks size, e.g.,
require.Equal/ assert.Len) so the tests fail on size regressions instead of just
non-empty values.
- Around line 87-111: The tests TestP256MLKEM768WrapUnwrapWrongKeyFails and
TestP384MLKEM1024WrapUnwrapWrongKeyFails currently accept any error; update each
to assert the specific unwrap failure by replacing the generic require.Error
check with a check that the returned error from P256MLKEM768UnwrapDEK /
P384MLKEM1024UnwrapDEK contains the string "AES-GCM decrypt failed" (e.g., use
require.ErrorContains(t, err, "AES-GCM decrypt failed") or an equivalent
assertion) so the wrong-key path is validated explicitly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6213bab2-4c80-4aae-9340-efd652fbb3d5
📒 Files selected for processing (2)
lib/ocrypto/hybrid_nist_test.golib/ocrypto/xwing_test.go
Comment on lines
+87
to
+111
| func TestP256MLKEM768WrapUnwrapWrongKeyFails(t *testing.T) { | ||
| keyPair, err := NewP256MLKEM768KeyPair() | ||
| require.NoError(t, err) | ||
| wrongKeyPair, err := NewP256MLKEM768KeyPair() | ||
| require.NoError(t, err) | ||
|
|
||
| wrapped, err := P256MLKEM768WrapDEK(keyPair.publicKey, []byte("top secret dek")) | ||
| require.NoError(t, err) | ||
|
|
||
| _, err = P256MLKEM768UnwrapDEK(wrongKeyPair.privateKey, wrapped) | ||
| require.Error(t, err) | ||
| } | ||
|
|
||
| func TestP384MLKEM1024WrapUnwrapWrongKeyFails(t *testing.T) { | ||
| keyPair, err := NewP384MLKEM1024KeyPair() | ||
| require.NoError(t, err) | ||
| wrongKeyPair, err := NewP384MLKEM1024KeyPair() | ||
| require.NoError(t, err) | ||
|
|
||
| wrapped, err := P384MLKEM1024WrapDEK(keyPair.publicKey, []byte("top secret dek")) | ||
| require.NoError(t, err) | ||
|
|
||
| _, err = P384MLKEM1024UnwrapDEK(wrongKeyPair.privateKey, wrapped) | ||
| require.Error(t, err) | ||
| } |
There was a problem hiding this comment.
Tighten wrong-key negative assertions to validate the expected failure path.
These tests currently pass on any error, including unrelated parse/size failures. Assert that the error contains "AES-GCM decrypt failed" (the unwrap path in lib/ocrypto/hybrid_nist.go wraps wrong-key failures with that message), so regressions don’t get masked.
Suggested patch
func TestP256MLKEM768WrapUnwrapWrongKeyFails(t *testing.T) {
@@
_, err = P256MLKEM768UnwrapDEK(wrongKeyPair.privateKey, wrapped)
require.Error(t, err)
+ assert.Contains(t, err.Error(), "AES-GCM decrypt failed")
}
@@
func TestP384MLKEM1024WrapUnwrapWrongKeyFails(t *testing.T) {
@@
_, err = P384MLKEM1024UnwrapDEK(wrongKeyPair.privateKey, wrapped)
require.Error(t, err)
+ assert.Contains(t, err.Error(), "AES-GCM decrypt failed")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestP256MLKEM768WrapUnwrapWrongKeyFails(t *testing.T) { | |
| keyPair, err := NewP256MLKEM768KeyPair() | |
| require.NoError(t, err) | |
| wrongKeyPair, err := NewP256MLKEM768KeyPair() | |
| require.NoError(t, err) | |
| wrapped, err := P256MLKEM768WrapDEK(keyPair.publicKey, []byte("top secret dek")) | |
| require.NoError(t, err) | |
| _, err = P256MLKEM768UnwrapDEK(wrongKeyPair.privateKey, wrapped) | |
| require.Error(t, err) | |
| } | |
| func TestP384MLKEM1024WrapUnwrapWrongKeyFails(t *testing.T) { | |
| keyPair, err := NewP384MLKEM1024KeyPair() | |
| require.NoError(t, err) | |
| wrongKeyPair, err := NewP384MLKEM1024KeyPair() | |
| require.NoError(t, err) | |
| wrapped, err := P384MLKEM1024WrapDEK(keyPair.publicKey, []byte("top secret dek")) | |
| require.NoError(t, err) | |
| _, err = P384MLKEM1024UnwrapDEK(wrongKeyPair.privateKey, wrapped) | |
| require.Error(t, err) | |
| } | |
| func TestP256MLKEM768WrapUnwrapWrongKeyFails(t *testing.T) { | |
| keyPair, err := NewP256MLKEM768KeyPair() | |
| require.NoError(t, err) | |
| wrongKeyPair, err := NewP256MLKEM768KeyPair() | |
| require.NoError(t, err) | |
| wrapped, err := P256MLKEM768WrapDEK(keyPair.publicKey, []byte("top secret dek")) | |
| require.NoError(t, err) | |
| _, err = P256MLKEM768UnwrapDEK(wrongKeyPair.privateKey, wrapped) | |
| require.Error(t, err) | |
| require.Contains(t, err.Error(), "AES-GCM decrypt failed") | |
| } | |
| func TestP384MLKEM1024WrapUnwrapWrongKeyFails(t *testing.T) { | |
| keyPair, err := NewP384MLKEM1024KeyPair() | |
| require.NoError(t, err) | |
| wrongKeyPair, err := NewP384MLKEM1024KeyPair() | |
| require.NoError(t, err) | |
| wrapped, err := P384MLKEM1024WrapDEK(keyPair.publicKey, []byte("top secret dek")) | |
| require.NoError(t, err) | |
| _, err = P384MLKEM1024UnwrapDEK(wrongKeyPair.privateKey, wrapped) | |
| require.Error(t, err) | |
| require.Contains(t, err.Error(), "AES-GCM decrypt failed") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ocrypto/hybrid_nist_test.go` around lines 87 - 111, The tests
TestP256MLKEM768WrapUnwrapWrongKeyFails and
TestP384MLKEM1024WrapUnwrapWrongKeyFails currently accept any error; update each
to assert the specific unwrap failure by replacing the generic require.Error
check with a check that the returned error from P256MLKEM768UnwrapDEK /
P384MLKEM1024UnwrapDEK contains the string "AES-GCM decrypt failed" (e.g., use
require.ErrorContains(t, err, "AES-GCM decrypt failed") or an equivalent
assertion) so the wrong-key path is validated explicitly.
Comment on lines
+197
to
+227
| func TestP256MLKEM768Encapsulate(t *testing.T) { | ||
| keyPair, err := NewP256MLKEM768KeyPair() | ||
| require.NoError(t, err) | ||
|
|
||
| pubKey, err := keyPair.PublicKeyInPemFormat() | ||
| require.NoError(t, err) | ||
|
|
||
| pubKeyRaw, err := P256MLKEM768PubKeyFromPem([]byte(pubKey)) | ||
| require.NoError(t, err) | ||
|
|
||
| combinedSecret, hybridCt, err := P256MLKEM768Encapsulate(pubKeyRaw) | ||
| require.NoError(t, err) | ||
| assert.NotEmpty(t, combinedSecret) | ||
| assert.NotEmpty(t, hybridCt) | ||
| } | ||
|
|
||
| func TestP384MLKEM1024Encapsulate(t *testing.T) { | ||
| keyPair, err := NewP384MLKEM1024KeyPair() | ||
| require.NoError(t, err) | ||
|
|
||
| pubKey, err := keyPair.PublicKeyInPemFormat() | ||
| require.NoError(t, err) | ||
|
|
||
| pubKeyRaw, err := P384MLKEM1024PubKeyFromPem([]byte(pubKey)) | ||
| require.NoError(t, err) | ||
|
|
||
| combinedSecret, hybridCt, err := P384MLKEM1024Encapsulate(pubKeyRaw) | ||
| require.NoError(t, err) | ||
| assert.NotEmpty(t, combinedSecret) | ||
| assert.NotEmpty(t, hybridCt) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Validate encapsulation ciphertext sizes, not just non-empty values.
assert.NotEmpty can miss size regressions. Use explicit ciphertext size assertions (P256MLKEM768CiphertextSize / P384MLKEM1024CiphertextSize) to enforce protocol invariants.
Suggested patch
func TestP256MLKEM768Encapsulate(t *testing.T) {
@@
combinedSecret, hybridCt, err := P256MLKEM768Encapsulate(pubKeyRaw)
require.NoError(t, err)
assert.NotEmpty(t, combinedSecret)
- assert.NotEmpty(t, hybridCt)
+ assert.Len(t, hybridCt, P256MLKEM768CiphertextSize)
}
@@
func TestP384MLKEM1024Encapsulate(t *testing.T) {
@@
combinedSecret, hybridCt, err := P384MLKEM1024Encapsulate(pubKeyRaw)
require.NoError(t, err)
assert.NotEmpty(t, combinedSecret)
- assert.NotEmpty(t, hybridCt)
+ assert.Len(t, hybridCt, P384MLKEM1024CiphertextSize)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ocrypto/hybrid_nist_test.go` around lines 197 - 227, The tests
TestP256MLKEM768Encapsulate and TestP384MLKEM1024Encapsulate currently only
check hybridCt is non-empty; change those assertions to validate the exact
ciphertext length using the protocol constants (P256MLKEM768CiphertextSize and
P384MLKEM1024CiphertextSize) against the hybridCt slice (and prefer require/
assert that checks size, e.g., require.Equal/ assert.Len) so the tests fail on
size regressions instead of just non-empty values.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
lib/ocrypto/hybrid_nist.go (1)
97-104: 🧹 Nitpick | 🔵 TrivialClarify CIRCL's implicit-rejection behavior next to
DecapsulateTo.
DecapsulateTointentionally does not fail on bad ciphertext, so a short note that tampering is detected by the later AES-GCM decrypt would prevent future “missing error handling” cleanups here.Also applies to: 128-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ocrypto/hybrid_nist.go` around lines 97 - 104, Add a short clarifying comment next to the call to sk.DecapsulateTo in mlkemDecapsulate (and the analogous block at lines 128–135) stating that CIRCL's DecapsulateTo intentionally does not return an error on malformed ciphertext (implicit-rejection) and that tampering is detected later by the AES-GCM decryption step; reference mlkem768.PrivateKey, DecapsulateTo and the subsequent AES-GCM decrypt to make it clear why no error is returned here and to prevent future reviewers from trying to add error handling at this callsite.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ocrypto/hybrid_nist.go`:
- Around line 307-320: Rename the scheme-specific helper defaultXWingSalt() to a
scheme-agnostic defaultHybridSalt() and update all callers (e.g.,
NewP256MLKEM768Decryptor, NewSaltedP256MLKEM768Decryptor,
NewP384MLKEM1024Decryptor, NewSaltedP384MLKEM1024Decryptor and the other
occurrences noted) to call defaultHybridSalt() instead; keep the implementation
identical (or make defaultHybridSalt() delegate to defaultXWingSalt() and then
remove the old name) so behavior is unchanged but the helper name is generic for
all hybrid NIST constructors.
In `@sdk/experimental/tdf/writer_test.go`:
- Around line 892-980: The tests testHybridXWingFlow, testHybridP256MLKEM768Flow
and testHybridP384MLKEM1024Flow currently only assert a non-empty WrappedKey;
update each to call validateManifestSchema(t, result.Manifest) and then perform
a full round-trip unwrap using the generated keyPair's private key (from
ocrypto.NewXWingKeyPair / NewP256MLKEM768KeyPair / NewP384MLKEM1024KeyPair) and
the unwrap implementation in lib/ocrypto/hybrid_nist.go to decrypt
result.Manifest.KeyAccessObjs[0].WrappedKey, asserting the unwrapped CEK equals
the expected content-encryption key used by the writer (or that decrypting the
encrypted segment recovers the original bytes); keep NewWriter, WriteSegment and
Finalize usage but add these schema and unwrap assertions to ensure writer and
hybrid_nist unwrap are compatible.
- Around line 883-890: The helper createTestAttributeWithAlgorithm currently
no-ops when the nested grants/KAS-keys are missing which hides test setup bugs;
change its signature to accept t *testing.T (e.g.,
createTestAttributeWithAlgorithm(t *testing.T, fqn, kasURL, kid string, alg
policy.Algorithm, pem string)) and replace the silent branch with a fail-fast
assertion (t.Fatalf or t.Fatalff) if value.GetGrants() is empty or
value.GetGrants()[0].GetKasKeys() is empty, then proceed to set
PublicKey.Algorithm and PublicKey.Pem; update all callers to pass the testing.T.
---
Duplicate comments:
In `@lib/ocrypto/hybrid_nist.go`:
- Around line 97-104: Add a short clarifying comment next to the call to
sk.DecapsulateTo in mlkemDecapsulate (and the analogous block at lines 128–135)
stating that CIRCL's DecapsulateTo intentionally does not return an error on
malformed ciphertext (implicit-rejection) and that tampering is detected later
by the AES-GCM decryption step; reference mlkem768.PrivateKey, DecapsulateTo and
the subsequent AES-GCM decrypt to make it clear why no error is returned here
and to prevent future reviewers from trying to add error handling at this
callsite.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ddf776b3-70e8-4011-b2bd-5a5f433f2483
📒 Files selected for processing (6)
lib/ocrypto/hybrid_nist.gosdk/basekey_test.gosdk/experimental/tdf/writer_test.goservice/pkg/db/marshalHelpers.goservice/pkg/db/marshalHelpers_test.goservice/policy/kasregistry/key_access_server_registry.proto
Comment on lines
+307
to
+320
| func NewP256MLKEM768Decryptor(privateKey []byte) (*HybridNISTDecryptor, error) { | ||
| return NewSaltedP256MLKEM768Decryptor(privateKey, defaultXWingSalt(), nil) | ||
| } | ||
|
|
||
| func NewSaltedP256MLKEM768Decryptor(privateKey, salt, info []byte) (*HybridNISTDecryptor, error) { | ||
| return newHybridNISTDecryptor(&p256mlkem768Params, privateKey, salt, info) | ||
| } | ||
|
|
||
| func NewP384MLKEM1024Decryptor(privateKey []byte) (*HybridNISTDecryptor, error) { | ||
| return NewSaltedP384MLKEM1024Decryptor(privateKey, defaultXWingSalt(), nil) | ||
| } | ||
|
|
||
| func NewSaltedP384MLKEM1024Decryptor(privateKey, salt, info []byte) (*HybridNISTDecryptor, error) { | ||
| return newHybridNISTDecryptor(&p384mlkem1024Params, privateKey, salt, info) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract a scheme-agnostic default salt helper.
These NIST paths now depend on defaultXWingSalt(). Keeping the X-Wing name here makes the default look scheme-specific and makes a future salt change easy to apply to only one hybrid family by mistake. Please rename/extract this as something like defaultHybridSalt().
Also applies to: 340-353, 522-525
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ocrypto/hybrid_nist.go` around lines 307 - 320, Rename the
scheme-specific helper defaultXWingSalt() to a scheme-agnostic
defaultHybridSalt() and update all callers (e.g., NewP256MLKEM768Decryptor,
NewSaltedP256MLKEM768Decryptor, NewP384MLKEM1024Decryptor,
NewSaltedP384MLKEM1024Decryptor and the other occurrences noted) to call
defaultHybridSalt() instead; keep the implementation identical (or make
defaultHybridSalt() delegate to defaultXWingSalt() and then remove the old name)
so behavior is unchanged but the helper name is generic for all hybrid NIST
constructors.
Comment on lines
+883
to
+890
| func createTestAttributeWithAlgorithm(fqn, kasURL, kid string, alg policy.Algorithm, pem string) *policy.Value { | ||
| value := createTestAttribute(fqn, kasURL, kid) | ||
| if len(value.GetGrants()) > 0 && len(value.GetGrants()[0].GetKasKeys()) > 0 { | ||
| value.GetGrants()[0].GetKasKeys()[0].PublicKey.Algorithm = alg | ||
| value.GetGrants()[0].GetKasKeys()[0].PublicKey.Pem = pem | ||
| } | ||
| return value | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make the fixture helper fail fast on missing grants.
Silently returning the original RSA fixture when the nested grant/KAS-key shape changes makes setup bugs harder to diagnose. In test code this helper should assert the expected shape instead of no-op'ing.
🧪 Tighten the helper
func createTestAttributeWithAlgorithm(fqn, kasURL, kid string, alg policy.Algorithm, pem string) *policy.Value {
value := createTestAttribute(fqn, kasURL, kid)
- if len(value.GetGrants()) > 0 && len(value.GetGrants()[0].GetKasKeys()) > 0 {
- value.GetGrants()[0].GetKasKeys()[0].PublicKey.Algorithm = alg
- value.GetGrants()[0].GetKasKeys()[0].PublicKey.Pem = pem
- }
+ if len(value.GetGrants()) == 0 || len(value.GetGrants()[0].GetKasKeys()) == 0 || value.GetGrants()[0].GetKasKeys()[0].PublicKey == nil {
+ panic("createTestAttributeWithAlgorithm: missing KAS key fixture")
+ }
+ value.GetGrants()[0].GetKasKeys()[0].PublicKey.Algorithm = alg
+ value.GetGrants()[0].GetKasKeys()[0].PublicKey.Pem = pem
return value
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/experimental/tdf/writer_test.go` around lines 883 - 890, The helper
createTestAttributeWithAlgorithm currently no-ops when the nested
grants/KAS-keys are missing which hides test setup bugs; change its signature to
accept t *testing.T (e.g., createTestAttributeWithAlgorithm(t *testing.T, fqn,
kasURL, kid string, alg policy.Algorithm, pem string)) and replace the silent
branch with a fail-fast assertion (t.Fatalf or t.Fatalff) if value.GetGrants()
is empty or value.GetGrants()[0].GetKasKeys() is empty, then proceed to set
PublicKey.Algorithm and PublicKey.Pem; update all callers to pass the testing.T.
Comment on lines
+892
to
+980
| func testHybridXWingFlow(t *testing.T) { | ||
| ctx := t.Context() | ||
|
|
||
| keyPair, err := ocrypto.NewXWingKeyPair() | ||
| require.NoError(t, err) | ||
| pubPEM, err := keyPair.PublicKeyInPemFormat() | ||
| require.NoError(t, err) | ||
|
|
||
| writer, err := NewWriter(ctx) | ||
| require.NoError(t, err) | ||
|
|
||
| _, err = writer.WriteSegment(ctx, 0, []byte("hybrid xwing test data")) | ||
| require.NoError(t, err) | ||
|
|
||
| attributes := []*policy.Value{ | ||
| createTestAttributeWithAlgorithm( | ||
| "https://example.com/attr/Classification/value/Secret", | ||
| testKAS1, "xwing-kid", | ||
| policy.Algorithm_ALGORITHM_HPQT_XWING, pubPEM, | ||
| ), | ||
| } | ||
| result, err := writer.Finalize(ctx, WithAttributeValues(attributes)) | ||
| require.NoError(t, err) | ||
| assert.NotEmpty(t, result.Data) | ||
|
|
||
| keyAccess := result.Manifest.KeyAccessObjs[0] | ||
| assert.Equal(t, "hybrid-wrapped", keyAccess.KeyType) | ||
| assert.NotEmpty(t, keyAccess.WrappedKey) | ||
| } | ||
|
|
||
| func testHybridP256MLKEM768Flow(t *testing.T) { | ||
| ctx := t.Context() | ||
|
|
||
| keyPair, err := ocrypto.NewP256MLKEM768KeyPair() | ||
| require.NoError(t, err) | ||
| pubPEM, err := keyPair.PublicKeyInPemFormat() | ||
| require.NoError(t, err) | ||
|
|
||
| writer, err := NewWriter(ctx) | ||
| require.NoError(t, err) | ||
|
|
||
| _, err = writer.WriteSegment(ctx, 0, []byte("hybrid p256 mlkem768 test data")) | ||
| require.NoError(t, err) | ||
|
|
||
| attributes := []*policy.Value{ | ||
| createTestAttributeWithAlgorithm( | ||
| "https://example.com/attr/Classification/value/Secret", | ||
| testKAS1, "p256mlkem768-kid", | ||
| policy.Algorithm_ALGORITHM_HPQT_SECP256R1_MLKEM768, pubPEM, | ||
| ), | ||
| } | ||
| result, err := writer.Finalize(ctx, WithAttributeValues(attributes)) | ||
| require.NoError(t, err) | ||
| assert.NotEmpty(t, result.Data) | ||
|
|
||
| keyAccess := result.Manifest.KeyAccessObjs[0] | ||
| assert.Equal(t, "hybrid-wrapped", keyAccess.KeyType) | ||
| assert.NotEmpty(t, keyAccess.WrappedKey) | ||
| } | ||
|
|
||
| func testHybridP384MLKEM1024Flow(t *testing.T) { | ||
| ctx := t.Context() | ||
|
|
||
| keyPair, err := ocrypto.NewP384MLKEM1024KeyPair() | ||
| require.NoError(t, err) | ||
| pubPEM, err := keyPair.PublicKeyInPemFormat() | ||
| require.NoError(t, err) | ||
|
|
||
| writer, err := NewWriter(ctx) | ||
| require.NoError(t, err) | ||
|
|
||
| _, err = writer.WriteSegment(ctx, 0, []byte("hybrid p384 mlkem1024 test data")) | ||
| require.NoError(t, err) | ||
|
|
||
| attributes := []*policy.Value{ | ||
| createTestAttributeWithAlgorithm( | ||
| "https://example.com/attr/Classification/value/Secret", | ||
| testKAS1, "p384mlkem1024-kid", | ||
| policy.Algorithm_ALGORITHM_HPQT_SECP384R1_MLKEM1024, pubPEM, | ||
| ), | ||
| } | ||
| result, err := writer.Finalize(ctx, WithAttributeValues(attributes)) | ||
| require.NoError(t, err) | ||
| assert.NotEmpty(t, result.Data) | ||
|
|
||
| keyAccess := result.Manifest.KeyAccessObjs[0] | ||
| assert.Equal(t, "hybrid-wrapped", keyAccess.KeyType) | ||
| assert.NotEmpty(t, keyAccess.WrappedKey) | ||
| } |
There was a problem hiding this comment.
These hybrid flows stop before the new read path.
Right now they only prove that the writer-side mapping emitted a non-empty WrappedKey. A bad hybrid-wrapped manifest shape or a wrap/unwrap incompatibility in lib/ocrypto/hybrid_nist.go would still pass here. Please add validateManifestSchema(t, result.Manifest) and a round-trip unwrap assertion with the matching generated private key.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/experimental/tdf/writer_test.go` around lines 892 - 980, The tests
testHybridXWingFlow, testHybridP256MLKEM768Flow and testHybridP384MLKEM1024Flow
currently only assert a non-empty WrappedKey; update each to call
validateManifestSchema(t, result.Manifest) and then perform a full round-trip
unwrap using the generated keyPair's private key (from ocrypto.NewXWingKeyPair /
NewP256MLKEM768KeyPair / NewP384MLKEM1024KeyPair) and the unwrap implementation
in lib/ocrypto/hybrid_nist.go to decrypt
result.Manifest.KeyAccessObjs[0].WrappedKey, asserting the unwrapped CEK equals
the expected content-encryption key used by the writer (or that decrypting the
encrypted segment recovers the original bytes); keep NewWriter, WriteSegment and
Finalize usage but add these schema and unwrap assertions to ensure writer and
hybrid_nist unwrap are compatible.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/grpc/index.html (1)
6315-6315:⚠️ Potential issue | 🟠 MajorBroaden
hybrid-wrappedwording beyond X-Wing-only phrasing.Line 6315 and Line 6700 still describe hybrid wrapping as X-Wing-specific, which is inaccurate now that NIST EC + ML-KEM variants are supported too. Please make this algorithm-agnostic (or enumerate all supported hybrid variants).
Because this HTML is generated, the source comments inservice/kas/kas.protoshould be updated and docs regenerated.📝 Suggested wording update
-Values: 'wrapped' (RSA-wrapped for ZTDF), 'ec-wrapped' (experimental ECDH-wrapped), 'hybrid-wrapped' (experimental X-Wing-wrapped) +Values: 'wrapped' (RSA-wrapped for ZTDF), 'ec-wrapped' (experimental ECDH-wrapped), 'hybrid-wrapped' (experimental hybrid PQ-wrapped, e.g., X-Wing and NIST EC+ML-KEM variants)-Optional: Empty for RSA-based or X-Wing-based ZTDF (key_type="wrapped" or key_type="hybrid-wrapped") +Optional: Empty for RSA-based or hybrid PQ-wrapped ZTDF (key_type="wrapped" or key_type="hybrid-wrapped")Also applies to: 6700-6700
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/grpc/index.html` at line 6315, The docs still describe the 'hybrid-wrapped' value as X-Wing-specific; update the source comment in service/kas/kas.proto where the enum/value 'hybrid-wrapped' (and any surrounding description for the Values list) is documented to make the wording algorithm-agnostic (e.g., "hybrid-wrapped (hybrid KEM combining classical EC and PQ/ML-KEM variants such as X-Wing, NIST EC+ML-KEM, etc.)" or enumerate supported hybrids), then regenerate the HTML/docs so lines referencing 'hybrid-wrapped' (shown in generated docs at the two occurrences) reflect the new, broadened wording.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/grpc/index.html`:
- Line 6315: The docs still describe the 'hybrid-wrapped' value as
X-Wing-specific; update the source comment in service/kas/kas.proto where the
enum/value 'hybrid-wrapped' (and any surrounding description for the Values
list) is documented to make the wording algorithm-agnostic (e.g.,
"hybrid-wrapped (hybrid KEM combining classical EC and PQ/ML-KEM variants such
as X-Wing, NIST EC+ML-KEM, etc.)" or enumerate supported hybrids), then
regenerate the HTML/docs so lines referencing 'hybrid-wrapped' (shown in
generated docs at the two occurrences) reflect the new, broadened wording.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7ff68288-35c0-4752-945c-722dde3d092b
📒 Files selected for processing (13)
docs/grpc/index.htmldocs/openapi/authorization/authorization.openapi.yamldocs/openapi/authorization/v2/authorization.openapi.yamldocs/openapi/policy/actions/actions.openapi.yamldocs/openapi/policy/attributes/attributes.openapi.yamldocs/openapi/policy/kasregistry/key_access_server_registry.openapi.yamldocs/openapi/policy/namespaces/namespaces.openapi.yamldocs/openapi/policy/objects.openapi.yamldocs/openapi/policy/obligations/obligations.openapi.yamldocs/openapi/policy/registeredresources/registered_resources.openapi.yamldocs/openapi/policy/resourcemapping/resource_mapping.openapi.yamldocs/openapi/policy/subjectmapping/subject_mapping.openapi.yamldocs/openapi/policy/unsafe/unsafe.openapi.yaml
| @@ -93,13 +93,24 @@ func encrypt(cmd *cobra.Command, args []string) error { | |||
| } | |||
|
|
|||
| func keyTypeForKeyType(alg string) (ocrypto.KeyType, error) { | |||
Member
There was a problem hiding this comment.
Make a new method, ParseKeyType, in lib/ocrypto, that performs this and returns an error as below, but call it 'unrecognized' instead of 'unsupported'
| EC SchemeType = "ec-wrapped" | ||
| RSA SchemeType = "wrapped" | ||
| EC SchemeType = "ec-wrapped" | ||
| Hybrid SchemeType = "hybrid-wrapped" |
Member
There was a problem hiding this comment.
Do we need to create this? Can we just use 'wrapped'?
| toolchain go1.25.8 | ||
|
|
||
| require ( | ||
| github.com/cloudflare/circl v1.6.3 |
Member
There was a problem hiding this comment.
Can we use crypto/mlkem? Are you using this for xwing?
Alternative: Filippo's implementation uses builtin mlkem:
https://github.com/FiloSottile/mlkem768/blob/2e7bebc7d88d185c7e886b447d036c810a458a01/xwing/xwing.go
Comment on lines
+39
to
+43
| P384MLKEM1024ECPublicKeySize = 97 // uncompressed P-384 point | ||
| P384MLKEM1024ECPrivateKeySize = 48 // P-384 scalar | ||
| P384MLKEM1024MLKEMPubKeySize = 1568 // mlkem1024.PublicKeySize | ||
| P384MLKEM1024MLKEMPrivKeySize = 3168 // mlkem1024.PrivateKeySize | ||
| P384MLKEM1024MLKEMCtSize = 1568 // mlkem1024.CiphertextSize |
Member
There was a problem hiding this comment.
these names are confusing. They seem to only exist to satisfy the linter? IMO these will be much more clear inlined at the site of their use.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Changes
(hpqt:secp384r1-mlkem1024)
Checklist
Testing Instructions
Summary by CodeRabbit
New Features
Documentation
Configuration
Tests