Skip to content

Stricter validation of ID in ocr signed responses #23003

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
russell-stern wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Stricter validation of ID in ocr signed responses #23003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
290bbe4
Stricter validation of ID in ocr signed responses
russell-stern Jul 1, 2026
5a7d8ba
Updated logging level
russell-stern Jul 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions core/capabilities/vault/vaultutils/json_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ func TestToCanonicalJSON_OmitsEmptyFields(t *testing.T) {
t.Parallel()

msg := &vaultcommon.CreateSecretsResponse{
RequestId: "owner::req-1",
Responses: []*vaultcommon.CreateSecretResponse{
{
Id: id,
Expand All @@ -63,12 +64,14 @@ func TestToCanonicalJSON_OmitsEmptyFields(t *testing.T) {
withEmptyFields, err := ToCanonicalJSON(msg, false)
require.NoError(t, err)
assert.JSONEq(t, `{
"requestId":"owner::req-1",
"responses":[{"id":{"owner":"owner","namespace":"main","key":"secret1"},"success":true,"error":""}]
}`, string(withEmptyFields))

canonicalJSON, err := ToCanonicalJSON(msg, true)
require.NoError(t, err)
assert.JSONEq(t, `{
"requestId":"owner::req-1",
"responses":[{"id":{"owner":"owner","namespace":"main","key":"secret1"},"success":true}]
}`, string(canonicalJSON))
assert.NotEqual(t, string(withEmptyFields), string(canonicalJSON))
Expand All @@ -83,7 +86,7 @@ func TestToCanonicalJSON_OmitsEmptyFields(t *testing.T) {

withEmptyFields, err := ToCanonicalJSON(msg, false)
require.NoError(t, err)
assert.JSONEq(t, `{"responses":[]}`, string(withEmptyFields))
assert.JSONEq(t, `{"responses":[],"requestId":""}`, string(withEmptyFields))

canonicalJSON, err := ToCanonicalJSON(msg, true)
require.NoError(t, err)
Expand All @@ -95,16 +98,17 @@ func TestToCanonicalJSON_OmitsEmptyFields(t *testing.T) {
t.Parallel()

msg := &vaultcommon.CreateSecretsResponse{
RequestId: "owner::req-1",
Responses: []*vaultcommon.CreateSecretResponse{},
}

withEmptyFields, err := ToCanonicalJSON(msg, false)
require.NoError(t, err)
assert.JSONEq(t, `{"responses":[]}`, string(withEmptyFields))
assert.JSONEq(t, `{"responses":[],"requestId":"owner::req-1"}`, string(withEmptyFields))

canonicalJSON, err := ToCanonicalJSON(msg, true)
require.NoError(t, err)
assert.JSONEq(t, `{}`, string(canonicalJSON))
assert.JSONEq(t, `{"requestId":"owner::req-1"}`, string(canonicalJSON))
assert.NotEqual(t, string(withEmptyFields), string(canonicalJSON))
})
}
49 changes: 49 additions & 0 deletions core/capabilities/vault/vaultutils/signed_payload_request_id.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package vaultutils

import (
"encoding/json"
"errors"
"fmt"

"google.golang.org/protobuf/encoding/protojson"

vaultcommon "github.com/smartcontractkit/chainlink-common/pkg/capabilities/actions/vault"
"github.com/smartcontractkit/chainlink/v2/core/capabilities/vault/vaulttypes"
)

// SignedPayloadRequestID extracts the requestId field from a vault OCR signed response payload.
func SignedPayloadRequestID(method string, payload json.RawMessage) (string, error) {
if len(payload) == 0 {
return "", errors.New("signed payload is empty")
}

unmarshal := protojson.UnmarshalOptions{DiscardUnknown: true}
switch method {
case vaulttypes.MethodSecretsCreate:
resp := &vaultcommon.CreateSecretsResponse{}
if err := unmarshal.Unmarshal(payload, resp); err != nil {
return "", fmt.Errorf("failed to unmarshal signed create secrets payload: %w", err)
}
return resp.RequestId, nil
case vaulttypes.MethodSecretsUpdate:
resp := &vaultcommon.UpdateSecretsResponse{}
if err := unmarshal.Unmarshal(payload, resp); err != nil {
return "", fmt.Errorf("failed to unmarshal signed update secrets payload: %w", err)
}
return resp.RequestId, nil
case vaulttypes.MethodSecretsDelete:
resp := &vaultcommon.DeleteSecretsResponse{}
if err := unmarshal.Unmarshal(payload, resp); err != nil {
return "", fmt.Errorf("failed to unmarshal signed delete secrets payload: %w", err)
}
return resp.RequestId, nil
case vaulttypes.MethodSecretsList:
resp := &vaultcommon.ListSecretIdentifiersResponse{}
if err := unmarshal.Unmarshal(payload, resp); err != nil {
return "", fmt.Errorf("failed to unmarshal signed list secret identifiers payload: %w", err)
}
return resp.RequestId, nil
default:
return "", fmt.Errorf("unsupported method for signed payload request id validation: %s", method)
}
}
29 changes: 29 additions & 0 deletions core/capabilities/vault/vaultutils/signed_payload_request_id_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package vaultutils

import (
"encoding/json"
"testing"

"github.com/stretchr/testify/require"

vaultcommon "github.com/smartcontractkit/chainlink-common/pkg/capabilities/actions/vault"
"github.com/smartcontractkit/chainlink/v2/core/capabilities/vault/vaulttypes"
)

func TestSignedPayloadRequestID(t *testing.T) {
t.Parallel()

payload, err := ToCanonicalJSON(&vaultcommon.CreateSecretsResponse{
RequestId: "owner::req-1",
Responses: []*vaultcommon.CreateSecretResponse{},
}, false)
require.NoError(t, err)

got, err := SignedPayloadRequestID(vaulttypes.MethodSecretsCreate, json.RawMessage(payload))
require.NoError(t, err)
require.Equal(t, "owner::req-1", got)

got, err = SignedPayloadRequestID(vaulttypes.MethodSecretsCreate, json.RawMessage(`{"responses":[]}`))
require.NoError(t, err)
require.Empty(t, got)
}
2 changes: 1 addition & 1 deletion core/scripts/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ require (
github.com/smartcontractkit/chain-selectors v1.0.104
github.com/smartcontractkit/chainlink-automation v0.8.1
github.com/smartcontractkit/chainlink-ccip/chains/evm v0.0.0-20260618155522-3600f66e26cd
github.com/smartcontractkit/chainlink-common v0.11.2-0.20260701091216-9264d4444ce0
github.com/smartcontractkit/chainlink-common v0.11.2-0.20260701150721-312a42976466
github.com/smartcontractkit/chainlink-common/keystore v1.2.0
github.com/smartcontractkit/chainlink-data-streams v0.1.15-0.20260522094612-5f9f748bd87a
github.com/smartcontractkit/chainlink-deployments-framework v0.111.1-0.20260612191326-e31c0ae4cd54
Expand Down
4 changes: 2 additions & 2 deletions core/scripts/go.sum
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

83 changes: 76 additions & 7 deletions core/services/gateway/handlers/vault/aggregator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,25 @@ import (
"strings"

"github.com/ethereum/go-ethereum/common"
"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/metric"

"github.com/smartcontractkit/chainlink-common/pkg/capabilities"
vaultcommon "github.com/smartcontractkit/chainlink-common/pkg/capabilities/actions/vault"
jsonrpc "github.com/smartcontractkit/chainlink-common/pkg/jsonrpc2"
"github.com/smartcontractkit/chainlink-common/pkg/logger"
"github.com/smartcontractkit/chainlink-common/pkg/settings/limits"
"github.com/smartcontractkit/chainlink/v2/core/capabilities/vault/vaulttypes"
"github.com/smartcontractkit/chainlink/v2/core/capabilities/vault/vaultutils"
)

var errSignedPayloadRequestIDMismatch = errors.New("signed payload request id mismatch")

type baseAggregator struct {
capabilitiesRegistry capabilitiesRegistry
capabilitiesRegistry capabilitiesRegistry
metrics *metrics
donID string
signedResponseRequestIDGate limits.GateLimiter
// vaultHandlerDonID scopes registry lookup when several vault DONs exist.
//
// Source: gateway job TOML [[gatewayConfig.ShardedDONs]] DonName (see deployment/cre/jobs/pkg/gateway_job.go),
Expand All @@ -33,18 +42,51 @@ type baseAggregator struct {
vaultHandlerDonID string
}

func (a *baseAggregator) Aggregate(ctx context.Context, l logger.Logger, resps map[string]jsonrpc.Response[json.RawMessage], currResp *jsonrpc.Response[json.RawMessage]) (*jsonrpc.Response[json.RawMessage], error) {
func methodSupportsSignedOCRValidation(method string) bool {
switch method {
case vaulttypes.MethodSecretsCreate,
vaulttypes.MethodSecretsUpdate,
vaulttypes.MethodSecretsDelete,
vaulttypes.MethodSecretsList:
return true
default:
return false
}
}

func (a *baseAggregator) signedResponseRequestIDEnabled(ctx context.Context, l logger.Logger) bool {
allowed, err := a.signedResponseRequestIDGate.Limit(ctx)
if err != nil {
l.Errorw("unexpected error evaluating CRE gate", "gate", "VaultSignedResponseRequestIDEnabled", "error", err)
return false
}
return allowed
}

func (a *baseAggregator) Aggregate(ctx context.Context, l logger.Logger, requestID string, resps map[string]jsonrpc.Response[json.RawMessage], currResp *jsonrpc.Response[json.RawMessage]) (*jsonrpc.Response[json.RawMessage], error) {
don, err := a.donForVaultCapability(ctx)
if err != nil {
return nil, fmt.Errorf("failed to get DON for vault capability: %w", err)
}

currResp, err = a.validateUsingSignatures(don.DON, don.Nodes, currResp)
if err == nil {
return currResp, nil
if a.signedResponseRequestIDEnabled(ctx, l) {
if methodSupportsSignedOCRValidation(currResp.Method) {

@prashantkumar1982 prashantkumar1982 Jul 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this condition should be at top level.
For supported methods, only allow signature validation.
For unsupported methods, only allow quorum based validation.

@prashantkumar1982 prashantkumar1982 Jul 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Especially for unsupported methods like GetPublicKey, we should ONLY rely on quorum, not a signed ocr response.

@russell-stern russell-stern Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

currResp, err = a.validateUsingSignatures(ctx, l, don.DON, don.Nodes, requestID, currResp, true)
if err == nil {
return currResp, nil
}

l.Debugw("failed to validate signatures, falling back to quorum aggregation", "error", err)
}
} else {
currResp, err = a.validateUsingSignatures(ctx, l, don.DON, don.Nodes, requestID, currResp, false)
if err == nil {
return currResp, nil
}

l.Debugw("failed to validate signatures, falling back to quorum aggregation", "error", err)

@prashantkumar1982 prashantkumar1982 Jul 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@russell-stern russell-stern Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets break this out into it's own PR to keep this one smaller

}

l.Debugw("failed to validate signatures, falling back to quorum aggregation", "error", err)
currResp, err = a.validateUsingQuorum(don.DON, resps, l)
if err != nil {
return nil, fmt.Errorf("failed to validate using quorum: %w", err)
Expand Down Expand Up @@ -211,7 +253,14 @@ func (a *baseAggregator) sha(resp *jsonrpc.Response[json.RawMessage]) (string, e
return copied.Digest()
}

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) {
a.metrics.requestInternalError.Add(ctx, 1, metric.WithAttributes(
attribute.String("don_id", a.donID),
attribute.String("error", "signed_payload_request_id_mismatch"),
))
}

func (a *baseAggregator) validateUsingSignatures(ctx context.Context, l logger.Logger, don capabilities.DON, nodes []capabilities.Node, requestID string, resp *jsonrpc.Response[json.RawMessage], validateSignedPayloadRequestID bool) (*jsonrpc.Response[json.RawMessage], error) {
if resp.Result == nil {
if resp.Error != nil {
return nil, errors.New("response has an error, cannot validate signatures. Error: " + resp.Error.Error())
Expand All @@ -235,5 +284,25 @@ func (a *baseAggregator) validateUsingSignatures(don capabilities.DON, nodes []c
return nil, fmt.Errorf("failed to validate signatures: %w", err)
}

if !validateSignedPayloadRequestID {
return resp, nil
}

payloadRequestID, err := vaultutils.SignedPayloadRequestID(resp.Method, r.Payload)
if err != nil {
l.Errorw("failed to read signed payload request id, discarding response", "requestID", requestID, "method", resp.Method, "error", err)
a.recordSignedPayloadRequestIDMismatch(ctx)
return nil, fmt.Errorf("%w: %w", errSignedPayloadRequestIDMismatch, err)
}
// Temporarily tolerate signed OCR reports from vault nodes that have not upgraded to
// include requestId in the signed payload. Once all vault nodes are upgraded, the
// gateway should start rejecting responses with a missing requestId.
// https://smartcontract-it.atlassian.net/browse/CRE-4875
if payloadRequestID != "" && payloadRequestID != requestID {
logger.Sugared(l).Criticalw("signed payload request id mismatch, discarding response", "requestID", requestID, "signedPayloadRequestID", payloadRequestID, "method", resp.Method)
a.recordSignedPayloadRequestIDMismatch(ctx)
return nil, errSignedPayloadRequestIDMismatch
}

return resp, nil
}
Loading
Loading