[PRIV-413/414/415] Misc improvements to the Vault#21458
[PRIV-413/414/415] Misc improvements to the Vault#21458cedric-cordenier wants to merge 3 commits intodevelopfrom
Conversation
product-security-plaid-production
bot
requested review from
agparadiso and
skudasov
|
👋 cedric-cordenier, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
cedric-cordenier
changed the title
|
I see you updated files related to
|
|
✅ No conflicts with other open PRs targeting |
There was a problem hiding this comment.
Pull request overview
Risk Rating: HIGH (changes touch Vault OCR reporting plugin validation/encryption logic and update shared dependency versions across multiple modules)
Updates Vault OCR plugin limits/validation and associated tests, alongside bumping chainlink-common across the repo’s Go modules.
Changes:
- Bump
github.com/smartcontractkit/chainlink-commontov0.10.1-0.20260309113338-432602d809ccacross root, deployment, scripts, and test modules. - Add Vault plugin enforcement for max request batch size and max encrypted share length, plus pending-queue observation size caps.
- Expand Vault plugin test coverage for ciphertext/label validation, batch limits, share sizing, and panic-safety cases.
Areas needing scrupulous human review:
core/services/ocr2/plugins/vault/plugin.go: new share encryption helper and the refactoredValidateObservationvalidation paths (batch limits, share-size checks, pending-queue size cap).core/services/ocr2/plugins/vault/plugin.go:stateTransitionGetSecretsshare aggregation behavior with the new share-size enforcement.
Suggested reviewers (per .github/CODEOWNERS):
- Vault OCR plugin changes (
/core/services/ocr*):@smartcontractkit/foundations@smartcontractkit/core - Dependency bumps & integration/system test modules:
@smartcontractkit/devex-tooling@smartcontractkit/core(and@smartcontractkit/foundationswhere applicable)
Reviewed changes
Copilot reviewed 10 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
core/services/ocr2/plugins/vault/plugin.go |
Adds new limiters/validation for share size + request batch size; refactors observation validation; adjusts share aggregation. |
core/services/ocr2/plugins/vault/plugin_test.go |
Adds extensive tests covering new validation and edge cases (batch limits, label validation, ciphertext size, panic safety). |
core/services/ocr2/plugins/vault/transmitter_test.go |
Updates test config construction to include the new max request batch size parameter. |
go.mod / go.sum |
Bumps chainlink-common version at repo root. |
deployment/go.mod / deployment/go.sum |
Bumps chainlink-common for deployment module. |
core/scripts/go.mod / core/scripts/go.sum |
Bumps chainlink-common for scripts module. |
integration-tests/go.mod / integration-tests/go.sum |
Bumps chainlink-common for integration tests module. |
integration-tests/load/go.mod / integration-tests/load/go.sum |
Bumps chainlink-common for load tests module. |
system-tests/lib/go.mod / system-tests/lib/go.sum |
Bumps chainlink-common for system-tests lib module. |
system-tests/tests/go.mod / system-tests/tests/go.sum |
Bumps chainlink-common for system-tests tests module. |
| return "", newUserError(fmt.Sprintf("invalid public key size: expected %d bytes, got %d bytes", curve25519.PointSize, len(publicKey))) | ||
| } | ||
| if secret == nil { | ||
| return nil, newUserError("key does not exist") | ||
|
|
||
| publicKeyLength := [curve25519.PointSize]byte(publicKey) | ||
| encrypted, err := box.SealAnonymous(nil, s.data, &publicKeyLength, rand.Reader) |
There was a problem hiding this comment.
publicKeyLength := [curve25519.PointSize]byte(publicKey) will not compile because publicKey is a []byte returned from hex.DecodeString. Convert by copying into a [curve25519.PointSize]byte (or use var pkArr [curve25519.PointSize]byte; copy(pkArr[:], publicKey)), then pass &pkArr to box.SealAnonymous.
- Validate the number of shares provided in an observation - Validate the max share size - Add a batch size check to validate observation - Defence in depth check to ensure Create/Update secrets have a label - Validate the max ciphertext size in ValidateObservation - Check against nil secret identifiers
|
| return nil | ||
| } | ||
|
|
||
| func (r *ReportingPlugin) validateListSecretIdentifiersObservation(o *vaultcommon.Observation) error { |
There was a problem hiding this comment.
We can also validate here that total responses are less than equal to the maxSecretsPerOwnerLimit right?
Requires
Supports