Skip to content

Commit f8abcbb

Browse files
prashantkumar1982prashantkumar1982
authored
vault refactor: move request authorization into gw handler (#21688)
* vault: move request authorization into gw handler * vault: address auth handler review feedback * vault: move request authorizer closer to use * vault: normalize forwarded request IDs before node auth * vault: avoid protobuf lock copies in auth normalization
1 parent b778c77 commit f8abcbb

6 files changed

Lines changed: 312 additions & 217 deletions

File tree

‎.changeset/vault-auth-gw-handler.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
"chainlink": patch
3+
---
4+
5+
#changed
6+
7+
Move Vault node-side request authorization into the gateway handler and remove duplicated authorization from the Vault capability.

‎core/capabilities/vault/capability.go‎

Lines changed: 0 additions & 144 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,8 @@ package vault
33
import (
44
"context"
55
"encoding/hex"
6-
"encoding/json"
76
"errors"
87
"fmt"
9-
"strconv"
108
"strings"
119
"time"
1210

@@ -17,7 +15,6 @@ import (
1715
"github.com/smartcontractkit/chainlink-common/pkg/capabilities"
1816
vaultcommon "github.com/smartcontractkit/chainlink-common/pkg/capabilities/actions/vault"
1917
"github.com/smartcontractkit/chainlink-common/pkg/capabilities/consensus/requests"
20-
jsonrpc "github.com/smartcontractkit/chainlink-common/pkg/jsonrpc2"
2118
"github.com/smartcontractkit/chainlink-common/pkg/logger"
2219
"github.com/smartcontractkit/chainlink-common/pkg/settings/cresettings"
2320
"github.com/smartcontractkit/chainlink-common/pkg/settings/limits"
@@ -32,7 +29,6 @@ type Capability struct {
3229
clock clockwork.Clock
3330
expiresAfter time.Duration
3431
handler *requests.Handler[*vaulttypes.Request, *vaulttypes.Response]
35-
requestAuthorizer RequestAuthorizer
3632
capabilitiesRegistry core.CapabilitiesRegistry
3733
publicKey *LazyPublicKey
3834
*RequestValidator
@@ -167,24 +163,6 @@ func (s *Capability) CreateSecrets(ctx context.Context, request *vaultcommon.Cre
167163
s.lggr.Debugf("RequestId: [%s] failed validation checks: %s", request.RequestId, err.Error())
168164
return nil, err
169165
}
170-
authorized, owner, err := s.authorizeCreateSecrets(ctx, *request) //nolint:govet // The mutex isn't used
171-
if !authorized || err != nil {
172-
s.lggr.Debugf("Request Id[%s] not authorized for owner: %s", request.RequestId, owner)
173-
return nil, errors.New("request ID: " + request.RequestId + " not authorized: " + err.Error())
174-
}
175-
if !strings.HasPrefix(request.RequestId, owner) {
176-
// Gateway should ensure it prefixes request ids with the owner, to ensure request uniqueness
177-
s.lggr.Debugf("Request ID: [%s] must start with owner address: [%s]", request.RequestId, owner)
178-
return nil, errors.New("request ID: " + request.RequestId + " must start with owner address: " + owner)
179-
}
180-
for idx, req := range request.EncryptedSecrets {
181-
// Ensure that users cannot access secrets belonging to other owners
182-
if req.Id.Owner != owner {
183-
s.lggr.Debugf("Secret ID owner: [%s] does not match authorized owner: [%s]", req.Id.Owner, owner)
184-
return nil, errors.New("secret ID owner: " + req.Id.Owner + " does not match authorized owner: " + owner + " at index " + strconv.Itoa(idx))
185-
}
186-
}
187-
s.lggr.Debugf("Processing authorized and normalized request [%s]", request.String())
188166
return s.handleRequest(ctx, request.RequestId, request)
189167
}
190168

@@ -195,24 +173,6 @@ func (s *Capability) UpdateSecrets(ctx context.Context, request *vaultcommon.Upd
195173
s.lggr.Debugf("RequestId: [%s] failed validation checks: %s", request.RequestId, err.Error())
196174
return nil, err
197175
}
198-
authorized, owner, err := s.authorizeUpdateSecrets(ctx, *request) //nolint:govet // The mutex isn't used
199-
if !authorized || err != nil {
200-
s.lggr.Debugf("Request Id[%s] not authorized for owner: %s", request.RequestId, owner)
201-
return nil, errors.New("request ID: " + request.RequestId + " not authorized: " + err.Error())
202-
}
203-
if !strings.HasPrefix(request.RequestId, owner) {
204-
// Gateway should ensure it prefixes request ids with the owner, to ensure request uniqueness
205-
s.lggr.Debugf("Request ID: [%s] must start with owner address: [%s]", request.RequestId, owner)
206-
return nil, errors.New("request ID: " + request.RequestId + " must start with owner address: " + owner)
207-
}
208-
for idx, req := range request.EncryptedSecrets {
209-
// Ensure that users cannot access secrets belonging to other owners
210-
if req.Id.Owner != owner {
211-
s.lggr.Debugf("Secret ID owner: [%s] does not match authorized owner: [%s]", req.Id.Owner, owner)
212-
return nil, errors.New("secret ID owner: " + req.Id.Owner + " does not match authorized owner: " + owner + " at index " + strconv.Itoa(idx))
213-
}
214-
}
215-
s.lggr.Debugf("Processing authorized and normalized request [%s]", request.String())
216176
return s.handleRequest(ctx, request.RequestId, request)
217177
}
218178

@@ -223,25 +183,6 @@ func (s *Capability) DeleteSecrets(ctx context.Context, request *vaultcommon.Del
223183
s.lggr.Debugf("Request: [%s] failed validation checks: %s", request.String(), err.Error())
224184
return nil, err
225185
}
226-
227-
authorized, owner, err := s.authorizeDeleteSecrets(ctx, *request) //nolint:govet // The mutex isn't used
228-
if !authorized || err != nil {
229-
s.lggr.Debugf("Request Id[%s] not authorized for owner: %s", request.RequestId, owner)
230-
return nil, errors.New("request ID: " + request.RequestId + " not authorized: " + err.Error())
231-
}
232-
if !strings.HasPrefix(request.RequestId, owner) {
233-
// Gateway should ensure it prefixes request ids with the owner, to ensure request uniqueness
234-
s.lggr.Debugf("Request ID: [%s] must start with owner address: [%s]", request.RequestId, owner)
235-
return nil, errors.New("request ID: " + request.RequestId + " must start with owner address: " + owner)
236-
}
237-
for idx, req := range request.Ids {
238-
// Ensure that users cannot access secrets belonging to other owners
239-
if req.Owner != owner {
240-
s.lggr.Debugf("Secret ID owner: [%s] does not match authorized owner: [%s]", req.Owner, owner)
241-
return nil, errors.New("secret ID owner: " + req.Owner + " does not match authorized owner: " + owner + " at index " + strconv.Itoa(idx))
242-
}
243-
}
244-
s.lggr.Debugf("Processing authorized and normalized request [%s]", request.String())
245186
return s.handleRequest(ctx, request.RequestId, request)
246187
}
247188

@@ -263,25 +204,6 @@ func (s *Capability) ListSecretIdentifiers(ctx context.Context, request *vaultco
263204
s.lggr.Debugf("Request: [%s] failed validation checks: %s", request.String(), err.Error())
264205
return nil, err
265206
}
266-
267-
authorized, owner, err := s.authorizeListSecrets(ctx, *request) //nolint:govet // The mutex isn't used
268-
if !authorized || err != nil {
269-
s.lggr.Debugf("Request ID[%s] not authorized for owner: %s", request.RequestId, owner)
270-
return nil, errors.New("request ID: " + request.RequestId + " not authorized: " + err.Error())
271-
}
272-
if !strings.HasPrefix(request.RequestId, owner) {
273-
// Gateway should ensure it prefixes request ids with the owner, to ensure request uniqueness
274-
s.lggr.Debugf("Request ID: [%s] must start with owner address: [%s]", request.RequestId, owner)
275-
return nil, errors.New("request ID: " + request.RequestId + " must start with owner address: " + owner)
276-
}
277-
// Ensures that users cannot access secrets belonging to other owners
278-
request.Owner = owner
279-
if request.Owner != owner {
280-
s.lggr.Debugf("Secret ID owner: [%s] does not match authorized owner: [%s]", request.Owner, owner)
281-
return nil, errors.New("secret ID owner: " + request.Owner + " does not match authorized owner: " + owner)
282-
}
283-
284-
s.lggr.Debugf("Processing authorized and normalized request [%s]", request.String())
285207
return s.handleRequest(ctx, request.RequestId, request)
286208
}
287209

@@ -334,76 +256,11 @@ func (s *Capability) handleRequest(ctx context.Context, requestID string, reques
334256
}
335257
}
336258

337-
func (s *Capability) getOriginalRequestID(transformedRequestID string) (string, error) {
338-
// The transformed RequestID provided to Vault Nodes is of format <owner>::<user-provided-id>.
339-
// However, the RequestAuthorizer expects just the <user-provided-id> as the JSONRequest's ID fields,
340-
// since that's what was used by the caller when generating the request digest.
341-
requestIDParts := strings.Split(transformedRequestID, vaulttypes.RequestIDSeparator)
342-
if len(requestIDParts) != 2 {
343-
return "", errors.New("internal error: request ID must be in format <owner>::<user-provided-id>")
344-
}
345-
return requestIDParts[1], nil
346-
}
347-
348-
func (s *Capability) authorizeCreateSecrets(ctx context.Context, request vaultcommon.CreateSecretsRequest) (bool, string, error) { //nolint:govet // The mutex isn't used
349-
originalRequestID, err := s.getOriginalRequestID(request.RequestId)
350-
if err != nil {
351-
return false, "", err
352-
}
353-
request.RequestId = originalRequestID
354-
355-
return s.isAuthorizedRequest(ctx, &request, originalRequestID, vaulttypes.MethodSecretsCreate)
356-
}
357-
358-
func (s *Capability) authorizeUpdateSecrets(ctx context.Context, request vaultcommon.UpdateSecretsRequest) (bool, string, error) { //nolint:govet // The mutex isn't used
359-
originalRequestID, err := s.getOriginalRequestID(request.RequestId)
360-
if err != nil {
361-
return false, "", err
362-
}
363-
request.RequestId = originalRequestID
364-
return s.isAuthorizedRequest(ctx, &request, originalRequestID, vaulttypes.MethodSecretsUpdate)
365-
}
366-
367-
func (s *Capability) authorizeDeleteSecrets(ctx context.Context, request vaultcommon.DeleteSecretsRequest) (bool, string, error) { //nolint:govet // The mutex isn't used
368-
originalRequestID, err := s.getOriginalRequestID(request.RequestId)
369-
if err != nil {
370-
return false, "", err
371-
}
372-
request.RequestId = originalRequestID
373-
return s.isAuthorizedRequest(ctx, &request, originalRequestID, vaulttypes.MethodSecretsDelete)
374-
}
375-
376-
func (s *Capability) authorizeListSecrets(ctx context.Context, request vaultcommon.ListSecretIdentifiersRequest) (bool, string, error) { //nolint:govet // The mutex isn't used
377-
originalRequestID, err := s.getOriginalRequestID(request.RequestId)
378-
if err != nil {
379-
return false, "", err
380-
}
381-
request.RequestId = originalRequestID
382-
return s.isAuthorizedRequest(ctx, &request, originalRequestID, vaulttypes.MethodSecretsList)
383-
}
384-
385-
func (s *Capability) isAuthorizedRequest(ctx context.Context, request any, requestID, method string) (bool, string, error) {
386-
var params json.RawMessage
387-
params, err := json.Marshal(request)
388-
if err != nil {
389-
return false, "", fmt.Errorf("could not marshal CreateSecretsRequest: %w", err)
390-
}
391-
s.lggr.Debugw("Authorizing request", "method", method, "requestID", requestID)
392-
jsonRequest := jsonrpc.Request[json.RawMessage]{
393-
Version: jsonrpc.JsonRpcVersion,
394-
ID: requestID,
395-
Method: method,
396-
Params: &params,
397-
}
398-
return s.requestAuthorizer.AuthorizeRequest(ctx, jsonRequest)
399-
}
400-
401259
func NewCapability(
402260
lggr logger.Logger,
403261
clock clockwork.Clock,
404262
expiresAfter time.Duration,
405263
handler *requests.Handler[*vaulttypes.Request, *vaulttypes.Response],
406-
requestAuthorizer RequestAuthorizer,
407264
capabilitiesRegistry core.CapabilitiesRegistry,
408265
publicKey *LazyPublicKey,
409266
limitsFactory limits.Factory,
@@ -417,7 +274,6 @@ func NewCapability(
417274
clock: clock,
418275
expiresAfter: expiresAfter,
419276
handler: handler,
420-
requestAuthorizer: requestAuthorizer,
421277
capabilitiesRegistry: capabilitiesRegistry,
422278
publicKey: publicKey,
423279
RequestValidator: NewRequestValidator(limiter),

‎core/capabilities/vault/capability_test.go‎

Lines changed: 8 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import (
1010
"github.com/ethereum/go-ethereum/common"
1111
"github.com/jonboulle/clockwork"
1212
"github.com/stretchr/testify/assert"
13-
"github.com/stretchr/testify/mock"
1413
"github.com/stretchr/testify/require"
1514
"google.golang.org/protobuf/proto"
1615
"google.golang.org/protobuf/types/known/anypb"
@@ -24,7 +23,6 @@ import (
2423
"github.com/smartcontractkit/chainlink-common/pkg/settings/cresettings"
2524
"github.com/smartcontractkit/chainlink-common/pkg/settings/limits"
2625
coreCapabilities "github.com/smartcontractkit/chainlink/v2/core/capabilities"
27-
vaultcapmocks "github.com/smartcontractkit/chainlink/v2/core/capabilities/vault/mocks"
2826
"github.com/smartcontractkit/chainlink/v2/core/capabilities/vault/vaulttypes"
2927
"github.com/smartcontractkit/chainlink/v2/core/logger"
3028
)
@@ -35,10 +33,9 @@ func TestCapability_CapabilityCall(t *testing.T) {
3533
expiry := 10 * time.Second
3634
store := requests.NewStore[*vaulttypes.Request]()
3735
handler := requests.NewHandler[*vaulttypes.Request, *vaulttypes.Response](lggr, store, clock, expiry)
38-
requestAuthorizer := vaultcapmocks.NewRequestAuthorizer(t)
3936
reg := coreCapabilities.NewRegistry(lggr)
4037
lf := limits.Factory{Settings: cresettings.DefaultGetter}
41-
capability, err := NewCapability(lggr, clock, expiry, handler, requestAuthorizer, reg, nil, lf)
38+
capability, err := NewCapability(lggr, clock, expiry, handler, reg, nil, lf)
4239
require.NoError(t, err)
4340
servicetest.Run(t, capability)
4441

@@ -133,10 +130,9 @@ func TestCapability_CapabilityCall_DuringSubscriptionPhase(t *testing.T) {
133130
expiry := 10 * time.Second
134131
store := requests.NewStore[*vaulttypes.Request]()
135132
handler := requests.NewHandler[*vaulttypes.Request, *vaulttypes.Response](lggr, store, clock, expiry)
136-
requestAuthorizer := vaultcapmocks.NewRequestAuthorizer(t)
137133
reg := coreCapabilities.NewRegistry(lggr)
138134
lf := limits.Factory{Settings: cresettings.DefaultGetter}
139-
capability, err := NewCapability(lggr, clock, expiry, handler, requestAuthorizer, reg, nil, lf)
135+
capability, err := NewCapability(lggr, clock, expiry, handler, reg, nil, lf)
140136
require.NoError(t, err)
141137
servicetest.Run(t, capability)
142138

@@ -304,10 +300,9 @@ func TestCapability_CapabilityCall_SecretIdentifierOwnerMismatch(t *testing.T) {
304300
expiry := 10 * time.Second
305301
store := requests.NewStore[*vaulttypes.Request]()
306302
handler := requests.NewHandler[*vaulttypes.Request, *vaulttypes.Response](lggr, store, clock, expiry)
307-
requestAuthorizer := vaultcapmocks.NewRequestAuthorizer(t)
308303
reg := coreCapabilities.NewRegistry(lggr)
309304
lf := limits.Factory{Settings: cresettings.DefaultGetter}
310-
capability, err := NewCapability(lggr, clock, expiry, handler, requestAuthorizer, reg, nil, lf)
305+
capability, err := NewCapability(lggr, clock, expiry, handler, reg, nil, lf)
311306
require.NoError(t, err)
312307
servicetest.Run(t, capability)
313308

@@ -382,10 +377,9 @@ func TestCapability_CapabilityCall_ReturnsIncorrectType(t *testing.T) {
382377
expiry := 10 * time.Second
383378
store := requests.NewStore[*vaulttypes.Request]()
384379
handler := requests.NewHandler[*vaulttypes.Request, *vaulttypes.Response](lggr, store, clock, expiry)
385-
requestAuthorizer := vaultcapmocks.NewRequestAuthorizer(t)
386380
reg := coreCapabilities.NewRegistry(lggr)
387381
lf := limits.Factory{Settings: cresettings.DefaultGetter}
388-
capability, err := NewCapability(lggr, clock, expiry, handler, requestAuthorizer, reg, nil, lf)
382+
capability, err := NewCapability(lggr, clock, expiry, handler, reg, nil, lf)
389383
require.NoError(t, err)
390384
servicetest.Run(t, capability)
391385

@@ -457,10 +451,9 @@ func TestCapability_CapabilityCall_TimeOut(t *testing.T) {
457451
expiry := 10 * time.Second
458452
store := requests.NewStore[*vaulttypes.Request]()
459453
handler := requests.NewHandler[*vaulttypes.Request, *vaulttypes.Response](lggr, store, fakeClock, expiry)
460-
requestAuthorizer := vaultcapmocks.NewRequestAuthorizer(t)
461454
reg := coreCapabilities.NewRegistry(lggr)
462455
lf := limits.Factory{Settings: cresettings.DefaultGetter}
463-
capability, err := NewCapability(lggr, fakeClock, expiry, handler, requestAuthorizer, reg, nil, lf)
456+
capability, err := NewCapability(lggr, fakeClock, expiry, handler, reg, nil, lf)
464457
require.NoError(t, err)
465458
servicetest.Run(t, capability)
466459

@@ -1064,24 +1057,6 @@ func TestCapability_CRUD(t *testing.T) {
10641057
return capability.DeleteSecrets(t.Context(), req)
10651058
},
10661059
},
1067-
{
1068-
name: "DeleteSecrets_Invalid_Owner",
1069-
response: nil,
1070-
error: "secret ID owner: random does not match authorized owner:",
1071-
call: func(t *testing.T, capability *Capability) (*vaulttypes.Response, error) {
1072-
req := &vault.DeleteSecretsRequest{
1073-
RequestId: requestID,
1074-
Ids: []*vault.SecretIdentifier{
1075-
{
1076-
Key: "Foo",
1077-
Namespace: "Bar",
1078-
Owner: "random",
1079-
},
1080-
},
1081-
}
1082-
return capability.DeleteSecrets(t.Context(), req)
1083-
},
1084-
},
10851060
{
10861061
name: "DeleteSecrets_Invalid_Duplicates",
10871062
error: "duplicate secret ID found",
@@ -1180,11 +1155,9 @@ func TestCapability_CRUD(t *testing.T) {
11801155
expiry := 10 * time.Second
11811156
store := requests.NewStore[*vaulttypes.Request]()
11821157
handler := requests.NewHandler[*vaulttypes.Request, *vaulttypes.Response](lggr, store, clock, expiry)
1183-
requestAuthorizer := vaultcapmocks.NewRequestAuthorizer(t)
1184-
requestAuthorizer.On("AuthorizeRequest", t.Context(), mock.Anything).Return(true, owner, nil).Maybe()
11851158
reg := coreCapabilities.NewRegistry(lggr)
11861159
lf := limits.Factory{Settings: cresettings.DefaultGetter}
1187-
capability, err := NewCapability(lggr, clock, expiry, handler, requestAuthorizer, reg, lpk, lf)
1160+
capability, err := NewCapability(lggr, clock, expiry, handler, reg, lpk, lf)
11881161
require.NoError(t, err)
11891162
servicetest.Run(t, capability)
11901163

@@ -1230,11 +1203,9 @@ func TestCapability_Lifecycle(t *testing.T) {
12301203
expiry := 10 * time.Second
12311204
store := requests.NewStore[*vaulttypes.Request]()
12321205
handler := requests.NewHandler[*vaulttypes.Request, *vaulttypes.Response](lggr, store, clock, expiry)
1233-
requestAuthorizer := vaultcapmocks.NewRequestAuthorizer(t)
1234-
requestAuthorizer.On("AuthorizeRequest", t.Context(), mock.Anything).Return(true, "owner", nil).Maybe()
12351206
reg := coreCapabilities.NewRegistry(lggr)
12361207
lf := limits.Factory{Settings: cresettings.DefaultGetter}
1237-
capability, err := NewCapability(lggr, clock, expiry, handler, requestAuthorizer, reg, nil, lf)
1208+
capability, err := NewCapability(lggr, clock, expiry, handler, reg, nil, lf)
12381209
require.NoError(t, err)
12391210

12401211
_, err = reg.GetExecutable(t.Context(), vault.CapabilityID)
@@ -1262,11 +1233,10 @@ func TestCapability_PublicKeyGet(t *testing.T) {
12621233
expiry := 10 * time.Second
12631234
store := requests.NewStore[*vaulttypes.Request]()
12641235
handler := requests.NewHandler[*vaulttypes.Request, *vaulttypes.Response](lggr, store, clock, expiry)
1265-
requestAuthorizer := vaultcapmocks.NewRequestAuthorizer(t)
12661236
reg := coreCapabilities.NewRegistry(lggr)
12671237
lpk := NewLazyPublicKey()
12681238
lf := limits.Factory{Settings: cresettings.DefaultGetter}
1269-
capability, err := NewCapability(lggr, clock, expiry, handler, requestAuthorizer, reg, lpk, lf)
1239+
capability, err := NewCapability(lggr, clock, expiry, handler, reg, lpk, lf)
12701240
require.NoError(t, err)
12711241
servicetest.Run(t, capability)
12721242

0 commit comments

Comments
 (0)