vault: validate encrypted value size in request validator#21760
vault: validate encrypted value size in request validator#21760prashantkumar1982 wants to merge 2 commits intorelease/2.40.0from
Conversation
(cherry picked from commit bcc73ae)
|
✅ No conflicts with other open PRs targeting |
|
I see you updated files related to
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06563543e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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.
NewCapability now creates a ciphertextLimiter, but Capability.Close() still only closes MaxRequestBatchSizeLimiter; this leaves the new limiter open for the lifetime of each start/stop cycle. In deployments where limiters maintain background state (tenant maps/watchers), repeated capability restarts will accumulate leaked resources and stale limit subscriptions. Please close MaxCiphertextLengthLimiter alongside the existing limiter in Close().
Useful? React with 👍 / 👎.
| 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.
NewHandler now allocates a ciphertextLimiter, but handler.Close() still only closes writeMethodsEnabled and MaxRequestBatchSizeLimiter. That means every handler restart can leak the newly added limiter instance and any associated internal state, which is especially problematic for long-running nodes that reload handlers. Add MaxCiphertextLengthLimiter.Close() to the shutdown join.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26a1afb6f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ciphertextLimiter, err := limits.MakeUpperBoundLimiter(limitsFactory, cresettings.Default.VaultCiphertextSizeLimit) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create ciphertext size limiter: %w", err) |
There was a problem hiding this comment.
When MakeUpperBoundLimiter fails here, NewCapability returns without closing the already-created request batch limiter. Since these limiters are closable and may hold settings/watcher state, repeated startup retries can accumulate leaked resources. Close limiter on this error path before returning.
Useful? React with 👍 / 👎.
| ciphertextLimiter, err := limits.MakeUpperBoundLimiter(limitsFactory, cresettings.Default.VaultCiphertextSizeLimit) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create ciphertext size limiter: %w", err) |
There was a problem hiding this comment.
This early return leaks the request batch limiter created just above when ciphertext limiter construction errors. In environments that retry handler initialization, this can leave stale limiter resources subscribed across attempts. Close the previously allocated limiter before returning this error.
Useful? React with 👍 / 👎.
|
4 participants
Summary
VaultCiphertextSizeLimitin the Vault request validator for create/update requestsEncryptedValuepayloads before label verificationTesting