Stricter validation of ID in ocr signed responses#23003
Conversation
product-security-plaid-production
Bot
requested review from
ilija42,
kalverra and
pavel-raykov
CORA - Pending Reviewers
Legend: ✅ Approved | ❌ Changes Requested | 💬 Commented | 🚫 Dismissed | ⏳ Pending | ❓ Unknown For more details, see the full review summary. |
|
I see you updated files related to
|
|
✅ No conflicts with other open PRs targeting |
| if err == nil { | ||
| return currResp, nil | ||
| if a.signedResponseRequestIDEnabled(ctx, l) { | ||
| if methodSupportsSignedOCRValidation(currResp.Method) { |
There was a problem hiding this comment.
I think this condition should be at top level.
For supported methods, only allow signature validation.
For unsupported methods, only allow quorum based validation.
There was a problem hiding this comment.
Especially for unsupported methods like GetPublicKey, we should ONLY rely on quorum, not a signed ocr response.
There was a problem hiding this comment.
This would be a change in logic from what we currently have so keeping with our new tenant of putting everything behind a feature flag I'd rather leave this here behind the feature flag
| return currResp, nil | ||
| } | ||
|
|
||
| l.Debugw("failed to validate signatures, falling back to quorum aggregation", "error", err) |
There was a problem hiding this comment.
For CRUD operations, we may need a more sophisticated strategy.
Valid responses can be of 2 types:
- valid signed ocr response
- OR an error(auth/validation failure) without ocr.
For 1st category, we should only trust a valid ocr response. If we see a ocr response but invalid, then throw it away and wait for more responses.
For 2nd, we should wait for quorum, unless we see another signed ocr response.
There was a problem hiding this comment.
Lets break this out into it's own PR to keep this one smaller
| // gateway should start rejecting responses with a missing requestId. | ||
| // https://smartcontract-it.atlassian.net/browse/CRE-4875 | ||
| if payloadRequestID != "" && payloadRequestID != requestID { | ||
| l.Errorw("signed payload request id mismatch, discarding response", "requestID", requestID, "signedPayloadRequestID", payloadRequestID, "method", resp.Method) |
There was a problem hiding this comment.
can we make this a critical error, since this should never happen in a normal use-case?
|
2 participants
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.