feat(ocrypto): add pure ML-KEM-768/1024 support and unify KEM wrap paths#3562
Open
dmihalcik-virtru wants to merge 25 commits into
Open
feat(ocrypto): add pure ML-KEM-768/1024 support and unify KEM wrap paths#3562dmihalcik-virtru wants to merge 25 commits into
dmihalcik-virtru wants to merge 25 commits into
Conversation
github-actions
Bot
added
comp:db
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds ML-KEM-768 and ML-KEM-1024 as first-class post-quantum KEM algorithms. A new ChangesML-KEM Direct Key Wrap
Sequence Diagram(s)sequenceDiagram
participant SDKClient
participant sdk_tdf as sdk/tdf.go
participant ocrypto_WrapDEK as ocrypto.WrapDEK
participant mlkemKEM
participant KASRewrap as service/kas/access/rewrap.go
participant KeyDelegator
participant StandardKEMCrypto
rect rgba(100, 149, 237, 0.5)
note over SDKClient, StandardKEMCrypto: TDF Encrypt (mlkem-wrapped)
SDKClient->>sdk_tdf: CreateTDF(mlkem:768 key)
sdk_tdf->>ocrypto_WrapDEK: WrapDEK(mlkem:768, kasPubKeyPEM, dek)
ocrypto_WrapDEK->>mlkemKEM: encapsulate(publicKey)
mlkemKEM-->>ocrypto_WrapDEK: sharedSecret, ciphertext
ocrypto_WrapDEK->>ocrypto_WrapDEK: AES-GCM encrypt DEK with sharedSecret (no KDF)
ocrypto_WrapDEK-->>sdk_tdf: kemEnvelope DER
sdk_tdf-->>SDKClient: TDF with KeyAccess{type:"mlkem-wrapped", wrappedKey: base64(envelope)}
end
rect rgba(60, 179, 113, 0.5)
note over SDKClient, StandardKEMCrypto: TDF Decrypt / KAS Rewrap
SDKClient->>KASRewrap: RewrapRequest{type:"mlkem-wrapped", wrappedKey}
KASRewrap->>KeyDelegator: Decrypt(kid, wrappedKey)
KeyDelegator->>StandardKEMCrypto: Decrypt(ciphertext)
StandardKEMCrypto->>mlkemKEM: decapsulate(privateKey, ciphertext)
mlkemKEM-->>StandardKEMCrypto: sharedSecret
StandardKEMCrypto->>StandardKEMCrypto: AES-GCM decrypt EncryptedDEK
StandardKEMCrypto-->>KASRewrap: plaintext DEK
KASRewrap-->>SDKClient: rewrapped DEK
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
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 native support for pure ML-KEM-768 and ML-KEM-1024 algorithms while refactoring the existing KEM infrastructure. By unifying the wrap/unwrap logic for all KEM schemes—including X-Wing and NIST hybrid variants—the codebase becomes more maintainable and consistent. These changes ensure that key management and cryptographic operations are streamlined across the platform, simplifying future algorithm integrations. Highlights
New Features🧠 You can now enable Memory (public preview) 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 the 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 counterproductive. 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. ML-KEM brings the quantum might, Unifying paths in the night. From X-Wing to hybrid grace, Security finds a steady pace. Footnotes
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
![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 pure ML-KEM (ML-KEM-768 and ML-KEM-1024) algorithms and unifies KEM-based key wrapping paths (ML-KEM, X-Wing, and NIST hybrids) under a single kem interface and kemEnvelope structure. Feedback on the changes highlights critical Go interface issues where returning concrete pointer types (*kemEncryptor, *kemDecryptor) or a concrete zero-value struct (AsymDecryption{}) instead of nil for interface return types can cause standard nil checks to fail due to typed nil pointer values.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+430
to
+440
| func newKEMEncryptor(k kem, publicKey, salt, info []byte) (*kemEncryptor, error) { | ||
| if len(publicKey) != k.pubSize() { | ||
| return nil, fmt.Errorf("invalid %s public key size: got %d want %d", k.keyType(), len(publicKey), k.pubSize()) | ||
| } | ||
| return &kemEncryptor{ | ||
| k: k, | ||
| publicKey: append([]byte(nil), publicKey...), | ||
| salt: cloneOrNil(salt), | ||
| info: cloneOrNil(info), | ||
| }, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Returning a concrete pointer type *kemEncryptor from newKEMEncryptor causes a subtle but critical Go gotcha when called by functions returning the PublicKeyEncryptor interface (like FromPublicPEMWithSalt). If newKEMEncryptor returns (nil, err), the caller directly returning it will produce a non-nil interface containing a typed nil pointer (PublicKeyEncryptor(*kemEncryptor(nil))). Any subsequent encryptor != nil check by SDK/service callers will evaluate to true, leading to unexpected behavior or panics. Changing the return type of newKEMEncryptor to the interface type PublicKeyEncryptor resolves this cleanly.
Suggested change
| func newKEMEncryptor(k kem, publicKey, salt, info []byte) (*kemEncryptor, error) { | |
| if len(publicKey) != k.pubSize() { | |
| return nil, fmt.Errorf("invalid %s public key size: got %d want %d", k.keyType(), len(publicKey), k.pubSize()) | |
| } | |
| return &kemEncryptor{ | |
| k: k, | |
| publicKey: append([]byte(nil), publicKey...), | |
| salt: cloneOrNil(salt), | |
| info: cloneOrNil(info), | |
| }, nil | |
| } | |
| func newKEMEncryptor(k kem, publicKey, salt, info []byte) (PublicKeyEncryptor, error) { | |
| if len(publicKey) != k.pubSize() { | |
| return nil, fmt.Errorf("invalid %s public key size: got %d want %d", k.keyType(), len(publicKey), k.pubSize()) | |
| } | |
| return &kemEncryptor{ | |
| k: k, | |
| publicKey: append([]byte(nil), publicKey...), | |
| salt: cloneOrNil(salt), | |
| info: cloneOrNil(info), | |
| }, nil | |
| } |
Comment on lines
+468
to
+478
| func newKEMDecryptor(k kem, privateKey, salt, info []byte) (*kemDecryptor, error) { | ||
| if len(privateKey) != k.privSize() { | ||
| return nil, fmt.Errorf("invalid %s private key size: got %d want %d", k.keyType(), len(privateKey), k.privSize()) | ||
| } | ||
| return &kemDecryptor{ | ||
| k: k, | ||
| privateKey: append([]byte(nil), privateKey...), | ||
| salt: cloneOrNil(salt), | ||
| info: cloneOrNil(info), | ||
| }, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Similar to newKEMEncryptor, returning a concrete pointer type *kemDecryptor from newKEMDecryptor causes a typed nil pointer interface issue when returned by functions expecting PrivateKeyDecryptor (like FromPrivatePEMWithSalt). Changing the return type of newKEMDecryptor to the interface type PrivateKeyDecryptor ensures that returning nil, err on error produces a true nil interface value.
Suggested change
| func newKEMDecryptor(k kem, privateKey, salt, info []byte) (*kemDecryptor, error) { | |
| if len(privateKey) != k.privSize() { | |
| return nil, fmt.Errorf("invalid %s private key size: got %d want %d", k.keyType(), len(privateKey), k.privSize()) | |
| } | |
| return &kemDecryptor{ | |
| k: k, | |
| privateKey: append([]byte(nil), privateKey...), | |
| salt: cloneOrNil(salt), | |
| info: cloneOrNil(info), | |
| }, nil | |
| } | |
| func newKEMDecryptor(k kem, privateKey, salt, info []byte) (PrivateKeyDecryptor, error) { | |
| if len(privateKey) != k.privSize() { | |
| return nil, fmt.Errorf("invalid %s private key size: got %d want %d", k.keyType(), len(privateKey), k.privSize()) | |
| } | |
| return &kemDecryptor{ | |
| k: k, | |
| privateKey: append([]byte(nil), privateKey...), | |
| salt: cloneOrNil(salt), | |
| info: cloneOrNil(info), | |
| }, nil | |
| } |
Comment on lines
+60
to
+61
| case !errors.Is(err, errNotKEM): | ||
| return AsymDecryption{}, err |
Contributor
There was a problem hiding this comment.
The function FromPrivatePEMWithSalt returns (PrivateKeyDecryptor, error), where PrivateKeyDecryptor is an interface. Returning a concrete zero-value struct AsymDecryption{} on error can lead to issues where the returned interface is not nil (it contains a non-nil type with a nil/zero value), which breaks standard decrypter != nil checks. It is highly recommended to return nil, err instead.
Suggested change
| case !errors.Is(err, errNotKEM): | |
| return AsymDecryption{}, err | |
| case !errors.Is(err, errNotKEM): | |
| return nil, err |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
service/kas/access/rewrap.go (1)
403-408:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove raw SRT/body logging from the debug path.
requestBody.String()andrbStringcan contain the caller’s auth token, wrapped key material, policy body, and client public key. Emitting them verbatim to logs is a credentials/PII leak; log identifiers or lengths only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/kas/access/rewrap.go` around lines 403 - 408, The debug call in p.Logger.DebugContext currently logs sensitive raw data via requestBody.String() and rbString; change it to avoid printing credentials/PII by logging only non-sensitive metadata (e.g., lengths, hashes, or request IDs). Update the DebugContext invocation in rewrap.go (the call to p.Logger.DebugContext that currently uses slog.String("rewrap_body", requestBody.String()) and slog.String("rewrap_srt", rbString)) to emit something like slog.Int("rewrap_body_len", requestBody.Len()) and slog.String("rewrap_srt_hash", shortHexHash(rbString)) or a request identifier instead of the full strings, ensuring no raw token, key material, policy, or client public key is logged.service/internal/security/in_process_provider.go (1)
82-90: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDispatch PKCS8 exports by declared algorithm.
The RSA → KEM → EC probe chain swallows the real provider error for the expected key type. If a KEM export fails because the stored key is malformed or unavailable, this falls through to
ECPublicKeyand returns a misleading error. Sincek.algorithmis already known, choose the exporter directly.♻️ Proposed refactor
case trust.KeyTypePKCS8: - // Try to get the key as an RSA key first - if rsaKey, err := k.cryptoProvider.RSAPublicKey(kid); err == nil { - return rsaKey, nil - } - if kemKey, err := k.cryptoProvider.KEMPublicKey(kid); err == nil { - return kemKey, nil - } - return k.cryptoProvider.ECPublicKey(kid) + switch { + case k.algorithm == AlgorithmRSA2048 || k.algorithm == AlgorithmRSA4096: + return k.cryptoProvider.RSAPublicKey(kid) + case ocrypto.IsKEMKeyType(k.algorithm): + return k.cryptoProvider.KEMPublicKey(kid) + default: + return k.cryptoProvider.ECPublicKey(kid) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/internal/security/in_process_provider.go` around lines 82 - 90, The PKCS8 handling currently probes RSAPublicKey → KEMPublicKey → ECPublicKey and can mask the true error for the expected key type; change the logic to dispatch directly based on k.algorithm (the declared algorithm on the key) and call only the corresponding exporter (e.g., call k.cryptoProvider.RSAPublicKey(kid) if k.algorithm indicates RSA, k.cryptoProvider.KEMPublicKey(kid) if KEM, or k.cryptoProvider.ECPublicKey(kid) if EC), returning that call's result so the real provider error is preserved instead of falling through to another exporter.docs/openapi/entityresolution/v2/entity_resolution.openapi.yaml (2)
94-180:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFix schema composition:
additionalProperties: falserejectsoneOfbranch-only fieldsIn
docs/openapi/entityresolution/v2/entity_resolution.openapi.yaml, bothauthorization.v2.Resourceandentity.Entitydeclare variant properties only insideoneOfbranches while settingadditionalProperties: falseon the parent. In OpenAPI 3.1 / JSON Schema 2020-12,additionalPropertiesdoesn’t account for properties defined only withinoneOfsubschemas, so those fields can be treated as “additional” and rejected. Move the union keys to the parent schema or switch tounevaluatedProperties: false.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/openapi/entityresolution/v2/entity_resolution.openapi.yaml` around lines 94 - 180, The parent schemas authorization.v2.Resource and entity.Entity declare their discriminant/variant fields only inside oneOf branches while setting additionalProperties: false, which causes JSON Schema 2020-12/OpenAPI 3.1 to treat those branch-only properties as "additional" and reject valid inputs; fix by moving the union keys (e.g., registeredResourceValueFqn, attributeValues under authorization.v2.Resource and claims, clientId, emailAddress, userName under entity.Entity) into the parent properties block (keeping the oneOf for required+titles) or alternatively replace additionalProperties: false with unevaluatedProperties: false on those parent schemas so branch properties are allowed and still restricted. Ensure authorization.v2.Resource.AttributeValues stays as a referenced schema and preserve ephemeralId and category as currently declared.
317-329:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFix
google.protobuf.Anyschema to match Protobuf JSON mapping (don’t overload it with Connect error detail{type,value,debug})
docs/openapi/entityresolution/v2/entity_resolution.openapi.yaml(317-329) definesgoogle.protobuf.Anyas an object withtype,value, and optionaldebug, but protobufgoogle.protobuf.AnyJSON encoding uses@type(and the embedded message JSON;valueis only for well-known types), not{type,value,debug}.- Connect error responses use a
detailsarray of objects shaped like{ type, value, debug }; reusing that shape asgoogle.protobuf.Anywill break any non-error fields typed asgoogle.protobuf.Any.- Keep a separate schema for Connect error detail entries (
details[]) and restoregoogle.protobuf.Anyto the Protobuf JSON (@type+ embedded JSON) representation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/openapi/entityresolution/v2/entity_resolution.openapi.yaml` around lines 317 - 329, Replace the overloaded schema named google.protobuf.Any so it matches Protobuf JSON mapping: define it as an object that uses an "`@type`" string field plus an arbitrary embedded JSON payload (allow additionalProperties) instead of the {type, value, debug} shape; remove the type/value/debug properties from google.protobuf.Any. Create a separate schema (e.g., ConnectErrorDetail or ErrorDetail) for the Connect error detail entries and use that schema for the details[] array (with properties type, value, debug) so error details stay isolated from true google.protobuf.Any usage; update any references for details[] to point to the new ConnectErrorDetail schema.docs/openapi/policy/attributes/attributes.openapi.yaml (2)
1940-2031:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFix
Get*Requestschemas rejecting the new union fields
docs/openapi/policy/attributes/attributes.openapi.yamlsetsadditionalProperties: falseon the parent for bothpolicy.attributes.GetAttributeRequestandpolicy.attributes.GetAttributeValueRequest, whileattributeId/valueId/fqnare defined only withinoneOfbranches—so validators still treat those keys as additional and effectively only the deprecatedidpath remains valid.- Fix in the proto source / OpenAPI generation so the “oneOf-only” properties are accounted for in the parent schema (e.g., move
additionalProperties: falseinto eachoneOfbranch and/or use composition-aware closure likeunevaluatedProperties), then regenerate the OpenAPI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/openapi/policy/attributes/attributes.openapi.yaml` around lines 1940 - 2031, The OpenAPI schemas policy.attributes.GetAttributeRequest and policy.attributes.GetAttributeValueRequest wrongly set additionalProperties: false at the parent while attributeId/valueId/fqn are only declared inside oneOf branches, causing validators to treat those keys as unexpected; fix by updating the proto/OpenAPI generation so the union-member properties are visible at the parent level—either move additionalProperties: false into each oneOf branch (inside each member schema) or replace the parent additionalProperties:false with unevaluatedProperties:false (or otherwise merge the oneOf properties into the parent properties block) for both GetAttributeRequest and GetAttributeValueRequest, then regenerate the OpenAPI artifacts so validators accept attributeId/valueId/fqn (reference schemas: policy.attributes.GetAttributeRequest and policy.attributes.GetAttributeValueRequest).
1154-1162:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix ineffective
algexclusion in OpenAPI (not: { enum: [0] })
not: { enum: [0] }only blocks the numeric literal0. For a protobuf JSON string enum (where enum values are represented as strings), that constraint is effectively redundant and won’t filterKAS_PUBLIC_KEY_ALG_ENUM_UNSPECIFIED. Update the proto source so regeneration emits anotthat excludes the actual UNSPECIFIED string value (not0).
Location:docs/openapi/policy/attributes/attributes.openapi.yaml:1154-1162.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/openapi/policy/attributes/attributes.openapi.yaml` around lines 1154 - 1162, The OpenAPI exclusion is blocking numeric 0 but not the protobuf JSON string for the UNSPECIFIED enum; fix the proto so the generator emits a NOT that excludes the actual UNSPECIFIED string value (not 0). Concretely, update the enum definition used by policy.KasPublicKeyAlgEnum so the UNSPECIFIED member is represented with the JSON/string name the OpenAPI generator emits (or add the proto/generator option that makes the generator output the string literal), then regenerate docs so the alg property’s not.enum contains the UNSPECIFIED string (matching policy.KasPublicKeyAlgEnum) instead of 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/openapi/authorization/authorization.openapi.yaml`:
- Around line 981-1010: The OpenAPI schema for the Connect error envelope is
wrong: the schema defines connect.error.detail as a single google.protobuf.Any
object but Connect uses details as an array; update the generator so the
connect.error schema uses a "details" property of type array with items
referencing '`#/components/schemas/google.protobuf.Any`' (replace the singular
"detail" property with "details[]"), adjust any mapping logic that converts
google.rpc.Status.details to an array (the code path that emits the
connect.error schema), then regenerate
docs/openapi/authorization/authorization.openapi.yaml so the Connect HTTP JSON
error representation matches the actual repeated details field.
In `@docs/openapi/kas/kas.openapi.yaml`:
- Around line 291-343: The kas.KeyAccessRewrapResult schema places error and
kasWrappedKey only inside oneOf branches while the parent has
additionalProperties: false and omits top-level required fields, causing valid
branch instances to be rejected; update the parent schema by either (a) moving
error and kasWrappedKey into the parent properties with appropriate types and
keep oneOf for semantics, or (b) replace additionalProperties: false with
unevaluatedProperties: false so branch-evaluated properties are allowed, and add
status and keyAccessObjectId to the parent required array so they are always
required; modify the kas.KeyAccessRewrapResult definition (oneOf, properties,
additionalProperties/unevaluatedProperties, required) accordingly.
In `@docs/openapi/policy/namespaces/namespaces.openapi.yaml`:
- Around line 875-913: The GetNamespaceRequest schema currently lists a oneOf
that only accepts fqn or namespaceId, so a payload with only the deprecated id
will fail validation; fix by adding a third oneOf branch that accepts an id-only
object (define properties.id: type string, format: uuid, title id, required: [
"id" ], keep deprecated: true) so GetNamespaceRequest will validate payloads
with only id, or alternatively remove the mention of id from the description if
you intend to disallow it; update the schema under GetNamespaceRequest
accordingly.
In `@docs/openapi/policy/subjectmapping/subject_mapping.openapi.yaml`:
- Around line 693-729: policy.Action currently sets additionalProperties: false
while defining custom and standard only inside the oneOf branches
(custom/standard), which causes valid variants to be rejected; update the
proto->OpenAPI generation so policy.Action uses unevaluatedProperties: false
(JSON Schema 2020-12 / OpenAPI 3.1) instead of additionalProperties: false, or
alternatively move the custom and standard property declarations to the
top-level properties of policy.Action while keeping the oneOf validation on
required fields; ensure the generator emits unevaluatedProperties at the
policy.Action schema level so the oneOf branches for custom and standard are
allowed.
In `@docs/openapi/wellknownconfiguration/wellknown_configuration.openapi.yaml`:
- Around line 152-197: The connect.error schema currently defines a singular
"detail" property; change it to "details" and make it an array of
google.protobuf.Any (e.g., type: array, items: $ref:
'`#/components/schemas/google.protobuf.Any`') so the ConnectRPC HTTP error JSON
representation matches the wire contract; update the schema under connect.error
(and any references) to use "details" (array) instead of "detail" and ensure
title/description remain accurate for connect.error and google.protobuf.Any.
In `@lib/ocrypto/benchmark_test.go`:
- Around line 600-604: The benchmark is using the raw 32-byte testDEK instead of
the actual AES-GCM ciphertext the production wrap path produces; update the
ASN1-Marshal benchmark (the blocks constructing kemEnvelope with EncryptedDEK)
to call the real wrap/encapsulation path to produce the AES-GCM ciphertext and
assign that to kemEnvelope.EncryptedDEK (instead of testDEK) before calling
asn1.Marshal; locate usages by the symbols kemEnvelope, EncryptedDEK, testDEK
and asn1.Marshal and do the same fix for the other two Marshal cases referenced
(lines ~631-635 and ~662-666) so the benchmark measures the real wire-format
size and cost.
In `@sdk/tdf.go`:
- Around line 678-686: If kasInfo.Algorithm is empty, detect the actual key type
from the provided PEM (kasInfo.PublicKey) before deciding KEM vs legacy; call
ocrypto.KeyType(...) on a derived algorithm or use ocrypto.IsKEMKeyType by
inferring the KEM type from the PEM, then proceed into the existing KEM branch
(generateWrapKeyWithKEM) so keyAccess.KeyType and keyAccess.WrappedKey are set
to the proper mlkem/hybrid scheme; alternatively, if you prefer strictness,
return an explicit error when kasInfo.Algorithm is empty instead of falling back
to the legacy path — update the logic around ocrypto.KeyType(kasInfo.Algorithm)
and the switch that uses ocrypto.IsKEMKeyType to incorporate the PEM-derived
type or the empty-algorithm rejection.
---
Outside diff comments:
In `@docs/openapi/entityresolution/v2/entity_resolution.openapi.yaml`:
- Around line 94-180: The parent schemas authorization.v2.Resource and
entity.Entity declare their discriminant/variant fields only inside oneOf
branches while setting additionalProperties: false, which causes JSON Schema
2020-12/OpenAPI 3.1 to treat those branch-only properties as "additional" and
reject valid inputs; fix by moving the union keys (e.g.,
registeredResourceValueFqn, attributeValues under authorization.v2.Resource and
claims, clientId, emailAddress, userName under entity.Entity) into the parent
properties block (keeping the oneOf for required+titles) or alternatively
replace additionalProperties: false with unevaluatedProperties: false on those
parent schemas so branch properties are allowed and still restricted. Ensure
authorization.v2.Resource.AttributeValues stays as a referenced schema and
preserve ephemeralId and category as currently declared.
- Around line 317-329: Replace the overloaded schema named google.protobuf.Any
so it matches Protobuf JSON mapping: define it as an object that uses an "`@type`"
string field plus an arbitrary embedded JSON payload (allow
additionalProperties) instead of the {type, value, debug} shape; remove the
type/value/debug properties from google.protobuf.Any. Create a separate schema
(e.g., ConnectErrorDetail or ErrorDetail) for the Connect error detail entries
and use that schema for the details[] array (with properties type, value, debug)
so error details stay isolated from true google.protobuf.Any usage; update any
references for details[] to point to the new ConnectErrorDetail schema.
In `@docs/openapi/policy/attributes/attributes.openapi.yaml`:
- Around line 1940-2031: The OpenAPI schemas
policy.attributes.GetAttributeRequest and
policy.attributes.GetAttributeValueRequest wrongly set additionalProperties:
false at the parent while attributeId/valueId/fqn are only declared inside oneOf
branches, causing validators to treat those keys as unexpected; fix by updating
the proto/OpenAPI generation so the union-member properties are visible at the
parent level—either move additionalProperties: false into each oneOf branch
(inside each member schema) or replace the parent additionalProperties:false
with unevaluatedProperties:false (or otherwise merge the oneOf properties into
the parent properties block) for both GetAttributeRequest and
GetAttributeValueRequest, then regenerate the OpenAPI artifacts so validators
accept attributeId/valueId/fqn (reference schemas:
policy.attributes.GetAttributeRequest and
policy.attributes.GetAttributeValueRequest).
- Around line 1154-1162: The OpenAPI exclusion is blocking numeric 0 but not the
protobuf JSON string for the UNSPECIFIED enum; fix the proto so the generator
emits a NOT that excludes the actual UNSPECIFIED string value (not 0).
Concretely, update the enum definition used by policy.KasPublicKeyAlgEnum so the
UNSPECIFIED member is represented with the JSON/string name the OpenAPI
generator emits (or add the proto/generator option that makes the generator
output the string literal), then regenerate docs so the alg property’s not.enum
contains the UNSPECIFIED string (matching policy.KasPublicKeyAlgEnum) instead of
0.
In `@service/internal/security/in_process_provider.go`:
- Around line 82-90: The PKCS8 handling currently probes RSAPublicKey →
KEMPublicKey → ECPublicKey and can mask the true error for the expected key
type; change the logic to dispatch directly based on k.algorithm (the declared
algorithm on the key) and call only the corresponding exporter (e.g., call
k.cryptoProvider.RSAPublicKey(kid) if k.algorithm indicates RSA,
k.cryptoProvider.KEMPublicKey(kid) if KEM, or k.cryptoProvider.ECPublicKey(kid)
if EC), returning that call's result so the real provider error is preserved
instead of falling through to another exporter.
In `@service/kas/access/rewrap.go`:
- Around line 403-408: The debug call in p.Logger.DebugContext currently logs
sensitive raw data via requestBody.String() and rbString; change it to avoid
printing credentials/PII by logging only non-sensitive metadata (e.g., lengths,
hashes, or request IDs). Update the DebugContext invocation in rewrap.go (the
call to p.Logger.DebugContext that currently uses slog.String("rewrap_body",
requestBody.String()) and slog.String("rewrap_srt", rbString)) to emit something
like slog.Int("rewrap_body_len", requestBody.Len()) and
slog.String("rewrap_srt_hash", shortHexHash(rbString)) or a request identifier
instead of the full strings, ensuring no raw token, key material, policy, or
client public key is logged.
🪄 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: 7e06f245-10ee-4ccc-aa31-fc6244c218c2
⛔ Files ignored due to path filters (2)
protocol/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 (60)
docs/grpc/index.htmldocs/openapi/authorization/authorization.openapi.yamldocs/openapi/authorization/v2/authorization.openapi.yamldocs/openapi/common/common.openapi.yamldocs/openapi/entity/entity.openapi.yamldocs/openapi/entityresolution/entity_resolution.openapi.yamldocs/openapi/entityresolution/v2/entity_resolution.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/keymanagement/key_management.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/selectors.openapi.yamldocs/openapi/policy/subjectmapping/subject_mapping.openapi.yamldocs/openapi/policy/unsafe/unsafe.openapi.yamldocs/openapi/wellknownconfiguration/wellknown_configuration.openapi.yamllib/ocrypto/asym_decryption.golib/ocrypto/asym_encryption.golib/ocrypto/benchmark_test.golib/ocrypto/ec_key_pair.golib/ocrypto/hybrid_common.golib/ocrypto/hybrid_nist.golib/ocrypto/hybrid_nist_test.golib/ocrypto/kem.golib/ocrypto/mlkem.golib/ocrypto/mlkem_format_test.golib/ocrypto/mlkem_test.golib/ocrypto/rsa_key_pair.golib/ocrypto/xwing.golib/ocrypto/xwing_test.gootdfctl/cmd/policy/kasKeys.gootdfctl/docs/man/policy/kas-registry/key/create.mdotdfctl/docs/man/policy/kas-registry/key/import.mdotdfctl/docs/man/policy/kas-registry/key/rotate.mdotdfctl/pkg/cli/sdkHelpers.gootdfctl/pkg/utils/pemvalidate.gosdk/basekey.gosdk/experimental/tdf/key_access.gosdk/experimental/tdf/keysplit/attributes.gosdk/granter.gosdk/tdf.goservice/cmd/keygen/main.goservice/internal/security/basic_manager.goservice/internal/security/crypto_provider.goservice/internal/security/in_process_provider.goservice/internal/security/in_process_provider_test.goservice/internal/security/standard_crypto.goservice/internal/security/test_helpers_test.goservice/kas/access/publicKey.goservice/kas/access/rewrap.goservice/policy/db/grant_mappings.goservice/policy/kasregistry/key_access_server_registry.protoservice/policy/objects.prototest/tdf-roundtrips.batstests-bdd/cukes/utils/utils_genKeys.go
Comment on lines
+981
to
+1010
| connect.error: | ||
| type: object | ||
| properties: | ||
| code: | ||
| type: string | ||
| examples: | ||
| - not_found | ||
| enum: | ||
| - canceled | ||
| - unknown | ||
| - invalid_argument | ||
| - deadline_exceeded | ||
| - not_found | ||
| - already_exists | ||
| - permission_denied | ||
| - resource_exhausted | ||
| - failed_precondition | ||
| - aborted | ||
| - out_of_range | ||
| - unimplemented | ||
| - internal | ||
| - unavailable | ||
| - data_loss | ||
| - unauthenticated | ||
| description: The status code, which should be an enum value of [google.rpc.Code][google.rpc.Code]. | ||
| message: | ||
| type: string | ||
| description: A developer-facing error message, which should be in English. Any user-facing error message should be localized and sent in the [google.rpc.Status.details][google.rpc.Status.details] field, or localized by the client. | ||
| detail: | ||
| $ref: '#/components/schemas/google.protobuf.Any' |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Connect's HTTP JSON error representation, is the field named detailsas a repeated array ordetail as a single object?
💡 Result:
In the Connect protocol's HTTP JSON error representation, the field is named details and it is a repeated array [1][2][3]. The structure of a Connect error in JSON typically includes the code (string), message (string), and an optional details field (array) [1][2][3]. The details field contains a list of objects that wrap Protobuf messages, often including fields such as type, value, and debug [1]. Example representation: { "code": "invalid_argument", "message": "name is required", "details": [ { "type": "google.protobuf.Struct", "value": "base64-encoded-protobuf", "debug": {"field": "email", "issue": "Email is required"} } ] } [1]
Citations:
- 1: https://connectrpc.com/docs/python/errors
- 2: https://github.com/connectrpc/connect-go/blob/e9aff4ae/protocol_connect.go
- 3: https://docs.tcn.com/connect-error-6575697d0
Restore Connect error envelope to document details[] (not detail)
The generated OpenAPI schema models connect.error.detail as a single object, but Connect’s HTTP JSON error representation uses details as a repeated array. This is a contract mismatch that will cause generated clients/docs to deserialize real error responses incorrectly. Fix the proto/generator source that produces docs/openapi/authorization/authorization.openapi.yaml, then regenerate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/openapi/authorization/authorization.openapi.yaml` around lines 981 -
1010, The OpenAPI schema for the Connect error envelope is wrong: the schema
defines connect.error.detail as a single google.protobuf.Any object but Connect
uses details as an array; update the generator so the connect.error schema uses
a "details" property of type array with items referencing
'`#/components/schemas/google.protobuf.Any`' (replace the singular "detail"
property with "details[]"), adjust any mapping logic that converts
google.rpc.Status.details to an array (the code path that emits the
connect.error schema), then regenerate
docs/openapi/authorization/authorization.openapi.yaml so the Connect HTTP JSON
error representation matches the actual repeated details field.
Comment on lines
291
to
343
| kas.KeyAccessRewrapResult: | ||
| type: object | ||
| allOf: | ||
| oneOf: | ||
| - properties: | ||
| metadata: | ||
| type: object | ||
| title: metadata | ||
| additionalProperties: | ||
| title: value | ||
| $ref: '#/components/schemas/google.protobuf.Value' | ||
| description: |- | ||
| Metadata associated with this KAO result (e.g., required obligations) | ||
| Optional: May contain obligation requirements or other policy metadata | ||
| Common keys: "X-Required-Obligations" with array of obligation FQNs | ||
| keyAccessObjectId: | ||
| error: | ||
| type: string | ||
| title: key_access_object_id | ||
| title: error | ||
| description: |- | ||
| Identifier matching the key_access_object_id from the request | ||
| Required: Always matches the ID from UnsignedRewrapRequest_WithKeyAccessObject | ||
| status: | ||
| Error message when rewrap failed | ||
| Present when status="fail" | ||
| Human-readable description of the failure reason | ||
| title: error | ||
| required: | ||
| - error | ||
| - properties: | ||
| kasWrappedKey: | ||
| type: string | ||
| title: status | ||
| description: |- | ||
| Status of the rewrap operation for this KAO | ||
| Required: Always | ||
| Values: "permit" (success), "fail" (failure) | ||
| - oneOf: | ||
| - type: object | ||
| properties: | ||
| error: | ||
| type: string | ||
| title: error | ||
| description: |- | ||
| Error message when rewrap failed | ||
| Present when status="fail" | ||
| Human-readable description of the failure reason | ||
| title: error | ||
| required: | ||
| - error | ||
| - type: object | ||
| properties: | ||
| kasWrappedKey: | ||
| type: string | ||
| title: kas_wrapped_key | ||
| format: byte | ||
| description: |- | ||
| Successfully rewrapped key encrypted with the session key | ||
| Present when status="permit" | ||
| Contains the DEK encrypted with the ephemeral session key | ||
| title: kas_wrapped_key | ||
| required: | ||
| - kasWrappedKey | ||
| format: byte | ||
| description: |- | ||
| Successfully rewrapped key encrypted with the session key | ||
| Present when status="permit" | ||
| Contains the DEK encrypted with the ephemeral session key | ||
| title: kas_wrapped_key | ||
| required: | ||
| - kasWrappedKey | ||
| properties: | ||
| metadata: | ||
| type: object | ||
| title: metadata | ||
| additionalProperties: | ||
| title: value | ||
| $ref: '#/components/schemas/google.protobuf.Value' | ||
| description: |- | ||
| Metadata associated with this KAO result (e.g., required obligations) | ||
| Optional: May contain obligation requirements or other policy metadata | ||
| Common keys: "X-Required-Obligations" with array of obligation FQNs | ||
| keyAccessObjectId: | ||
| type: string | ||
| title: key_access_object_id | ||
| description: |- | ||
| Identifier matching the key_access_object_id from the request | ||
| Required: Always matches the ID from UnsignedRewrapRequest_WithKeyAccessObject | ||
| status: | ||
| type: string | ||
| title: status | ||
| description: |- | ||
| Status of the rewrap operation for this KAO | ||
| Required: Always | ||
| Values: "permit" (success), "fail" (failure) | ||
| title: KeyAccessRewrapResult | ||
| additionalProperties: false | ||
| description: Result of a key access object rewrap operation |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In OpenAPI 3.1 / JSON Schema 2020-12, does additionalProperties: falseon an object allow properties declared only insideoneOfsubschemas, or are those keys still treated as additional unless the parent declares them or usesunevaluatedProperties?
💡 Result:
In OpenAPI 3.1 and JSON Schema 2020-12, the additionalProperties: false keyword does not "see" into oneOf (or other applicator) subschemas [1][2]. It only validates properties declared in properties or patternProperties within the same schema object where additionalProperties is defined [3][2]. If you declare additionalProperties: false at the same level as a oneOf keyword, any properties defined only inside the oneOf subschemas will be treated as additional and will cause validation to fail, even if they are logically part of the expected object [1][4][2]. To correctly restrict properties while using oneOf (or allOf/anyOf), you should use unevaluatedProperties: false instead [4][2]. Unlike additionalProperties, the unevaluatedProperties keyword is "applicator-aware" [5]. It tracks all properties that have been successfully validated by any schema in the entire evaluation path—including those inside oneOf, anyOf, allOf, and $ref—and only fails if there are properties remaining that have not been accounted for by any of these subschemas [6][5][2]. Summary of best practice: - Do not use additionalProperties: false when using composition keywords (oneOf, anyOf, allOf) if you intend for those subschemas to define allowed properties [4][2]. - Use unevaluatedProperties: false at the root or relevant level to ensure that all properties are fully accounted for, including those inherited or defined via composition [6][5].
Citations:
- 1: https://stackoverflow.com/questions/78113257/json-schema-oneof-and-additionalproperties-confusion
- 2: https://jsonic.io/guides/json-schema-additional-properties
- 3: https://github.com/json-schema-org/json-schema-spec/blob/main/specs/jsonschema-core.md
- 4: https://stackoverflow.com/questions/73481514/which-is-the-correct-allof-syntax-for-openapi-3-schema-inheritance
- 5: https://www.learnjsonschema.com/2020-12/unevaluated/unevaluatedproperties
- 6: OpenAPI/JsonSchema - use unevaluatedProperties instead of additionalProperties and support setting to false microsoft/typespec#5961
Fix kas.KeyAccessRewrapResult schema: branch fields get rejected by additionalProperties: false
In docs/openapi/kas/kas.openapi.yaml (lines 291-343), error and kasWrappedKey are defined only inside oneOf branches, but the parent object sets additionalProperties: false. JSON Schema/OpenAPI validators can treat branch-only keys as “additional” at the parent level, so both success and failure instances may fail validation. Also, status and keyAccessObjectId are described as “Required: Always” but are not included in the parent schema’s top-level required list.
Change the parent constraint to allow branch properties (e.g., use unevaluatedProperties: false or define error/kasWrappedKey in the parent properties / adjust required) and add top-level required entries for status and keyAccessObjectId.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/openapi/kas/kas.openapi.yaml` around lines 291 - 343, The
kas.KeyAccessRewrapResult schema places error and kasWrappedKey only inside
oneOf branches while the parent has additionalProperties: false and omits
top-level required fields, causing valid branch instances to be rejected; update
the parent schema by either (a) moving error and kasWrappedKey into the parent
properties with appropriate types and keep oneOf for semantics, or (b) replace
additionalProperties: false with unevaluatedProperties: false so
branch-evaluated properties are allowed, and add status and keyAccessObjectId to
the parent required array so they are always required; modify the
kas.KeyAccessRewrapResult definition (oneOf, properties,
additionalProperties/unevaluatedProperties, required) accordingly.
Comment on lines
+875
to
+913
| oneOf: | ||
| - properties: | ||
| id: | ||
| fqn: | ||
| type: string | ||
| title: id | ||
| format: uuid | ||
| description: Deprecated | ||
| deprecated: true | ||
| - oneOf: | ||
| - type: object | ||
| properties: | ||
| fqn: | ||
| type: string | ||
| title: fqn | ||
| minLength: 1 | ||
| format: uri | ||
| title: fqn | ||
| required: | ||
| - fqn | ||
| - type: object | ||
| properties: | ||
| namespaceId: | ||
| type: string | ||
| title: namespace_id | ||
| format: uuid | ||
| description: 'option (buf.validate.oneof).required = true; // TODO: enable this when we remove the deprecated field' | ||
| minLength: 1 | ||
| format: uri | ||
| title: fqn | ||
| required: | ||
| - fqn | ||
| - properties: | ||
| namespaceId: | ||
| type: string | ||
| title: namespace_id | ||
| required: | ||
| - namespaceId | ||
| format: uuid | ||
| description: 'option (buf.validate.oneof).required = true; // TODO: enable this when we remove the deprecated field' | ||
| title: namespace_id | ||
| required: | ||
| - namespaceId | ||
| properties: | ||
| id: | ||
| type: string | ||
| title: id | ||
| format: uuid | ||
| description: Deprecated | ||
| deprecated: true | ||
| title: GetNamespaceRequest | ||
| additionalProperties: false | ||
| description: | | ||
| exclusive_fields // Either use deprecated 'id' field or one of 'namespace_id' or 'fqn', but not both | ||
| required_fields // Either id or one of namespace_id or fqn must be set | ||
| description: |+ | ||
| Either use deprecated 'id' field or one of 'namespace_id' or 'fqn', but not both: | ||
| ``` | ||
| !(has(this.id) && (has(this.namespace_id) || has(this.fqn))) | ||
| ``` | ||
|
|
||
| Either id or one of namespace_id or fqn must be set: | ||
| ``` | ||
| has(this.id) || has(this.namespace_id) || has(this.fqn) | ||
| ``` | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import pkgutil
import subprocess
import sys
from pathlib import Path
missing = [m for m in ("yaml", "jsonschema") if pkgutil.find_loader(m) is None]
if missing:
subprocess.check_call([sys.executable, "-m", "pip", "install", "--quiet", "pyyaml", "jsonschema"])
import yaml
from jsonschema import Draft202012Validator
path = Path("docs/openapi/policy/namespaces/namespaces.openapi.yaml")
doc = yaml.safe_load(path.read_text())
schema = doc["components"]["schemas"]["policy.namespaces.GetNamespaceRequest"]
validator = Draft202012Validator(schema)
cases = {
"id_only": {"id": "123e4567-e89b-12d3-a456-426614174000"},
"namespaceId_only": {"namespaceId": "123e4567-e89b-12d3-a456-426614174000"},
"fqn_only": {"fqn": "https://example.com"},
}
for name, instance in cases.items():
errors = [e.message for e in validator.iter_errors(instance)]
print(f"{name}: {'valid' if not errors else errors}")
PYRepository: opentdf/platform
Length of output: 1935
Fix GetNamespaceRequest schema contract so deprecated id is actually valid
In policy.namespaces.GetNamespaceRequest (docs/openapi/policy/namespaces/namespaces.openapi.yaml:875-913), the description states callers may use deprecated id, but the top-level oneOf only includes branches that require fqn or namespaceId. A payload with only {"id": ...} matches neither branch, so schema validation will reject it. Update the schema/source to add an id-only oneOf branch (or remove the id wording from the description if it’s not supported).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/openapi/policy/namespaces/namespaces.openapi.yaml` around lines 875 -
913, The GetNamespaceRequest schema currently lists a oneOf that only accepts
fqn or namespaceId, so a payload with only the deprecated id will fail
validation; fix by adding a third oneOf branch that accepts an id-only object
(define properties.id: type string, format: uuid, title id, required: [ "id" ],
keep deprecated: true) so GetNamespaceRequest will validate payloads with only
id, or alternatively remove the mention of id from the description if you intend
to disallow it; update the schema under GetNamespaceRequest accordingly.
Comment on lines
693
to
729
| policy.Action: | ||
| type: object | ||
| allOf: | ||
| oneOf: | ||
| - properties: | ||
| id: | ||
| type: string | ||
| title: id | ||
| description: Generated uuid in database | ||
| name: | ||
| custom: | ||
| type: string | ||
| title: name | ||
| namespace: | ||
| title: namespace | ||
| description: Namespace context for this action | ||
| $ref: '#/components/schemas/policy.Namespace' | ||
| metadata: | ||
| title: metadata | ||
| $ref: '#/components/schemas/common.Metadata' | ||
| - oneOf: | ||
| - type: object | ||
| properties: | ||
| custom: | ||
| type: string | ||
| title: custom | ||
| description: Deprecated | ||
| title: custom | ||
| required: | ||
| - custom | ||
| - type: object | ||
| properties: | ||
| standard: | ||
| title: standard | ||
| description: Deprecated | ||
| $ref: '#/components/schemas/policy.Action.StandardAction' | ||
| description: Deprecated | ||
| title: custom | ||
| required: | ||
| - custom | ||
| - properties: | ||
| standard: | ||
| title: standard | ||
| required: | ||
| - standard | ||
| description: Deprecated | ||
| $ref: '#/components/schemas/policy.Action.StandardAction' | ||
| title: standard | ||
| required: | ||
| - standard | ||
| properties: | ||
| id: | ||
| type: string | ||
| title: id | ||
| description: Generated uuid in database | ||
| name: | ||
| type: string | ||
| title: name | ||
| namespace: | ||
| title: namespace | ||
| description: Namespace context for this action | ||
| $ref: '#/components/schemas/policy.Namespace' | ||
| metadata: | ||
| title: metadata | ||
| $ref: '#/components/schemas/common.Metadata' | ||
| title: Action | ||
| additionalProperties: false | ||
| description: An action an entity can take |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In OpenAPI 3.1 / JSON Schema 2020-12, does additionalProperties: falseon an object allow properties declared only insideoneOfsubschemas, or are those keys still treated as additional unless the parent declares them or usesunevaluatedProperties?
💡 Result:
In JSON Schema 2020-12 (which OpenAPI 3.1 uses), setting additionalProperties: false on an object does NOT “see” properties declared only within oneOf subschemas. Keys that are not declared in the object schema’s own sibling properties/patternProperties (within that same schema object) are treated as additional, even if a oneOf branch would validate them—unless you use unevaluatedProperties instead. Practical implication: if your parent schema has type: object and additionalProperties: false, and the only mention of some key k is inside oneOf branches’ properties, then a validator that implements additionalProperties as specified will still reject k as “additional” at the parent level. unevaluatedProperties: false is designed to fix this by allowing the closing check to account for properties evaluated/covered by subschemas (including those reached via oneOf). Evidence: - additionalProperties “only recognizes” the sibling properties and patternProperties in the same schema object, and warns that it can’t look inside composition such as allOf/oneOf; this leads to properties declared in subschemas being invisible to additionalProperties. [1][2][3] - unevaluatedProperties is explicitly intended as a generalization of additionalProperties that can “recognize properties declared in subschemas” (including composition keywords), so it avoids rejecting those keys. [1][3][4]
Citations:
- 1: https://json-schema.org/understanding-json-schema/reference/object?highlight=dependencies
- 2: https://github.com/sourcemeta/learnjsonschema.com/blob/main/content/2020-12/applicator/additionalProperties.markdown
- 3: https://github.com/json-schema-org/json-schema-spec/blob/main/specs/jsonschema-core.md
- 4: "unevaluatedProperties" to facilitate re-use and better schema organization json-schema-org/json-schema-spec#556
Fix policy.Action OpenAPI schema: additionalProperties: false conflicts with oneOf-only custom/standard properties (docs/openapi/policy/subjectmapping/subject_mapping.openapi.yaml, lines 693-729).
policy.Action is an object with additionalProperties: false, but custom and standard are declared only inside oneOf branches. Under OpenAPI 3.1 / JSON Schema 2020-12 rules, additionalProperties: false won’t allow properties that are only present in oneOf subschemas—so both action variants can be rejected. Fix the proto schema that generates this OpenAPI and regenerate (e.g., restructure the schema or use unevaluatedProperties instead of relying on oneOf-scoped properties).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/openapi/policy/subjectmapping/subject_mapping.openapi.yaml` around lines
693 - 729, policy.Action currently sets additionalProperties: false while
defining custom and standard only inside the oneOf branches (custom/standard),
which causes valid variants to be rejected; update the proto->OpenAPI generation
so policy.Action uses unevaluatedProperties: false (JSON Schema 2020-12 /
OpenAPI 3.1) instead of additionalProperties: false, or alternatively move the
custom and standard property declarations to the top-level properties of
policy.Action while keeping the oneOf validation on required fields; ensure the
generator emits unevaluatedProperties at the policy.Action schema level so the
oneOf branches for custom and standard are allowed.
Comment on lines
+152
to
+197
| connect.error: | ||
| type: object | ||
| properties: | ||
| code: | ||
| type: string | ||
| examples: | ||
| - not_found | ||
| enum: | ||
| - canceled | ||
| - unknown | ||
| - invalid_argument | ||
| - deadline_exceeded | ||
| - not_found | ||
| - already_exists | ||
| - permission_denied | ||
| - resource_exhausted | ||
| - failed_precondition | ||
| - aborted | ||
| - out_of_range | ||
| - unimplemented | ||
| - internal | ||
| - unavailable | ||
| - data_loss | ||
| - unauthenticated | ||
| description: The status code, which should be an enum value of [google.rpc.Code][google.rpc.Code]. | ||
| message: | ||
| type: string | ||
| description: A developer-facing error message, which should be in English. Any user-facing error message should be localized and sent in the [google.rpc.Status.details][google.rpc.Status.details] field, or localized by the client. | ||
| detail: | ||
| $ref: '#/components/schemas/google.protobuf.Any' | ||
| title: Connect Error | ||
| additionalProperties: true | ||
| description: 'Error type returned by Connect: https://connectrpc.com/docs/go/errors/#http-representation' | ||
| google.protobuf.Any: | ||
| type: object | ||
| properties: | ||
| type: | ||
| type: string | ||
| value: | ||
| type: string | ||
| format: binary | ||
| debug: | ||
| type: object | ||
| additionalProperties: true | ||
| additionalProperties: true | ||
| description: Contains an arbitrary serialized message along with a @type that describes the type of the serialized message. |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the JSON HTTP error representation for ConnectRPC errors? Does the payload use a detailsarray or a singledetail object?
💡 Result:
ConnectRPC (Connect protocol) represents errors over HTTP as a JSON object with top-level fields like "code" and "message", and it uses a "details" array (not a single "detail" object). For example, the official Connect error docs show a response body containing "details": [ { "type":..., "value":..., "debug":... } ]. [1] So: - Use details: [...] (array) - Not detail: {...} (single object)
Citations:
Fix connect.error schema to use details as an array, not detail as a single object.
ConnectRPC’s HTTP error JSON representation uses a details array ("details": [ { "type": ..., "value": ..., "debug": ... } ]), so the published schema changing from details[] to a single detail object is a wire-contract break for clients/docs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/openapi/wellknownconfiguration/wellknown_configuration.openapi.yaml`
around lines 152 - 197, The connect.error schema currently defines a singular
"detail" property; change it to "details" and make it an array of
google.protobuf.Any (e.g., type: array, items: $ref:
'`#/components/schemas/google.protobuf.Any`') so the ConnectRPC HTTP error JSON
representation matches the wire contract; update the schema under connect.error
(and any references) to use "details" (array) instead of "detail" and ensure
title/description remain accurate for connect.error and google.protobuf.Any.
Comment on lines
600
to
604
| b.Run("XWing/ASN1-Marshal", func(b *testing.B) { | ||
| wrapped := XWingWrappedKey{XWingCiphertext: xwingCt, EncryptedDEK: testDEK} | ||
| wrapped := kemEnvelope{KEMCiphertext: xwingCt, EncryptedDEK: testDEK} | ||
| for b.Loop() { | ||
| sinkBytes, errSink = asn1.Marshal(wrapped) | ||
| } |
There was a problem hiding this comment.
Benchmark the real wrapped payload.
These ASN1-Marshal cases populate kemEnvelope.EncryptedDEK with the raw 32-byte testDEK, not the AES-GCM ciphertext that the real wrap path serializes. That makes the marshal benchmark and envelope size materially more optimistic than the production wire format for all three schemes.
Also applies to: 631-635, 662-666
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/ocrypto/benchmark_test.go` around lines 600 - 604, The benchmark is using
the raw 32-byte testDEK instead of the actual AES-GCM ciphertext the production
wrap path produces; update the ASN1-Marshal benchmark (the blocks constructing
kemEnvelope with EncryptedDEK) to call the real wrap/encapsulation path to
produce the AES-GCM ciphertext and assign that to kemEnvelope.EncryptedDEK
(instead of testDEK) before calling asn1.Marshal; locate usages by the symbols
kemEnvelope, EncryptedDEK, testDEK and asn1.Marshal and do the same fix for the
other two Marshal cases referenced (lines ~631-635 and ~662-666) so the
benchmark measures the real wire-format size and cost.
Comment on lines
678
to
686
| ktype := ocrypto.KeyType(kasInfo.Algorithm) | ||
| switch { | ||
| case ocrypto.IsHybridKeyType(ktype): | ||
| wrappedKey, err := generateWrapKeyWithHybrid(kasInfo.Algorithm, kasInfo.PublicKey, symKey) | ||
| case ocrypto.IsKEMKeyType(ktype): | ||
| wrappedKey, scheme, err := generateWrapKeyWithKEM(ktype, kasInfo.PublicKey, symKey) | ||
| if err != nil { | ||
| return KeyAccess{}, err | ||
| } | ||
| keyAccess.KeyType = kHybridWrapped | ||
| keyAccess.KeyType = scheme | ||
| keyAccess.WrappedKey = wrappedKey |
There was a problem hiding this comment.
Infer the wrap scheme from the PEM when kasInfo.Algorithm is empty.
This branch only keys off kasInfo.Algorithm. A caller that passes a raw ML-KEM/X-Wing PEM in kasInfo.PublicKey but leaves Algorithm empty will skip the KEM path, fall back to the legacy wrapper branch, and produce wrapped instead of mlkem-wrapped/hybrid-wrapped (or fail before that). That breaks the direct-PEM KEM flow this PR is adding. Detect the key type from the PEM before this switch, or reject empty algorithms up front.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/tdf.go` around lines 678 - 686, If kasInfo.Algorithm is empty, detect the
actual key type from the provided PEM (kasInfo.PublicKey) before deciding KEM vs
legacy; call ocrypto.KeyType(...) on a derived algorithm or use
ocrypto.IsKEMKeyType by inferring the KEM type from the PEM, then proceed into
the existing KEM branch (generateWrapKeyWithKEM) so keyAccess.KeyType and
keyAccess.WrappedKey are set to the proper mlkem/hybrid scheme; alternatively,
if you prefer strictness, return an explicit error when kasInfo.Algorithm is
empty instead of falling back to the legacy path — update the logic around
ocrypto.KeyType(kasInfo.Algorithm) and the switch that uses ocrypto.IsKEMKeyType
to incorporate the PEM-derived type or the empty-algorithm rejection.
Contributor
Add pure ML-KEM (Module-Lattice-Based Key-Encapsulation Mechanism) support alongside existing hybrid post-quantum algorithms. Changes: - Add ALGORITHM_MLKEM_768 and ALGORITHM_MLKEM_1024 to proto enums - Add ML-KEM key types (mlkem:768, mlkem:1024) to ocrypto library - Implement MLKEMKeyPair and MLKEM1024KeyPair with key generation - Update proto validation to accept ML-KEM algorithms - Regenerate all protocol buffers and OpenAPI docs This is a partial implementation. Remaining work includes: - Add ML-KEM encryption/decryption to asym_encryption.go - Add ML-KEM support to SDK and service layers - Add corresponding tests Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implement complete ML-KEM 768/1024 encryption and decryption support in the ocrypto library. Encryption changes (asym_encryption.go): - Add MLKEM SchemeType constant - Add MLKEMEncryptor768 and MLKEMEncryptor1024 types - Implement newMLKEM768/newMLKEM1024 constructors - Handle mlkem.EncapsulationKey in FromPublicPEMWithSalt - Implement all PublicKeyEncryptor interface methods - Use AES-GCM with shared secret from KEM encapsulation Decryption changes (asym_decryption.go): - Add DecryptWithEphemeralKey method to interface - Add MLKEMDecryptor768 and MLKEMDecryptor1024 types - Handle "MLKEM DECAPSULATION KEY" PEM blocks - Implement Decrypt and DecryptWithEphemeralKey methods - Use AES-GCM with shared secret from KEM decapsulation Also updated existing decryptors: - Add DecryptWithEphemeralKey to AsymDecryption (RSA) - Add DecryptWithEphemeralKey to ECDecryptor (ECDH) - Add DecryptWithEphemeralKey to XWingDecryptor (hybrid) - Add DecryptWithEphemeralKey to HybridNISTDecryptor (hybrid) All lib/ocrypto tests pass. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add ML-KEM-768 and ML-KEM-1024 support to the KAS service layer: - Add AlgorithmMLKEM768 and AlgorithmMLKEM1024 constants - Add ML-KEM decryption support to BasicManager - Add StandardMLKEMCrypto type for ML-KEM key management - Add MLKEMPublicKey method for public key export - Add ML-KEM case to StandardCrypto.Decrypt method Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Complete ML-KEM integration across the service layer: - Add ML-KEM algorithms to InProcessProvider key listing and decryption - Add ML-KEM support to public key export in KAS access layer - Add ML-KEM algorithm mappings in policy grant_mappings Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Add ML-KEM-768 and ML-KEM-1024 algorithm support to SDK: - Add ML-KEM cases to KeyTypeToPolicyAlgorithm conversion - Add ML-KEM cases to PolicyAlgorithmToKeyType conversion - Add ML-KEM enum mappings in convertAlgEnum2Simple - Add ML-KEM string mappings in algProto2String Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Add ML-KEM-768 and ML-KEM-1024 support to otdfctl and experimental SDK: - Add ML-KEM key generation in kasKeys command - Add "mlkem:768" and "mlkem:1024" string mappings in CLI helpers - Add ML-KEM validation in PEM validator - Add ML-KEM enum conversions in experimental keysplit SDK Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Add ML-KEM-768 and ML-KEM-1024 key generation: - Add ML-KEM key generation to service keygen command - Add ML-KEM key generation to BDD test utilities - Generate kas-mlkem768-private.pem and kas-mlkem768-public.pem - Generate kas-mlkem1024-private.pem and kas-mlkem1024-public.pem Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Add pure post-quantum encryption tests for ML-KEM-768 and ML-KEM-1024 algorithms. Tests validate KAO type (mlkem-wrapped), KID assignment (m1, m2), and successful encrypt/decrypt roundtrips. Also updates KAS config to include ML-KEM keys. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Refactors plain ML-KEM (768/1024) to use ASN.1 DER encoding and HKDF key derivation, matching the X-Wing and Hybrid NIST patterns. Stores ML-KEM ciphertext concatenated with wrapped DEK in ASN.1 structure instead of overloading ephemeralKey metadata. - Add lib/ocrypto/mlkem.go with X-Wing-style wrap/unwrap functions - Add comprehensive test coverage in mlkem_test.go - Preserve backwards compatibility by renaming old types to Legacy variants - Update otdfctl documentation to include mlkem:768 and mlkem:1024 algorithms Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…Legacy ML-KEM types
Remove DecryptWithEphemeralKey method from PrivateKeyDecryptor interface,
making it EC-specific. Only ECDecryptor needs ephemeral keys for ECDH.
Remove Legacy ML-KEM types and implementations:
- MLKEMEncryptor768Legacy, MLKEMEncryptor1024Legacy
- MLKEMDecryptor768Legacy, MLKEMDecryptor1024Legacy
- Old "MLKEM DECAPSULATION KEY" PEM handling
- Helper functions newMLKEM768(), newMLKEM1024()
Add PEM block constants for new ML-KEM implementation:
- PEMBlockMLKEM768PublicKey, PEMBlockMLKEM768PrivateKey
- PEMBlockMLKEM1024PublicKey, PEMBlockMLKEM1024PrivateKey
Remove DecryptWithEphemeralKey from all non-EC decryptors:
- MLKEMDecryptor768, MLKEMDecryptor1024
- XWingDecryptor, HybridNISTDecryptor
- AsymDecryption (RSA)
Update service layer to use Decrypt() for ML-KEM instead of
DecryptWithEphemeralKey. EC decryption continues to use the
EC-specific DecryptWithEphemeralKey method.
Breaking changes:
- Old ML-KEM PEM format ("MLKEM DECAPSULATION KEY") no longer supported
- Callers must use concrete ECDecryptor type for DecryptWithEphemeralKey
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
ML-KEM keys loaded through InProcessProvider failed FindKeyByID and Decrypt with "could not determine key type" because the type switch in determineKeyType was missing the StandardMLKEMCrypto case. Add the case and a regression test exercising both mlkem:768 and mlkem:1024 through determineKeyType and FindKeyByID. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Cache the ML-KEM PrivateKeyDecryptor on StandardMLKEMCrypto during loadKey instead of re-parsing the PEM on every Decrypt call. Mirrors the existing RSA pattern. Also fixes a latent bug in ocrypto.MLKEMKeyPair PEM writers: they emitted "MLKEM DECAPSULATION KEY" / "MLKEM ENCAPSULATOR" block types, but commit 40d10ce removed parser support for those headers. Updated the writers to use the PEMBlockMLKEM{768,1024}{Private,Public}Key constants the parser now recognizes. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
The key_algorithm CEL validation on ListKeysRequest only accepted algorithm values 0-8, which excluded the post-quantum ML-KEM-768 (20) and ML-KEM-1024 (21) values. Filtering by these algorithms returned a validation error even though the server could store and serve them. RotateKeyRequest.NewKey already includes 20 and 21 in its allowed set. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
ExportPublicKey fell through RSA/Hybrid/XWing to ECPublicKey, so pure ML-KEM keys returned ErrCertNotFound and KAS PublicKey requests for mlkem:768/mlkem:1024 failed with not_found. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
InProcessProvider.Decrypt rejected empty ephemeralPublicKey for ML-KEM, but StandardCrypto.Decrypt and BasicManager.Decrypt both reject non-empty values. The KEM ciphertext is the encapsulation; there is no separate ephemeral key. Invert the check to match the HPQT case above and pass nil downstream. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
MLKEMEncryptor768/1024.PublicKeyInPemFormat emitted "MLKEM ENCAPSULATOR", but FromPublicPEMWithSalt dispatches on PEMBlockMLKEM768PublicKey / PEMBlockMLKEM1024PublicKey, breaking PEM round-trip. Use the canonical constants to match the X-Wing pattern and the existing MLKEMKeyPair serialization in ec_key_pair.go. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Mirrors the negative assertion already in TestInProcessProviderDetermineKeyType. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Replace the custom "MLKEM768 PUBLIC KEY" / "MLKEM1024 PRIVATE KEY" PEM labels (raw key bytes with no ASN.1 envelope) with standard "PUBLIC KEY" and "PRIVATE KEY" labels carrying RFC 5280 SubjectPublicKeyInfo and RFC 5958 OneAsymmetricKey, with the algorithm conveyed by NIST OIDs 2.16.840.1.101.3.4.4.2 (ML-KEM-768) and 2.16.840.1.101.3.4.4.3 (ML-KEM-1024). The private-key PKCS#8 inner CHOICE uses [0] IMPLICIT OCTET STRING (seed form, 64 bytes) per draft-ietf-lamps-kyber-certificates. The encoding is hand-rolled rather than via crypto/x509 because stdlib ML-KEM support in MarshalPKIXPublicKey / MarshalPKCS8PrivateKey landed in Go 1.26 and this module pins go 1.25. FromPublicPEMWithSalt / FromPrivatePEMWithSalt now peek at the OID after PEM decode and route ML-KEM blobs to the existing encryptor/decryptor constructors. Non-ML-KEM blobs fall through to the existing RSA/EC parsers unchanged. The hybrid SECP256R1-MLKEM768, SECP384R1-MLKEM1024, and X-Wing schemes keep their custom PEM labels for now; conformance to IETF composite-KEM and X-Wing drafts is tracked under DSPX-3396 and will land in a separate PR off main. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Add support for generating KAOs with type=mlkem-wrapped when using pure ML-KEM wrapping keys (MLKEM768, MLKEM1024), while maintaining backwards compatibility for reading type=wrapped KAOs. Changes: - Add IsMLKEMKeyType() helper in lib/ocrypto/ec_key_pair.go - Add kMLKEMWrapped constant and ML-KEM case to createKeyAccess() in sdk/tdf.go - Implement generateWrapKeyWithMLKEM() in sdk/tdf.go - Add ML-KEM handling to wrapKeyWithPublicKey() in sdk/experimental/tdf/key_access.go - Implement wrapKeyWithMLKEM() in sdk/experimental/tdf/key_access.go This ensures integration tests pass which expect mlkem-wrapped type for pure ML-KEM keys, while type=wrapped continues for RSA keys. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Make MLKEM768WrapDEK and MLKEM1024WrapDEK accept multiple input formats: - Raw key bytes (1184/1568 bytes) - fast path - SPKI DER (1206/1590 bytes) - PEM-wrapped SPKI (~1686/~2206 bytes) This fixes the issue introduced in 6a7480d where KAS started returning SPKI-encoded PEM keys but callers expected raw bytes. Instead of requiring all callers to decode manually, the crypto library now handles format detection transparently. Changes: - Add normalizeMLKEMPublicKey() helper for format detection - Export ParseMLKEMPublicSPKI() and OidMLKEM768/OidMLKEM1024 constants - Update all internal references to use exported names - Add comprehensive tests for format handling Benefits: - Backward compatible (raw keys still work) - Simpler callers (no manual PEM decoding needed) - Better encapsulation (format logic in crypto library) - Future-proof (handles new formats automatically) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Adds support for pure ML-KEM key agreement objects in the KAS rewrap handler. The SDK now emits type="mlkem-wrapped" for MLKEM768/MLKEM1024 KAOs, and this change adds the corresponding case to handle decryption. Follows the hybrid-wrapped pattern: uses HybridTDFEnabled flag, no ephemeral key processing, and generic error messages per security guidelines. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Collapse the three near-identical KEM wrap/unwrap implementations behind one OID-keyed registry and a single internal kem contract. ML-KEM-768, ML-KEM-1024, X-Wing, P-256+ML-KEM-768, and P-384+ML-KEM-1024 now share one envelope type, one wrap function, one unwrap function, and one encryptor/decryptor pair routed through FromPublicPEM / FromPrivatePEM. Service and SDK callers shrink accordingly: StandardXWingCrypto, StandardHybridCrypto, and StandardMLKEMCrypto fold into a single StandardKEMCrypto; the per-algorithm wrap dispatch in sdk/tdf.go and sdk/experimental/tdf/key_access.go collapses to one IsKEMKeyType branch calling ocrypto.WrapDEK. Wire formats are preserved byte-for-byte (hybrid-wrapped, mlkem-wrapped). The OID registry leaves the planned hybrid-PEM-to-SPKI follow-up as a near-zero change: three OID constants plus three registry entries. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
DSPX-3383-alternative-simplified
branch
from
8c6e15e to
455f280
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/openapi/policy/kasregistry/key_access_server_registry.openapi.yaml`:
- Around line 1331-1376: The oneOf constraint in GetKeyAccessServerRequest only
validates kasId, name, or uri, but excludes the deprecated id field. This causes
schema-driven clients to reject requests that only contain id, even though the
description claims id is supported. Add id as a fourth branch within the oneOf
constraint (alongside kasId, name, and uri), with id as a required property in
that branch. Keep the deprecated marking and description on the id property to
maintain backward compatibility while allowing the schema to correctly validate
id-only payloads.
In `@docs/openapi/policy/obligations/obligations.openapi.yaml`:
- Around line 733-735: The examples for the google.protobuf.Timestamp fields
contain invalid duration literals (1s, 1.000340012s) instead of valid RFC3339
timestamp strings. Replace the examples at
docs/openapi/policy/obligations/obligations.openapi.yaml lines 733-735,
docs/openapi/policy/registeredresources/registered_resources.openapi.yaml lines
558-560, docs/openapi/policy/resourcemapping/resource_mapping.openapi.yaml lines
538-540, and docs/openapi/policy/subjectmapping/subject_mapping.openapi.yaml
lines 600-602 with proper RFC3339 formatted timestamp strings (e.g.,
2024-01-15T10:30:45Z) to match the declared date-time format and ensure OpenAPI
validation compliance.
In `@docs/openapi/policy/unsafe/unsafe.openapi.yaml`:
- Around line 819-822: The constraint at line 821 uses `not: enum: [0]` to
exclude a numeric zero value, but the `alg` field schema is defined with string
enum values from `policy.KasPublicKeyAlgEnum`, not numeric values. Replace the
numeric `0` in the `not: enum` constraint with the actual string enum value that
represents the unspecified state (such as `KAS_PUBLIC_KEY_ALG_ENUM_UNSPECIFIED`
or its equivalent string representation) to properly block the intended enum
value.
- Around line 507-509: The examples on lines 507-508 contain "1s" and
"1.000340012s" which are duration format strings, but the schema declares
format: date-time on line 509, which requires RFC 3339 formatted timestamps.
Replace the invalid examples with valid RFC 3339 date-time strings (e.g.,
"1970-01-01T00:00:01Z" and "1970-01-01T00:00:01.000340012Z") that conform to the
declared date-time format to ensure OpenAPI compliance and prevent schema
validation failures.
- Around line 755-757: The OpenAPI schema for subject_external_values has
minItems: 1 applied at the string item level, but minItems is only valid for
arrays and will be ignored. To enforce non-empty strings, replace the minItems:
1 that appears under the items section (after the title: subject_external_values
line) with minLength: 1 instead, while keeping the minItems: 1 constraint at the
array level unchanged.
In `@otdfctl/cmd/policy/kasKeys.go`:
- Around line 356-363: The condition on line 357 uses the OR operator to check
if kasIdentifier is not empty or if id is not a UUID, which forces identifier
mode and clears id whenever --kas is provided, even if id is already a valid
UUID. Change the logical operator from OR to AND in the condition checking
kasIdentifier and the UUID classification so that identifier mode is only
entered when BOTH --kas is provided AND the key is not already a UUID, allowing
direct UUID lookup to be preserved when the key is already a UUID. Apply the
same logical fix to the equivalent code around line 602-609 that handles oldKey.
🪄 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: 27e8d4f6-ac7b-431c-b707-6f9ca44875dc
⛔ Files ignored due to path filters (2)
protocol/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 (53)
adr/decisions/2026-06-16-mlkem-direct-key-wrap.mddocs/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.yamllib/ocrypto/asym_decryption.golib/ocrypto/asym_encryption.golib/ocrypto/benchmark_test.golib/ocrypto/ec_key_pair.golib/ocrypto/hybrid_common.golib/ocrypto/hybrid_nist.golib/ocrypto/hybrid_nist_test.golib/ocrypto/kem.golib/ocrypto/mlkem.golib/ocrypto/mlkem_format_test.golib/ocrypto/mlkem_test.golib/ocrypto/rsa_key_pair.golib/ocrypto/xwing.golib/ocrypto/xwing_test.gootdfctl/cmd/policy/kasKeys.gootdfctl/docs/man/policy/kas-registry/key/create.mdotdfctl/docs/man/policy/kas-registry/key/import.mdotdfctl/docs/man/policy/kas-registry/key/rotate.mdotdfctl/pkg/cli/sdkHelpers.gootdfctl/pkg/utils/pemvalidate.gosdk/basekey.gosdk/experimental/tdf/key_access.gosdk/experimental/tdf/keysplit/attributes.gosdk/granter.gosdk/tdf.goservice/cmd/keygen/main.goservice/internal/security/basic_manager.goservice/internal/security/crypto_provider.goservice/internal/security/in_process_provider.goservice/internal/security/in_process_provider_test.goservice/internal/security/standard_crypto.goservice/internal/security/test_helpers_test.goservice/kas/access/publicKey.goservice/kas/access/rewrap.goservice/policy/db/grant_mappings.goservice/policy/kasregistry/key_access_server_registry.protoservice/policy/objects.prototest/tdf-roundtrips.batstests-bdd/cukes/utils/utils_genKeys.go
💤 Files with no reviewable changes (19)
- service/policy/db/grant_mappings.go
- sdk/experimental/tdf/key_access.go
- service/policy/objects.proto
- service/policy/kasregistry/key_access_server_registry.proto
- service/internal/security/crypto_provider.go
- sdk/experimental/tdf/keysplit/attributes.go
- service/internal/security/in_process_provider_test.go
- sdk/granter.go
- sdk/basekey.go
- service/cmd/keygen/main.go
- sdk/tdf.go
- tests-bdd/cukes/utils/utils_genKeys.go
- service/internal/security/test_helpers_test.go
- service/internal/security/basic_manager.go
- service/kas/access/publicKey.go
- test/tdf-roundtrips.bats
- service/kas/access/rewrap.go
- service/internal/security/standard_crypto.go
- service/internal/security/in_process_provider.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/openapi/policy/kasregistry/key_access_server_registry.openapi.yaml`:
- Around line 1331-1376: The oneOf constraint in GetKeyAccessServerRequest only
validates kasId, name, or uri, but excludes the deprecated id field. This causes
schema-driven clients to reject requests that only contain id, even though the
description claims id is supported. Add id as a fourth branch within the oneOf
constraint (alongside kasId, name, and uri), with id as a required property in
that branch. Keep the deprecated marking and description on the id property to
maintain backward compatibility while allowing the schema to correctly validate
id-only payloads.
In `@docs/openapi/policy/obligations/obligations.openapi.yaml`:
- Around line 733-735: The examples for the google.protobuf.Timestamp fields
contain invalid duration literals (1s, 1.000340012s) instead of valid RFC3339
timestamp strings. Replace the examples at
docs/openapi/policy/obligations/obligations.openapi.yaml lines 733-735,
docs/openapi/policy/registeredresources/registered_resources.openapi.yaml lines
558-560, docs/openapi/policy/resourcemapping/resource_mapping.openapi.yaml lines
538-540, and docs/openapi/policy/subjectmapping/subject_mapping.openapi.yaml
lines 600-602 with proper RFC3339 formatted timestamp strings (e.g.,
2024-01-15T10:30:45Z) to match the declared date-time format and ensure OpenAPI
validation compliance.
In `@docs/openapi/policy/unsafe/unsafe.openapi.yaml`:
- Around line 819-822: The constraint at line 821 uses `not: enum: [0]` to
exclude a numeric zero value, but the `alg` field schema is defined with string
enum values from `policy.KasPublicKeyAlgEnum`, not numeric values. Replace the
numeric `0` in the `not: enum` constraint with the actual string enum value that
represents the unspecified state (such as `KAS_PUBLIC_KEY_ALG_ENUM_UNSPECIFIED`
or its equivalent string representation) to properly block the intended enum
value.
- Around line 507-509: The examples on lines 507-508 contain "1s" and
"1.000340012s" which are duration format strings, but the schema declares
format: date-time on line 509, which requires RFC 3339 formatted timestamps.
Replace the invalid examples with valid RFC 3339 date-time strings (e.g.,
"1970-01-01T00:00:01Z" and "1970-01-01T00:00:01.000340012Z") that conform to the
declared date-time format to ensure OpenAPI compliance and prevent schema
validation failures.
- Around line 755-757: The OpenAPI schema for subject_external_values has
minItems: 1 applied at the string item level, but minItems is only valid for
arrays and will be ignored. To enforce non-empty strings, replace the minItems:
1 that appears under the items section (after the title: subject_external_values
line) with minLength: 1 instead, while keeping the minItems: 1 constraint at the
array level unchanged.
In `@otdfctl/cmd/policy/kasKeys.go`:
- Around line 356-363: The condition on line 357 uses the OR operator to check
if kasIdentifier is not empty or if id is not a UUID, which forces identifier
mode and clears id whenever --kas is provided, even if id is already a valid
UUID. Change the logical operator from OR to AND in the condition checking
kasIdentifier and the UUID classification so that identifier mode is only
entered when BOTH --kas is provided AND the key is not already a UUID, allowing
direct UUID lookup to be preserved when the key is already a UUID. Apply the
same logical fix to the equivalent code around line 602-609 that handles oldKey.
🪄 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: 27e8d4f6-ac7b-431c-b707-6f9ca44875dc
⛔ Files ignored due to path filters (2)
protocol/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 (53)
adr/decisions/2026-06-16-mlkem-direct-key-wrap.mddocs/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.yamllib/ocrypto/asym_decryption.golib/ocrypto/asym_encryption.golib/ocrypto/benchmark_test.golib/ocrypto/ec_key_pair.golib/ocrypto/hybrid_common.golib/ocrypto/hybrid_nist.golib/ocrypto/hybrid_nist_test.golib/ocrypto/kem.golib/ocrypto/mlkem.golib/ocrypto/mlkem_format_test.golib/ocrypto/mlkem_test.golib/ocrypto/rsa_key_pair.golib/ocrypto/xwing.golib/ocrypto/xwing_test.gootdfctl/cmd/policy/kasKeys.gootdfctl/docs/man/policy/kas-registry/key/create.mdotdfctl/docs/man/policy/kas-registry/key/import.mdotdfctl/docs/man/policy/kas-registry/key/rotate.mdotdfctl/pkg/cli/sdkHelpers.gootdfctl/pkg/utils/pemvalidate.gosdk/basekey.gosdk/experimental/tdf/key_access.gosdk/experimental/tdf/keysplit/attributes.gosdk/granter.gosdk/tdf.goservice/cmd/keygen/main.goservice/internal/security/basic_manager.goservice/internal/security/crypto_provider.goservice/internal/security/in_process_provider.goservice/internal/security/in_process_provider_test.goservice/internal/security/standard_crypto.goservice/internal/security/test_helpers_test.goservice/kas/access/publicKey.goservice/kas/access/rewrap.goservice/policy/db/grant_mappings.goservice/policy/kasregistry/key_access_server_registry.protoservice/policy/objects.prototest/tdf-roundtrips.batstests-bdd/cukes/utils/utils_genKeys.go
💤 Files with no reviewable changes (19)
- service/policy/db/grant_mappings.go
- sdk/experimental/tdf/key_access.go
- service/policy/objects.proto
- service/policy/kasregistry/key_access_server_registry.proto
- service/internal/security/crypto_provider.go
- sdk/experimental/tdf/keysplit/attributes.go
- service/internal/security/in_process_provider_test.go
- sdk/granter.go
- sdk/basekey.go
- service/cmd/keygen/main.go
- sdk/tdf.go
- tests-bdd/cukes/utils/utils_genKeys.go
- service/internal/security/test_helpers_test.go
- service/internal/security/basic_manager.go
- service/kas/access/publicKey.go
- test/tdf-roundtrips.bats
- service/kas/access/rewrap.go
- service/internal/security/standard_crypto.go
- service/internal/security/in_process_provider.go
🛑 Comments failed to post (6)
docs/openapi/policy/kasregistry/key_access_server_registry.openapi.yaml (1)
1331-1376:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
GetKeyAccessServerRequestrejects the deprecatedidpath it claims to support.Line 1331 defines
oneOfonly forkasId/name/uri, while Line 1359 defines deprecatedidoutsideoneOf; at Line 1368 the description still saysidis allowed. Anid-only payload won’t satisfy anyoneOfbranch, so schema-driven clients will reject it.Suggested fix
policy.kasregistry.GetKeyAccessServerRequest: type: object oneOf: + - properties: + id: + type: string + title: id + format: uuid + description: Deprecated + deprecated: true + title: id + required: + - id - properties: kasId: type: string title: kas_id format: uuid🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/openapi/policy/kasregistry/key_access_server_registry.openapi.yaml` around lines 1331 - 1376, The oneOf constraint in GetKeyAccessServerRequest only validates kasId, name, or uri, but excludes the deprecated id field. This causes schema-driven clients to reject requests that only contain id, even though the description claims id is supported. Add id as a fourth branch within the oneOf constraint (alongside kasId, name, and uri), with id as a required property in that branch. Keep the deprecated marking and description on the id property to maintain backward compatibility while allowing the schema to correctly validate id-only payloads.docs/openapi/policy/obligations/obligations.openapi.yaml (1)
733-735:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
google.protobuf.Timestampexamples are not validdate-timevalues.The shared schema declares
format: date-timeand documents RFC3339 strings, but these examples use duration literals (1s,1.000340012s). That can break strict OpenAPI validation and mislead client generation.
docs/openapi/policy/obligations/obligations.openapi.yaml#L733-L735: replace examples with RFC3339 timestamp strings.docs/openapi/policy/registeredresources/registered_resources.openapi.yaml#L558-L560: replace examples with RFC3339 timestamp strings.docs/openapi/policy/resourcemapping/resource_mapping.openapi.yaml#L538-L540: replace examples with RFC3339 timestamp strings.docs/openapi/policy/subjectmapping/subject_mapping.openapi.yaml#L600-L602: replace examples with RFC3339 timestamp strings.📍 Affects 4 files
docs/openapi/policy/obligations/obligations.openapi.yaml#L733-L735(this comment)docs/openapi/policy/registeredresources/registered_resources.openapi.yaml#L558-L560docs/openapi/policy/resourcemapping/resource_mapping.openapi.yaml#L538-L540docs/openapi/policy/subjectmapping/subject_mapping.openapi.yaml#L600-L602🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/openapi/policy/obligations/obligations.openapi.yaml` around lines 733 - 735, The examples for the google.protobuf.Timestamp fields contain invalid duration literals (1s, 1.000340012s) instead of valid RFC3339 timestamp strings. Replace the examples at docs/openapi/policy/obligations/obligations.openapi.yaml lines 733-735, docs/openapi/policy/registeredresources/registered_resources.openapi.yaml lines 558-560, docs/openapi/policy/resourcemapping/resource_mapping.openapi.yaml lines 538-540, and docs/openapi/policy/subjectmapping/subject_mapping.openapi.yaml lines 600-602 with proper RFC3339 formatted timestamp strings (e.g., 2024-01-15T10:30:45Z) to match the declared date-time format and ensure OpenAPI validation compliance.docs/openapi/policy/unsafe/unsafe.openapi.yaml (3)
507-509:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
google.protobuf.Timestampexamples violate declareddate-timeformat.Line 507 and Line 508 use
"1s"/"1.000340012s"while this schema isformat: date-time(Line 509). These examples are invalid for OpenAPI date-time and can break schema-driven docs/tests.Suggested fix
examples: - - 1s - - 1.000340012s + - "1970-01-01T00:00:01Z" + - "1970-01-01T00:00:01.000340012Z" format: date-time📝 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.- "1970-01-01T00:00:01Z" - "1970-01-01T00:00:01.000340012Z" format: date-time🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/openapi/policy/unsafe/unsafe.openapi.yaml` around lines 507 - 509, The examples on lines 507-508 contain "1s" and "1.000340012s" which are duration format strings, but the schema declares format: date-time on line 509, which requires RFC 3339 formatted timestamps. Replace the invalid examples with valid RFC 3339 date-time strings (e.g., "1970-01-01T00:00:01Z" and "1970-01-01T00:00:01.000340012Z") that conform to the declared date-time format to ensure OpenAPI compliance and prevent schema validation failures.
755-757:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
minItemsis placed on a string item schema instead of usingminLength.Line 755 applies
minItemsunderitems.type: string; this keyword is for arrays and is ignored here. If intent is non-empty strings, enforceminLength: 1on the item schema.Suggested fix
subjectExternalValues: type: array items: type: string - minItems: 1 + minLength: 1 title: subject_external_values minItems: 1📝 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.minLength: 1 title: subject_external_values minItems: 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/openapi/policy/unsafe/unsafe.openapi.yaml` around lines 755 - 757, The OpenAPI schema for subject_external_values has minItems: 1 applied at the string item level, but minItems is only valid for arrays and will be ignored. To enforce non-empty strings, replace the minItems: 1 that appears under the items section (after the title: subject_external_values line) with minLength: 1 instead, while keeping the minItems: 1 constraint at the array level unchanged.
819-822:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
not: enum: [0]is ineffective against this string enum schema.Line 821 excludes numeric
0, butpolicy.KasPublicKeyAlgEnumis represented as enum strings. This does not blockKAS_PUBLIC_KEY_ALG_ENUM_UNSPECIFIEDas intended.Suggested fix
alg: not: enum: - - 0 + - KAS_PUBLIC_KEY_ALG_ENUM_UNSPECIFIED🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/openapi/policy/unsafe/unsafe.openapi.yaml` around lines 819 - 822, The constraint at line 821 uses `not: enum: [0]` to exclude a numeric zero value, but the `alg` field schema is defined with string enum values from `policy.KasPublicKeyAlgEnum`, not numeric values. Replace the numeric `0` in the `not: enum` constraint with the actual string enum value that represents the unspecified state (such as `KAS_PUBLIC_KEY_ALG_ENUM_UNSPECIFIED` or its equivalent string representation) to properly block the intended enum value.otdfctl/cmd/policy/kasKeys.go (1)
356-363:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve UUID path when
--key/--oldKeyis already a UUID.Line 357 and Line 603 use
kasIdentifier != "" || ..., so providing--kasforces identifier mode and then clearsid/oldKeyat Line 362 and Line 608. That bypasses direct UUID lookup/rotate behavior.Suggested fix
@@ - kasIdentifier := c.Flags.GetOptionalString("kas") - if kasIdentifier != "" || utils.ClassifyString(id) != utils.StringTypeUUID { + if utils.ClassifyString(id) != utils.StringTypeUUID { identifier, err = getKasKeyIdentifier(c) if err != nil { cli.ExitWithError("Invalid key identifier", err) } id = "" } @@ - kasIdentifier := c.Flags.GetOptionalString("kas") - if kasIdentifier != "" || utils.ClassifyString(oldKey) != utils.StringTypeUUID { + if utils.ClassifyString(oldKey) != utils.StringTypeUUID { identifier, err = getKasKeyIdentifier(c) if err != nil { cli.ExitWithError("Invalid key identifier", err) } oldKey = "" }Also applies to: 602-609
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@otdfctl/cmd/policy/kasKeys.go` around lines 356 - 363, The condition on line 357 uses the OR operator to check if kasIdentifier is not empty or if id is not a UUID, which forces identifier mode and clears id whenever --kas is provided, even if id is already a valid UUID. Change the logical operator from OR to AND in the condition checking kasIdentifier and the UUID classification so that identifier mode is only entered when BOTH --kas is provided AND the key is not already a UUID, allowing direct UUID lookup to be preserved when the key is already a UUID. Apply the same logical fix to the equivalent code around line 602-609 that handles oldKey.
Contributor
…DF references The HKDF-removal for mlkem-wrapped landed in 455f280; flip the ADR to accepted and update two comments that still described the old HKDF-everywhere behaviour for pure ML-KEM (kemWrapKeySize and defaultTDFSalt docstring). Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
Contributor
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
1 participant
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.
Summary
mlkem-wrappedKAO scheme.2.16.840.1.101.3.4.4.{2,3}).FromPublicPEM/FromPrivatePEM. Service and SDK callers collapse from per-algorithm switches to a singleIsKEMKeyTypebranch.Wire formats are preserved byte-for-byte; existing
hybrid-wrappedKAOs round-trip unchanged. The OID-keyed registry leaves the planned hybrid-PEM-to-SPKI follow-up as a near-zero change (three OID constants + three registry entries).Jira: DSPX-3383
Test plan
cd lib/ocrypto && go test ./...cd service && go test ./...cd sdk && go test ./...cd sdk && go test -run TestREADMECodeBlocksgolangci-lint run ./lib/ocrypto/ ./service/internal/security/ ./sdk/...— zero new issuesbats test/tdf-roundtrips.bats— ML-KEM-768 / ML-KEM-1024 / X-Wing / P-256+ML-KEM-768 / P-384+ML-KEM-1024 round-trips pass end-to-end🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
mlkem-wrappedkey access object type for ML-KEM-based DEK wrapping.Documentation
mlkem:768andmlkem:1024options.