Introduce key_ids as eventual replacement for key_id in EncryptedData #273
Conversation
WalkthroughThe pull request introduces a modification to the encryption mechanism by adding support for multiple key identifiers. In the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
proto/c1/connector/v2/resource.proto (1)
183-183: LGTM: Proto definition aligns with implementationThe addition of
repeated string key_ids = 7is well-structured:
- Field number 7 doesn't conflict with existing fields
- Type matches the implementation in jwk.go
- Comment clearly documents the intent to replace key_id in the future
For future reference, consider documenting the migration timeline and deprecation process for the
key_idfield. This will help consumers prepare for the eventual transition tokey_ids.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pb/c1/connector/v2/resource.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (2)
pkg/crypto/providers/jwk/jwk.go(1 hunks)proto/c1/connector/v2/resource.proto(1 hunks)
🔇 Additional comments (2)
pkg/crypto/providers/jwk/jwk.go (1)
116-116: LGTM: KeyIds field correctly populated
The implementation maintains backward compatibility by populating both KeyId and KeyIds fields with the same thumbprint value. This aligns with the PR objective of gradually transitioning to multi-recipient encryption support.
Let's verify the usage of KeyIds field across the codebase:
✅ Verification successful
Let's gather more information about the usage of KeyIds to ensure there are no compatibility issues.
Let's verify one more aspect to ensure the KeyId (singular) field is properly handled alongside KeyIds.
LGTM: KeyIds field implementation is correct and maintains compatibility
The implementation is verified to be correct because:
- The proto definition shows KeyIds is intended to replace KeyId in the future
- Both KeyId and KeyIds fields are properly populated with the same thumbprint value
- The change maintains backward compatibility by setting both fields
- The implementation aligns with the gradual transition to multi-recipient encryption support
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any existing consumers of the KeyIds field
# to ensure the change won't break existing code
rg -l "KeyIds" --type go
Length of output: 90
Script:
#!/bin/bash
# Check the actual usage context of KeyIds in both files
rg "KeyIds" -B 2 -A 2 --type go
Length of output: 1312
Script:
#!/bin/bash
# Check the relationship between KeyId and KeyIds in the struct and their usage
rg "KeyId[^s]" -B 2 -A 2 --type go pkg/crypto/providers/jwk/jwk.go
Length of output: 488
proto/c1/connector/v2/resource.proto (1)
183-183: Verify security implications of dual key identifiers
While the implementation is correct, maintaining both key_id and key_ids fields requires careful handling in security-sensitive contexts.
Let's check for any security-sensitive uses of these fields:
✅ Verification successful
Dual key identifiers implementation is secure and properly transitioning
The verification shows that the implementation is secure and follows best practices:
- The
key_idsfield is properly implemented as a future replacement forkey_id, with both fields being used in a compatible way. - The encryption provider (JWK) correctly populates both fields, setting
KeyIdfor backward compatibility andKeyIdsfor future use. - The implementation in
pkg/crypto/providers/jwk/jwk.goshows proper handling:
return &v2.EncryptedData{
KeyId: tp,
KeyIds: []string{tp},
// ...
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for security-critical code paths handling key identifiers
# Check for key validation or verification logic
rg -B 3 -A 3 "key_id|key_ids" --type go | grep -i "verif\|valid\|check"
# Look for encryption/decryption logic using these fields
rg -B 3 -A 3 "key_id|key_ids" --type go | grep -i "encrypt\|decrypt"
Length of output: 636
Script:
#!/bin/bash
# Let's look more broadly at the proto definition and related code
# First, check the full proto definition around these fields
rg -B 10 -A 10 "key_ids = 7" --type proto
# Look for any direct usage of these fields in the codebase
rg -B 5 -A 5 "\.KeyIds|\.KeyId\b" --type go
# Check for any encryption-related types or interfaces that might use these fields
ast-grep --pattern 'type $_ interface {
$$$
Encrypt($$$) {
$$$
}
$$$
}'
Length of output: 6956
Script:
#!/bin/bash
# Let's examine the proto file directly and its context
cat proto/c1/connector/v2/resource.proto
# Also check for any migration or compatibility handling code
rg -B 5 -A 5 "EncryptedData" --type go | grep -v "pb.go"
# Look for any tests that might reveal the intended usage
rg -B 5 -A 5 "key_id.*key_ids|key_ids.*key_id" --type go
Length of output: 28434
To make multi-recipient encryption rollout easier, let's introduce the proto change now, so consumers like cone can be made ready for the eventual move to multiple keys per Encrypted data package.
| @@ -175,11 +175,12 @@ message CreateAccountResponse { | |||
|
|
|||
| message EncryptedData { | |||
| string provider = 1; | |||
| string key_id = 2; | |||
| string key_id = 2 [deprecated = true]; | |||
| string name = 3; | |||
| string description = 4; | |||
| string schema = 5; // optional | |||
| bytes encrypted_bytes = 6; // if 'schema' is set, this should be JSON. | |||
There was a problem hiding this comment.
CreateAccountResponse contains a repeated EncryptedData, so why not just encrypt the data multiple times, once for each key id? That we can have the same data decryptable by multiple keys is an artifact of certain encryption schemes, not something that can be generalized.
to make multi-recipient encryption rollout easier, lets introduce the proto change now, so consumers like cone can be made ready for the eventual move to multiple keys per Encrypted data package.
Summary by CodeRabbit
New Features
Bug Fixes