diff --git a/.changeset/vault-auth-gw-handler.md b/.changeset/vault-auth-gw-handler.md new file mode 100644 index 00000000000..5834309ebf9 --- /dev/null +++ b/.changeset/vault-auth-gw-handler.md @@ -0,0 +1,7 @@ +--- +"chainlink": patch +--- + +#changed + +Move Vault node-side request authorization into the gateway handler and remove duplicated authorization from the Vault capability. diff --git a/core/capabilities/vault/capability.go b/core/capabilities/vault/capability.go index 55375a08673..74430246c6b 100644 --- a/core/capabilities/vault/capability.go +++ b/core/capabilities/vault/capability.go @@ -3,10 +3,8 @@ package vault import ( "context" "encoding/hex" - "encoding/json" "errors" "fmt" - "strconv" "strings" "time" @@ -17,7 +15,6 @@ import ( "github.com/smartcontractkit/chainlink-common/pkg/capabilities" vaultcommon "github.com/smartcontractkit/chainlink-common/pkg/capabilities/actions/vault" "github.com/smartcontractkit/chainlink-common/pkg/capabilities/consensus/requests" - jsonrpc "github.com/smartcontractkit/chainlink-common/pkg/jsonrpc2" "github.com/smartcontractkit/chainlink-common/pkg/logger" "github.com/smartcontractkit/chainlink-common/pkg/settings/cresettings" "github.com/smartcontractkit/chainlink-common/pkg/settings/limits" @@ -32,7 +29,6 @@ type Capability struct { clock clockwork.Clock expiresAfter time.Duration handler *requests.Handler[*vaulttypes.Request, *vaulttypes.Response] - requestAuthorizer RequestAuthorizer capabilitiesRegistry core.CapabilitiesRegistry publicKey *LazyPublicKey *RequestValidator @@ -167,24 +163,6 @@ func (s *Capability) CreateSecrets(ctx context.Context, request *vaultcommon.Cre s.lggr.Debugf("RequestId: [%s] failed validation checks: %s", request.RequestId, err.Error()) return nil, err } - authorized, owner, err := s.authorizeCreateSecrets(ctx, *request) //nolint:govet // The mutex isn't used - if !authorized || err != nil { - s.lggr.Debugf("Request Id[%s] not authorized for owner: %s", request.RequestId, owner) - return nil, errors.New("request ID: " + request.RequestId + " not authorized: " + err.Error()) - } - if !strings.HasPrefix(request.RequestId, owner) { - // Gateway should ensure it prefixes request ids with the owner, to ensure request uniqueness - s.lggr.Debugf("Request ID: [%s] must start with owner address: [%s]", request.RequestId, owner) - return nil, errors.New("request ID: " + request.RequestId + " must start with owner address: " + owner) - } - for idx, req := range request.EncryptedSecrets { - // Ensure that users cannot access secrets belonging to other owners - if req.Id.Owner != owner { - s.lggr.Debugf("Secret ID owner: [%s] does not match authorized owner: [%s]", req.Id.Owner, owner) - return nil, errors.New("secret ID owner: " + req.Id.Owner + " does not match authorized owner: " + owner + " at index " + strconv.Itoa(idx)) - } - } - s.lggr.Debugf("Processing authorized and normalized request [%s]", request.String()) return s.handleRequest(ctx, request.RequestId, request) } @@ -195,24 +173,6 @@ func (s *Capability) UpdateSecrets(ctx context.Context, request *vaultcommon.Upd s.lggr.Debugf("RequestId: [%s] failed validation checks: %s", request.RequestId, err.Error()) return nil, err } - authorized, owner, err := s.authorizeUpdateSecrets(ctx, *request) //nolint:govet // The mutex isn't used - if !authorized || err != nil { - s.lggr.Debugf("Request Id[%s] not authorized for owner: %s", request.RequestId, owner) - return nil, errors.New("request ID: " + request.RequestId + " not authorized: " + err.Error()) - } - if !strings.HasPrefix(request.RequestId, owner) { - // Gateway should ensure it prefixes request ids with the owner, to ensure request uniqueness - s.lggr.Debugf("Request ID: [%s] must start with owner address: [%s]", request.RequestId, owner) - return nil, errors.New("request ID: " + request.RequestId + " must start with owner address: " + owner) - } - for idx, req := range request.EncryptedSecrets { - // Ensure that users cannot access secrets belonging to other owners - if req.Id.Owner != owner { - s.lggr.Debugf("Secret ID owner: [%s] does not match authorized owner: [%s]", req.Id.Owner, owner) - return nil, errors.New("secret ID owner: " + req.Id.Owner + " does not match authorized owner: " + owner + " at index " + strconv.Itoa(idx)) - } - } - s.lggr.Debugf("Processing authorized and normalized request [%s]", request.String()) return s.handleRequest(ctx, request.RequestId, request) } @@ -223,25 +183,6 @@ func (s *Capability) DeleteSecrets(ctx context.Context, request *vaultcommon.Del s.lggr.Debugf("Request: [%s] failed validation checks: %s", request.String(), err.Error()) return nil, err } - - authorized, owner, err := s.authorizeDeleteSecrets(ctx, *request) //nolint:govet // The mutex isn't used - if !authorized || err != nil { - s.lggr.Debugf("Request Id[%s] not authorized for owner: %s", request.RequestId, owner) - return nil, errors.New("request ID: " + request.RequestId + " not authorized: " + err.Error()) - } - if !strings.HasPrefix(request.RequestId, owner) { - // Gateway should ensure it prefixes request ids with the owner, to ensure request uniqueness - s.lggr.Debugf("Request ID: [%s] must start with owner address: [%s]", request.RequestId, owner) - return nil, errors.New("request ID: " + request.RequestId + " must start with owner address: " + owner) - } - for idx, req := range request.Ids { - // Ensure that users cannot access secrets belonging to other owners - if req.Owner != owner { - s.lggr.Debugf("Secret ID owner: [%s] does not match authorized owner: [%s]", req.Owner, owner) - return nil, errors.New("secret ID owner: " + req.Owner + " does not match authorized owner: " + owner + " at index " + strconv.Itoa(idx)) - } - } - s.lggr.Debugf("Processing authorized and normalized request [%s]", request.String()) return s.handleRequest(ctx, request.RequestId, request) } @@ -263,25 +204,6 @@ func (s *Capability) ListSecretIdentifiers(ctx context.Context, request *vaultco s.lggr.Debugf("Request: [%s] failed validation checks: %s", request.String(), err.Error()) return nil, err } - - authorized, owner, err := s.authorizeListSecrets(ctx, *request) //nolint:govet // The mutex isn't used - if !authorized || err != nil { - s.lggr.Debugf("Request ID[%s] not authorized for owner: %s", request.RequestId, owner) - return nil, errors.New("request ID: " + request.RequestId + " not authorized: " + err.Error()) - } - if !strings.HasPrefix(request.RequestId, owner) { - // Gateway should ensure it prefixes request ids with the owner, to ensure request uniqueness - s.lggr.Debugf("Request ID: [%s] must start with owner address: [%s]", request.RequestId, owner) - return nil, errors.New("request ID: " + request.RequestId + " must start with owner address: " + owner) - } - // Ensures that users cannot access secrets belonging to other owners - request.Owner = owner - if request.Owner != owner { - s.lggr.Debugf("Secret ID owner: [%s] does not match authorized owner: [%s]", request.Owner, owner) - return nil, errors.New("secret ID owner: " + request.Owner + " does not match authorized owner: " + owner) - } - - s.lggr.Debugf("Processing authorized and normalized request [%s]", request.String()) return s.handleRequest(ctx, request.RequestId, request) } @@ -334,76 +256,11 @@ func (s *Capability) handleRequest(ctx context.Context, requestID string, reques } } -func (s *Capability) getOriginalRequestID(transformedRequestID string) (string, error) { - // The transformed RequestID provided to Vault Nodes is of format ::. - // However, the RequestAuthorizer expects just the as the JSONRequest's ID fields, - // since that's what was used by the caller when generating the request digest. - requestIDParts := strings.Split(transformedRequestID, vaulttypes.RequestIDSeparator) - if len(requestIDParts) != 2 { - return "", errors.New("internal error: request ID must be in format ::") - } - return requestIDParts[1], nil -} - -func (s *Capability) authorizeCreateSecrets(ctx context.Context, request vaultcommon.CreateSecretsRequest) (bool, string, error) { //nolint:govet // The mutex isn't used - originalRequestID, err := s.getOriginalRequestID(request.RequestId) - if err != nil { - return false, "", err - } - request.RequestId = originalRequestID - - return s.isAuthorizedRequest(ctx, &request, originalRequestID, vaulttypes.MethodSecretsCreate) -} - -func (s *Capability) authorizeUpdateSecrets(ctx context.Context, request vaultcommon.UpdateSecretsRequest) (bool, string, error) { //nolint:govet // The mutex isn't used - originalRequestID, err := s.getOriginalRequestID(request.RequestId) - if err != nil { - return false, "", err - } - request.RequestId = originalRequestID - return s.isAuthorizedRequest(ctx, &request, originalRequestID, vaulttypes.MethodSecretsUpdate) -} - -func (s *Capability) authorizeDeleteSecrets(ctx context.Context, request vaultcommon.DeleteSecretsRequest) (bool, string, error) { //nolint:govet // The mutex isn't used - originalRequestID, err := s.getOriginalRequestID(request.RequestId) - if err != nil { - return false, "", err - } - request.RequestId = originalRequestID - return s.isAuthorizedRequest(ctx, &request, originalRequestID, vaulttypes.MethodSecretsDelete) -} - -func (s *Capability) authorizeListSecrets(ctx context.Context, request vaultcommon.ListSecretIdentifiersRequest) (bool, string, error) { //nolint:govet // The mutex isn't used - originalRequestID, err := s.getOriginalRequestID(request.RequestId) - if err != nil { - return false, "", err - } - request.RequestId = originalRequestID - return s.isAuthorizedRequest(ctx, &request, originalRequestID, vaulttypes.MethodSecretsList) -} - -func (s *Capability) isAuthorizedRequest(ctx context.Context, request any, requestID, method string) (bool, string, error) { - var params json.RawMessage - params, err := json.Marshal(request) - if err != nil { - return false, "", fmt.Errorf("could not marshal CreateSecretsRequest: %w", err) - } - s.lggr.Debugw("Authorizing request", "method", method, "requestID", requestID) - jsonRequest := jsonrpc.Request[json.RawMessage]{ - Version: jsonrpc.JsonRpcVersion, - ID: requestID, - Method: method, - Params: ¶ms, - } - return s.requestAuthorizer.AuthorizeRequest(ctx, jsonRequest) -} - func NewCapability( lggr logger.Logger, clock clockwork.Clock, expiresAfter time.Duration, handler *requests.Handler[*vaulttypes.Request, *vaulttypes.Response], - requestAuthorizer RequestAuthorizer, capabilitiesRegistry core.CapabilitiesRegistry, publicKey *LazyPublicKey, limitsFactory limits.Factory, @@ -417,7 +274,6 @@ func NewCapability( clock: clock, expiresAfter: expiresAfter, handler: handler, - requestAuthorizer: requestAuthorizer, capabilitiesRegistry: capabilitiesRegistry, publicKey: publicKey, RequestValidator: NewRequestValidator(limiter), diff --git a/core/capabilities/vault/capability_test.go b/core/capabilities/vault/capability_test.go index 66c188c466c..220ac11e95e 100644 --- a/core/capabilities/vault/capability_test.go +++ b/core/capabilities/vault/capability_test.go @@ -10,7 +10,6 @@ import ( "github.com/ethereum/go-ethereum/common" "github.com/jonboulle/clockwork" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/types/known/anypb" @@ -24,7 +23,6 @@ import ( "github.com/smartcontractkit/chainlink-common/pkg/settings/cresettings" "github.com/smartcontractkit/chainlink-common/pkg/settings/limits" coreCapabilities "github.com/smartcontractkit/chainlink/v2/core/capabilities" - vaultcapmocks "github.com/smartcontractkit/chainlink/v2/core/capabilities/vault/mocks" "github.com/smartcontractkit/chainlink/v2/core/capabilities/vault/vaulttypes" "github.com/smartcontractkit/chainlink/v2/core/logger" ) @@ -35,10 +33,9 @@ func TestCapability_CapabilityCall(t *testing.T) { expiry := 10 * time.Second store := requests.NewStore[*vaulttypes.Request]() handler := requests.NewHandler[*vaulttypes.Request, *vaulttypes.Response](lggr, store, clock, expiry) - requestAuthorizer := vaultcapmocks.NewRequestAuthorizer(t) reg := coreCapabilities.NewRegistry(lggr) lf := limits.Factory{Settings: cresettings.DefaultGetter} - capability, err := NewCapability(lggr, clock, expiry, handler, requestAuthorizer, reg, nil, lf) + capability, err := NewCapability(lggr, clock, expiry, handler, reg, nil, lf) require.NoError(t, err) servicetest.Run(t, capability) @@ -133,10 +130,9 @@ func TestCapability_CapabilityCall_DuringSubscriptionPhase(t *testing.T) { expiry := 10 * time.Second store := requests.NewStore[*vaulttypes.Request]() handler := requests.NewHandler[*vaulttypes.Request, *vaulttypes.Response](lggr, store, clock, expiry) - requestAuthorizer := vaultcapmocks.NewRequestAuthorizer(t) reg := coreCapabilities.NewRegistry(lggr) lf := limits.Factory{Settings: cresettings.DefaultGetter} - capability, err := NewCapability(lggr, clock, expiry, handler, requestAuthorizer, reg, nil, lf) + capability, err := NewCapability(lggr, clock, expiry, handler, reg, nil, lf) require.NoError(t, err) servicetest.Run(t, capability) @@ -304,10 +300,9 @@ func TestCapability_CapabilityCall_SecretIdentifierOwnerMismatch(t *testing.T) { expiry := 10 * time.Second store := requests.NewStore[*vaulttypes.Request]() handler := requests.NewHandler[*vaulttypes.Request, *vaulttypes.Response](lggr, store, clock, expiry) - requestAuthorizer := vaultcapmocks.NewRequestAuthorizer(t) reg := coreCapabilities.NewRegistry(lggr) lf := limits.Factory{Settings: cresettings.DefaultGetter} - capability, err := NewCapability(lggr, clock, expiry, handler, requestAuthorizer, reg, nil, lf) + capability, err := NewCapability(lggr, clock, expiry, handler, reg, nil, lf) require.NoError(t, err) servicetest.Run(t, capability) @@ -382,10 +377,9 @@ func TestCapability_CapabilityCall_ReturnsIncorrectType(t *testing.T) { expiry := 10 * time.Second store := requests.NewStore[*vaulttypes.Request]() handler := requests.NewHandler[*vaulttypes.Request, *vaulttypes.Response](lggr, store, clock, expiry) - requestAuthorizer := vaultcapmocks.NewRequestAuthorizer(t) reg := coreCapabilities.NewRegistry(lggr) lf := limits.Factory{Settings: cresettings.DefaultGetter} - capability, err := NewCapability(lggr, clock, expiry, handler, requestAuthorizer, reg, nil, lf) + capability, err := NewCapability(lggr, clock, expiry, handler, reg, nil, lf) require.NoError(t, err) servicetest.Run(t, capability) @@ -457,10 +451,9 @@ func TestCapability_CapabilityCall_TimeOut(t *testing.T) { expiry := 10 * time.Second store := requests.NewStore[*vaulttypes.Request]() handler := requests.NewHandler[*vaulttypes.Request, *vaulttypes.Response](lggr, store, fakeClock, expiry) - requestAuthorizer := vaultcapmocks.NewRequestAuthorizer(t) reg := coreCapabilities.NewRegistry(lggr) lf := limits.Factory{Settings: cresettings.DefaultGetter} - capability, err := NewCapability(lggr, fakeClock, expiry, handler, requestAuthorizer, reg, nil, lf) + capability, err := NewCapability(lggr, fakeClock, expiry, handler, reg, nil, lf) require.NoError(t, err) servicetest.Run(t, capability) @@ -1064,24 +1057,6 @@ func TestCapability_CRUD(t *testing.T) { return capability.DeleteSecrets(t.Context(), req) }, }, - { - name: "DeleteSecrets_Invalid_Owner", - response: nil, - error: "secret ID owner: random does not match authorized owner:", - call: func(t *testing.T, capability *Capability) (*vaulttypes.Response, error) { - req := &vault.DeleteSecretsRequest{ - RequestId: requestID, - Ids: []*vault.SecretIdentifier{ - { - Key: "Foo", - Namespace: "Bar", - Owner: "random", - }, - }, - } - return capability.DeleteSecrets(t.Context(), req) - }, - }, { name: "DeleteSecrets_Invalid_Duplicates", error: "duplicate secret ID found", @@ -1180,11 +1155,9 @@ func TestCapability_CRUD(t *testing.T) { expiry := 10 * time.Second store := requests.NewStore[*vaulttypes.Request]() handler := requests.NewHandler[*vaulttypes.Request, *vaulttypes.Response](lggr, store, clock, expiry) - requestAuthorizer := vaultcapmocks.NewRequestAuthorizer(t) - requestAuthorizer.On("AuthorizeRequest", t.Context(), mock.Anything).Return(true, owner, nil).Maybe() reg := coreCapabilities.NewRegistry(lggr) lf := limits.Factory{Settings: cresettings.DefaultGetter} - capability, err := NewCapability(lggr, clock, expiry, handler, requestAuthorizer, reg, lpk, lf) + capability, err := NewCapability(lggr, clock, expiry, handler, reg, lpk, lf) require.NoError(t, err) servicetest.Run(t, capability) @@ -1230,11 +1203,9 @@ func TestCapability_Lifecycle(t *testing.T) { expiry := 10 * time.Second store := requests.NewStore[*vaulttypes.Request]() handler := requests.NewHandler[*vaulttypes.Request, *vaulttypes.Response](lggr, store, clock, expiry) - requestAuthorizer := vaultcapmocks.NewRequestAuthorizer(t) - requestAuthorizer.On("AuthorizeRequest", t.Context(), mock.Anything).Return(true, "owner", nil).Maybe() reg := coreCapabilities.NewRegistry(lggr) lf := limits.Factory{Settings: cresettings.DefaultGetter} - capability, err := NewCapability(lggr, clock, expiry, handler, requestAuthorizer, reg, nil, lf) + capability, err := NewCapability(lggr, clock, expiry, handler, reg, nil, lf) require.NoError(t, err) _, err = reg.GetExecutable(t.Context(), vault.CapabilityID) @@ -1262,11 +1233,10 @@ func TestCapability_PublicKeyGet(t *testing.T) { expiry := 10 * time.Second store := requests.NewStore[*vaulttypes.Request]() handler := requests.NewHandler[*vaulttypes.Request, *vaulttypes.Response](lggr, store, clock, expiry) - requestAuthorizer := vaultcapmocks.NewRequestAuthorizer(t) reg := coreCapabilities.NewRegistry(lggr) lpk := NewLazyPublicKey() lf := limits.Factory{Settings: cresettings.DefaultGetter} - capability, err := NewCapability(lggr, clock, expiry, handler, requestAuthorizer, reg, lpk, lf) + capability, err := NewCapability(lggr, clock, expiry, handler, reg, lpk, lf) require.NoError(t, err) servicetest.Run(t, capability) diff --git a/core/capabilities/vault/gw_handler.go b/core/capabilities/vault/gw_handler.go index ee6178dcc06..94b1563142e 100644 --- a/core/capabilities/vault/gw_handler.go +++ b/core/capabilities/vault/gw_handler.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "strings" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" @@ -59,23 +60,25 @@ type GatewayHandler struct { services.Service eng *services.Engine - secretsService vaulttypes.SecretsService - gatewayConnector gatewayConnector - lggr logger.Logger - metrics *metrics + secretsService vaulttypes.SecretsService + gatewayConnector gatewayConnector + requestAuthorizer RequestAuthorizer + lggr logger.Logger + metrics *metrics } -func NewGatewayHandler(secretsService vaulttypes.SecretsService, connector gatewayConnector, lggr logger.Logger) (*GatewayHandler, error) { +func NewGatewayHandler(secretsService vaulttypes.SecretsService, connector gatewayConnector, requestAuthorizer RequestAuthorizer, lggr logger.Logger) (*GatewayHandler, error) { metrics, err := newMetrics() if err != nil { return nil, fmt.Errorf("failed to create metrics: %w", err) } gh := &GatewayHandler{ - secretsService: secretsService, - gatewayConnector: connector, - lggr: lggr.Named(HandlerName), - metrics: metrics, + secretsService: secretsService, + gatewayConnector: connector, + requestAuthorizer: requestAuthorizer, + lggr: lggr.Named(HandlerName), + metrics: metrics, } gh.Service, gh.eng = services.Config{ Name: "GatewayHandler", @@ -113,13 +116,33 @@ func (h *GatewayHandler) HandleGatewayMessage(ctx context.Context, gatewayID str var response *jsonrpc.Response[json.RawMessage] switch req.Method { case vaulttypes.MethodSecretsCreate: - response = h.handleSecretsCreate(ctx, gatewayID, req) + owner, authErr := h.authorizeAndPrefixRequest(ctx, req) + if authErr != nil { + response = h.errorResponse(ctx, gatewayID, req, api.FatalError, authErr) + break + } + response = h.handleSecretsCreate(ctx, gatewayID, req, owner) case vaulttypes.MethodSecretsUpdate: - response = h.handleSecretsUpdate(ctx, gatewayID, req) + owner, authErr := h.authorizeAndPrefixRequest(ctx, req) + if authErr != nil { + response = h.errorResponse(ctx, gatewayID, req, api.FatalError, authErr) + break + } + response = h.handleSecretsUpdate(ctx, gatewayID, req, owner) case vaulttypes.MethodSecretsDelete: - response = h.handleSecretsDelete(ctx, gatewayID, req) + owner, authErr := h.authorizeAndPrefixRequest(ctx, req) + if authErr != nil { + response = h.errorResponse(ctx, gatewayID, req, api.FatalError, authErr) + break + } + response = h.handleSecretsDelete(ctx, gatewayID, req, owner) case vaulttypes.MethodSecretsList: - response = h.handleSecretsList(ctx, gatewayID, req) + owner, authErr := h.authorizeAndPrefixRequest(ctx, req) + if authErr != nil { + response = h.errorResponse(ctx, gatewayID, req, api.FatalError, authErr) + break + } + response = h.handleSecretsList(ctx, gatewayID, req, owner) case vaulttypes.MethodPublicKeyGet: response = h.handlePublicKeyGet(ctx, gatewayID, req) default: @@ -138,14 +161,109 @@ func (h *GatewayHandler) HandleGatewayMessage(ctx context.Context, gatewayID str return nil } -func (h *GatewayHandler) handleSecretsCreate(ctx context.Context, gatewayID string, req *jsonrpc.Request[json.RawMessage]) *jsonrpc.Response[json.RawMessage] { +func (h *GatewayHandler) authorizeAndPrefixRequest(ctx context.Context, req *jsonrpc.Request[json.RawMessage]) (string, error) { + if h.requestAuthorizer == nil { + err := errors.New("request authorizer is nil") + h.lggr.Errorw("failed to authorize gateway request", "method", req.Method, "requestID", req.ID, "error", err) + return "", err + } + + originalRequestID := req.ID + incomingOwner := "" + if idx := strings.Index(req.ID, vaulttypes.RequestIDSeparator); idx != -1 { + incomingOwner = req.ID[:idx] + originalRequestID = req.ID[idx+len(vaulttypes.RequestIDSeparator):] + } + + authReq := *req + authReq.ID = originalRequestID + if err := stripPrefixedRequestIDFromParams(&authReq, originalRequestID); err != nil { + h.lggr.Errorw("failed to normalize gateway request for authorization", "method", req.Method, "requestID", originalRequestID, "error", err) + return "", err + } + + h.lggr.Debugw("authorizing gateway request", "method", req.Method, "requestID", originalRequestID) + isAuthorized, owner, err := h.requestAuthorizer.AuthorizeRequest(ctx, authReq) + if !isAuthorized { + authErr := fmt.Errorf("request not authorized: %w", err) + h.lggr.Errorw("gateway request authorization failed", "method", req.Method, "requestID", originalRequestID, "owner", owner, "error", authErr) + return "", authErr + } + if incomingOwner != "" && normalizeOwner(incomingOwner) != normalizeOwner(owner) { + prefixErr := fmt.Errorf("request owner prefix %q does not match authorized owner %q", incomingOwner, owner) + h.lggr.Errorw("gateway request owner prefix mismatch", "method", req.Method, "requestID", originalRequestID, "incomingOwner", incomingOwner, "authorizedOwner", owner, "error", prefixErr) + return "", prefixErr + } + + req.ID = owner + vaulttypes.RequestIDSeparator + originalRequestID + h.lggr.Debugw("authorized gateway request", "method", req.Method, "requestID", req.ID, "owner", owner) + return owner, nil +} + +func stripPrefixedRequestIDFromParams(req *jsonrpc.Request[json.RawMessage], originalRequestID string) error { + if req.Params == nil { + return nil + } + + switch req.Method { + case vaulttypes.MethodSecretsCreate: + parsed := &vaultcommon.CreateSecretsRequest{} + if err := json.Unmarshal(*req.Params, parsed); err != nil { + return err + } + parsed.RequestId = originalRequestID + return rewriteRequestParams(req, parsed) + case vaulttypes.MethodSecretsUpdate: + parsed := &vaultcommon.UpdateSecretsRequest{} + if err := json.Unmarshal(*req.Params, parsed); err != nil { + return err + } + parsed.RequestId = originalRequestID + return rewriteRequestParams(req, parsed) + case vaulttypes.MethodSecretsDelete: + parsed := &vaultcommon.DeleteSecretsRequest{} + if err := json.Unmarshal(*req.Params, parsed); err != nil { + return err + } + parsed.RequestId = originalRequestID + return rewriteRequestParams(req, parsed) + case vaulttypes.MethodSecretsList: + parsed := &vaultcommon.ListSecretIdentifiersRequest{} + if err := json.Unmarshal(*req.Params, parsed); err != nil { + return err + } + parsed.RequestId = originalRequestID + return rewriteRequestParams(req, parsed) + default: + return nil + } +} + +func rewriteRequestParams(req *jsonrpc.Request[json.RawMessage], payload any) error { + params, err := json.Marshal(payload) + if err != nil { + return err + } + raw := json.RawMessage(params) + req.Params = &raw + return nil +} + +func (h *GatewayHandler) handleSecretsCreate(ctx context.Context, gatewayID string, req *jsonrpc.Request[json.RawMessage], owner string) *jsonrpc.Response[json.RawMessage] { vaultCapRequest := vaultcommon.CreateSecretsRequest{} if err := json.Unmarshal(*req.Params, &vaultCapRequest); err != nil { return h.errorResponse(ctx, gatewayID, req, api.UserMessageParseError, err) } vaultCapRequest.RequestId = req.ID + for idx, encryptedSecret := range vaultCapRequest.EncryptedSecrets { + if encryptedSecret != nil && encryptedSecret.Id != nil && normalizeOwner(encryptedSecret.Id.Owner) != normalizeOwner(owner) { + h.lggr.Debugw("create secrets request owner mismatch", "requestID", req.ID, "secretOwner", encryptedSecret.Id.Owner, "authorizedOwner", owner, "index", idx) + return h.errorResponse(ctx, gatewayID, req, api.FatalError, fmt.Errorf("secret ID owner %q does not match authorized owner %q at index %d", encryptedSecret.Id.Owner, owner, idx)) + } + } + h.lggr.Debugf("Processing authorized and normalized create secrets request [%s]", vaultCapRequest.String()) vaultCapResponse, err := h.secretsService.CreateSecrets(ctx, &vaultCapRequest) if err != nil { return h.errorResponse(ctx, gatewayID, req, api.FatalError, err) @@ -158,12 +276,20 @@ func (h *GatewayHandler) handleSecretsCreate(ctx context.Context, gatewayID stri return jsonResponse } -func (h *GatewayHandler) handleSecretsUpdate(ctx context.Context, gatewayID string, req *jsonrpc.Request[json.RawMessage]) *jsonrpc.Response[json.RawMessage] { +func (h *GatewayHandler) handleSecretsUpdate(ctx context.Context, gatewayID string, req *jsonrpc.Request[json.RawMessage], owner string) *jsonrpc.Response[json.RawMessage] { vaultCapRequest := vaultcommon.UpdateSecretsRequest{} if err := json.Unmarshal(*req.Params, &vaultCapRequest); err != nil { return h.errorResponse(ctx, gatewayID, req, api.UserMessageParseError, err) } + vaultCapRequest.RequestId = req.ID + for idx, encryptedSecret := range vaultCapRequest.EncryptedSecrets { + if encryptedSecret != nil && encryptedSecret.Id != nil && normalizeOwner(encryptedSecret.Id.Owner) != normalizeOwner(owner) { + h.lggr.Debugw("update secrets request owner mismatch", "requestID", req.ID, "secretOwner", encryptedSecret.Id.Owner, "authorizedOwner", owner, "index", idx) + return h.errorResponse(ctx, gatewayID, req, api.FatalError, fmt.Errorf("secret ID owner %q does not match authorized owner %q at index %d", encryptedSecret.Id.Owner, owner, idx)) + } + } + h.lggr.Debugf("Processing authorized and normalized update secrets request [%s]", vaultCapRequest.String()) vaultCapResponse, err := h.secretsService.UpdateSecrets(ctx, &vaultCapRequest) if err != nil { return h.errorResponse(ctx, gatewayID, req, api.FatalError, err) @@ -176,12 +302,20 @@ func (h *GatewayHandler) handleSecretsUpdate(ctx context.Context, gatewayID stri return jsonResponse } -func (h *GatewayHandler) handleSecretsDelete(ctx context.Context, gatewayID string, req *jsonrpc.Request[json.RawMessage]) *jsonrpc.Response[json.RawMessage] { +func (h *GatewayHandler) handleSecretsDelete(ctx context.Context, gatewayID string, req *jsonrpc.Request[json.RawMessage], owner string) *jsonrpc.Response[json.RawMessage] { r := &vaultcommon.DeleteSecretsRequest{} if err := json.Unmarshal(*req.Params, r); err != nil { return h.errorResponse(ctx, gatewayID, req, api.UserMessageParseError, err) } + r.RequestId = req.ID + for idx, secretID := range r.Ids { + if secretID != nil && normalizeOwner(secretID.Owner) != normalizeOwner(owner) { + h.lggr.Debugw("delete secrets request owner mismatch", "requestID", req.ID, "secretOwner", secretID.Owner, "authorizedOwner", owner, "index", idx) + return h.errorResponse(ctx, gatewayID, req, api.FatalError, fmt.Errorf("secret ID owner %q does not match authorized owner %q at index %d", secretID.Owner, owner, idx)) + } + } + h.lggr.Debugf("Processing authorized and normalized delete secrets request [%s]", r.String()) resp, err := h.secretsService.DeleteSecrets(ctx, r) if err != nil { return h.errorResponse(ctx, gatewayID, req, api.HandlerError, fmt.Errorf("failed to delete secrets: %w", err)) @@ -200,12 +334,15 @@ func (h *GatewayHandler) handleSecretsDelete(ctx context.Context, gatewayID stri } } -func (h *GatewayHandler) handleSecretsList(ctx context.Context, gatewayID string, req *jsonrpc.Request[json.RawMessage]) *jsonrpc.Response[json.RawMessage] { +func (h *GatewayHandler) handleSecretsList(ctx context.Context, gatewayID string, req *jsonrpc.Request[json.RawMessage], owner string) *jsonrpc.Response[json.RawMessage] { r := &vaultcommon.ListSecretIdentifiersRequest{} if err := json.Unmarshal(*req.Params, r); err != nil { return h.errorResponse(ctx, gatewayID, req, api.UserMessageParseError, err) } + r.RequestId = req.ID + r.Owner = owner + h.lggr.Debugf("Processing authorized and normalized list secrets request [%s]", r.String()) resp, err := h.secretsService.ListSecretIdentifiers(ctx, r) if err != nil { return h.errorResponse(ctx, gatewayID, req, api.HandlerError, fmt.Errorf("failed to list secret identifiers: %w", err)) diff --git a/core/capabilities/vault/gw_handler_test.go b/core/capabilities/vault/gw_handler_test.go index 7d0398658d8..293544e9d0a 100644 --- a/core/capabilities/vault/gw_handler_test.go +++ b/core/capabilities/vault/gw_handler_test.go @@ -12,6 +12,7 @@ import ( vaultcommon "github.com/smartcontractkit/chainlink-common/pkg/capabilities/actions/vault" jsonrpc "github.com/smartcontractkit/chainlink-common/pkg/jsonrpc2" vaultcap "github.com/smartcontractkit/chainlink/v2/core/capabilities/vault" + vaultcapmocks "github.com/smartcontractkit/chainlink/v2/core/capabilities/vault/mocks" "github.com/smartcontractkit/chainlink/v2/core/capabilities/vault/vaulttypes" vaulttypesmocks "github.com/smartcontractkit/chainlink/v2/core/capabilities/vault/vaulttypes/mocks" "github.com/smartcontractkit/chainlink/v2/core/logger" @@ -25,16 +26,21 @@ func TestGatewayHandler_HandleGatewayMessage(t *testing.T) { tests := []struct { name string - setupMocks func(*vaulttypesmocks.SecretsService, *connector_mocks.GatewayConnector) + setupMocks func(*vaulttypesmocks.SecretsService, *connector_mocks.GatewayConnector, *vaultcapmocks.RequestAuthorizer) request *jsonrpc.Request[json.RawMessage] expectedError bool }{ { name: "success - create secrets", - setupMocks: func(ss *vaulttypesmocks.SecretsService, gc *connector_mocks.GatewayConnector) { + setupMocks: func(ss *vaulttypesmocks.SecretsService, gc *connector_mocks.GatewayConnector, ra *vaultcapmocks.RequestAuthorizer) { + ra.EXPECT().AuthorizeRequest(mock.Anything, mock.MatchedBy(func(req jsonrpc.Request[json.RawMessage]) bool { + return req.Method == vaulttypes.MethodSecretsCreate && req.ID == "1" + })).Return(true, "0xabc", nil) ss.EXPECT().CreateSecrets(mock.Anything, mock.MatchedBy(func(req *vaultcommon.CreateSecretsRequest) bool { return len(req.EncryptedSecrets) == 1 && - req.EncryptedSecrets[0].Id.Key == "test-secret" + req.EncryptedSecrets[0].Id.Key == "test-secret" && + req.EncryptedSecrets[0].Id.Owner == "0xAbC" && + req.RequestId == "0xabc"+vaulttypes.RequestIDSeparator+"1" })).Return(&vaulttypes.Response{ID: "test-secret"}, nil) gc.On("SendToGateway", mock.Anything, "gateway-1", mock.MatchedBy(func(resp *jsonrpc.Response[json.RawMessage]) bool { @@ -50,7 +56,8 @@ func TestGatewayHandler_HandleGatewayMessage(t *testing.T) { EncryptedSecrets: []*vaultcommon.EncryptedSecret{ { Id: &vaultcommon.SecretIdentifier{ - Key: "test-secret", + Key: "test-secret", + Owner: "0xAbC", }, EncryptedValue: "encrypted-value", }, @@ -64,7 +71,8 @@ func TestGatewayHandler_HandleGatewayMessage(t *testing.T) { }, { name: "failure - service error", - setupMocks: func(ss *vaulttypesmocks.SecretsService, gc *connector_mocks.GatewayConnector) { + setupMocks: func(ss *vaulttypesmocks.SecretsService, gc *connector_mocks.GatewayConnector, ra *vaultcapmocks.RequestAuthorizer) { + ra.EXPECT().AuthorizeRequest(mock.Anything, mock.Anything).Return(true, "0xabc", nil) ss.EXPECT().CreateSecrets(mock.Anything, mock.Anything).Return(nil, errors.New("service error")) gc.On("SendToGateway", mock.Anything, "gateway-1", mock.MatchedBy(func(resp *jsonrpc.Response[json.RawMessage]) bool { @@ -81,7 +89,8 @@ func TestGatewayHandler_HandleGatewayMessage(t *testing.T) { EncryptedSecrets: []*vaultcommon.EncryptedSecret{ { Id: &vaultcommon.SecretIdentifier{ - Key: "test-secret", + Key: "test-secret", + Owner: "0xAbC", }, EncryptedValue: "encrypted-value", }, @@ -95,7 +104,7 @@ func TestGatewayHandler_HandleGatewayMessage(t *testing.T) { }, { name: "failure - invalid method", - setupMocks: func(ss *vaulttypesmocks.SecretsService, gc *connector_mocks.GatewayConnector) { + setupMocks: func(ss *vaulttypesmocks.SecretsService, gc *connector_mocks.GatewayConnector, ra *vaultcapmocks.RequestAuthorizer) { gc.On("SendToGateway", mock.Anything, "gateway-1", mock.MatchedBy(func(resp *jsonrpc.Response[json.RawMessage]) bool { return resp.Error != nil && resp.Error.Code == api.ToJSONRPCErrorCode(api.UnsupportedMethodError) @@ -109,10 +118,10 @@ func TestGatewayHandler_HandleGatewayMessage(t *testing.T) { }, { name: "failure - invalid request params", - setupMocks: func(ss *vaulttypesmocks.SecretsService, gc *connector_mocks.GatewayConnector) { + setupMocks: func(ss *vaulttypesmocks.SecretsService, gc *connector_mocks.GatewayConnector, ra *vaultcapmocks.RequestAuthorizer) { gc.On("SendToGateway", mock.Anything, "gateway-1", mock.MatchedBy(func(resp *jsonrpc.Response[json.RawMessage]) bool { return resp.Error != nil && - resp.Error.Code == api.ToJSONRPCErrorCode(api.UserMessageParseError) + resp.Error.Code == api.ToJSONRPCErrorCode(api.FatalError) })).Return(nil) }, request: &jsonrpc.Request[json.RawMessage]{ @@ -127,12 +136,16 @@ func TestGatewayHandler_HandleGatewayMessage(t *testing.T) { }, { name: "success - delete secrets", - setupMocks: func(ss *vaulttypesmocks.SecretsService, gc *connector_mocks.GatewayConnector) { + setupMocks: func(ss *vaulttypesmocks.SecretsService, gc *connector_mocks.GatewayConnector, ra *vaultcapmocks.RequestAuthorizer) { + ra.EXPECT().AuthorizeRequest(mock.Anything, mock.MatchedBy(func(req jsonrpc.Request[json.RawMessage]) bool { + return req.Method == vaulttypes.MethodSecretsDelete && req.ID == "1" + })).Return(true, "0xabc", nil) ss.EXPECT().DeleteSecrets(mock.Anything, mock.MatchedBy(func(req *vaultcommon.DeleteSecretsRequest) bool { return len(req.Ids) == 1 && req.Ids[0].Key == "Foo" && req.Ids[0].Namespace == "Bar" && - req.Ids[0].Owner == "Owner" + req.Ids[0].Owner == "0xAbC" && + req.RequestId == "0xabc"+vaulttypes.RequestIDSeparator+"1" })).Return(&vaulttypes.Response{ID: "test-secret"}, nil) gc.On("SendToGateway", mock.Anything, "gateway-1", mock.MatchedBy(func(resp *jsonrpc.Response[json.RawMessage]) bool { @@ -147,10 +160,119 @@ func TestGatewayHandler_HandleGatewayMessage(t *testing.T) { RequestId: "test-secret", Ids: []*vaultcommon.SecretIdentifier{ { - Key: "Foo", Namespace: "Bar", - Owner: "Owner", + Owner: "0xAbC", + }, + }, + }) + raw := json.RawMessage(params) + return &raw + }(), + }, + expectedError: false, + }, + { + name: "failure - unauthorized request", + setupMocks: func(ss *vaulttypesmocks.SecretsService, gc *connector_mocks.GatewayConnector, ra *vaultcapmocks.RequestAuthorizer) { + ra.EXPECT().AuthorizeRequest(mock.Anything, mock.Anything).Return(false, "", errors.New("not allowlisted")) + gc.On("SendToGateway", mock.Anything, "gateway-1", mock.MatchedBy(func(resp *jsonrpc.Response[json.RawMessage]) bool { + return resp.Error != nil && + resp.Error.Code == api.ToJSONRPCErrorCode(api.FatalError) && + resp.Error.Message == "request not authorized: not allowlisted" + })).Return(nil) + }, + request: &jsonrpc.Request[json.RawMessage]{ + Method: vaulttypes.MethodSecretsCreate, + ID: "1", + Params: func() *json.RawMessage { + params, _ := json.Marshal(vaultcommon.CreateSecretsRequest{ + EncryptedSecrets: []*vaultcommon.EncryptedSecret{ + { + Id: &vaultcommon.SecretIdentifier{ + Key: "test-secret", + Owner: "0xAbC", + }, + EncryptedValue: "encrypted-value", + }, + }, + }) + raw := json.RawMessage(params) + return &raw + }(), + }, + expectedError: false, + }, + { + name: "success - strips owner prefix from forwarded request before authorization", + setupMocks: func(ss *vaulttypesmocks.SecretsService, gc *connector_mocks.GatewayConnector, ra *vaultcapmocks.RequestAuthorizer) { + ra.EXPECT().AuthorizeRequest(mock.Anything, mock.MatchedBy(func(req jsonrpc.Request[json.RawMessage]) bool { + if req.Method != vaulttypes.MethodSecretsCreate || req.ID != "1" || req.Params == nil { + return false + } + + var parsed vaultcommon.CreateSecretsRequest + if err := json.Unmarshal(*req.Params, &parsed); err != nil { + return false + } + + return parsed.RequestId == "1" && + len(parsed.EncryptedSecrets) == 1 && + parsed.EncryptedSecrets[0].Id != nil && + parsed.EncryptedSecrets[0].Id.Owner == "0xAbC" + })).Return(true, "0xabc", nil) + ss.EXPECT().CreateSecrets(mock.Anything, mock.MatchedBy(func(req *vaultcommon.CreateSecretsRequest) bool { + return req.RequestId == "0xabc"+vaulttypes.RequestIDSeparator+"1" + })).Return(&vaulttypes.Response{ID: "test-secret"}, nil) + + gc.On("SendToGateway", mock.Anything, "gateway-1", mock.MatchedBy(func(resp *jsonrpc.Response[json.RawMessage]) bool { + return resp.Error == nil + })).Return(nil) + }, + request: &jsonrpc.Request[json.RawMessage]{ + Method: vaulttypes.MethodSecretsCreate, + ID: "0xAbC" + vaulttypes.RequestIDSeparator + "1", + Params: func() *json.RawMessage { + params, _ := json.Marshal(vaultcommon.CreateSecretsRequest{ + RequestId: "0xAbC" + vaulttypes.RequestIDSeparator + "1", + EncryptedSecrets: []*vaultcommon.EncryptedSecret{ + { + Id: &vaultcommon.SecretIdentifier{ + Key: "test-secret", + Owner: "0xAbC", + }, + EncryptedValue: "encrypted-value", + }, + }, + }) + raw := json.RawMessage(params) + return &raw + }(), + }, + expectedError: false, + }, + { + name: "failure - owner mismatch against authorized owner", + setupMocks: func(ss *vaulttypesmocks.SecretsService, gc *connector_mocks.GatewayConnector, ra *vaultcapmocks.RequestAuthorizer) { + ra.EXPECT().AuthorizeRequest(mock.Anything, mock.Anything).Return(true, "0xdef", nil) + gc.On("SendToGateway", mock.Anything, "gateway-1", mock.MatchedBy(func(resp *jsonrpc.Response[json.RawMessage]) bool { + return resp.Error != nil && + resp.Error.Code == api.ToJSONRPCErrorCode(api.FatalError) && + resp.Error.Message == `secret ID owner "0xabc" does not match authorized owner "0xdef" at index 0` + })).Return(nil) + }, + request: &jsonrpc.Request[json.RawMessage]{ + Method: vaulttypes.MethodSecretsCreate, + ID: "1", + Params: func() *json.RawMessage { + params, _ := json.Marshal(vaultcommon.CreateSecretsRequest{ + EncryptedSecrets: []*vaultcommon.EncryptedSecret{ + { + Id: &vaultcommon.SecretIdentifier{ + Key: "test-secret", + Owner: "0xabc", + }, + EncryptedValue: "encrypted-value", }, }, }) @@ -166,10 +288,11 @@ func TestGatewayHandler_HandleGatewayMessage(t *testing.T) { t.Run(tt.name, func(t *testing.T) { secretsService := vaulttypesmocks.NewSecretsService(t) gwConnector := connector_mocks.NewGatewayConnector(t) + requestAuthorizer := vaultcapmocks.NewRequestAuthorizer(t) - tt.setupMocks(secretsService, gwConnector) + tt.setupMocks(secretsService, gwConnector, requestAuthorizer) - handler, err := vaultcap.NewGatewayHandler(secretsService, gwConnector, lggr) + handler, err := vaultcap.NewGatewayHandler(secretsService, gwConnector, requestAuthorizer, lggr) require.NoError(t, err) err = handler.HandleGatewayMessage(ctx, "gateway-1", tt.request) @@ -189,8 +312,9 @@ func TestGatewayHandler_Lifecycle(t *testing.T) { secretsService := vaulttypesmocks.NewSecretsService(t) gwConnector := connector_mocks.NewGatewayConnector(t) + requestAuthorizer := vaultcapmocks.NewRequestAuthorizer(t) - handler, err := vaultcap.NewGatewayHandler(secretsService, gwConnector, lggr) + handler, err := vaultcap.NewGatewayHandler(secretsService, gwConnector, requestAuthorizer, lggr) require.NoError(t, err) t.Run("start", func(t *testing.T) { diff --git a/core/services/ocr2/delegate.go b/core/services/ocr2/delegate.go index a975a7777cb..ca1c7f226ba 100644 --- a/core/services/ocr2/delegate.go +++ b/core/services/ocr2/delegate.go @@ -720,13 +720,14 @@ func (d *Delegate) newServicesVaultPlugin( expiryDuration := cfg.RequestExpiryDuration.Duration() requestStoreHandler := requests.NewHandler(lggr, requestStore, clock, expiryDuration) lpk := vaultcap.NewLazyPublicKey() - vaultCapability, err := vaultcap.NewCapability(lggr, clock, expiryDuration, requestStoreHandler, vaultcap.NewRequestAuthorizer(lggr, syncer), capabilitiesRegistry, lpk, limitsFactory) + vaultCapability, err := vaultcap.NewCapability(lggr, clock, expiryDuration, requestStoreHandler, capabilitiesRegistry, lpk, limitsFactory) if err != nil { return nil, fmt.Errorf("failed to instantiate vault plugin: failed to create vault capability: %w", err) } srvs = append(srvs, vaultCapability) - handler, err := vaultcap.NewGatewayHandler(vaultCapability, gwconnector, d.lggr) + requestAuthorizer := vaultcap.NewRequestAuthorizer(lggr, syncer) + handler, err := vaultcap.NewGatewayHandler(vaultCapability, gwconnector, requestAuthorizer, d.lggr) if err != nil { return nil, fmt.Errorf("failed to instantiate vault plugin: failed to create vault handler: %w", err) }