[DRAFT] PQC xwing (codex generated) - #3210
Conversation
github-actions
Bot
added
comp:db
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 post-quantum cryptographic (PQC) capabilities to the system by adding support for ML-KEM and the X-Wing hybrid KEM. These changes include new encryption/decryption implementations, updated key management logic, and necessary modifications to the protocol definitions to enable these algorithms across the platform. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Quantum threats are on the way, But X-Wing keeps the keys at bay. With ML-KEM we stand our ground, Where post-quantum safety is found. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for Post-Quantum Cryptography (PQC) by implementing ML-KEM (768 and 1024) and the X-Wing hybrid KEM. The changes span the cryptographic library (ocrypto), SDK key access logic, and KAS rewrap/indexing services, along with necessary protobuf updates for policy definitions. Feedback focuses on improving the clarity and conciseness of new error messages, ensuring binary data in metadata is Base64 encoded rather than cast directly to strings, and simplifying logic within the security providers and key access layers.
| decap1024, err1024 := mlkem.NewDecapsulationKey1024(block.Bytes) | ||
| if err1024 != nil { | ||
| return nil, fmt.Errorf("mlkem.NewDecapsulationKey1024 failed after mlkem.NewDecapsulationKey768 failed: %w / %w", err, err1024) |
There was a problem hiding this comment.
The error message here is a bit verbose and might be confusing. If mlkem.NewDecapsulationKey768 fails, the subsequent call to mlkem.NewDecapsulationKey1024 failing with a different error might obscure the root cause. Consider a more concise error message or logging the first error separately before returning a combined error.
| func (d MLKEMDecryptor768) Decrypt(_ []byte) ([]byte, error) { | ||
| return nil, errors.New("ciphertext encapsulation is required for ML-KEM decryption") |
There was a problem hiding this comment.
The error message "ciphertext encapsulation is required for ML-KEM decryption" is technically correct but could be clearer. ML-KEM uses encapsulation/decapsulation, not traditional encryption/decryption of a ciphertext directly. Perhaps "ML-KEM decryption requires an encapsulated ciphertext, not a direct ciphertext" or similar.
| func (d MLKEMDecryptor1024) Decrypt(_ []byte) ([]byte, error) { | ||
| return nil, errors.New("ciphertext encapsulation is required for ML-KEM decryption") |
There was a problem hiding this comment.
The error message "ciphertext encapsulation is required for ML-KEM decryption" is technically correct but could be clearer. ML-KEM uses encapsulation/decapsulation, not traditional encryption/decryption of a ciphertext directly. Perhaps "ML-KEM decryption requires an encapsulated ciphertext, not a direct ciphertext" or similar.
| encap1024, err1024 := mlkem.NewEncapsulationKey1024(block.Bytes) | ||
| if err1024 != nil { | ||
| return nil, fmt.Errorf("mlkem.NewEncapsulationKey1024 failed after mlkem.NewEncapsulationKey768 failed: %w / %w", err, err1024) |
There was a problem hiding this comment.
Similar to the decryption side, the error message mlkem.NewEncapsulationKey1024 failed after mlkem.NewEncapsulationKey768 failed: %w / %w is quite verbose. It might be better to log the first error and then return a simpler error for the second failure, or combine them more concisely.
|
|
||
| func (e MLKEMEncryptor768) Metadata() (map[string]string, error) { | ||
| m := make(map[string]string) | ||
| m["encapsulatedKey"] = string(e.EphemeralKey()) |
There was a problem hiding this comment.
Converting e.EphemeralKey() (which is []byte) directly to a string using string() can lead to unexpected results if the byte slice does not represent valid UTF-8. It's generally safer to base64 encode binary data when storing it as a string, especially in metadata.
| m["encapsulatedKey"] = string(e.EphemeralKey()) | |
| m["encapsulatedKey"] = Base64Encode(e.EphemeralKey()) |
| if encryptor.Type() == ocrypto.EC { | ||
| ephemeralPublicKeyPEM, err := encryptor.PublicKeyInPemFormat() | ||
| if err != nil { | ||
| return "", "", "", fmt.Errorf("failed to encode EC ephemeral public key: %w", err) | ||
| } | ||
| return wrappedKey, keyType, ephemeralPublicKeyPEM, nil |
There was a problem hiding this comment.
The ephemeralPublicKeyPEM variable is only used if encryptor.Type() == ocrypto.EC. If encryptor.Type() is ocrypto.MLKEM or ocrypto.Hybrid, the ephemeralKey is base64 encoded directly. This logic could be simplified by always base64 encoding ephemeralKey and letting the PublicKeyInPemFormat handle the PEM encoding for EC keys internally if needed, or by having a consistent return type for EphemeralKey() that is always base64 encoded if it's meant for string representation.
|
|
||
| keyAccess := keyAccessList[0] | ||
| assert.Equal(t, "eccWrapped", keyAccess.KeyType, "EC keys should use 'eccWrapped' key type") | ||
| assert.Equal(t, "ec-wrapped", keyAccess.KeyType, "EC keys should use 'ec-wrapped' key type") |
There was a problem hiding this comment.
The previous value eccWrapped was changed to ec-wrapped. This is a good change for consistency with the SchemeType enum, but it's worth noting if this was a deliberate correction or just a refactor. If eccWrapped was used elsewhere, those might need updating too.
| require.NoError(t, err, "Should wrap key with EC public key") | ||
| assert.NotEmpty(t, wrappedKey, "Should return wrapped key") | ||
| assert.Equal(t, "eccWrapped", keyType, "EC keys should use 'eccWrapped' type") | ||
| assert.Equal(t, "ec-wrapped", keyType, "EC keys should use 'ec-wrapped' type") |
There was a problem hiding this comment.
The previous value eccWrapped was changed to ec-wrapped. This is a good change for consistency with the SchemeType enum, but it's worth noting if this was a deliberate correction or just a refactor. If eccWrapped was used elsewhere, those might need updating too.
| case ocrypto.RSA2048Key, ocrypto.RSA4096Key, ocrypto.MLKEM768Key, ocrypto.MLKEM1024Key, ocrypto.HybridXWingKey: | ||
| return "", errors.New("certificates only available for EC keys") |
There was a problem hiding this comment.
The error message "certificates only available for EC keys" is duplicated. It can be simplified by having a single return statement after the switch, or by making the default case return this error.
| if !ok { | ||
| return "", ErrCertNotFound | ||
| } |
There was a problem hiding this comment.
There's a redundant if !ok check here. The if !ok on line 348 already handles the case where k is not found. This can be removed.
| if !ok { | |
| return "", ErrCertNotFound | |
| } | |
| if !ok { | |
| return "", ErrCertNotFound | |
| } | |
| rsa, ok := k.(PrivateKeyCrypto) |
Signed-off-by: David Mihalcik <dmihalcik@virtru.com>
Signed-off-by: David Mihalcik <dmihalcik@virtru.com>
21ec012 to
0d61cb0
Compare
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
Merged in #3276 |
1 participant
PROMPT