Stricter validation of ID in ocr signed responses#23003
Open
russell-stern wants to merge 1 commit into
Open
Conversation
product-security-plaid-production
Bot
requested review from
ilija42,
kalverra and
pavel-raykov
Contributor
CORA - Pending Reviewers
Legend: ✅ Approved | ❌ Changes Requested | 💬 Commented | 🚫 Dismissed | ⏳ Pending | ❓ Unknown For more details, see the full review summary. |
Contributor
|
I see you updated files related to
|
Contributor
|
✅ No conflicts with other open PRs targeting |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This change closes a security gap in the vault gateway aggregator where OCR signature validation did not bind a node response to the pending gateway request. JSON-RPC
idandSignedOCRResponsewrapper fields are node-controlled and outside the signed bytes, so they cannot be trusted for request correlation.The fix introduces an authoritative
request_idinside the signed OCR payload for vault create/update/delete/list responses, sets it on the vault OCR plugin side, and validates it in the gateway aggregator after signature verification.Depends on: chainlink-common#2143
Problem
vaulttypes.ValidateSignatureshashes onlySignedOCRResponse.Payload+Context. Previously, signed create/update/delete/list payloads contained no gateway request identifier. A node could return a signature-valid response for a different pending request, and the gateway had no way to detect the mismatch using trusted (signed) data.Solution
chainlink-common (dependency bump)
Bump
github.com/smartcontractkit/chainlink-commontov0.11.2-0.20260610205450-f1cc365ddb71, addingrequest_idto:CreateSecretsResponseUpdateSecretsResponseDeleteSecretsResponseListSecretIdentifiersResponseVault OCR plugin (
plugin.go)When generating JSON OCR reports for create/update/delete/list, clone the outcome response and set
RequestId = o.Idbefore canonical JSON signing.o.Idis the gateway-prefixed pending queue ID (authorizedOwner::userRequestID).Gateway aggregator (
aggregator.go)vault.secrets.create,vault.secrets.update,vault.secrets.delete, andvault.secrets.list.vault.publicKey.getuses quorum only — it does not go through OCR signed-response validation.ValidateSignatures, unmarshal the signed payload and compare embeddedrequestIdto the pending gateway request ID (ar.req.ID, also owner-prefixed).requestIdis present and mismatched (log +gateway_vault_request_internal_errormetric withsigned_payload_request_id_mismatch).requestIdduring rolling vault node upgrades; comment notes this should be tightened once all vault nodes are upgraded.resp.ID/ wrapper fields for request binding.Handler wiring (
handler.go)Pass pending
ar.req.IDintoAggregate()so signature validation compares against the gateway-internal prefixed request ID.Tests
GetPublicKeyskipping signature validation.requestIdembedded in canonical JSON payload.requestIdin signed JSON.requestIdmatchesauthorizedOwner::userRequestID(not just JSON-RPC responseid).Upgrade / rollout notes
requestIdis tolerated until vault nodes pick up the OCR plugin change.requestId.GetPublicKeyintentionally remains quorum-based because that path does not use OCR signed reports.