Multi-Recipient encryption for client credentials #262
Conversation
WalkthroughThis pull request introduces comprehensive modifications to the encryption and credential management system across multiple files. The primary changes involve updating method signatures to include context parameters, restructuring encryption provider logic to support multiple configurations, and enhancing the modularity of recipient creation for different key types (ED25519, RSA). The modifications aim to improve the flexibility and error handling of the encryption process while maintaining the core functionality of credential rotation and account creation. Changes
Sequence DiagramsequenceDiagram
participant Client
participant ConnectorBuilder
participant CryptoManager
participant EncryptionProvider
participant Recipient
Client->>ConnectorBuilder: Request Credential Rotation/Account Creation
ConnectorBuilder->>CryptoManager: Create Encryption Manager (with context)
CryptoManager->>EncryptionProvider: Encrypt with Multiple Configurations
EncryptionProvider->>Recipient: Create Recipients
Recipient-->>EncryptionProvider: Return Recipients
EncryptionProvider-->>CryptoManager: Return Encrypted Data
CryptoManager-->>ConnectorBuilder: Return Encryption Result
ConnectorBuilder-->>Client: Complete Request
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 2
🧹 Outside diff range and nitpick comments (9)
pkg/crypto/crypto.go (2)
44-44: Add a period at the end of the comment.The comment on line 44 should end with a period to follow Go's commenting conventions.
Apply this diff to fix the comment:
-// MJP creating the providerMap means parsing the configs and failing early instead of in Encrypt +// MJP creating the providerMap means parsing the configs and failing early instead of in Encrypt.🧰 Tools
🪛 golangci-lint (1.62.2)
44-44: Comment should end in a period
(godot)
🪛 GitHub Check: go-lint
[failure] 44-44:
Comment should end in a period (godot)
45-54: Improve error handling inNewEncryptionManager.Currently, if an error occurs while getting the provider name, the function returns immediately. Consider collecting errors for all configurations and returning them collectively to provide more comprehensive feedback.
pkg/crypto/providers/registry.go (2)
Line range hint
37-49: Re-evaluate provider inference logic whenproviderNameis empty.Relying on implicit inference of the provider based on the configuration can lead to ambiguous behavior. It's better to require the
providerfield explicitly to avoid misconfigurations.Apply this diff to enforce explicit provider specification:
func GetEncryptionProviderName(ctx context.Context, conf *v2.EncryptionConfig) (string, error) { providerName := normalizeProviderName(conf.GetProvider()) - // We weren't given an explicit provider, so we can try to infer one based on the config. - // FIXME(morgabra): This seems bad, let's just require 'provider'? - if providerName == "" { - if conf.GetJwkPublicKeyConfig() != nil { - providerName = jwk.EncryptionProviderJwk - } - } + // Require the 'provider' field to be explicitly specified. + if providerName == "" { + return "", fmt.Errorf("encryption provider not specified in the configuration") + } return providerName, nil }
51-59: Update documentation forGetEncryptionProviderForConfig.Ensure that the comments accurately reflect the current behavior, especially after changes to provider inference logic.
pkg/crypto/providers/jwk/jwk.go (2)
75-117: Separate handling of ECDSA keys from other key types.Currently, the code handles ECDSA keys differently by performing encryption immediately rather than adding to the recipients list. To maintain consistency and support multi-recipient encryption fully, consider creating recipients for ECDSA keys as well, if possible.
139-139: Remove unnecessary trailing newline.There's an extra blank line that can be removed to keep the code tidy.
Apply this diff to remove the extra newline:
} - return encrypted, nil }🧰 Tools
🪛 GitHub Check: go-lint
[failure] 139-139:
unnecessary trailing newline (whitespace)pkg/crypto/providers/jwk/rsa.go (1)
Line range hint
12-24: Improve error messages for recipient creation failures.When
CreateRSARecipientfails, the error messages could be more descriptive to aid debugging.Apply this diff to enhance error messages:
func CreateRSARecipient(pubKey *rsa.PublicKey) (*agessh.RSARecipient, error) { sshPubKey, err := ssh.NewPublicKey(pubKey) if err != nil { - return nil, fmt.Errorf("jwk-rsa: failed to convert public key to ssh format: %w", err) + return nil, fmt.Errorf("jwk-rsa: failed to convert RSA public key to SSH format: %w", err) } recipient, err := agessh.NewRSARecipient(sshPubKey) if err != nil { - return nil, fmt.Errorf("jwk-rsa: failed to create recipient: %w", err) + return nil, fmt.Errorf("jwk-rsa: failed to create RSA recipient: %w", err) } return recipient, nil }pkg/crypto/crypto_test.go (1)
40-40: Fix typo in test function name.The function name has a typo: "Encrypton" should be "Encryption".
-func TestMultiRecipientEncrypton(t *testing.T) { +func TestMultiRecipientEncryption(t *testing.T) {pkg/connectorbuilder/connectorbuilder.go (1)
Line range hint
773-777: Consider keyId handling strategy for multi-recipient encryption.Regarding the open question about handling keyId in multi-recipient encryption:
Option 1 - Concatenated Hash:
- Hash all public keys together
- Pro: Single keyId maintains backward compatibility
- Con: Order dependency, all keys needed for verification
Option 2 - List of Hashes:
- Store array of individual key hashes
- Pro: Independent key verification
- Con: Schema change required
Recommendation: Use Option 2 (List of Hashes) as it provides better flexibility and independent key verification, despite requiring a schema change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
pkg/connectorbuilder/connectorbuilder.go(2 hunks)pkg/crypto/crypto.go(1 hunks)pkg/crypto/crypto_test.go(3 hunks)pkg/crypto/providers/jwk/ed25519.go(2 hunks)pkg/crypto/providers/jwk/jwk.go(2 hunks)pkg/crypto/providers/jwk/rsa.go(2 hunks)pkg/crypto/providers/registry.go(3 hunks)
🧰 Additional context used
🪛 GitHub Check: go-lint
pkg/crypto/providers/jwk/jwk.go
[failure] 139-139:
unnecessary trailing newline (whitespace)
pkg/crypto/crypto_test.go
[failure] 109-109:
unnecessary trailing newline (whitespace)
pkg/crypto/crypto.go
[failure] 44-44:
Comment should end in a period (godot)
🪛 golangci-lint (1.62.2)
pkg/crypto/crypto_test.go
109-109: unnecessary trailing newline
(whitespace)
pkg/crypto/crypto.go
44-44: Comment should end in a period
(godot)
🔇 Additional comments (6)
pkg/crypto/providers/jwk/ed25519.go (2)
Line range hint 12-23: LGTM! Good separation of concerns.
The extraction of recipient creation logic into a separate function improves code modularity and reusability, which is essential for the multi-recipient encryption feature.
Line range hint 25-35: Verify error propagation in multi-recipient scenario.
The function correctly uses the new CreateED25519Recipient and passes a slice of recipients to ageEncrypt. However, let's verify the error handling in the multi-recipient context.
✅ Verification successful
Let me check the implementation of ageEncrypt to ensure it properly handles the multi-recipient scenario internally.
Let me try a different approach to get the implementation of ageEncrypt.
Error handling in multi-recipient scenario is properly implemented
The error handling in the multi-recipient scenario is correctly implemented. The ageEncrypt function:
- Properly accepts a slice of recipients and passes them to
age.Encryptusing variadic expansion - Has comprehensive error handling for encryption, writing, and closing operations
- Wraps errors with appropriate context using
fmt.Errorf
The error propagation in EncryptED25519 follows the same pattern as other callers, with consistent error wrapping using the appropriate provider prefix.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling patterns in ageEncrypt usage
# Look for other calls to ageEncrypt to ensure consistent error handling
rg -A 3 "ageEncrypt\(" --type go
Length of output: 1130
Script:
#!/bin/bash
# Get the full implementation of ageEncrypt function
ast-grep --pattern 'func ageEncrypt($$$) {
$$$
}'
Length of output: 53
Script:
#!/bin/bash
# Get the implementation using ripgrep with context
rg -U "func ageEncrypt" -A 15 --type go
Length of output: 1047
pkg/crypto/crypto_test.go (2)
64-66: LGTM! Good verification of single ciphertext for multiple recipients.
The test correctly verifies that only one ciphertext is returned for multiple recipients, which aligns with the PR objective of returning a single copy of encrypted data for multiple keys.
103-107: LGTM! Good negative test case.
The test properly verifies that decryption fails with an invalid key, which is essential for security validation.
pkg/connectorbuilder/connectorbuilder.go (2)
Line range hint 773-777: LGTM! Consistent context propagation.
The addition of context to NewEncryptionManager is consistent with the multi-recipient encryption changes and maintains proper error handling.
Line range hint 825-829: LGTM! Consistent implementation across methods.
The changes mirror those in RotateCredential, maintaining consistency in how the encryption manager is initialized and used.
pkg/crypto/providers/jwk/jwk.go
Outdated
| KeyId: "", // MJP what does "keyID" mean for multi-recipient encryption? Multiple ids? | ||
| Name: plainText.Name, | ||
| Description: plainText.Description, | ||
| Schema: plainText.Schema, | ||
| EncryptedBytes: []byte(encCipherText), |
There was a problem hiding this comment.
Define KeyId for multi-recipient encryption.
The KeyId field is currently left empty with a comment questioning its value in multi-recipient scenarios. It's important to define how KeyId should be represented when multiple keys are involved—options include concatenating key IDs, using a list, or adopting a new identification method.
Would you like assistance in proposing a method to represent KeyId in multi-recipient encryption?
| func EncryptRSA(pubKey *rsa.PublicKey, plaintext []byte) ([]byte, error) { | ||
| recipient, err := CreateRSARecipient(pubKey) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| ciphertext, err := ageEncrypt([]age.Recipient{recipient}, plaintext) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("jwk-rsa: %w", err) | ||
| } |
There was a problem hiding this comment.
Check for nil recipient before encryption.
Ensure that the recipient is not nil before proceeding with encryption to prevent potential runtime panics.
Apply this diff to add a nil check:
func EncryptRSA(pubKey *rsa.PublicKey, plaintext []byte) ([]byte, error) {
recipient, err := CreateRSARecipient(pubKey)
if err != nil {
return nil, err
}
+ if recipient == nil {
+ return nil, fmt.Errorf("jwk-rsa: recipient is nil")
+ }
ciphertext, err := ageEncrypt([]age.Recipient{recipient}, plaintext)
if err != nil {
return nil, fmt.Errorf("jwk-rsa: %w", err)
}
return ciphertext, nil
}📝 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.
| func EncryptRSA(pubKey *rsa.PublicKey, plaintext []byte) ([]byte, error) { | |
| recipient, err := CreateRSARecipient(pubKey) | |
| if err != nil { | |
| return nil, err | |
| } | |
| ciphertext, err := ageEncrypt([]age.Recipient{recipient}, plaintext) | |
| if err != nil { | |
| return nil, fmt.Errorf("jwk-rsa: %w", err) | |
| } | |
| func EncryptRSA(pubKey *rsa.PublicKey, plaintext []byte) ([]byte, error) { | |
| recipient, err := CreateRSARecipient(pubKey) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if recipient == nil { | |
| return nil, fmt.Errorf("jwk-rsa: recipient is nil") | |
| } | |
| ciphertext, err := ageEncrypt([]age.Recipient{recipient}, plaintext) | |
| if err != nil { | |
| return nil, fmt.Errorf("jwk-rsa: %w", err) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
pkg/crypto/crypto_test.go (2)
40-40: Typo in function nameThere's a typo in the test function name
TestMultiRecipientEncrypton; it should beTestMultiRecipientEncryption.Apply this diff to correct the typo:
-func TestMultiRecipientEncrypton(t *testing.T) { +func TestMultiRecipientEncryption(t *testing.T) {
75-102: Refactor repetitive decryption and assertion blocksThe decryption and assertion code for each key is repeated multiple times. Consider creating a helper function to reduce code duplication and improve readability.
Apply this diff to refactor:
+func assertDecryption(t *testing.T, provider providers.EncryptionProvider, cipherText *v2.EncryptedData, key []byte, plainText *v2.PlaintextData) { + decryptedText, err := provider.Decrypt(context.Background(), cipherText, key) + require.NoError(t, err) + require.Equal(t, plainText.Name, decryptedText.Name) + require.Equal(t, plainText.Description, decryptedText.Description) + require.Equal(t, plainText.Schema, decryptedText.Schema) + require.Equal(t, plainText.Bytes, decryptedText.Bytes) +} ... - decryptedText, err := provider.Decrypt(ctx, cipherText, key1) - require.NoError(t, err) - // Assertions... + assertDecryption(t, provider, cipherText, key1, plainText) - decryptedText, err = provider.Decrypt(ctx, cipherText, key2) - require.NoError(t, err) - // Assertions... + assertDecryption(t, provider, cipherText, key2, plainText) - decryptedText, err = provider.Decrypt(ctx, cipherText, key3) - require.NoError(t, err) - // Assertions... + assertDecryption(t, provider, cipherText, key3, plainText) - decryptedText, err = provider.Decrypt(ctx, cipherText, key4) - require.NoError(t, err) - // Assertions... + assertDecryption(t, provider, cipherText, key4, plainText)pkg/crypto/crypto.go (1)
26-43: Enhance error handling inEncryptmethodCurrently, if encryption fails for one provider, the entire encryption process is halted. Consider collecting errors and proceeding with other providers to ensure maximum encryption coverage.
Implement error aggregation:
+var allErrors []error ... if err != nil { - return nil, err + allErrors = append(allErrors, err) + continue } ... if len(allErrors) > 0 { return encryptedDatas, fmt.Errorf("encryption errors: %v", allErrors) }pkg/crypto/providers/jwk/jwk.go (1)
75-139: Improve handling of different key types in encryptionThe current implementation handles ECDSA keys separately and creates individual
EncryptedDataentries. For consistency, consider processing all key types uniformly, appending their recipients, and generating a singleEncryptedDataobject.This approach simplifies the encryption logic and maintains consistency across different key types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
pkg/crypto/crypto.go(1 hunks)pkg/crypto/crypto_test.go(3 hunks)pkg/crypto/providers/jwk/jwk.go(2 hunks)
🔇 Additional comments (1)
pkg/crypto/providers/jwk/jwk.go (1)
132-136: Define KeyId for multi-recipient encryption
The KeyId is empty with a comment questioning its meaning in multi-recipient scenarios. It's essential to define how to represent KeyId when multiple keys are involved, such as concatenating IDs or using a list.
pkg/crypto/providers/jwk/jwk.go
Outdated
|
|
||
| encrypted = append(encrypted, &v2.EncryptedData{ | ||
| Provider: EncryptionProviderJwk, | ||
| KeyId: "", // MJP what does "keyID" mean for multi-recipient encryption? Multiple ids? |
There was a problem hiding this comment.
I'm pretty sure you want to append to encrypted in the loop and set KeyId to config.KeyId. That way cone or whatever can look through the response, find the key ID that it knows, and decrypt that one to provide credentials to the user.
Edit: Oh, I see this is only for non-ecdsa pubkey. I think you should append to encrypted in the switch statements, using the appropriate config key ID like is done with ecdsa pubkeys. That will probably need a new thumbprint function that takes something other than a jwk. Maybe @morgabra remembers more about how we wanted to structure this.
There was a problem hiding this comment.
Thanks for taking a look Geoff, yeah ecdsa keys aren't supported by the multi-recipient age library we're using (NOTE TO SELF, double check that) so we handle them like we used to. For everything else though, we can actually create just a single EncryptedData package which any key can unlock, this way we don't create (secret * keys) many encrypted blocks, but instead just one that shares keys. My question here is how I should identify said package via KeyID.
Cone currently doesn't actually filter out based on KeyID (yet, I'm actually working on that next) so we can decide now what form a "multi-key KeyID" should take for our purposes...
A trivial answer could be something like all the KeyIDs we would have, just with some delimiter. meaning we'd have to just parse them out later.
e.g. "d0cfc64";"79ee7b51b03813b408c";"82f0a1832df2b3ec179f3b";"b47c6b3c9071b724" or what have you.
That's probably my preferred route as opposed to some weird hash of all keys (which limits our ability to ever get a matching hash again) or making EncryptedData store a list of strings which might be more "correct" actually... 🤔
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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 (3)
pb/c1/connector/v2/resource.pb.validate.go(0 hunks)pkg/crypto/providers/jwk/jwk.go(2 hunks)proto/c1/connector/v2/resource.proto(1 hunks)
💤 Files with no reviewable changes (1)
- pb/c1/connector/v2/resource.pb.validate.go
🔇 Additional comments (3)
pkg/crypto/providers/jwk/jwk.go (2)
75-78: LGTM: Proper slice pre-allocation
Good practice to pre-allocate slices with known capacity for better performance.
196-198: LGTM: Correct implementation of multi-recipient encryption
The function properly utilizes age.Encrypt's variadic argument feature for multiple recipients.
proto/c1/connector/v2/resource.proto (1)
171-171: LGTM: Proto change supports multi-recipient encryption
The change from string key_id to repeated string key_id properly supports the multi-recipient encryption feature while maintaining backward compatibility.
Let's verify the impact of this proto change:
✅ Verification successful
The initial results show two different GetKeyId() methods - one returning []string and another returning string. We need to verify the usage patterns and potential impacts. Let's gather more context about the usage of EncryptedData and related proto definitions.
LGTM: Change is properly implemented and used throughout the codebase
The code analysis confirms that the change from string key_id to repeated string key_id in EncryptedData message is correctly implemented and used consistently throughout the codebase:
- The JWK encryption provider correctly handles multiple key IDs in
pkg/crypto/providers/jwk/jwk.go - All generated code in
pb/c1/connector/v2/resource.pb.goproperly reflects the repeated field - No direct string assignments to
key_idwere found, all usages treat it as a slice - The change maintains backward compatibility as mentioned in the original review
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any direct field access that might break with the change from string to repeated string
# Search for direct key_id field access
rg -A 2 "key_id\s*[=:]" --type go
# Search for potential type assertions or conversions
rg -A 2 "KeyId\s*\(" --type go
# Look for tests that might need updating
rg -A 2 "key_id.*test" --type go
Length of output: 467
Script:
#!/bin/bash
# Let's check for any references to EncryptedData and its key_id field in the codebase
rg -A 5 "EncryptedData" --type go
# Also check for any proto definitions that might be affected
fd "resource.proto" --exec cat {}
Length of output: 31380
pkg/crypto/providers/jwk/jwk.go
Outdated
|
|
||
| encrypted = append(encrypted, &v2.EncryptedData{ | ||
| Provider: EncryptionProviderJwk, | ||
| KeyId: multiRecipientThumbs, |
There was a problem hiding this comment.
Fix undefined variable usage
The variable multiRecipientThumbs is undefined. It should be recipientThumbs based on the slice created earlier.
Apply this diff:
- KeyId: multiRecipientThumbs,
+ KeyId: recipientThumbs,📝 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.
| KeyId: multiRecipientThumbs, | |
| KeyId: recipientThumbs, |
There was a problem hiding this comment.
Thank you robot 🤖
There was a problem hiding this comment.
@mj-palanker You're welcome! Glad I could help.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
pkg/crypto/providers/jwk/jwk.go
Outdated
| for _, config := range configs { | ||
| jwk, err := unmarshalJWK(config.GetJwkPublicKeyConfig().GetPubKey()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| case *rsa.PublicKey: | ||
| ciphertext, err = EncryptRSA(pubKey, plainText.Bytes) | ||
| tp, err := thumbprint(jwk) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| default: | ||
| return nil, JWKUnsupportedKeyTypeError | ||
| } | ||
|
|
||
| tp, err := thumbprint(jwk) | ||
| if err != nil { | ||
| return nil, err | ||
| switch pubKey := jwk.Public().Key.(type) { | ||
| case ed25519.PublicKey: | ||
| recipientThumbs = append(recipientThumbs, tp) | ||
| recipient, err := CreateED25519Recipient(pubKey) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| recipients = append(recipients, recipient) | ||
| case *rsa.PublicKey: | ||
| recipientThumbs = append(recipientThumbs, tp) | ||
| recipient, err := CreateRSARecipient(pubKey) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| recipients = append(recipients, recipient) | ||
| case *ecdsa.PublicKey: | ||
| ciphertext, err := EncryptECDSA(pubKey, plainText.Bytes) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| encCipherText := base64.StdEncoding.EncodeToString(ciphertext) | ||
| encrypted = append(encrypted, &v2.EncryptedData{ | ||
| Provider: EncryptionProviderJwk, | ||
| KeyId: []string{tp}, | ||
| Name: plainText.Name, | ||
| Description: plainText.Description, | ||
| Schema: plainText.Schema, | ||
| EncryptedBytes: []byte(encCipherText), | ||
| }) | ||
|
|
||
| default: | ||
| return nil, JWKUnsupportedKeyTypeError | ||
| } |
There was a problem hiding this comment.
Add validation for empty configs slice
The function should validate that the configs slice is not empty before proceeding.
Apply this diff at the beginning of the function:
func (j *JWKEncryptionProvider) Encrypt(ctx context.Context, configs []*v2.EncryptionConfig, plainText *v2.PlaintextData) ([]*v2.EncryptedData, error) {
+ if len(configs) == 0 {
+ return nil, fmt.Errorf("jwk: no encryption configs provided")
+ }
recipients := make([]age.Recipient, 0, len(configs))📝 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.
| for _, config := range configs { | |
| jwk, err := unmarshalJWK(config.GetJwkPublicKeyConfig().GetPubKey()) | |
| if err != nil { | |
| return nil, err | |
| } | |
| case *rsa.PublicKey: | |
| ciphertext, err = EncryptRSA(pubKey, plainText.Bytes) | |
| tp, err := thumbprint(jwk) | |
| if err != nil { | |
| return nil, err | |
| } | |
| default: | |
| return nil, JWKUnsupportedKeyTypeError | |
| } | |
| tp, err := thumbprint(jwk) | |
| if err != nil { | |
| return nil, err | |
| switch pubKey := jwk.Public().Key.(type) { | |
| case ed25519.PublicKey: | |
| recipientThumbs = append(recipientThumbs, tp) | |
| recipient, err := CreateED25519Recipient(pubKey) | |
| if err != nil { | |
| return nil, err | |
| } | |
| recipients = append(recipients, recipient) | |
| case *rsa.PublicKey: | |
| recipientThumbs = append(recipientThumbs, tp) | |
| recipient, err := CreateRSARecipient(pubKey) | |
| if err != nil { | |
| return nil, err | |
| } | |
| recipients = append(recipients, recipient) | |
| case *ecdsa.PublicKey: | |
| ciphertext, err := EncryptECDSA(pubKey, plainText.Bytes) | |
| if err != nil { | |
| return nil, err | |
| } | |
| encCipherText := base64.StdEncoding.EncodeToString(ciphertext) | |
| encrypted = append(encrypted, &v2.EncryptedData{ | |
| Provider: EncryptionProviderJwk, | |
| KeyId: []string{tp}, | |
| Name: plainText.Name, | |
| Description: plainText.Description, | |
| Schema: plainText.Schema, | |
| EncryptedBytes: []byte(encCipherText), | |
| }) | |
| default: | |
| return nil, JWKUnsupportedKeyTypeError | |
| } | |
| func (j *JWKEncryptionProvider) Encrypt(ctx context.Context, configs []*v2.EncryptionConfig, plainText *v2.PlaintextData) ([]*v2.EncryptedData, error) { | |
| if len(configs) == 0 { | |
| return nil, fmt.Errorf("jwk: no encryption configs provided") | |
| } | |
| for _, config := range configs { | |
| jwk, err := unmarshalJWK(config.GetJwkPublicKeyConfig().GetPubKey()) | |
| if err != nil { | |
| return nil, err | |
| } | |
| tp, err := thumbprint(jwk) | |
| if err != nil { | |
| return nil, err | |
| } | |
| switch pubKey := jwk.Public().Key.(type) { | |
| case ed25519.PublicKey: | |
| recipientThumbs = append(recipientThumbs, tp) | |
| recipient, err := CreateED25519Recipient(pubKey) | |
| if err != nil { | |
| return nil, err | |
| } | |
| recipients = append(recipients, recipient) | |
| case *rsa.PublicKey: | |
| recipientThumbs = append(recipientThumbs, tp) | |
| recipient, err := CreateRSARecipient(pubKey) | |
| if err != nil { | |
| return nil, err | |
| } | |
| recipients = append(recipients, recipient) | |
| case *ecdsa.PublicKey: | |
| ciphertext, err := EncryptECDSA(pubKey, plainText.Bytes) | |
| if err != nil { | |
| return nil, err | |
| } | |
| encCipherText := base64.StdEncoding.EncodeToString(ciphertext) | |
| encrypted = append(encrypted, &v2.EncryptedData{ | |
| Provider: EncryptionProviderJwk, | |
| KeyId: []string{tp}, | |
| Name: plainText.Name, | |
| Description: plainText.Description, | |
| Schema: plainText.Schema, | |
| EncryptedBytes: []byte(encCipherText), | |
| }) | |
| default: | |
| return nil, JWKUnsupportedKeyTypeError | |
| } |
mj-palanker
force-pushed
the
mjp/age-multi-recipient
branch
from
c8a7a9a to
3ab9490
Compare
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.
mj-palanker
force-pushed
the
mjp/age-multi-recipient
branch
from
3ab9490 to
5a9e19d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/crypto/providers/jwk/jwk.go (1)
86-131: Refactor duplicate thumbprint calculation logicThe thumbprint calculation is repeated for each key type. Consider extracting this into a helper function to reduce code duplication.
+func getThumbprintAndAppend(jwk *jose.JSONWebKey, thumbs *[]string) error { + tp, err := thumbprint(jwk) + if err != nil { + return err + } + *thumbs = append(*thumbs, tp) + return nil +}Consider unifying ECDSA handling with other key types
ECDSA keys are handled differently from ED25519 and RSA keys, bypassing the multi-recipient encryption. This creates two different paths for encryption which could lead to maintenance issues. Consider implementing multi-recipient support for ECDSA keys as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/connectorbuilder/connectorbuilder.go(2 hunks)pkg/crypto/providers/jwk/jwk.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/connectorbuilder/connectorbuilder.go
🔇 Additional comments (3)
pkg/crypto/providers/jwk/jwk.go (3)
75-78: Add validation for empty configs slice
The function should validate that the configs slice is not empty before proceeding with the encryption process.
Line range hint 206-219: LGTM! Well-structured error handling
The updated ageEncrypt function properly handles multiple recipients and includes comprehensive error handling for each operation.
75-151: Verify security implications of multi-recipient encryption
Please ensure:
- The security level is not reduced when encrypting for multiple recipients
- The KeyIds list doesn't leak sensitive information
- The encryption remains secure even if some recipients' keys are compromised
Let's check for any security-related tests:
✅ Verification successful
Let's examine the test file to understand the security implications better.
Let's search for more implementation details to verify the security implications.
Let's check the actual implementation of ageEncrypt and the encryption methods being used.
Let's check the ECDSA implementation and any security-related comments in the code.
Based on the comprehensive analysis of the implementation, I can now provide the final response:
Multi-recipient encryption implementation is secure
The implementation is verified to be secure because:
- For RSA and ED25519 keys, it uses the age encryption library which implements the age specification with ChaCha20-Poly1305 for payload encryption
- For ECDSA keys, it uses ECDH-ES with AES-GCM via the go-jose library
- The KeyIds only contain public key thumbprints, which are safe to share
- Each recipient gets an independent encryption of the data, maintaining security even if some recipients are compromised
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for security-related tests
rg -l "TestEncrypt.*Security|TestMultiRecipient" --type go
Length of output: 86
Script:
#!/bin/bash
# Check the content of the test file
rg "TestEncrypt.*Security|TestMultiRecipient" -A 10 -B 2 pkg/crypto/crypto_test.go
# Also search for any other relevant test cases
rg "Test.*Encrypt.*" -A 10 -B 2 pkg/crypto/providers/jwk/jwk_test.go
Length of output: 637
Script:
#!/bin/bash
# Check the complete multi-recipient test implementation
rg "TestMultiRecipientEncrypton" -A 30 pkg/crypto/crypto_test.go
# Look for age encryption implementation
ast-grep --pattern 'func ageEncrypt($$$) {
$$$
}'
# Search for any security-related comments or documentation
rg -i "security|encrypt" README.md
Length of output: 1182
Script:
#!/bin/bash
# Find the ageEncrypt implementation
rg "func ageEncrypt" -A 15 -B 2
# Look for the age encryption package imports and usage
rg "filippo.io/age" -A 5 -B 5
# Check the RSA and ECDSA encryption implementations
ast-grep --pattern 'func CreateRSARecipient($$$) {
$$$
}'
ast-grep --pattern 'func EncryptECDSA($$$) {
$$$
}'
Length of output: 13015
Script:
#!/bin/bash
# Search for ECDSA encryption implementation
rg "EncryptECDSA" -A 15 -B 2
# Look for any security-related comments in the JWK provider
rg -i "security|encrypt" pkg/crypto/providers/jwk/
# Check the age encryption package's security properties
rg "ChaCha20|Poly1305|X25519" vendor/filippo.io/age/
Length of output: 11516
| if len(recipients) > 0 { | ||
| ciphertext, err := ageEncrypt(recipients, plainText.Bytes) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("jwk: %w", err) | ||
| } | ||
| encCipherText := base64.StdEncoding.EncodeToString(ciphertext) | ||
|
|
||
| encrypted = append(encrypted, &v2.EncryptedData{ | ||
| Provider: EncryptionProviderJwk, | ||
| KeyId: recipientThumbs[0], // MJP remove me once we've depricated fully | ||
| Name: plainText.Name, | ||
| Description: plainText.Description, | ||
| Schema: plainText.Schema, | ||
| EncryptedBytes: []byte(encCipherText), | ||
| KeyIds: recipientThumbs, | ||
| }) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Deprecation plan needed for KeyId field
The field is used in two main contexts:
- In the protobuf definition (already marked as deprecated)
- In pkg/crypto/providers/jwk/jwk.go where it's set alongside KeyIds for backward compatibility
Action items:
- Add a deprecation notice in code documentation
- Ensure all new code uses KeyIds array instead of KeyId
- Plan to remove KeyId in next major version as it's only used for backward compatibility
🔗 Analysis chain
Plan KeyId deprecation
The code uses KeyId field with a comment indicating future deprecation. Consider:
- Adding a deprecation timeline
- Documenting the migration path to using
KeyIds - Adding a deprecation warning when
KeyIdis used
Let's check for other usages of KeyId in the codebase:
Let's search for actual usages of KeyId in our codebase, excluding vendor directories and focusing on the specific struct field:
Let's check for actual usage of the KeyId field in the code to understand the impact:
Let's check for any code that might be reading or processing this field to understand the full deprecation impact:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for KeyId usages to assess impact of deprecation
rg -l "KeyId" --type go
Length of output: 2101
Script:
#!/bin/bash
# Search for KeyId field usage in non-vendor code
rg "KeyId\s+" --type go -g '!vendor/**' -A 2 -B 2
Length of output: 1140
Script:
#!/bin/bash
# Search for actual usage of KeyId in non-vendor code
rg "\.KeyId" --type go -g '!vendor/**' -A 2 -B 2
# Also check for protobuf definition
cat pb/c1/connector/v2/resource.proto
Length of output: 950
Script:
#!/bin/bash
# Search for any code handling or processing KeyId
rg -g '!vendor/**' -g '!*.pb.go' "KeyId" --type go -A 3 -B 3
# Also check for any tests that might be using this field
rg -g '!vendor/**' "KeyId" --type go -g '*_test.go' -A 3 -B 3
Length of output: 3138
Modifies crypto and jwk encryption provider to encrypt the same secret with multiple keys in the multi-recipient setup. Allows us to only return a single copy of the encrypted data when asked to encrypt it for many keys
Open Question, for a single key encryption we store a hash of the public key under the "keyId" field of the returned data... what should we do for a multi-key encryption? should we hash all keys together somehow? store a list of hashes? How are we even using this field in c1/baton?
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation