feat(core): Deprecate ECKeyPair in favor of asym_*#3293
feat(core): Deprecate ECKeyPair in favor of asym_*#3293dmihalcik-virtru wants to merge 9 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 51 minutes and 3 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors asymmetric key APIs: introduces PrivateKeyDecryptor/PublicKeyEncryptor methods and factories, centralizes PEM helpers, changes EC ephemeral key handling (adds EphemeralPublicKeyInPemFormat), and updates SDK and service call sites to use new decryptor/encryptor interfaces. Also adds tests and minor gitignore newline fix. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
comp:sdk
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 refactors the cryptographic key management system by deprecating the 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. 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. Old key pairs now must fade, New interfaces take their place, Decryption's path is made. 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 the PrivateKeyDecryptor interface to unify RSA and EC private key operations, such as decryption and shared secret derivation, across the ocrypto package and SDK. It refactors RsaKeyPair, ECKeyPair, and KASClient to implement this interface while deprecating legacy functions. Feedback suggests that several nil checks in the KeyType methods are redundant and should be removed to simplify the code and avoid hiding initialization issues.
| if asymDecryption.PrivateKey == nil { | ||
| return KeyType("rsa:[unknown]") | ||
| } |
There was a problem hiding this comment.
The check for asymDecryption.PrivateKey == nil is redundant here because PrivateKey is a field of the struct and the method is called on an instance. If the instance is initialized, this field should be checked at the point of creation or usage, not inside every getter method. This adds unnecessary complexity.
There was a problem hiding this comment.
yeah, that would make sense.. unfortunately FromRSA and the corresponding SDK WithSession...RSA options don't return error so it would be hard to wire that in at the moment
| if e.sk == nil { | ||
| return KeyType("ec:[unknown]") | ||
| } |
There was a problem hiding this comment.
Similar to the RSA implementation, checking for e.sk == nil inside the KeyType() method is defensive programming that might hide initialization issues. It is better to ensure the struct is properly initialized upon creation.
| if keyPair.PrivateKey == nil { | ||
| return KeyType("ec:[unknown]") | ||
| } |
There was a problem hiding this comment.
Checking for keyPair.PrivateKey == nil in KeyType() is redundant if the struct is properly initialized. Consider removing this check to simplify the method.
| if keyPair.privateKey == nil { | ||
| return KeyType("rsa:[unknown]") | ||
| } |
There was a problem hiding this comment.
The check for keyPair.privateKey == nil is redundant. If the RsaKeyPair struct is properly initialized, this field should be valid. Removing this check simplifies the code.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
- Remove DeriveSharedKey from PrivateKeyDecryptor interface, eliminating false mutual-exclusion between RSA and EC implementations - Replace SDK encrypt-side DeriveSharedKey with FromPublicPEMWithSalt+Encrypt in generateWrapKeyWithEC (tdf.go) - Replace SDK decrypt-side DeriveSharedKey with DecryptWithEphemeralKey in handleECKeyResponse/processECResponse (kas_client.go) - Service-side and experimental XOR callers type-assert to ECDecryptor and call DeriveSharedKey on the concrete type directly - Fix silent failure chain in newECIES, EphemeralKey, and Metadata - Fix convCurve to return error instead of nil (prevents nil-panic) - Extract parseEphemeralPublicKey helper to fix nestif lint violation - Fix ECKeyPair.Decrypt/Public/DeriveSharedKey to use NewECDecryptor so TDF salt is propagated correctly through the shim - Add asym_decryption_test.go with coverage for new interface and methods - Update ec_key_pair_test.go and ec_decrypt_compressed_test.go to use non-deprecated APIs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
DSPX-1089-deprecate-eckp
branch
from
66d1f7d to
08055b2
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/tdf.go (1)
676-686:⚠️ Potential issue | 🟠 MajorDerive the wrap scheme from the parsed public key, not
kasInfo.Algorithm.Line 677 still trusts metadata to choose the EC/RSA path. If
kasInfo.Algorithmis empty or stale whilekasInfo.PublicKeyis actually EC, this falls into the RSA branch,generateWrapKeyWithRSAwill still encrypt with the EC encryptor returned byFromPublicPEM, and the manifest is emitted aswrappedwithout anEphemeralPublicKey. That key-access object is not rewrappable later.Please branch on the parsed key/encryptor type instead, or at least fail fast when the PEM type and declared algorithm disagree.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/tdf.go` around lines 676 - 686, The branch decision should be based on the parsed public key/encryptor type rather than trusting kasInfo.Algorithm; parse kasInfo.PublicKey (via the same flow that returns an encryptor from FromPublicPEM) and inspect its key type (EC vs RSA) before choosing generateWrapKeyWithEC or generateWrapKeyWithRSA, or else return an error if the parsed key type conflicts with kasInfo.Algorithm; update the logic around ktype/kasInfo.Algorithm, generateWrapKeyWithEC, generateWrapKeyWithRSA, FromPublicPEM, and the keyAccess fields (KeyType, WrappedKey, EphemeralPublicKey) so the manifest correctly records an EphemeralPublicKey for EC keys and prevents creating non-rewrappable entries when the PEM and declared algorithm disagree.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ocrypto/asym_decryption_test.go`:
- Around line 136-153: The TestAsymDecryptionKeyType loop should be converted to
subtests so failures show which RSA size failed: for each bits in {RSA2048Size,
RSA4096Size} call t.Run with a descriptive name (e.g. "RSA2048"/"RSA4096" or
fmt.Sprintf("%d", bits)), capture bits in the closure, and move t.Parallel()
into the subtest body; inside the subtest keep the existing logic
(NewRSAKeyPair, PrivateKeyInPemFormat, FromPrivatePEM, type assertion to
AsymDecryption and the KeyType checks against RSA2048Key/RSA4096Key).
In `@lib/ocrypto/asym_encryption.go`:
- Around line 288-293: Define a small interface (e.g.,
EphemeralPublicKeyExporter) that declares EphemeralPublicKeyInPemFormat()
(string, error) and have ECEncryptor implicitly implement it (the method already
exists), then update the public API to return or accept that interface where
callers need to export the ephemeral PEM instead of requiring a concrete
ocrypto.ECEncryptor; ensure references to PublicKeyEncryptor remain unchanged
unless the API should explicitly include the new interface, and update call
sites to use the new EphemeralPublicKeyExporter interface for PEM export without
downcasting to ECEncryptor.
In `@lib/ocrypto/ec_key_pair.go`:
- Around line 480-488: The two exported helpers (ECPrivateKeyInPemFormat,
ECPublicKeyInPemFormat) pass value-type ecdsa.PrivateKey/ecdsa.PublicKey to
privateKeyInPemFormat which only handles pointer cases, so zero-value structs
bypass nil checks; update privateKeyInPemFormat to add switch cases for the
value types (case ecdsa.PrivateKey: and case ecdsa.PublicKey:) and perform the
same validity checks as for pointers (e.g., for ecdsa.PrivateKey verify key.D !=
nil and for ecdsa.PublicKey verify key.X != nil && key.Y != nil), returning the
same error ("failed to generate PEM formatted private key"/appropriate public
key error) when those fields are nil to prevent falling through to
x509.MarshalPKCS8PrivateKey with a zero-value.
In `@sdk/experimental/tdf/key_access.go`:
- Around line 186-201: The EC wrapper currently returns the non-canonical string
"eccWrapped" from wrapKeyWithEC; update the return to use the canonical key type
"ec-wrapped" so it matches the rest of the codebase (see wrapKeyWithEC in
key_access.go and the canonical uses in sdk/tdf.go and
lib/ocrypto/asym_encryption.go), keeping the other return values and error
handling unchanged.
- Around line 165-182: The code currently chooses the wrapping branch using
pubKeyInfo.Algorithm (ktype) even after parsing the PEM; instead, base the
decision on the parsed kasPublicKey's runtime type: after
ocrypto.FromPublicPEM(...) check if kasPublicKey implements ocrypto.ECEncryptor
(or use a type switch) and call wrapKeyWithEC(ktype, epk, symKey) when it does,
otherwise call wrapKeyWithRSA(kasPublicKey, symKey); update error paths to
report a clear mismatch if the parsed key cannot perform the expected operations
and remove reliance on pubKeyInfo.Algorithm for branching.
In `@sdk/kas_client.go`:
- Around line 196-208: In handleECKeyResponse, explicitly validate that
response.GetSessionPublicKey() is not empty before calling pem.Decode so you can
return a clearer error; check the returned string from
kas.RewrapResponse.GetSessionPublicKey(), if it's empty return a descriptive
error like "empty KAS session public key" and only then call pem.Decode,
preserving the existing session key type assertion (k.sessionKey as
ocrypto.ECDecryptor) and the subsequent call to k.processECResponse with
block.Bytes.
---
Outside diff comments:
In `@sdk/tdf.go`:
- Around line 676-686: The branch decision should be based on the parsed public
key/encryptor type rather than trusting kasInfo.Algorithm; parse
kasInfo.PublicKey (via the same flow that returns an encryptor from
FromPublicPEM) and inspect its key type (EC vs RSA) before choosing
generateWrapKeyWithEC or generateWrapKeyWithRSA, or else return an error if the
parsed key type conflicts with kasInfo.Algorithm; update the logic around
ktype/kasInfo.Algorithm, generateWrapKeyWithEC, generateWrapKeyWithRSA,
FromPublicPEM, and the keyAccess fields (KeyType, WrappedKey,
EphemeralPublicKey) so the manifest correctly records an EphemeralPublicKey for
EC keys and prevents creating non-rewrappable entries when the PEM and declared
algorithm disagree.
🪄 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: 0a7022c7-f77c-4a72-8811-b1965976ce94
📒 Files selected for processing (21)
.gitignorelib/ocrypto/asym_decryption.golib/ocrypto/asym_decryption_test.golib/ocrypto/asym_encryption.golib/ocrypto/ec_decrypt_compressed_test.golib/ocrypto/ec_key_pair.golib/ocrypto/ec_key_pair_test.golib/ocrypto/rsa_key_pair.gosdk/codegen/runner/generate.gosdk/experimental/tdf/key_access.gosdk/experimental/tdf/key_access_test.gosdk/kas_client.gosdk/kas_client_test.gosdk/tdf.gosdk/tdf_config.gosdk/tdf_test.goservice/internal/security/basic_manager.goservice/internal/security/basic_manager_test.goservice/internal/security/in_process_provider.goservice/kas/access/rewrap.goservice/trust/key_manager.go
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Fix incorrect/stale comments (encrypt vs decrypt, misleading deprecation targets, stale TODOs), improve error messages to be actionable, add nil validation to NewSaltedECDecryptor, fix value-type nil-guard bypass in ECPrivateKeyInPemFormat/ECPublicKeyInPemFormat, and add ECIES round-trip tests across all EC curves plus a test verifying recipient vs ephemeral key PEM separation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/ocrypto/asym_decryption.go (1)
233-320:⚠️ Potential issue | 🔴 Critical
DecryptWithEphemeralKeydereferences a nil private key without a guard.Line 241 (
e.sk.ECDH) and line 313 (e.sk.Curve()in the fallback path) will panic on a zero-valueECDecryptor{}or whenNewECDecryptor(nil)is called. ThePublic()andKeyType()methods already protect this case; this method should do the same.🔧 Proposed fix
func (e ECDecryptor) DecryptWithEphemeralKey(data, ephemeral []byte) ([]byte, error) { + if e.sk == nil { + return nil, errors.New("failed to decrypt, private key is empty") + } ek, err := e.parseEphemeralPublicKey(ephemeral) if err != nil { return nil, fmt.Errorf("failed to parse ephemeral public key: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ocrypto/asym_decryption.go` around lines 233 - 320, The methods DecryptWithEphemeralKey and parseEphemeralPublicKey can panic when the ECDecryptor's private key (e.sk) is nil; add the same nil-check guard used in deriveSharedKey (or Public()/KeyType()) at the start of DecryptWithEphemeralKey and before calling e.sk.Curve() in parseEphemeralPublicKey, returning a clear error like "failed to derive shared key, private key is empty" if e.sk == nil so all code paths fail safely instead of dereferencing a nil pointer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ocrypto/asym_decryption_test.go`:
- Around line 95-102: Extend the NilKeyGuards subtest to also call
ECDecryptor{}.DecryptWithEphemeralKey(...) and assert it returns an error (and
does not panic); specifically, in the t.Run "NilKeyGuards" add a call like
nilDec.DecryptWithEphemeralKey(...) with the minimal/zero inputs used by other
tests (e.g., nil or empty byte slices for ciphertext/ephemeral public key) and
use require.Error(t, err) to ensure the zero-value ECDecryptor path that
dereferenced e.sk is covered; keep the rest of the assertions for Public() and
PrivateKeyInPemFormat() unchanged.
In `@lib/ocrypto/ec_key_pair.go`:
- Around line 237-258: Add a nil check for keyPair.PrivateKey at the start of
ECKeyPair.Decrypt, ECKeyPair.Public, and ECKeyPair.DeriveSharedKey: if
keyPair.PrivateKey == nil return a descriptive error (matching the style used in
PublicKeyInPemFormat/KeySize/KeyType and related types) instead of calling
ConvertToECDHPrivateKey(keyPair.PrivateKey) so you avoid dereferencing a nil
receiver inside ConvertToECDHPrivateKey/ k.ECDH().
In `@sdk/experimental/tdf/key_access_test.go`:
- Around line 93-100: Add a regression test to ensure wrapKeyWithPublicKey uses
the parsed PEM type, not stale pubKeyInfo.Algorithm: create an EC key via
NewECPrivateKey/ECCModeSecp256r1, derive its PEM with
Public().PublicKeyInPemFormat(), then construct a pubKeyInfo-like input where
Algorithm is bogus (e.g., "rsa:2048") and call wrapKeyWithPublicKey (or the test
helper that routes to it); assert the function follows the parsed PEM (EC
branch) and succeeds. Add the inverse case too (RSA PEM with Algorithm
"ec:secp256r1") to validate both directions; place these cases alongside the
existing EC tests in key_access_test.go and use require.NoError/require.Equal
assertions already used in the file.
In `@service/trust/key_manager.go`:
- Around line 33-35: Update the Depracation comment for DeriveKey to explicitly
state that callers should use Decrypt and that DeriveKey remains exported only
for API compatibility; note that DelegatingKeyService will forward calls but
most current key manager implementations intentionally return an
unsupported-operation error (e.g., ErrUnsupportedOperation or similar) —
document that expected behavior and the recommended migration path (call Decrypt
with the same inputs) in the comment above DeriveKey and in any public docs so
integrators are not misled; reference the DeriveKey method,
DelegatingKeyService, and the key manager implementations in the comment so
readers can locate the implementation-specific unsupported behavior.
---
Outside diff comments:
In `@lib/ocrypto/asym_decryption.go`:
- Around line 233-320: The methods DecryptWithEphemeralKey and
parseEphemeralPublicKey can panic when the ECDecryptor's private key (e.sk) is
nil; add the same nil-check guard used in deriveSharedKey (or
Public()/KeyType()) at the start of DecryptWithEphemeralKey and before calling
e.sk.Curve() in parseEphemeralPublicKey, returning a clear error like "failed to
derive shared key, private key is empty" if e.sk == nil so all code paths fail
safely instead of dereferencing a nil pointer.
🪄 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: 685e56f0-5a60-4f54-a857-87c3aaaddcad
📒 Files selected for processing (6)
lib/ocrypto/asym_decryption.golib/ocrypto/asym_decryption_test.golib/ocrypto/ec_key_pair.gosdk/experimental/tdf/key_access.gosdk/experimental/tdf/key_access_test.goservice/trust/key_manager.go
| t.Run("NilKeyGuards", func(t *testing.T) { | ||
| t.Parallel() | ||
| nilDec := ECDecryptor{} | ||
| _, err := nilDec.Public() | ||
| require.Error(t, err) | ||
| _, err = nilDec.PrivateKeyInPemFormat() | ||
| require.Error(t, err) | ||
| }) |
There was a problem hiding this comment.
Add a regression check for zero-value DecryptWithEphemeralKey.
NilKeyGuards currently misses the one EC path that still dereferences e.sk. Extending this subtest to assert an error from ECDecryptor{}.DecryptWithEphemeralKey(...) would catch the panic fixed above.
🧪 Proposed test addition
t.Run("NilKeyGuards", func(t *testing.T) {
t.Parallel()
nilDec := ECDecryptor{}
_, err := nilDec.Public()
require.Error(t, err)
_, err = nilDec.PrivateKeyInPemFormat()
require.Error(t, err)
+ _, err = nilDec.DecryptWithEphemeralKey(nil, nil)
+ require.Error(t, err)
})As per coding guidelines, "Run go test ./... or make test and ensure all existing tests pass; add tests for new functionality".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ocrypto/asym_decryption_test.go` around lines 95 - 102, Extend the
NilKeyGuards subtest to also call ECDecryptor{}.DecryptWithEphemeralKey(...) and
assert it returns an error (and does not panic); specifically, in the t.Run
"NilKeyGuards" add a call like nilDec.DecryptWithEphemeralKey(...) with the
minimal/zero inputs used by other tests (e.g., nil or empty byte slices for
ciphertext/ephemeral public key) and use require.Error(t, err) to ensure the
zero-value ECDecryptor path that dereferenced e.sk is covered; keep the rest of
the assertions for Public() and PrivateKeyInPemFormat() unchanged.
| func (keyPair ECKeyPair) Decrypt(data []byte) ([]byte, error) { | ||
| ecdhPrivateKey, err := ConvertToECDHPrivateKey(keyPair.PrivateKey) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| dec, err := NewECDecryptor(ecdhPrivateKey) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return dec.Decrypt(data) | ||
| } | ||
|
|
||
| func (keyPair ECKeyPair) Public() (PublicKeyEncryptor, error) { | ||
| ecdhPrivateKey, err := ConvertToECDHPrivateKey(keyPair.PrivateKey) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| dec, err := NewECDecryptor(ecdhPrivateKey) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return dec.Public() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and find the methods mentioned
cat -n lib/ocrypto/ec_key_pair.go | head -100Repository: opentdf/platform
Length of output: 2908
🏁 Script executed:
# Get file size to understand scope
wc -l lib/ocrypto/ec_key_pair.goRepository: opentdf/platform
Length of output: 90
🏁 Script executed:
# Look for the ConvertToECDHPrivateKey function and the methods in question
rg -n "ConvertToECDHPrivateKey|func.*Decrypt|func.*Public|func.*DeriveSharedKey" lib/ocrypto/ec_key_pair.goRepository: opentdf/platform
Length of output: 1544
🏁 Script executed:
# Check the full implementation around lines 237-279
sed -n '237,279p' lib/ocrypto/ec_key_pair.goRepository: opentdf/platform
Length of output: 1108
🏁 Script executed:
# Look at the ConvertToECDHPrivateKey function implementation
rg -A 15 "func ConvertToECDHPrivateKey" lib/ocrypto/Repository: opentdf/platform
Length of output: 1004
🏁 Script executed:
# Check if there's an existing pattern of nil-checking in similar methods in this file
rg -B 3 "private key is empty" lib/ocrypto/Repository: opentdf/platform
Length of output: 1649
🏁 Script executed:
# Check the entire function signatures and implementations
sed -n '200,350p' lib/ocrypto/ec_key_pair.goRepository: opentdf/platform
Length of output: 4241
Add nil guards for keyPair.PrivateKey in Decrypt, Public, and DeriveSharedKey methods.
Lines 237, 249, and 269 call ConvertToECDHPrivateKey(keyPair.PrivateKey) without checking if keyPair.PrivateKey is nil. When a zero-value ECKeyPair{} is used, the nil pointer still matches the type switch, causing k.ECDH() to dereference a nil receiver and panic instead of returning an error.
This is inconsistent with other methods in the same struct (PublicKeyInPemFormat, KeySize, KeyType) and related types (ECDecryptor, AsymDecryption, RsaKeyPair), all of which guard against nil private keys.
🔧 Proposed fix
func (keyPair ECKeyPair) Decrypt(data []byte) ([]byte, error) {
+ if keyPair.PrivateKey == nil {
+ return nil, errors.New("failed to decrypt, private key is empty")
+ }
ecdhPrivateKey, err := ConvertToECDHPrivateKey(keyPair.PrivateKey)
if err != nil {
return nil, err
}
dec, err := NewECDecryptor(ecdhPrivateKey)
if err != nil {
return nil, err
}
return dec.Decrypt(data)
}
func (keyPair ECKeyPair) Public() (PublicKeyEncryptor, error) {
+ if keyPair.PrivateKey == nil {
+ return nil, errors.New("failed to generate public key encryptor, private key is empty")
+ }
ecdhPrivateKey, err := ConvertToECDHPrivateKey(keyPair.PrivateKey)
if err != nil {
return nil, err
}
dec, err := NewECDecryptor(ecdhPrivateKey)
if err != nil {
return nil, err
}
return dec.Public()
}
func (keyPair ECKeyPair) DeriveSharedKey(publicKeyInPem string) ([]byte, error) {
+ if keyPair.PrivateKey == nil {
+ return nil, errors.New("failed to derive shared key, private key is empty")
+ }
ecdhPrivateKey, err := ConvertToECDHPrivateKey(keyPair.PrivateKey)
if err != nil {
return nil, err
}
dec, err := NewECDecryptor(ecdhPrivateKey)
if err != nil {
return nil, err
}
return dec.deriveSharedKey(publicKeyInPem)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ocrypto/ec_key_pair.go` around lines 237 - 258, Add a nil check for
keyPair.PrivateKey at the start of ECKeyPair.Decrypt, ECKeyPair.Public, and
ECKeyPair.DeriveSharedKey: if keyPair.PrivateKey == nil return a descriptive
error (matching the style used in PublicKeyInPemFormat/KeySize/KeyType and
related types) instead of calling ConvertToECDHPrivateKey(keyPair.PrivateKey) so
you avoid dereferencing a nil receiver inside ConvertToECDHPrivateKey/ k.ECDH().
| ecPrivateKey, err := ocrypto.NewECPrivateKey(ocrypto.ECCModeSecp256r1) | ||
| require.NoError(t, err, "Should generate EC private key") | ||
|
|
||
| ecPublicKeyPEM, err := ecKeyPair.PublicKeyInPemFormat() | ||
| ecPublicKey, err := ecPrivateKey.Public() | ||
| require.NoError(t, err, "Should derive EC public key") | ||
|
|
||
| ecPublicKeyPEM, err := ecPublicKey.PublicKeyInPemFormat() | ||
| require.NoError(t, err, "Should get public key in PEM format") |
There was a problem hiding this comment.
Add a regression case for stale Algorithm metadata.
These updated EC tests still use matching Algorithm + PEM pairs, so they would not catch a regression where wrapKeyWithPublicKey starts routing off pubKeyInfo.Algorithm again. Please add at least one case with an EC PEM plus Algorithm: "rsa:2048" (and/or the inverse) and assert the branch still follows the parsed PEM type.
As per coding guidelines **/*.go: Run go test ./... or make test and ensure all existing tests pass; add tests for new functionality.
Also applies to: 112-112, 403-410, 423-423
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/experimental/tdf/key_access_test.go` around lines 93 - 100, Add a
regression test to ensure wrapKeyWithPublicKey uses the parsed PEM type, not
stale pubKeyInfo.Algorithm: create an EC key via
NewECPrivateKey/ECCModeSecp256r1, derive its PEM with
Public().PublicKeyInPemFormat(), then construct a pubKeyInfo-like input where
Algorithm is bogus (e.g., "rsa:2048") and call wrapKeyWithPublicKey (or the test
helper that routes to it); assert the function follows the parsed PEM (EC
branch) and succeeds. Add the inverse case too (RSA PEM with Algorithm
"ec:secp256r1") to validate both directions; place these cases alongside the
existing EC tests in key_access_test.go and use require.NoError/require.Equal
assertions already used in the file.
| // | ||
| // Deprecated: Directly use Decrypt when appropriate. | ||
| DeriveKey(ctx context.Context, key KeyDetails, ephemeralPublicKeyBytes []byte, curve elliptic.Curve) (ProtectedKey, error) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Deprecation guidance is too vague for an active public method.
Line 34 suggests migrating to Decrypt, but DeriveKey is still callable through DelegatingKeyService and current managers return unsupported operation. Please document that behavior explicitly to prevent misleading integrations.
Suggested doc tweak
// DeriveKey computes an agreed upon secret key derived from an ECDH exchange.
//
-// Deprecated: Directly use Decrypt when appropriate.
+// Deprecated: This method is being phased out and may return "unsupported operation"
+// in key manager implementations. Prefer the Decrypt-based unwrap flow where applicable.
DeriveKey(ctx context.Context, key KeyDetails, ephemeralPublicKeyBytes []byte, curve elliptic.Curve) (ProtectedKey, error)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/trust/key_manager.go` around lines 33 - 35, Update the Depracation
comment for DeriveKey to explicitly state that callers should use Decrypt and
that DeriveKey remains exported only for API compatibility; note that
DelegatingKeyService will forward calls but most current key manager
implementations intentionally return an unsupported-operation error (e.g.,
ErrUnsupportedOperation or similar) — document that expected behavior and the
recommended migration path (call Decrypt with the same inputs) in the comment
above DeriveKey and in any public docs so integrators are not misled; reference
the DeriveKey method, DelegatingKeyService, and the key manager implementations
in the comment so readers can locate the implementation-specific unsupported
behavior.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
|
I'm moving this to draft since it is too big and covers too much ground. I'll sequence some of the useful changes out |
1 participant
Proposed Changes
Checklist
Testing Instructions
Summary by CodeRabbit
New Features
Deprecations
Bug Fixes
Tests