-
Notifications
You must be signed in to change notification settings - Fork 2k
vault: validate encrypted value size in request validator #21759
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| package vault | ||
|
|
||
| import ( | ||
| "encoding/hex" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
|
|
||
| vaultcommon "github.com/smartcontractkit/chainlink-common/pkg/capabilities/actions/vault" | ||
| pkgconfig "github.com/smartcontractkit/chainlink-common/pkg/config" | ||
| "github.com/smartcontractkit/chainlink-common/pkg/settings/limits" | ||
| ) | ||
|
|
||
| func TestRequestValidator_CiphertextSizeLimit(t *testing.T) { | ||
| validator := NewRequestValidator( | ||
| limits.NewUpperBoundLimiter(10), | ||
| limits.NewUpperBoundLimiter[pkgconfig.Size](10*pkgconfig.Byte), | ||
| ) | ||
|
|
||
| id := &vaultcommon.SecretIdentifier{ | ||
| Key: "key", | ||
| Namespace: "namespace", | ||
| Owner: "0x1111111111111111111111111111111111111111", | ||
| } | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| call func(*testing.T, *RequestValidator, string) error | ||
| value string | ||
| errSubstr string | ||
| }{ | ||
| { | ||
| name: "create accepts ciphertext at the limit", | ||
| call: func(t *testing.T, validator *RequestValidator, value string) error { | ||
| return validator.ValidateCreateSecretsRequest(nil, &vaultcommon.CreateSecretsRequest{ | ||
| RequestId: "request-id", | ||
| EncryptedSecrets: []*vaultcommon.EncryptedSecret{ | ||
| {Id: id, EncryptedValue: value}, | ||
| }, | ||
| }) | ||
| }, | ||
| value: hex.EncodeToString(make([]byte, 10)), | ||
| }, | ||
| { | ||
| name: "create rejects ciphertext above the limit", | ||
| call: func(t *testing.T, validator *RequestValidator, value string) error { | ||
| return validator.ValidateCreateSecretsRequest(nil, &vaultcommon.CreateSecretsRequest{ | ||
| RequestId: "request-id", | ||
| EncryptedSecrets: []*vaultcommon.EncryptedSecret{ | ||
| {Id: id, EncryptedValue: value}, | ||
| }, | ||
| }) | ||
| }, | ||
| value: hex.EncodeToString(make([]byte, 11)), | ||
| errSubstr: "ciphertext size exceeds maximum allowed size", | ||
| }, | ||
| { | ||
| name: "update accepts ciphertext at the limit", | ||
| call: func(t *testing.T, validator *RequestValidator, value string) error { | ||
| return validator.ValidateUpdateSecretsRequest(nil, &vaultcommon.UpdateSecretsRequest{ | ||
| RequestId: "request-id", | ||
| EncryptedSecrets: []*vaultcommon.EncryptedSecret{ | ||
| {Id: id, EncryptedValue: value}, | ||
| }, | ||
| }) | ||
| }, | ||
| value: hex.EncodeToString(make([]byte, 10)), | ||
| }, | ||
| { | ||
| name: "update rejects ciphertext above the limit", | ||
| call: func(t *testing.T, validator *RequestValidator, value string) error { | ||
| return validator.ValidateUpdateSecretsRequest(nil, &vaultcommon.UpdateSecretsRequest{ | ||
| RequestId: "request-id", | ||
| EncryptedSecrets: []*vaultcommon.EncryptedSecret{ | ||
| {Id: id, EncryptedValue: value}, | ||
| }, | ||
| }) | ||
| }, | ||
| value: hex.EncodeToString(make([]byte, 11)), | ||
| errSubstr: "ciphertext size exceeds maximum allowed size", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| err := tt.call(t, validator, tt.value) | ||
| if tt.errSubstr == "" { | ||
| require.NoError(t, err) | ||
| return | ||
| } | ||
|
|
||
| require.Error(t, err) | ||
| require.ErrorContains(t, err, tt.errSubstr) | ||
| }) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -197,6 +197,10 @@ func NewHandler(methodConfig json.RawMessage, donConfig *config.DONConfig, don g | |
| if err != nil { | ||
| return nil, fmt.Errorf("could not create request batch size limiter: %w", err) | ||
| } | ||
| ciphertextLimiter, err := limits.MakeUpperBoundLimiter(limitsFactory, cresettings.Default.VaultCiphertextSizeLimit) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Useful? React with 👍 / 👎. |
||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create ciphertext size limiter: %w", err) | ||
| } | ||
|
|
||
| writeMethodsEnabled, err := limits.MakeGateLimiter(limitsFactory, cresettings.Default.GatewayVaultManagementEnabled) | ||
| if err != nil { | ||
|
|
@@ -218,7 +222,7 @@ func NewHandler(methodConfig json.RawMessage, donConfig *config.DONConfig, don g | |
| metrics: metrics, | ||
| aggregator: &baseAggregator{capabilitiesRegistry: capabilitiesRegistry}, | ||
| clock: clock, | ||
| RequestValidator: vaultcap.NewRequestValidator(limiter), | ||
| RequestValidator: vaultcap.NewRequestValidator(limiter, ciphertextLimiter), | ||
| }, nil | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NewCapabilitynow allocates a second limiter for ciphertext size, but(*Capability).Closestill only closesMaxRequestBatchSizeLimiter. On nodes where this capability is restarted (process reloads, integration tests, hot reconfiguration), the unclosedMaxCiphertextLengthLimitercan leave behind limiter resources/goroutines and accumulate over time; the new limiter should be closed alongside the existing one.Useful? React with 👍 / 👎.