Bind vault signed OCR responses to gateway request ID via signed payload#22799
Bind vault signed OCR responses to gateway request ID via signed payload#22799prashantkumar1982 wants to merge 10 commits into
Conversation
|
✅ No conflicts with other open PRs targeting |
prashantkumar1982
changed the title
|
I see you updated files related to
|
|
|
product-security-plaid-production
Bot
requested review from
ChrisAmora,
MStreet3 and
patrickhuie19
|
|
||
| func (a *baseAggregator) validateUsingSignatures(don capabilities.DON, nodes []capabilities.Node, resp *jsonrpc.Response[json.RawMessage]) (*jsonrpc.Response[json.RawMessage], error) { | ||
| func (a *baseAggregator) recordSignedPayloadRequestIDMismatch(ctx context.Context) { | ||
| if a.metrics == nil { |
There was a problem hiding this comment.
AI smell :( Can we remove this? metrics should not be nil in production, I think we should try to enforce that invariant
| rep, err := r.generateJSONReport(o.Id, o.RequestType, o.GetCreateSecretsResponse()) | ||
| createResp := proto.Clone(o.GetCreateSecretsResponse()).(*vaultcommon.CreateSecretsResponse) | ||
| createResp.RequestId = o.Id | ||
| rep, err := r.generateJSONReport(o.Id, o.RequestType, createResp) |
There was a problem hiding this comment.
@prashantkumar1982 What's the plan for rolling this out? Don't we need an atomic feature flag here otherwise nodes will generate different outputs?
There was a problem hiding this comment.
Since these are just direct Gateway -> Vault DON calls, they don't go through don2don.
And since these are ocr signed, the gateway doesn't need quorum. Just any signed valid response is ok.
So I figured no need of flags here.
There was a problem hiding this comment.
we decided to add gate
| } | ||
|
|
||
| func sendVaultSignedOCRRequestToGateway(t *testing.T, gatewayURL string, jsonRequest jsonrpc.Request[json.RawMessage]) jsonrpc.Response[vaulttypes.SignedOCRResponse] { | ||
| func signedPayloadRequestIDFromPayload(t *testing.T, method string, payload json.RawMessage) string { |
There was a problem hiding this comment.
This looks pretty similar to the function introduced in aggregator
Is it worth having function to marshal/unmarshal messages based on the method name as a utility? This switch pattern comes back a lot in relation to unmarshaling
There was a problem hiding this comment.
I moved it to a util function.
I didn't yet do the marshal/unmarshal, because it will affect much more files, better to do separately.
Plus not sure how the refactor will look like.
CORA - Pending Reviewers
Legend: ✅ Approved | ❌ Changes Requested | 💬 Commented | 🚫 Dismissed | ⏳ Pending | ❓ Unknown For more details, see the full review summary. |
… into fix/vault-signature-payload-request-id-validation
… into fix/vault-signature-payload-request-id-validation
|
3 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.