vault refactor: move request authorization into gw handler

.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.

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 <owner>::<user-provided-id>.
-	// However, the RequestAuthorizer expects just the <user-provided-id> 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 <owner>::<user-provided-id>")
-	}
-	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:  &params,
-	}
-	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),

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)