From a117e7381418ae014c172a441190d0d56f7b4a68 Mon Sep 17 00:00:00 2001 From: Prashant Yadav <34992934+prashantkumar1982@users.noreply.github.com> Date: Sat, 2 May 2026 15:17:29 -0700 Subject: [PATCH] Fix vault secret label validation (#22257) * Fix vault secret label validation * Fix vault lint issues * code fixes for failing tests * code fixes for failing tests * lint * tests * tests lint * debug logs * token leeway * Allow org-only JWT vault requests --- core/capabilities/vault/capability.go | 79 ++++- core/capabilities/vault/gw_handler.go | 26 -- core/capabilities/vault/gw_handler_test.go | 18 +- core/capabilities/vault/jwt_based_auth.go | 91 +++++- .../capabilities/vault/jwt_based_auth_test.go | 178 ++++++++++-- .../org_id_to_workflow_owner_linker_test.go | 269 +++++++++++++++++- core/capabilities/vault/validator.go | 61 ++-- core/capabilities/vault/validator_test.go | 44 ++- .../gateway/handlers/vault/handler.go | 86 ++++-- .../gateway/handlers/vault/handler_test.go | 168 ++++++++++- .../plugins/vault/kvstore_wrapper_test.go | 20 ++ core/services/ocr2/plugins/vault/plugin.go | 27 +- .../ocr2/plugins/vault/plugin_test.go | 60 ++++ core/services/workflows/v2/secrets.go | 13 +- core/services/workflows/v2/secrets_test.go | 56 +++- system-tests/lib/cre/features/vault/vault.go | 16 -- system-tests/lib/cre/workflow/secrets.go | 14 +- .../tests/smoke/cre/v2_vault_don_test.go | 134 +++++++-- .../smoke/cre/v2_vault_don_test_helpers.go | 55 +++- 19 files changed, 1230 insertions(+), 185 deletions(-) diff --git a/core/capabilities/vault/capability.go b/core/capabilities/vault/capability.go index bf6d96a4858..79720b57799 100644 --- a/core/capabilities/vault/capability.go +++ b/core/capabilities/vault/capability.go @@ -169,33 +169,41 @@ func (s *Capability) Execute(ctx context.Context, request capabilities.Capabilit func (s *Capability) CreateSecrets(ctx context.Context, request *vaultcommon.CreateSecretsRequest) (*vaulttypes.Response, error) { s.lggr.Debugf("Received Request: %s", request.String()) - err := s.ValidateCreateSecretsRequest(ctx, s.publicKey.Get(), request) - if err != nil { - s.lggr.Debugf("RequestId: [%s] failed validation checks: %s", request.RequestId, err.Error()) - return nil, err - } resolvedIdentity, err := s.resolveRequestIdentity(ctx, request.OrgId, request.WorkflowOwner) if err != nil { return nil, err } request.OrgId = resolvedIdentity.OrgID request.WorkflowOwner = resolvedIdentity.WorkflowOwner + if ownerErr := validateEncryptedSecretOwnersMatchResolvedIdentity(request.EncryptedSecrets, resolvedIdentity); ownerErr != nil { + s.lggr.Debugf("RequestId: [%s] failed identity owner checks: %s", request.RequestId, ownerErr.Error()) + return nil, ownerErr + } + err = s.ValidateCreateSecretsRequest(ctx, s.publicKey.Get(), request, false) + if err != nil { + s.lggr.Debugf("RequestId: [%s] failed validation checks: %s", request.RequestId, err.Error()) + return nil, err + } return s.handleRequest(ctx, request.RequestId, request) } func (s *Capability) UpdateSecrets(ctx context.Context, request *vaultcommon.UpdateSecretsRequest) (*vaulttypes.Response, error) { s.lggr.Debugf("Received Request: %s", request.String()) - err := s.ValidateUpdateSecretsRequest(ctx, s.publicKey.Get(), request) - if err != nil { - s.lggr.Debugf("RequestId: [%s] failed validation checks: %s", request.RequestId, err.Error()) - return nil, err - } resolvedIdentity, err := s.resolveRequestIdentity(ctx, request.OrgId, request.WorkflowOwner) if err != nil { return nil, err } request.OrgId = resolvedIdentity.OrgID request.WorkflowOwner = resolvedIdentity.WorkflowOwner + if ownerErr := validateEncryptedSecretOwnersMatchResolvedIdentity(request.EncryptedSecrets, resolvedIdentity); ownerErr != nil { + s.lggr.Debugf("RequestId: [%s] failed identity owner checks: %s", request.RequestId, ownerErr.Error()) + return nil, ownerErr + } + err = s.ValidateUpdateSecretsRequest(ctx, s.publicKey.Get(), request, false) + if err != nil { + s.lggr.Debugf("RequestId: [%s] failed validation checks: %s", request.RequestId, err.Error()) + return nil, err + } return s.handleRequest(ctx, request.RequestId, request) } @@ -212,6 +220,10 @@ func (s *Capability) DeleteSecrets(ctx context.Context, request *vaultcommon.Del } request.OrgId = resolvedIdentity.OrgID request.WorkflowOwner = resolvedIdentity.WorkflowOwner + if err := validateSecretIdentifierOwnersMatchResolvedIdentity(request.Ids, resolvedIdentity); err != nil { + s.lggr.Debugf("Request: [%s] failed identity owner checks: %s", request.String(), err.Error()) + return nil, err + } return s.handleRequest(ctx, request.RequestId, request) } @@ -239,6 +251,10 @@ func (s *Capability) ListSecretIdentifiers(ctx context.Context, request *vaultco } request.OrgId = resolvedIdentity.OrgID request.WorkflowOwner = resolvedIdentity.WorkflowOwner + if err := validateOwnerMatchesResolvedIdentity("owner", request.Owner, resolvedIdentity); err != nil { + s.lggr.Debugf("Request: [%s] failed identity owner checks: %s", request.String(), err.Error()) + return nil, err + } return s.handleRequest(ctx, request.RequestId, request) } @@ -267,6 +283,47 @@ func normalizeOwner(owner string) string { return strings.ToLower(strings.TrimPrefix(owner, "0x")) } +func validateEncryptedSecretOwnersMatchResolvedIdentity(encryptedSecrets []*vaultcommon.EncryptedSecret, resolvedIdentity LinkedVaultRequestIdentity) error { + for idx, encryptedSecret := range encryptedSecrets { + if encryptedSecret == nil || encryptedSecret.Id == nil { + continue + } + if err := validateOwnerMatchesResolvedIdentity(fmt.Sprintf("encrypted secret owner at index %d", idx), encryptedSecret.Id.Owner, resolvedIdentity); err != nil { + return err + } + } + + return nil +} + +func validateSecretIdentifierOwnersMatchResolvedIdentity(ids []*vaultcommon.SecretIdentifier, resolvedIdentity LinkedVaultRequestIdentity) error { + for idx, id := range ids { + if id == nil { + continue + } + if err := validateOwnerMatchesResolvedIdentity(fmt.Sprintf("secret identifier owner at index %d", idx), id.Owner, resolvedIdentity); err != nil { + return err + } + } + + return nil +} + +func validateOwnerMatchesResolvedIdentity(field string, owner string, resolvedIdentity LinkedVaultRequestIdentity) error { + if resolvedIdentity.WorkflowOwner == "" && resolvedIdentity.OrgID == "" { + return nil + } + + if resolvedIdentity.WorkflowOwner != "" && normalizeOwner(owner) == normalizeOwner(resolvedIdentity.WorkflowOwner) { + return nil + } + if resolvedIdentity.OrgID != "" && owner == resolvedIdentity.OrgID { + return nil + } + + return fmt.Errorf("%s %q must match resolved workflow owner %q or org_id %q", field, owner, resolvedIdentity.WorkflowOwner, resolvedIdentity.OrgID) +} + func (s *Capability) handleRequest(ctx context.Context, requestID string, request proto.Message) (*vaulttypes.Response, error) { respCh := make(chan *vaulttypes.Response, 1) s.handler.SendRequest(ctx, &vaulttypes.Request{ @@ -314,7 +371,7 @@ func NewCapability( orgResolver orgresolver.OrgResolver, limitsFactory limits.Factory, ) (*Capability, error) { - limiter, err := limits.MakeBoundLimiter(limitsFactory, cresettings.Default.VaultRequestBatchSizeLimit) + limiter, err := limits.MakeUpperBoundLimiter(limitsFactory, cresettings.Default.VaultRequestBatchSizeLimit) if err != nil { return nil, fmt.Errorf("could not create request batch size limiter: %w", err) } diff --git a/core/capabilities/vault/gw_handler.go b/core/capabilities/vault/gw_handler.go index f15627b2e91..595e0017075 100644 --- a/core/capabilities/vault/gw_handler.go +++ b/core/capabilities/vault/gw_handler.go @@ -239,11 +239,6 @@ func (h *GatewayHandler) authorizeAndPrefixRequest(ctx context.Context, req *jso return nil, authErr } authorizedOwner := authResult.AuthorizedOwner() - if incomingOwner != "" && normalizeOwner(incomingOwner) != normalizeOwner(authorizedOwner) { - prefixErr := fmt.Errorf("request owner prefix %q does not match authorized owner %q", incomingOwner, authorizedOwner) - h.lggr.Errorw("gateway request owner prefix mismatch", "method", req.Method, "requestID", originalRequestID, "incomingOwner", incomingOwner, "authorizedOwner", authorizedOwner, "error", prefixErr) - return nil, prefixErr - } req.ID = authorizedOwner + vaulttypes.RequestIDSeparator + originalRequestID h.lggr.Debugw("authorized gateway request", "method", req.Method, "requestID", req.ID, "owner", authorizedOwner, "orgID", authResult.OrgID(), "workflowOwner", authResult.WorkflowOwner()) @@ -334,17 +329,10 @@ func (h *GatewayHandler) handleSecretsCreate(ctx context.Context, gatewayID stri return h.errorResponse(ctx, gatewayID, req, api.UserMessageParseError, err) } - authorizedOwner := authResult.AuthorizedOwner() vaultCapRequest.RequestId = req.ID if err := setAuthorizedIdentityFields(&vaultCapRequest, authResult); err != nil { return h.errorResponse(ctx, gatewayID, req, api.FatalError, err) } - for idx, encryptedSecret := range vaultCapRequest.EncryptedSecrets { - if encryptedSecret != nil && encryptedSecret.Id != nil && normalizeOwner(encryptedSecret.Id.Owner) != normalizeOwner(authorizedOwner) { - h.lggr.Debugw("create secrets request owner mismatch", "requestID", req.ID, "secretOwner", encryptedSecret.Id.Owner, "authorizedOwner", authorizedOwner, "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, authorizedOwner, idx)) - } - } h.lggr.Debugf("Processing authorized and normalized create secrets request [%s]", vaultCapRequest.String()) vaultCapResponse, err := h.secretsService.CreateSecrets(ctx, &vaultCapRequest) @@ -364,17 +352,10 @@ func (h *GatewayHandler) handleSecretsUpdate(ctx context.Context, gatewayID stri if err := json.Unmarshal(*req.Params, &vaultCapRequest); err != nil { return h.errorResponse(ctx, gatewayID, req, api.UserMessageParseError, err) } - authorizedOwner := authResult.AuthorizedOwner() vaultCapRequest.RequestId = req.ID if err := setAuthorizedIdentityFields(&vaultCapRequest, authResult); err != nil { return h.errorResponse(ctx, gatewayID, req, api.FatalError, err) } - for idx, encryptedSecret := range vaultCapRequest.EncryptedSecrets { - if encryptedSecret != nil && encryptedSecret.Id != nil && normalizeOwner(encryptedSecret.Id.Owner) != normalizeOwner(authorizedOwner) { - h.lggr.Debugw("update secrets request owner mismatch", "requestID", req.ID, "secretOwner", encryptedSecret.Id.Owner, "authorizedOwner", authorizedOwner, "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, authorizedOwner, idx)) - } - } h.lggr.Debugf("Processing authorized and normalized update secrets request [%s]", vaultCapRequest.String()) vaultCapResponse, err := h.secretsService.UpdateSecrets(ctx, &vaultCapRequest) @@ -394,17 +375,10 @@ func (h *GatewayHandler) handleSecretsDelete(ctx context.Context, gatewayID stri if err := json.Unmarshal(*req.Params, r); err != nil { return h.errorResponse(ctx, gatewayID, req, api.UserMessageParseError, err) } - authorizedOwner := authResult.AuthorizedOwner() r.RequestId = req.ID if err := setAuthorizedIdentityFields(r, authResult); err != nil { return h.errorResponse(ctx, gatewayID, req, api.FatalError, err) } - for idx, secretID := range r.Ids { - if secretID != nil && normalizeOwner(secretID.Owner) != normalizeOwner(authorizedOwner) { - h.lggr.Debugw("delete secrets request owner mismatch", "requestID", req.ID, "secretOwner", secretID.Owner, "authorizedOwner", authorizedOwner, "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, authorizedOwner, idx)) - } - } h.lggr.Debugf("Processing authorized and normalized delete secrets request [%s]", r.String()) resp, err := h.secretsService.DeleteSecrets(ctx, r) diff --git a/core/capabilities/vault/gw_handler_test.go b/core/capabilities/vault/gw_handler_test.go index f71b3ea6daa..050a8ed4a73 100644 --- a/core/capabilities/vault/gw_handler_test.go +++ b/core/capabilities/vault/gw_handler_test.go @@ -419,7 +419,7 @@ func TestGatewayHandler_HandleGatewayMessage(t *testing.T) { expectedError: false, }, { - name: "success - strips owner prefix from forwarded request before authorization", + name: "success - replaces owner prefix from forwarded request after authorization", setupMocks: func(ss *vaulttypesmocks.SecretsService, gc *connector_mocks.GatewayConnector, ra *vaultcapmocks.Authorizer) { 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 { @@ -448,10 +448,10 @@ func TestGatewayHandler_HandleGatewayMessage(t *testing.T) { }, request: &jsonrpc.Request[json.RawMessage]{ Method: vaulttypes.MethodSecretsCreate, - ID: "0xAbC" + vaulttypes.RequestIDSeparator + "1", + ID: "0xDef" + vaulttypes.RequestIDSeparator + "1", Params: func() *json.RawMessage { params, _ := json.Marshal(vaultcommon.CreateSecretsRequest{ - RequestId: "0xAbC" + vaulttypes.RequestIDSeparator + "1", + RequestId: "0xDef" + vaulttypes.RequestIDSeparator + "1", EncryptedSecrets: []*vaultcommon.EncryptedSecret{ { Id: &vaultcommon.SecretIdentifier{ @@ -469,13 +469,21 @@ func TestGatewayHandler_HandleGatewayMessage(t *testing.T) { expectedError: false, }, { - name: "failure - owner mismatch against authorized owner", + name: "failure - capability rejects owner mismatch", setupMocks: func(ss *vaulttypesmocks.SecretsService, gc *connector_mocks.GatewayConnector, ra *vaultcapmocks.Authorizer) { ra.EXPECT().AuthorizeRequest(mock.Anything, mock.Anything).Return(authResult("", "0xdef"), 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.Owner == "0xabc" && + req.RequestId == "0xdef"+vaulttypes.RequestIDSeparator+"1" && + req.OrgId == "" && + req.WorkflowOwner == "0xdef" + })).Return(nil, errors.New("capability owner validation failed")) 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` + resp.Error.Message == "capability owner validation failed" })).Return(nil) }, request: &jsonrpc.Request[json.RawMessage]{ diff --git a/core/capabilities/vault/jwt_based_auth.go b/core/capabilities/vault/jwt_based_auth.go index 06787741b25..eb6cdce8bf9 100644 --- a/core/capabilities/vault/jwt_based_auth.go +++ b/core/capabilities/vault/jwt_based_auth.go @@ -16,11 +16,13 @@ import ( "github.com/golang-jwt/jwt/v5" + 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/services" "github.com/smartcontractkit/chainlink-common/pkg/settings/cresettings" "github.com/smartcontractkit/chainlink-common/pkg/settings/limits" + "github.com/smartcontractkit/chainlink/v2/core/capabilities/vault/vaulttypes" ) var ( @@ -79,7 +81,7 @@ type jsonWebKeySet struct { // JWTBasedAuth verifies Auth0-issued RS256 JWTs using the provider's // public JWKS endpoint and extracts Vault-specific claims (org_id, -// workflow_owner, request_digest). It is safe for concurrent use. +// optional workflow_owner, request_digest). It is safe for concurrent use. // // JWKS keys are fetched lazily on the first token validation and refreshed // on key-ID misses, rate-limited to at most once per JWKSRefreshInterval. @@ -220,6 +222,14 @@ func (v *jwtBasedAuth) AuthorizeRequest(ctx context.Context, req jsonrpc.Request return nil, fmt.Errorf("request digest mismatch: computed=%s claimed=%s", requestDigest, claims.RequestDigest) } + if claims.WorkflowOwner == "" { + if err := validateOrgIDOwnedVaultRequest(req, claims.OrgID); err != nil { + wrappedErr := fmt.Errorf("%w: %w", ErrMissingWorkflowOwner, err) + v.lggr.Debugw("JWTBasedAuth missing workflow owner rejected non-org-owned request", "method", req.Method, "requestID", req.ID, "orgID", claims.OrgID, "error", wrappedErr) + return nil, fmt.Errorf("invalid JWT auth token: %w", wrappedErr) + } + } + v.lggr.Debugw("JWTBasedAuth authorization succeeded", "method", req.Method, "requestID", req.ID, "orgID", claims.OrgID, "workflowOwner", claims.WorkflowOwner, "digest", requestDigest, "expiresAt", claims.ExpiresAt.UTC().Unix()) return &AuthResult{ orgID: claims.OrgID, @@ -231,7 +241,7 @@ func (v *jwtBasedAuth) AuthorizeRequest(ctx context.Context, req jsonrpc.Request // validateToken verifies the JWT signature via Auth0 JWKS, validates // standard claims (iss, aud, exp), and extracts Vault-specific claims -// (org_id, workflow_owner, request_digest). +// (org_id, optional workflow_owner, request_digest). func (v *jwtBasedAuth) validateToken(ctx context.Context, tokenString string) (*JWTClaims, error) { if tokenString == "" { return nil, ErrMissingToken @@ -262,9 +272,10 @@ func (v *jwtBasedAuth) validateToken(ctx context.Context, tokenString string) (* jwt.WithAudience(v.audience), jwt.WithExpirationRequired(), jwt.WithIssuedAt(), + jwt.WithLeeway(time.Minute), ) if err != nil { - return nil, fmt.Errorf("%w: %w", ErrInvalidToken, err) + return nil, fmt.Errorf("%w: %w. Expected Issuer: %s, Actual Issuer: %s", ErrInvalidToken, err, v.issuerURL, unverified.Claims.(jwt.MapClaims)["iss"]) } claims, ok := token.Claims.(jwt.MapClaims) @@ -332,13 +343,81 @@ func extractAuthorizationDetails(claims jwt.MapClaims) (workflowOwner, requestDi if requestDigest == "" { return "", "", ErrMissingRequestDigest } - if workflowOwner == "" { - return "", "", ErrMissingWorkflowOwner - } return workflowOwner, requestDigest, nil } +func validateOrgIDOwnedVaultRequest(req jsonrpc.Request[json.RawMessage], orgID string) error { + if orgID == "" { + return ErrMissingOrgID + } + if req.Params == nil { + return errors.New("request params are required") + } + + switch req.Method { + case vaulttypes.MethodSecretsCreate: + parsed := &vaultcommon.CreateSecretsRequest{} + if err := json.Unmarshal(*req.Params, parsed); err != nil { + return fmt.Errorf("failed to parse create secrets request: %w", err) + } + return validateEncryptedSecretOwnersMatchOrgID(parsed.EncryptedSecrets, orgID) + case vaulttypes.MethodSecretsUpdate: + parsed := &vaultcommon.UpdateSecretsRequest{} + if err := json.Unmarshal(*req.Params, parsed); err != nil { + return fmt.Errorf("failed to parse update secrets request: %w", err) + } + return validateEncryptedSecretOwnersMatchOrgID(parsed.EncryptedSecrets, orgID) + case vaulttypes.MethodSecretsDelete: + parsed := &vaultcommon.DeleteSecretsRequest{} + if err := json.Unmarshal(*req.Params, parsed); err != nil { + return fmt.Errorf("failed to parse delete secrets request: %w", err) + } + return validateSecretIdentifierOwnersMatchOrgID(parsed.Ids, orgID) + case vaulttypes.MethodSecretsList: + parsed := &vaultcommon.ListSecretIdentifiersRequest{} + if err := json.Unmarshal(*req.Params, parsed); err != nil { + return fmt.Errorf("failed to parse list secrets request: %w", err) + } + if parsed.Owner != orgID { + return fmt.Errorf("list secrets owner %q does not match org_id %q", parsed.Owner, orgID) + } + return nil + default: + return fmt.Errorf("method %q does not carry org-owned secret identifiers", req.Method) + } +} + +func validateEncryptedSecretOwnersMatchOrgID(encryptedSecrets []*vaultcommon.EncryptedSecret, orgID string) error { + if len(encryptedSecrets) == 0 { + return errors.New("encrypted secrets must contain at least one identifier") + } + for idx, encryptedSecret := range encryptedSecrets { + if encryptedSecret == nil || encryptedSecret.Id == nil { + return fmt.Errorf("encrypted secret at index %d must include an identifier", idx) + } + if encryptedSecret.Id.Owner != orgID { + return fmt.Errorf("encrypted secret owner at index %d %q does not match org_id %q", idx, encryptedSecret.Id.Owner, orgID) + } + } + return nil +} + +func validateSecretIdentifierOwnersMatchOrgID(ids []*vaultcommon.SecretIdentifier, orgID string) error { + if len(ids) == 0 { + return errors.New("secret identifiers must not be empty") + } + for idx, id := range ids { + if id == nil { + return fmt.Errorf("secret identifier at index %d must not be nil", idx) + } + if id.Owner != orgID { + return fmt.Errorf("secret identifier owner at index %d %q does not match org_id %q", idx, id.Owner, orgID) + } + } + return nil +} + // resolveSigningKey looks up the RSA public key for the given kid from the // JWKS cache, refreshing the cache if necessary. func (v *jwtBasedAuth) resolveSigningKey(ctx context.Context, kid string) (*rsa.PublicKey, error) { diff --git a/core/capabilities/vault/jwt_based_auth_test.go b/core/capabilities/vault/jwt_based_auth_test.go index ecaae8e4bf8..3a7e614f67c 100644 --- a/core/capabilities/vault/jwt_based_auth_test.go +++ b/core/capabilities/vault/jwt_based_auth_test.go @@ -117,11 +117,11 @@ func createTestJWT(t *testing.T, key testRSAKey, claims jwt.MapClaims) string { func validTestClaims(issuer, audience string) jwt.MapClaims { return jwt.MapClaims{ - "iss": issuer, - "aud": audience, - "exp": jwt.NewNumericDate(time.Now().Add(5 * time.Minute)), - "iat": jwt.NewNumericDate(time.Now()), - "org_id": "org_test123", + "iss": issuer, + "aud": audience, + "exp": jwt.NewNumericDate(time.Now().Add(5 * time.Minute)), + "iat": jwt.NewNumericDate(time.Now()), + "org_id": "org_test123", ClaimVaultSecretManagementEnabled: "true", "authorization_details": []interface{}{ map[string]interface{}{ @@ -167,7 +167,7 @@ func TestJWTBasedAuth_ValidToken(t *testing.T) { assert.False(t, result.ExpiresAt.IsZero()) } -func TestJWTBasedAuth_RejectsTokenWithoutWorkflowOwner(t *testing.T) { +func TestJWTBasedAuth_ValidTokenWithoutWorkflowOwner(t *testing.T) { rsaKey := generateTestRSAKey(t, "key-1") jwksServer := newTestJWKSServer(t, rsaKey) @@ -176,11 +176,11 @@ func TestJWTBasedAuth_RejectsTokenWithoutWorkflowOwner(t *testing.T) { v := newTestValidator(t, issuer, audience) claims := jwt.MapClaims{ - "iss": issuer, - "aud": audience, - "exp": jwt.NewNumericDate(time.Now().Add(5 * time.Minute)), - "iat": jwt.NewNumericDate(time.Now()), - "org_id": "org_no_wfowner", + "iss": issuer, + "aud": audience, + "exp": jwt.NewNumericDate(time.Now().Add(5 * time.Minute)), + "iat": jwt.NewNumericDate(time.Now()), + "org_id": "org_no_wfowner", ClaimVaultSecretManagementEnabled: "true", "authorization_details": []interface{}{ map[string]interface{}{ @@ -192,8 +192,11 @@ func TestJWTBasedAuth_RejectsTokenWithoutWorkflowOwner(t *testing.T) { tokenString := createTestJWT(t, rsaKey, claims) result, err := v.validateToken(context.Background(), tokenString) - require.Nil(t, result) - require.ErrorIs(t, err, ErrMissingWorkflowOwner) + require.NoError(t, err) + require.NotNil(t, result) + require.Equal(t, "org_no_wfowner", result.OrgID) + require.Empty(t, result.WorkflowOwner) + require.Equal(t, "digest456", result.RequestDigest) } func TestJWTBasedAuth_ExpiredToken(t *testing.T) { @@ -406,11 +409,11 @@ func TestJWTBasedAuth_AuthorizationDetailsFromTypedArray(t *testing.T) { v := newTestValidator(t, issuer, audience) claims := jwt.MapClaims{ - "iss": issuer, - "aud": audience, - "exp": jwt.NewNumericDate(time.Now().Add(5 * time.Minute)), - "iat": jwt.NewNumericDate(time.Now()), - "org_id": "org_single", + "iss": issuer, + "aud": audience, + "exp": jwt.NewNumericDate(time.Now().Add(5 * time.Minute)), + "iat": jwt.NewNumericDate(time.Now()), + "org_id": "org_single", ClaimVaultSecretManagementEnabled: "true", "authorization_details": []interface{}{ map[string]interface{}{"type": "request_digest", "value": "single_digest"}, @@ -558,11 +561,11 @@ func TestJWTBasedAuth_AuthorizeCreateRequestFromRawJSON(t *testing.T) { require.NoError(t, err) token := createTestJWT(t, rsaKey, jwt.MapClaims{ - "iss": issuer, - "aud": audience, - "exp": jwt.NewNumericDate(time.Now().Add(5 * time.Minute)), - "iat": jwt.NewNumericDate(time.Now()), - "org_id": "org-123", + "iss": issuer, + "aud": audience, + "exp": jwt.NewNumericDate(time.Now().Add(5 * time.Minute)), + "iat": jwt.NewNumericDate(time.Now()), + "org_id": "org-123", ClaimVaultSecretManagementEnabled: "true", "authorization_details": []interface{}{ map[string]interface{}{ @@ -585,6 +588,135 @@ func TestJWTBasedAuth_AuthorizeCreateRequestFromRawJSON(t *testing.T) { require.Equal(t, digest, authResult.Digest()) } +func TestJWTBasedAuth_AuthorizeCreateRequestWithoutWorkflowOwnerWhenIdentifiersUseOrgID(t *testing.T) { + rsaKey := generateTestRSAKey(t, "key-1") + jwksServer := newTestJWKSServer(t, rsaKey) + + issuer := jwksServer.URL() + "/" + audience := "https://vault.test.chain.link" + v := newTestValidator(t, issuer, audience) + + rawRequest := []byte(`{"jsonrpc":"2.0","id":"req-1","method":"vault.secrets.create","params":{"request_id":"req-1","encrypted_secrets":[{"id":{"key":"7611","namespace":"main","owner":"org-123"},"encrypted_value":"cipher+/=="}]}}`) + req, err := jsonrpc.DecodeRequest[json.RawMessage](rawRequest, "") + require.NoError(t, err) + + digest, err := req.Digest() + require.NoError(t, err) + + token := createTestJWT(t, rsaKey, jwt.MapClaims{ + "iss": issuer, + "aud": audience, + "exp": jwt.NewNumericDate(time.Now().Add(5 * time.Minute)), + "iat": jwt.NewNumericDate(time.Now()), + "org_id": "org-123", + ClaimVaultSecretManagementEnabled: "true", + "authorization_details": []interface{}{ + map[string]interface{}{ + "type": "request_digest", + "value": digest, + }, + }, + }) + + req, err = jsonrpc.DecodeRequest[json.RawMessage](rawRequest, token) + require.NoError(t, err) + + authResult, err := v.AuthorizeRequest(t.Context(), req) + require.NoError(t, err) + require.Equal(t, "org-123", authResult.OrgID()) + require.Empty(t, authResult.WorkflowOwner()) + require.Equal(t, digest, authResult.Digest()) +} + +func TestJWTBasedAuth_RejectsCreateRequestWithoutWorkflowOwnerWhenIdentifierOwnerDiffers(t *testing.T) { + rsaKey := generateTestRSAKey(t, "key-1") + jwksServer := newTestJWKSServer(t, rsaKey) + + issuer := jwksServer.URL() + "/" + audience := "https://vault.test.chain.link" + v := newTestValidator(t, issuer, audience) + + rawRequest := []byte(`{"jsonrpc":"2.0","id":"req-1","method":"vault.secrets.create","params":{"request_id":"req-1","encrypted_secrets":[{"id":{"key":"7611","namespace":"main","owner":"0xAbCd"},"encrypted_value":"cipher+/=="}]}}`) + req, err := jsonrpc.DecodeRequest[json.RawMessage](rawRequest, "") + require.NoError(t, err) + + digest, err := req.Digest() + require.NoError(t, err) + + token := createTestJWT(t, rsaKey, jwt.MapClaims{ + "iss": issuer, + "aud": audience, + "exp": jwt.NewNumericDate(time.Now().Add(5 * time.Minute)), + "iat": jwt.NewNumericDate(time.Now()), + "org_id": "org-123", + ClaimVaultSecretManagementEnabled: "true", + "authorization_details": []interface{}{ + map[string]interface{}{ + "type": "request_digest", + "value": digest, + }, + }, + }) + + req, err = jsonrpc.DecodeRequest[json.RawMessage](rawRequest, token) + require.NoError(t, err) + + authResult, err := v.AuthorizeRequest(t.Context(), req) + require.Nil(t, authResult) + require.Error(t, err) + require.ErrorIs(t, err, ErrMissingWorkflowOwner) + require.ErrorContains(t, err, `encrypted secret owner at index 0 "0xAbCd" does not match org_id "org-123"`) +} + +func TestJWTBasedAuth_ValidateOrgIDOwnedVaultRequest(t *testing.T) { + tests := []struct { + name string + raw string + wantErr string + }{ + { + name: "create org owner", + raw: `{"jsonrpc":"2.0","id":"req-1","method":"vault.secrets.create","params":{"encrypted_secrets":[{"id":{"key":"key","namespace":"main","owner":"org-123"},"encrypted_value":"cipher"}]}}`, + }, + { + name: "update org owner", + raw: `{"jsonrpc":"2.0","id":"req-1","method":"vault.secrets.update","params":{"encrypted_secrets":[{"id":{"key":"key","namespace":"main","owner":"org-123"},"encrypted_value":"cipher"}]}}`, + }, + { + name: "delete org owner", + raw: `{"jsonrpc":"2.0","id":"req-1","method":"vault.secrets.delete","params":{"ids":[{"key":"key","namespace":"main","owner":"org-123"}]}}`, + }, + { + name: "list org owner", + raw: `{"jsonrpc":"2.0","id":"req-1","method":"vault.secrets.list","params":{"owner":"org-123","namespace":"main"}}`, + }, + { + name: "list workflow owner rejected", + raw: `{"jsonrpc":"2.0","id":"req-1","method":"vault.secrets.list","params":{"owner":"0xAbCd","namespace":"main"}}`, + wantErr: `list secrets owner "0xAbCd" does not match org_id "org-123"`, + }, + { + name: "delete workflow owner rejected", + raw: `{"jsonrpc":"2.0","id":"req-1","method":"vault.secrets.delete","params":{"ids":[{"key":"key","namespace":"main","owner":"0xAbCd"}]}}`, + wantErr: `secret identifier owner at index 0 "0xAbCd" does not match org_id "org-123"`, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + req, err := jsonrpc.DecodeRequest[json.RawMessage]([]byte(tc.raw), "") + require.NoError(t, err) + + err = validateOrgIDOwnedVaultRequest(req, "org-123") + if tc.wantErr == "" { + require.NoError(t, err) + return + } + require.ErrorContains(t, err, tc.wantErr) + }) + } +} + func setDefaultGetter(t *testing.T, payload string) { t.Helper() diff --git a/core/capabilities/vault/org_id_to_workflow_owner_linker_test.go b/core/capabilities/vault/org_id_to_workflow_owner_linker_test.go index 47d5c7c93e8..0a7e905add7 100644 --- a/core/capabilities/vault/org_id_to_workflow_owner_linker_test.go +++ b/core/capabilities/vault/org_id_to_workflow_owner_linker_test.go @@ -10,6 +10,9 @@ import ( "github.com/jonboulle/clockwork" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "google.golang.org/protobuf/proto" + + "github.com/smartcontractkit/tdh2/go/tdh2/tdh2easy" vaultcommon "github.com/smartcontractkit/chainlink-common/pkg/capabilities/actions/vault" "github.com/smartcontractkit/chainlink-common/pkg/capabilities/consensus/requests" @@ -19,6 +22,7 @@ import ( "github.com/smartcontractkit/chainlink-common/pkg/settings/limits" coreCapabilities "github.com/smartcontractkit/chainlink/v2/core/capabilities" "github.com/smartcontractkit/chainlink/v2/core/capabilities/vault/vaulttypes" + "github.com/smartcontractkit/chainlink/v2/core/capabilities/vault/vaultutils" "github.com/smartcontractkit/chainlink/v2/core/logger" ) @@ -66,12 +70,13 @@ func TestCapability_ListSecretIdentifiers_OrgIDOnlySkipsResolver(t *testing.T) { resolver := &testOrgResolver{orgID: "unexpected"} payload := captureListRequest(t, "request-2", resolver, true, &vaultcommon.ListSecretIdentifiersRequest{ RequestId: "request-2", - Owner: "0xabc123", + Owner: "org-999", Namespace: "ns", OrgId: "org-999", }) require.NotNil(t, payload) + assert.Equal(t, "org-999", payload.Owner) assert.Equal(t, "org-999", payload.OrgId) assert.Empty(t, payload.WorkflowOwner) assert.Empty(t, resolver.calledWith) @@ -83,13 +88,14 @@ func TestCapability_ListSecretIdentifiers_VerifiesWorkflowOwnerAgainstOrgID(t *t resolver := &testOrgResolver{orgID: "org-999"} payload := captureListRequest(t, "request-verify", resolver, true, &vaultcommon.ListSecretIdentifiersRequest{ RequestId: "request-verify", - Owner: "0xabc123", + Owner: "trusted-owner", Namespace: "ns", OrgId: "org-999", WorkflowOwner: "trusted-owner", }) require.NotNil(t, payload) + assert.Equal(t, "trusted-owner", payload.Owner) assert.Equal(t, "org-999", payload.OrgId) assert.Equal(t, "trusted-owner", payload.WorkflowOwner) assert.Equal(t, []string{"trusted-owner"}, resolver.calledWith) @@ -138,6 +144,198 @@ func TestCapability_ListSecretIdentifiers_GateClosedLeavesFieldsUntouched(t *tes assert.Empty(t, resolver.calledWith) } +func TestCapability_CreateSecrets_ResolvesOrgIDBeforeLabelValidation(t *testing.T) { + t.Parallel() + + orgID := "org-123" + workflowOwner := "0x0001020304050607080900010203040506070809" + encryptedSecret, capability, store := newCapabilityWithOrgIDEncryptedSecret(t, orgID) + + request := &vaultcommon.CreateSecretsRequest{ + RequestId: "request-create", + WorkflowOwner: workflowOwner, + EncryptedSecrets: []*vaultcommon.EncryptedSecret{ + { + Id: &vaultcommon.SecretIdentifier{ + Key: "secret", + Namespace: "main", + Owner: workflowOwner, + }, + EncryptedValue: encryptedSecret, + }, + }, + } + + captured := respondWithCapturedPayload[*vaultcommon.CreateSecretsRequest](t, store, request.RequestId) + _, err := capability.CreateSecrets(t.Context(), request) + require.NoError(t, err) + result := <-captured + require.NoError(t, result.err) + payload := result.payload + + assert.Equal(t, orgID, payload.OrgId) + assert.Equal(t, workflowOwner, payload.WorkflowOwner) +} + +func TestCapability_UpdateSecrets_ResolvesOrgIDBeforeLabelValidation(t *testing.T) { + t.Parallel() + + orgID := "org-123" + workflowOwner := "0x0001020304050607080900010203040506070809" + encryptedSecret, capability, store := newCapabilityWithOrgIDEncryptedSecret(t, orgID) + + request := &vaultcommon.UpdateSecretsRequest{ + RequestId: "request-update", + WorkflowOwner: workflowOwner, + EncryptedSecrets: []*vaultcommon.EncryptedSecret{ + { + Id: &vaultcommon.SecretIdentifier{ + Key: "secret", + Namespace: "main", + Owner: workflowOwner, + }, + EncryptedValue: encryptedSecret, + }, + }, + } + + captured := respondWithCapturedPayload[*vaultcommon.UpdateSecretsRequest](t, store, request.RequestId) + _, err := capability.UpdateSecrets(t.Context(), request) + require.NoError(t, err) + result := <-captured + require.NoError(t, result.err) + payload := result.payload + + assert.Equal(t, orgID, payload.OrgId) + assert.Equal(t, workflowOwner, payload.WorkflowOwner) +} + +func TestCapability_CreateSecrets_AllowsResolvedOrgIDOwner(t *testing.T) { + t.Parallel() + + orgID := "org-123" + workflowOwner := "0x0001020304050607080900010203040506070809" + encryptedSecret, capability, store := newCapabilityWithOrgIDEncryptedSecret(t, orgID) + + request := &vaultcommon.CreateSecretsRequest{ + RequestId: "request-create-org-owner", + WorkflowOwner: workflowOwner, + EncryptedSecrets: []*vaultcommon.EncryptedSecret{ + { + Id: &vaultcommon.SecretIdentifier{ + Key: "secret", + Namespace: "main", + Owner: orgID, + }, + EncryptedValue: encryptedSecret, + }, + }, + } + + captured := respondWithCapturedPayload[*vaultcommon.CreateSecretsRequest](t, store, request.RequestId) + _, err := capability.CreateSecrets(t.Context(), request) + require.NoError(t, err) + result := <-captured + require.NoError(t, result.err) + payload := result.payload + + assert.Equal(t, orgID, payload.OrgId) + assert.Equal(t, workflowOwner, payload.WorkflowOwner) + assert.Equal(t, orgID, payload.EncryptedSecrets[0].Id.Owner) +} + +func TestCapability_RejectsOwnersOutsideResolvedIdentity(t *testing.T) { + t.Parallel() + + orgID := "org-123" + workflowOwner := "0x0001020304050607080900010203040506070809" + encryptedSecret, capability, store := newCapabilityWithOrgIDEncryptedSecret(t, orgID) + + tests := []struct { + name string + requestID string + call func() (*vaulttypes.Response, error) + }{ + { + name: "create", + requestID: "request-create-owner-mismatch", + call: func() (*vaulttypes.Response, error) { + return capability.CreateSecrets(t.Context(), &vaultcommon.CreateSecretsRequest{ + RequestId: "request-create-owner-mismatch", + WorkflowOwner: workflowOwner, + EncryptedSecrets: []*vaultcommon.EncryptedSecret{ + { + Id: &vaultcommon.SecretIdentifier{ + Key: "secret", + Namespace: "main", + Owner: "otherowner", + }, + EncryptedValue: encryptedSecret, + }, + }, + }) + }, + }, + { + name: "update", + requestID: "request-update-owner-mismatch", + call: func() (*vaulttypes.Response, error) { + return capability.UpdateSecrets(t.Context(), &vaultcommon.UpdateSecretsRequest{ + RequestId: "request-update-owner-mismatch", + WorkflowOwner: workflowOwner, + EncryptedSecrets: []*vaultcommon.EncryptedSecret{ + { + Id: &vaultcommon.SecretIdentifier{ + Key: "secret", + Namespace: "main", + Owner: "otherowner", + }, + EncryptedValue: encryptedSecret, + }, + }, + }) + }, + }, + { + name: "delete", + requestID: "request-delete-owner-mismatch", + call: func() (*vaulttypes.Response, error) { + return capability.DeleteSecrets(t.Context(), &vaultcommon.DeleteSecretsRequest{ + RequestId: "request-delete-owner-mismatch", + WorkflowOwner: workflowOwner, + Ids: []*vaultcommon.SecretIdentifier{ + { + Key: "secret", + Namespace: "main", + Owner: "otherowner", + }, + }, + }) + }, + }, + { + name: "list", + requestID: "request-list-owner-mismatch", + call: func() (*vaulttypes.Response, error) { + return capability.ListSecretIdentifiers(t.Context(), &vaultcommon.ListSecretIdentifiersRequest{ + RequestId: "request-list-owner-mismatch", + Owner: "otherowner", + Namespace: "main", + WorkflowOwner: workflowOwner, + }) + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + _, err := tc.call() + require.ErrorContains(t, err, "must match resolved workflow owner") + assert.Empty(t, store.GetByIDs([]string{tc.requestID})) + }) + } +} + func TestCapability_ListSecretIdentifiers_RejectsMissingWorkflowOwnerWhenOrgIDMissing(t *testing.T) { t.Parallel() @@ -220,6 +418,73 @@ func captureListRequest(t *testing.T, requestID string, resolver orgresolver.Org return capturedPayload } +func newCapabilityWithOrgIDEncryptedSecret(t *testing.T, orgID string) (string, *Capability, *requests.Store[*vaulttypes.Request]) { + t.Helper() + + _, pk, _, err := tdh2easy.GenerateKeys(1, 3) + require.NoError(t, err) + encryptedSecret, err := vaultutils.EncryptSecretWithOrgID("secret-value", pk, orgID) + require.NoError(t, err) + + lggr := logger.TestLogger(t) + clock := clockwork.NewFakeClock() + expiry := 10 * time.Second + store := requests.NewStore[*vaulttypes.Request]() + handler := requests.NewHandler[*vaulttypes.Request, *vaulttypes.Response](lggr, store, clock, expiry) + reg := coreCapabilities.NewRegistry(lggr) + lpk := NewLazyPublicKey() + lpk.Set(pk) + + capability, err := NewCapability(lggr, clock, expiry, handler, reg, lpk, &testOrgResolver{orgID: orgID}, newVaultOrgIDAsSecretOwnerLimitsFactory(t, true)) + require.NoError(t, err) + servicetest.Run(t, capability) + + return encryptedSecret, capability, store +} + +type capturedPayload[T proto.Message] struct { + payload T + err error +} + +func respondWithCapturedPayload[T proto.Message](t *testing.T, store *requests.Store[*vaulttypes.Request], requestID string) <-chan capturedPayload[T] { + t.Helper() + + captured := make(chan capturedPayload[T], 1) + go func() { + for { + select { + case <-t.Context().Done(): + return + default: + reqs := store.GetByIDs([]string{requestID}) + if len(reqs) != 1 { + continue + } + + payload, ok := reqs[0].Payload.(T) + if !ok { + captured <- capturedPayload[T]{err: fmt.Errorf("unexpected payload type %T", reqs[0].Payload)} + return + } + + clonedMessage := proto.Clone(payload) + cloned, ok := clonedMessage.(T) + if !ok { + captured <- capturedPayload[T]{err: fmt.Errorf("unexpected cloned payload type %T", clonedMessage)} + return + } + + captured <- capturedPayload[T]{payload: cloned} + reqs[0].SendResponse(t.Context(), &vaulttypes.Response{ID: requestID, Payload: []byte("ok")}) + return + } + } + }() + + return captured +} + func newVaultOrgIDAsSecretOwnerLimitsFactory(t *testing.T, enabled bool) limits.Factory { t.Helper() diff --git a/core/capabilities/vault/validator.go b/core/capabilities/vault/validator.go index 58592e6460a..da5a69cb0f8 100644 --- a/core/capabilities/vault/validator.go +++ b/core/capabilities/vault/validator.go @@ -22,17 +22,17 @@ type RequestValidator struct { MaxCiphertextLengthLimiter limits.BoundLimiter[pkgconfig.Size] } -func (r *RequestValidator) ValidateCreateSecretsRequest(ctx context.Context, publicKey *tdh2easy.PublicKey, request *vaultcommon.CreateSecretsRequest) error { - return r.validateWriteRequest(ctx, publicKey, request.RequestId, request.OrgId, request.WorkflowOwner, request.EncryptedSecrets) +func (r *RequestValidator) ValidateCreateSecretsRequest(ctx context.Context, publicKey *tdh2easy.PublicKey, request *vaultcommon.CreateSecretsRequest, skipLabelValidation bool) error { + return r.validateWriteRequest(ctx, publicKey, request.RequestId, request.OrgId, request.WorkflowOwner, request.EncryptedSecrets, skipLabelValidation) } -func (r *RequestValidator) ValidateUpdateSecretsRequest(ctx context.Context, publicKey *tdh2easy.PublicKey, request *vaultcommon.UpdateSecretsRequest) error { - return r.validateWriteRequest(ctx, publicKey, request.RequestId, request.OrgId, request.WorkflowOwner, request.EncryptedSecrets) +func (r *RequestValidator) ValidateUpdateSecretsRequest(ctx context.Context, publicKey *tdh2easy.PublicKey, request *vaultcommon.UpdateSecretsRequest, skipLabelValidation bool) error { + return r.validateWriteRequest(ctx, publicKey, request.RequestId, request.OrgId, request.WorkflowOwner, request.EncryptedSecrets, skipLabelValidation) } // validateWriteRequest performs common validation for CreateSecrets and UpdateSecrets requests. // It treats publicKey as optional, since it can be nil if the gateway nodes don't have the public key cached yet. -func (r *RequestValidator) validateWriteRequest(ctx context.Context, publicKey *tdh2easy.PublicKey, id string, orgID string, workflowOwner string, encryptedSecrets []*vaultcommon.EncryptedSecret) error { +func (r *RequestValidator) validateWriteRequest(ctx context.Context, publicKey *tdh2easy.PublicKey, id string, orgID string, workflowOwner string, encryptedSecrets []*vaultcommon.EncryptedSecret, skipLabelValidation bool) error { if id == "" { return errors.New("request ID must not be empty") } @@ -66,13 +66,19 @@ func (r *RequestValidator) validateWriteRequest(ctx context.Context, publicKey * if err := r.validateCiphertextSize(ctx, req.EncryptedValue); err != nil { return fmt.Errorf("secret encrypted value at index %d is invalid: %w", idx, err) } - expectedWorkflowOwner := workflowOwner - if expectedWorkflowOwner == "" && orgID == "" { - expectedWorkflowOwner = req.Id.Owner - } - err := EnsureRightLabelOnSecret(publicKey, req.EncryptedValue, expectedWorkflowOwner, orgID) - if err != nil { - return errors.New("Encrypted Secret at index [" + strconv.Itoa(idx) + "] doesn't have owner as the label. Error: " + err.Error()) + if skipLabelValidation { + if _, err := verifyEncryptedSecret(publicKey, req.EncryptedValue); err != nil { + return errors.New("Encrypted Secret at index [" + strconv.Itoa(idx) + "] is invalid. Error: " + err.Error()) + } + } else { + expectedWorkflowOwner := workflowOwner + if expectedWorkflowOwner == "" && orgID == "" { + expectedWorkflowOwner = req.Id.Owner + } + err := EnsureRightLabelOnSecret(publicKey, req.EncryptedValue, expectedWorkflowOwner, orgID) + if err != nil { + return errors.New("Encrypted Secret at index [" + strconv.Itoa(idx) + "] doesn't have owner as the label. Error: " + err.Error()) + } } _, ok := uniqueIDs[vaulttypes.KeyFor(req.Id)] if ok { @@ -169,20 +175,13 @@ func NewRequestValidator( // parameter can be empty to skip that check. The function succeeds if the label matches // at least one non-empty owner. func EnsureRightLabelOnSecret(publicKey *tdh2easy.PublicKey, secret string, workflowOwner string, orgID string) error { - cipherText := &tdh2easy.Ciphertext{} - cipherBytes, err := hex.DecodeString(secret) + cipherText, err := verifyEncryptedSecret(publicKey, secret) if err != nil { - return errors.New("failed to decode encrypted value:" + err.Error()) + return err } - if publicKey == nil { - // Public key can be nil if gateway cache isn't populated yet (immediately after gateway reboots). - // Ok to not validate in such cases, since this validation also runs on Vault Nodes. + if cipherText == nil { return nil } - err = cipherText.UnmarshalVerify(cipherBytes, publicKey) - if err != nil { - return errors.New("failed to verify encrypted value:" + err.Error()) - } secretLabel := cipherText.Label() expectedLabels := make([]string, 0, 2) @@ -204,3 +203,21 @@ func EnsureRightLabelOnSecret(publicKey *tdh2easy.PublicKey, secret string, work return errors.New("secret label [" + hex.EncodeToString(secretLabel[:]) + "] does not match any of the provided owner labels; expectedLabels=[" + strings.Join(expectedLabels, ", ") + "]") } + +func verifyEncryptedSecret(publicKey *tdh2easy.PublicKey, secret string) (*tdh2easy.Ciphertext, error) { + cipherBytes, err := hex.DecodeString(secret) + if err != nil { + return nil, errors.New("failed to decode encrypted value:" + err.Error()) + } + if publicKey == nil { + // Public key can be nil if gateway cache isn't populated yet (immediately after gateway reboots). + // Ok to not validate in such cases, since this validation also runs on Vault Nodes. + return nil, nil + } + + cipherText := &tdh2easy.Ciphertext{} + if err := cipherText.UnmarshalVerify(cipherBytes, publicKey); err != nil { + return nil, errors.New("failed to verify encrypted value:" + err.Error()) + } + return cipherText, nil +} diff --git a/core/capabilities/vault/validator_test.go b/core/capabilities/vault/validator_test.go index dfbefe41076..9cfe7cecaec 100644 --- a/core/capabilities/vault/validator_test.go +++ b/core/capabilities/vault/validator_test.go @@ -241,7 +241,7 @@ func TestRequestValidator_CiphertextSizeLimit(t *testing.T) { EncryptedSecrets: []*vaultcommon.EncryptedSecret{ {Id: id, EncryptedValue: value}, }, - }) + }, false) }, value: hex.EncodeToString(make([]byte, 10)), }, @@ -253,7 +253,7 @@ func TestRequestValidator_CiphertextSizeLimit(t *testing.T) { EncryptedSecrets: []*vaultcommon.EncryptedSecret{ {Id: id, EncryptedValue: value}, }, - }) + }, false) }, value: hex.EncodeToString(make([]byte, 11)), errSubstr: "ciphertext size exceeds maximum allowed size", @@ -266,7 +266,7 @@ func TestRequestValidator_CiphertextSizeLimit(t *testing.T) { EncryptedSecrets: []*vaultcommon.EncryptedSecret{ {Id: id, EncryptedValue: value}, }, - }) + }, false) }, value: hex.EncodeToString(make([]byte, 10)), }, @@ -278,7 +278,7 @@ func TestRequestValidator_CiphertextSizeLimit(t *testing.T) { EncryptedSecrets: []*vaultcommon.EncryptedSecret{ {Id: id, EncryptedValue: value}, }, - }) + }, false) }, value: hex.EncodeToString(make([]byte, 11)), errSubstr: "ciphertext size exceeds maximum allowed size", @@ -324,7 +324,7 @@ func TestRequestValidator_ValidateCreateSecretsRequest_UsesRequestIdentityForOrg EncryptedValue: encrypted, }, }, - }) + }, false) require.NoError(t, err) } @@ -351,7 +351,39 @@ func TestRequestValidator_ValidateCreateSecretsRequest_FallsBackToSecretOwnerFor EncryptedValue: encrypted, }, }, - }) + }, false) + + require.NoError(t, err) +} + +func TestRequestValidator_ValidateCreateSecretsRequest_SkipsLabelValidationWithBool(t *testing.T) { + pk, _ := generateTestKeys(t) + validator := NewRequestValidator( + limits.NewUpperBoundLimiter(10), + limits.NewUpperBoundLimiter[pkgconfig.Size](10*pkgconfig.KByte), + ) + + orgID := "org_2xAbCdEfGhIjKlMnOpQrStUvWxYz" + workflowOwner := "0x0001020304050607080900010203040506070809" + encrypted := encryptWithOrgIDLabel(t, pk, orgID) + request := &vaultcommon.CreateSecretsRequest{ + RequestId: "request-id", + WorkflowOwner: workflowOwner, + EncryptedSecrets: []*vaultcommon.EncryptedSecret{ + { + Id: &vaultcommon.SecretIdentifier{ + Key: "key", + Namespace: "namespace", + Owner: workflowOwner, + }, + EncryptedValue: encrypted, + }, + }, + } + + err := validator.ValidateCreateSecretsRequest(t.Context(), pk, request, false) + require.ErrorContains(t, err, "doesn't have owner as the label") + err = validator.ValidateCreateSecretsRequest(t.Context(), pk, request, true) require.NoError(t, err) } diff --git a/core/services/gateway/handlers/vault/handler.go b/core/services/gateway/handlers/vault/handler.go index b063ba94adf..0559a494cce 100644 --- a/core/services/gateway/handlers/vault/handler.go +++ b/core/services/gateway/handlers/vault/handler.go @@ -18,6 +18,7 @@ import ( "github.com/smartcontractkit/tdh2/go/tdh2/tdh2easy" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" + "google.golang.org/protobuf/proto" "github.com/smartcontractkit/chainlink-common/pkg/beholder" "github.com/smartcontractkit/chainlink-common/pkg/capabilities" @@ -144,9 +145,10 @@ type handler struct { nodeRateLimiter *ratelimit.RateLimiter requestTimeout time.Duration - writeMethodsEnabled limits.GateLimiter - activeRequests map[string]*activeRequest - metrics *metrics + writeMethodsEnabled limits.GateLimiter + orgIDAsSecretOwnerEnabled limits.GateLimiter + activeRequests map[string]*activeRequest + metrics *metrics aggregator aggregator @@ -237,24 +239,29 @@ func newHandlerWithAuthorizer(methodConfig json.RawMessage, donConfig *config.DO if err != nil { return nil, fmt.Errorf("could not create vault mgmt limiter: %w", err) } + orgIDAsSecretOwnerEnabled, err := limits.MakeGateLimiter(limitsFactory, cresettings.Default.VaultOrgIdAsSecretOwnerEnabled) + if err != nil { + return nil, fmt.Errorf("could not create vault org ID as secret owner limiter: %w", err) + } return &handler{ - methodConfig: cfg, - donConfig: donConfig, - don: don, - lggr: logger.Named(lggr, "VaultHandler:"+donConfig.DonId), - requestTimeout: time.Duration(cfg.RequestTimeoutSec) * time.Second, - nodeRateLimiter: nodeRateLimiter, - writeMethodsEnabled: writeMethodsEnabled, - activeRequests: make(map[string]*activeRequest), - mu: sync.RWMutex{}, - authorizer: authorizer, - jwtAuth: jwtAuth, - stopCh: make(services.StopChan), - metrics: metrics, - aggregator: &baseAggregator{capabilitiesRegistry: capabilitiesRegistry}, - clock: clock, - RequestValidator: vaultcap.NewRequestValidator(limiter, ciphertextLimiter), + methodConfig: cfg, + donConfig: donConfig, + don: don, + lggr: logger.Named(lggr, "VaultHandler:"+donConfig.DonId), + requestTimeout: time.Duration(cfg.RequestTimeoutSec) * time.Second, + nodeRateLimiter: nodeRateLimiter, + writeMethodsEnabled: writeMethodsEnabled, + orgIDAsSecretOwnerEnabled: orgIDAsSecretOwnerEnabled, + activeRequests: make(map[string]*activeRequest), + mu: sync.RWMutex{}, + authorizer: authorizer, + jwtAuth: jwtAuth, + stopCh: make(services.StopChan), + metrics: metrics, + aggregator: &baseAggregator{capabilitiesRegistry: capabilitiesRegistry}, + clock: clock, + RequestValidator: vaultcap.NewRequestValidator(limiter, ciphertextLimiter), }, nil } @@ -300,6 +307,7 @@ func (h *handler) Close() error { return errors.Join( jwtAuthErr, h.writeMethodsEnabled.Close(), + h.orgIDAsSecretOwnerEnabled.Close(), h.MaxRequestBatchSizeLimiter.Close(), ) }) @@ -577,6 +585,14 @@ func (h *handler) sendSuccessResponse(ctx context.Context, l logger.Logger, ar * return h.sendResponse(ctx, ar, successResp) } +func (h *handler) skipSecretLabelValidation(ctx context.Context, orgID string) (bool, error) { + orgIDAsSecretOwnerEnabled, err := h.orgIDAsSecretOwnerEnabled.Limit(ctx) + if err != nil { + return false, err + } + return orgIDAsSecretOwnerEnabled && orgID == "", nil +} + func (h *handler) handleSecretsCreate(ctx context.Context, ar *activeRequest) error { l := logger.With(h.lggr, "method", ar.req.Method, "requestID", ar.req.ID) @@ -600,7 +616,21 @@ func (h *handler) handleSecretsCreate(ctx context.Context, ar *activeRequest) er } } _, cachedPublicKey := h.getCachedPublicKey() - err = h.ValidateCreateSecretsRequest(ctx, cachedPublicKey, createSecretsRequest) + skipLabelValidation, err := h.skipSecretLabelValidation(ctx, createSecretsRequest.OrgId) + if err != nil { + l.Errorw("error checking if VaultOrgIdAsSecretOwnerEnabled is enabled", "error", err) + return h.sendResponse(ctx, ar, h.errorResponse(ar.req, api.FatalError, errors.New("error checking if VaultOrgIdAsSecretOwnerEnabled is enabled: "+err.Error()), nil)) + } + validationRequest := createSecretsRequest + if createSecretsRequest.OrgId != "" { + // JWT-authenticated requests carry OrgId, so the gateway can verify the + // org label directly. Clear WorkflowOwner only in this validation copy so + // workflow-owner-labeled ciphertext is rejected, while the forwarded + // request still preserves the authorized identity fields. + validationRequest = proto.Clone(createSecretsRequest).(*vaultcommon.CreateSecretsRequest) + validationRequest.WorkflowOwner = "" + } + err = h.ValidateCreateSecretsRequest(ctx, cachedPublicKey, validationRequest, skipLabelValidation) if err != nil { l.Warnw("failed to validate create secrets request", "error", err) return h.sendResponse(ctx, ar, h.errorResponse(ar.req, api.InvalidParamsError, fmt.Errorf("failed to validate create secrets request: %w", err), nil)) @@ -641,7 +671,21 @@ func (h *handler) handleSecretsUpdate(ctx context.Context, ar *activeRequest) er } } _, cachedPublicKey := h.getCachedPublicKey() - vaultCapErr := h.ValidateUpdateSecretsRequest(ctx, cachedPublicKey, updateSecretsRequest) + skipLabelValidation, err := h.skipSecretLabelValidation(ctx, updateSecretsRequest.OrgId) + if err != nil { + l.Errorw("error checking if VaultOrgIdAsSecretOwnerEnabled is enabled", "error", err) + return h.sendResponse(ctx, ar, h.errorResponse(ar.req, api.FatalError, errors.New("error checking if VaultOrgIdAsSecretOwnerEnabled is enabled: "+err.Error()), nil)) + } + validationRequest := updateSecretsRequest + if updateSecretsRequest.OrgId != "" { + // JWT-authenticated requests carry OrgId, so the gateway can verify the + // org label directly. Clear WorkflowOwner only in this validation copy so + // workflow-owner-labeled ciphertext is rejected, while the forwarded + // request still preserves the authorized identity fields. + validationRequest = proto.Clone(updateSecretsRequest).(*vaultcommon.UpdateSecretsRequest) + validationRequest.WorkflowOwner = "" + } + vaultCapErr := h.ValidateUpdateSecretsRequest(ctx, cachedPublicKey, validationRequest, skipLabelValidation) if vaultCapErr != nil { l.Warnw("failed to validate update secrets request", "error", vaultCapErr) return h.sendResponse(ctx, ar, h.errorResponse(ar.req, api.InvalidParamsError, fmt.Errorf("failed to validate update secrets request: %w", vaultCapErr), nil)) diff --git a/core/services/gateway/handlers/vault/handler_test.go b/core/services/gateway/handlers/vault/handler_test.go index 3c06ea9a906..030582be25d 100644 --- a/core/services/gateway/handlers/vault/handler_test.go +++ b/core/services/gateway/handlers/vault/handler_test.go @@ -10,6 +10,7 @@ import ( "testing" "time" + ethcommon "github.com/ethereum/go-ethereum/common" "github.com/jonboulle/clockwork" p2ptypes "github.com/smartcontractkit/libocr/ragep2p/types" "github.com/smartcontractkit/tdh2/go/tdh2/tdh2easy" @@ -23,10 +24,12 @@ import ( jsonrpc "github.com/smartcontractkit/chainlink-common/pkg/jsonrpc2" "github.com/smartcontractkit/chainlink-common/pkg/logger" "github.com/smartcontractkit/chainlink-common/pkg/ratelimit" + "github.com/smartcontractkit/chainlink-common/pkg/settings" "github.com/smartcontractkit/chainlink-common/pkg/settings/cresettings" "github.com/smartcontractkit/chainlink-common/pkg/settings/limits" vaultcap "github.com/smartcontractkit/chainlink/v2/core/capabilities/vault" "github.com/smartcontractkit/chainlink/v2/core/capabilities/vault/vaulttypes" + "github.com/smartcontractkit/chainlink/v2/core/capabilities/vault/vaultutils" "github.com/smartcontractkit/chainlink/v2/core/services/gateway/api" "github.com/smartcontractkit/chainlink/v2/core/services/gateway/config" @@ -41,6 +44,10 @@ var NodeOne = config.NodeConfig{ } func setupHandler(t *testing.T) (handlers.Handler, *common.Callback, *mocks.DON, *clockwork.FakeClock) { + return setupHandlerWithLimitsFactory(t, limits.Factory{Settings: cresettings.DefaultGetter}) +} + +func setupHandlerWithLimitsFactory(t *testing.T, limitsFactory limits.Factory) (handlers.Handler, *common.Callback, *mocks.DON, *clockwork.FakeClock) { lggr := logger.Test(t) don := mocks.NewDON(t) donConfig := &config.DONConfig{ @@ -60,7 +67,6 @@ func setupHandler(t *testing.T) (handlers.Handler, *common.Callback, *mocks.DON, require.NoError(t, err) clock := clockwork.NewFakeClock() - limitsFactory := limits.Factory{Settings: cresettings.DefaultGetter} authorizer := vaultcap.NewAuthorizer(&stubAllowListBasedAuth{clock: clock}, nil, lggr) handler, err := newHandlerWithAuthorizer(methodConfig, donConfig, don, nil, authorizer, nil, lggr, clock, limitsFactory) require.NoError(t, err) @@ -69,6 +75,27 @@ func setupHandler(t *testing.T) (handlers.Handler, *common.Callback, *mocks.DON, return handler, cb, don, clock } +func newVaultOrgIDAsSecretOwnerLimitsFactory(t *testing.T, enabled bool) limits.Factory { + t.Helper() + + getter, err := settings.NewJSONGetter([]byte(fmt.Sprintf(`{"global":{"VaultOrgIdAsSecretOwnerEnabled":%t}}`, enabled))) + require.NoError(t, err) + + return limits.Factory{Settings: getter} +} + +func cacheVaultPublicKeyForTest(t *testing.T, h *handler, pk *tdh2easy.PublicKey) { + t.Helper() + + pkBytes, err := pk.Marshal() + require.NoError(t, err) + publicKeyResponseBytes, err := json.Marshal(&vaultcommon.GetPublicKeyResponse{PublicKey: hex.EncodeToString(pkBytes)}) + require.NoError(t, err) + + h.cachedPublicKeyGetResponse = publicKeyResponseBytes + h.cachedPublicKeyObject = pk +} + type stubAllowListBasedAuth struct { clock clockwork.Clock } @@ -105,7 +132,7 @@ type mockCapabilitiesRegistry struct { var owner = "test_owner" func (m *mockCapabilitiesRegistry) DONsForCapability(_ context.Context, _ string) ([]capabilities.DONWithNodes, error) { - members := []p2ptypes.PeerID{} + members := make([]p2ptypes.PeerID, 0, len(m.Nodes)) for _, n := range m.Nodes { members = append(members, *n.PeerID) } @@ -299,6 +326,143 @@ func TestVaultHandler_HandleJSONRPCUserMessage(t *testing.T) { require.Equal(t, "org-1"+vaulttypes.RequestIDSeparator+"1", forwardedCreateRequest.RequestId) }) + t.Run("rejects org ID labeled allowlist create when org ID owner flag is disabled", func(t *testing.T) { + _, pk, _, err := tdh2easy.GenerateKeys(1, 3) + require.NoError(t, err) + orgID := "org_2xAbCdEfGhIjKlMnOpQrStUvWxYz" + encryptedSecret, err := vaultutils.EncryptSecretWithOrgID("test_secret", pk, orgID) + require.NoError(t, err) + + h, callback, don, _ := setupHandlerWithLimitsFactory(t, newVaultOrgIDAsSecretOwnerLimitsFactory(t, false)) + cacheVaultPublicKeyForTest(t, h.(*handler), pk) + + reqData := &vaultcommon.CreateSecretsRequest{ + EncryptedSecrets: []*vaultcommon.EncryptedSecret{ + { + Id: &vaultcommon.SecretIdentifier{ + Key: "test_id", + Owner: owner, + }, + EncryptedValue: encryptedSecret, + }, + }, + } + reqDataBytes, err := json.Marshal(reqData) + require.NoError(t, err) + + req := jsonrpc.Request[json.RawMessage]{ + ID: "org-id-labeled-secret", + Method: vaulttypes.MethodSecretsCreate, + Params: (*json.RawMessage)(&reqDataBytes), + } + + err = h.HandleJSONRPCUserMessage(t.Context(), req, callback) + require.NoError(t, err) + + ctx, cancel := context.WithTimeout(t.Context(), time.Second) + defer cancel() + resp, err := callback.Wait(ctx) + require.NoError(t, err) + var createResponse jsonrpc.Response[vaultcommon.CreateSecretsResponse] + require.NoError(t, json.Unmarshal(resp.RawResponse, &createResponse)) + require.ErrorContains(t, createResponse.Error, "doesn't have owner as the label") + require.Equal(t, api.ToJSONRPCErrorCode(api.InvalidParamsError), createResponse.Error.Code) + don.AssertNotCalled(t, "SendToNode", mock.Anything, mock.Anything, mock.Anything) + }) + + t.Run("skips gateway label validation for org ID labeled allowlist create when org ID owner flag is enabled", func(t *testing.T) { + _, pk, _, err := tdh2easy.GenerateKeys(1, 3) + require.NoError(t, err) + orgID := "org_2xAbCdEfGhIjKlMnOpQrStUvWxYz" + encryptedSecret, err := vaultutils.EncryptSecretWithOrgID("test_secret", pk, orgID) + require.NoError(t, err) + + h, callback, don, _ := setupHandlerWithLimitsFactory(t, newVaultOrgIDAsSecretOwnerLimitsFactory(t, true)) + cacheVaultPublicKeyForTest(t, h.(*handler), pk) + + reqData := &vaultcommon.CreateSecretsRequest{ + EncryptedSecrets: []*vaultcommon.EncryptedSecret{ + { + Id: &vaultcommon.SecretIdentifier{ + Key: "test_id", + Owner: owner, + }, + EncryptedValue: encryptedSecret, + }, + }, + } + reqDataBytes, err := json.Marshal(reqData) + require.NoError(t, err) + + req := jsonrpc.Request[json.RawMessage]{ + ID: "org-id-labeled-secret", + Method: vaulttypes.MethodSecretsCreate, + Params: (*json.RawMessage)(&reqDataBytes), + } + + var forwarded jsonrpc.Request[json.RawMessage] + don.On("SendToNode", mock.Anything, mock.Anything, mock.Anything).Run(func(args mock.Arguments) { + forwarded = *args.Get(2).(*jsonrpc.Request[json.RawMessage]) + }).Return(nil).Once() + + err = h.HandleJSONRPCUserMessage(t.Context(), req, callback) + require.NoError(t, err) + + don.AssertExpectations(t) + require.NotNil(t, forwarded.Params) + var forwardedCreateRequest vaultcommon.CreateSecretsRequest + require.NoError(t, json.Unmarshal(*forwarded.Params, &forwardedCreateRequest)) + require.Empty(t, forwardedCreateRequest.OrgId) + require.Equal(t, owner, forwardedCreateRequest.WorkflowOwner) + require.Equal(t, owner+vaulttypes.RequestIDSeparator+req.ID, forwardedCreateRequest.RequestId) + }) + + t.Run("rejects workflow owner labeled jwt create when org ID owner flag is enabled", func(t *testing.T) { + _, pk, _, err := tdh2easy.GenerateKeys(1, 3) + require.NoError(t, err) + orgID := "org_2xAbCdEfGhIjKlMnOpQrStUvWxYz" + workflowOwner := "0x0001020304050607080900010203040506070809" + encryptedSecret, err := vaultutils.EncryptSecretWithWorkflowOwner("test_secret", pk, ethcommon.HexToAddress(workflowOwner)) + require.NoError(t, err) + + h, callback, don, clock := setupHandlerWithLimitsFactory(t, newVaultOrgIDAsSecretOwnerLimitsFactory(t, true)) + h.(*handler).authorizer = &stubAuthorizer{result: vaultcap.NewAuthResult(orgID, workflowOwner, "digest-1", clock.Now().Add(time.Minute).Unix())} + cacheVaultPublicKeyForTest(t, h.(*handler), pk) + + reqData := &vaultcommon.CreateSecretsRequest{ + EncryptedSecrets: []*vaultcommon.EncryptedSecret{ + { + Id: &vaultcommon.SecretIdentifier{ + Key: "test_id", + Owner: orgID, + }, + EncryptedValue: encryptedSecret, + }, + }, + } + reqDataBytes, err := json.Marshal(reqData) + require.NoError(t, err) + + req := jsonrpc.Request[json.RawMessage]{ + ID: "workflow-owner-labeled-secret", + Method: vaulttypes.MethodSecretsCreate, + Params: (*json.RawMessage)(&reqDataBytes), + } + + err = h.HandleJSONRPCUserMessage(t.Context(), req, callback) + require.NoError(t, err) + + ctx, cancel := context.WithTimeout(t.Context(), time.Second) + defer cancel() + resp, err := callback.Wait(ctx) + require.NoError(t, err) + var createResponse jsonrpc.Response[vaultcommon.CreateSecretsResponse] + require.NoError(t, json.Unmarshal(resp.RawResponse, &createResponse)) + require.ErrorContains(t, createResponse.Error, "doesn't have owner as the label") + require.Equal(t, api.ToJSONRPCErrorCode(api.InvalidParamsError), createResponse.Error.Code) + don.AssertNotCalled(t, "SendToNode", mock.Anything, mock.Anything, mock.Anything) + }) + t.Run("nil EncryptedSecrets inside CreateSecrets body", func(t *testing.T) { var wg sync.WaitGroup h, callback, _, _ := setupHandler(t) diff --git a/core/services/ocr2/plugins/vault/kvstore_wrapper_test.go b/core/services/ocr2/plugins/vault/kvstore_wrapper_test.go index e6fbbf07fe8..dc99877a871 100644 --- a/core/services/ocr2/plugins/vault/kvstore_wrapper_test.go +++ b/core/services/ocr2/plugins/vault/kvstore_wrapper_test.go @@ -451,6 +451,26 @@ func TestKVStoreWrapper_DeleteSecret_CleansBothOwners(t *testing.T) { assert.Nil(t, woSecret) } +func TestKVStoreWrapper_DeleteSecret_PreservesOtherNamespacesForSameKey(t *testing.T) { + ctx := t.Context() + _, inner := newMigrationTestStore(t) + writeTestSecret(ctx, t, inner, testOrgID, "main", "shared_key", []byte("main-data")) + writeTestSecret(ctx, t, inner, testOrgID, "alt", "shared_key", []byte("alt-data")) + + store := NewKVStoreWrapper(inner, true, logger.TestLogger(t)) + id := &vault.SecretIdentifier{Owner: testOrgID, Namespace: "main", Key: "shared_key"} + require.NoError(t, store.DeleteSecret(ctx, id, testOrgID, testWorkflowOwner)) + + mainSecret, err := inner.GetSecret(ctx, &vault.SecretIdentifier{Owner: testOrgID, Namespace: "main", Key: "shared_key"}) + require.NoError(t, err) + assert.Nil(t, mainSecret) + + altSecret, err := inner.GetSecret(ctx, &vault.SecretIdentifier{Owner: testOrgID, Namespace: "alt", Key: "shared_key"}) + require.NoError(t, err) + require.NotNil(t, altSecret) + assert.Equal(t, []byte("alt-data"), altSecret.EncryptedSecret) +} + func TestKVStoreWrapper_DeleteSecret_NotFoundAnywhere(t *testing.T) { ctx := t.Context() _, inner := newMigrationTestStore(t) diff --git a/core/services/ocr2/plugins/vault/plugin.go b/core/services/ocr2/plugins/vault/plugin.go index 8048e6d5be0..89944ff64d6 100644 --- a/core/services/ocr2/plugins/vault/plugin.go +++ b/core/services/ocr2/plugins/vault/plugin.go @@ -395,7 +395,7 @@ func (r *ReportingPlugin) orgIDAsSecretOwnerEnabled(ctx context.Context) bool { return r.cfg.OrgIDAsSecretOwnerEnabled.AllowErr(ctx) == nil } -// canonicalResponseID rewrites successful CRUD responses to the canonical owner identity. +// canonicalResponseID rewrites Vault responses to the canonical owner identity. // // When VaultOrgIdAsSecretOwnerEnabled is on, requests may still arrive keyed by // workflow owner for backwards compatibility with existing clients and allowlist-based @@ -662,7 +662,7 @@ func (r *ReportingPlugin) observeGetSecrets(ctx context.Context, reader ReadKVSt logUserErrorAware(r.lggr, "failed to observe get secret request item", ierr, "id", secretRequest.Id) errorMsg := userFacingError(ierr, "failed to handle get secret request") resps = append(resps, &vaultcommon.SecretResponse{ - Id: secretRequest.Id, + Id: r.canonicalResponseID(ctx, secretRequest.Id, tp.OrgId), Result: &vaultcommon.SecretResponse_Error{ Error: errorMsg, }, @@ -767,7 +767,7 @@ func (r *ReportingPlugin) observeGetSecretsRequest(ctx context.Context, reader R } return &vaultcommon.SecretResponse{ - Id: id, + Id: r.canonicalResponseID(ctx, id, orgID), Result: &vaultcommon.SecretResponse_Data{ Data: &vaultcommon.SecretData{ EncryptedValue: hex.EncodeToString(secret.EncryptedSecret), @@ -1310,27 +1310,27 @@ func (r *ReportingPlugin) validateGetSecretsObservation(ctx context.Context, o * // we should have max 1 share per observation per encrypted key req, resp := o.GetGetSecretsRequest(), o.GetGetSecretsResponse() reqMap := map[string]*vaultcommon.SecretRequest{} - for _, r := range req.Requests { - if r.Id == nil { + for _, secretRequest := range req.Requests { + if secretRequest.Id == nil { return errors.New("GetSecrets request contains nil secret identifier") } - key := vaulttypes.KeyFor(r.Id) + key := vaulttypes.KeyFor(r.canonicalResponseID(ctx, secretRequest.Id, req.OrgId)) if _, ok := reqMap[key]; ok { return fmt.Errorf("duplicate request found for item %s", key) } - reqMap[key] = r + reqMap[key] = secretRequest } respMap := map[string]*vaultcommon.SecretResponse{} - for _, r := range resp.Responses { - if r.Id == nil { + for _, secretResponse := range resp.Responses { + if secretResponse.Id == nil { return errors.New("GetSecrets response contains nil secret identifier") } - key := vaulttypes.KeyFor(r.Id) + key := vaulttypes.KeyFor(secretResponse.Id) if _, ok := respMap[key]; ok { return fmt.Errorf("duplicate response found for item %s", key) } - respMap[key] = r + respMap[key] = secretResponse } if len(reqMap) != len(respMap) { @@ -1338,7 +1338,8 @@ func (r *ReportingPlugin) validateGetSecretsObservation(ctx context.Context, o * } for _, rq := range reqMap { - key := vaulttypes.KeyFor(rq.Id) + responseID := r.canonicalResponseID(ctx, rq.Id, req.OrgId) + key := vaulttypes.KeyFor(responseID) rsp, ok := respMap[key] if !ok { return fmt.Errorf("missing response for request with id %s", key) @@ -1351,7 +1352,7 @@ func (r *ReportingPlugin) validateGetSecretsObservation(ctx context.Context, o * return errors.New("observation must contain a share per encryption key provided") } - innerCtx := contexts.WithCRE(ctx, contexts.CRE{Owner: rq.Id.Owner}) + innerCtx := contexts.WithCRE(ctx, contexts.CRE{Owner: responseID.Owner}) for _, ds := range decryptionShares { if len(ds.Shares) != 1 { return errors.New("observation must have exactly 1 share per encryption key") diff --git a/core/services/ocr2/plugins/vault/plugin_test.go b/core/services/ocr2/plugins/vault/plugin_test.go index aaf7c2208d8..c559e8f16be 100644 --- a/core/services/ocr2/plugins/vault/plugin_test.go +++ b/core/services/ocr2/plugins/vault/plugin_test.go @@ -1110,6 +1110,8 @@ func TestPlugin_Observation_GetSecretsRequest_OrgIdLabelAcceptedWhenEnabled(t *t require.Len(t, obs.Observations, 1) batchResp := obs.Observations[0].GetGetSecretsResponse() require.Len(t, batchResp.Responses, 1) + require.NotNil(t, batchResp.Responses[0].GetId()) + assert.Equal(t, orgID, batchResp.Responses[0].GetId().GetOwner()) assert.Empty(t, batchResp.Responses[0].GetError()) } @@ -6870,6 +6872,64 @@ func TestPlugin_ValidateObservation_GetSecretsRequest(t *testing.T) { require.ErrorContains(t, err, "invalid observation: share provided exceeds maximum size allowed") } +func TestPlugin_ValidateObservation_GetSecretsRequest_OrgIDResponseOwner(t *testing.T) { + lggr := logger.TestLogger(t) + _, pk, shares, err := tdh2easy.GenerateKeys(1, 3) + require.NoError(t, err) + + cfg := makeReportingPluginConfig(t, 1, pk, shares[0], 1, 1024, 30, 30, 30, 10) + cfg.OrgIDAsSecretOwnerEnabled = limits.NewGateLimiter(true) + r := &ReportingPlugin{ + lggr: lggr, + metrics: newTestMetrics(t), + cfg: cfg, + } + + workflowOwner := "workflowowner" + orgID := "org_2xAbCdEfGhIjKlMnOpQrStUvWxYz" + secretID := &vaultcommon.SecretIdentifier{ + Owner: workflowOwner, + Namespace: "main", + Key: "secret", + } + responseID := &vaultcommon.SecretIdentifier{ + Owner: orgID, + Namespace: secretID.Namespace, + Key: secretID.Key, + } + + obs := &vaultcommon.Observation{ + Id: "request-1", + RequestType: vaultcommon.RequestType_GET_SECRETS, + Request: &vaultcommon.Observation_GetSecretsRequest{ + GetSecretsRequest: &vaultcommon.GetSecretsRequest{ + Requests: []*vaultcommon.SecretRequest{ + {Id: secretID}, + }, + OrgId: orgID, + WorkflowOwner: workflowOwner, + }, + }, + Response: &vaultcommon.Observation_GetSecretsResponse{ + GetSecretsResponse: &vaultcommon.GetSecretsResponse{ + Responses: []*vaultcommon.SecretResponse{ + { + Id: responseID, + Result: &vaultcommon.SecretResponse_Error{ + Error: "not found", + }, + }, + }, + }, + }, + } + + require.NoError(t, r.validateObservation(t.Context(), obs)) + + cfg.OrgIDAsSecretOwnerEnabled = limits.NewGateLimiter(false) + require.ErrorContains(t, r.validateObservation(t.Context(), obs), "missing response for request with id workflowowner::main::secret") +} + func TestPlugin_ValidateObservation_PanicsOnEmptyShares(t *testing.T) { lggr := logger.TestLogger(t) store := requests.NewStore[*vaulttypes.Request]() diff --git a/core/services/workflows/v2/secrets.go b/core/services/workflows/v2/secrets.go index 3183b7ffa89..dbb7907338f 100644 --- a/core/services/workflows/v2/secrets.go +++ b/core/services/workflows/v2/secrets.go @@ -192,6 +192,9 @@ func (s *secretsFetcher) getSecretsForBatch(ctx context.Context, request *sdkpb. if err != nil { return nil, fmt.Errorf("failed to evaluate vault org_id gate: %w", err) } + if orgIDGateEnabled && s.orgID == "" { + return nil, errors.New("org_id is required when VaultOrgIdAsSecretOwnerEnabled is enabled") + } metadata := capabilities.RequestMetadata{ WorkflowOwner: s.workflowOwner, WorkflowName: s.workflowName, @@ -217,6 +220,10 @@ func (s *secretsFetcher) getSecretsForBatch(ctx context.Context, request *sdkpb. if err != nil { return nil, fmt.Errorf("could not normalize workflowOwner: %w", err) } + responseOwner := owner + if orgIDGateEnabled { + responseOwner = s.orgID + } logKeys := make([]string, 0, len(request.Requests)) for _, r := range request.Requests { @@ -275,15 +282,15 @@ func (s *secretsFetcher) getSecretsForBatch(ctx context.Context, request *sdkpb. if namespace == "" { namespace = vaulttypes.DefaultNamespace } - key := keyFor(owner, namespace, r.Id) + key := keyFor(responseOwner, namespace, r.Id) resp, ok := m[key] if !ok { errorMessage := "could not find response for the request: " + key - errorResponse := s.wrapErrorResponse(lggr, r.Id, namespace, owner, errorMessage) + errorResponse := s.wrapErrorResponse(lggr, r.Id, namespace, responseOwner, errorMessage) sdkResp = append(sdkResp, &errorResponse) continue } - response := s.getSecretForSingleRequest(logger.With(lggr, "key", key), r.Id, owner, namespace, cfg, resp) + response := s.getSecretForSingleRequest(logger.With(lggr, "key", key), r.Id, responseOwner, namespace, cfg, resp) sdkResp = append(sdkResp, &response) } return sdkResp, nil diff --git a/core/services/workflows/v2/secrets_test.go b/core/services/workflows/v2/secrets_test.go index b6a6208ce16..03515f322c2 100644 --- a/core/services/workflows/v2/secrets_test.go +++ b/core/services/workflows/v2/secrets_test.go @@ -458,7 +458,7 @@ func TestSecretsFetcher_ForwardsOrgIDAndWorkflowOwner(t *testing.T) { Id: &vault.SecretIdentifier{ Key: "Foo", Namespace: "Bar", - Owner: normalizedOwner, + Owner: "org-123", }, Result: &vault.SecretResponse_Error{Error: "not found"}, }, @@ -484,7 +484,7 @@ func TestSecretsFetcher_ForwardsOrgIDAndWorkflowOwner(t *testing.T) { workflowEncryptionKey, ) - _, err = sf.GetSecrets(t.Context(), &sdkpb.GetSecretsRequest{ + resp, err := sf.GetSecrets(t.Context(), &sdkpb.GetSecretsRequest{ Requests: []*sdkpb.SecretRequest{ { Id: "Foo", @@ -496,6 +496,58 @@ func TestSecretsFetcher_ForwardsOrgIDAndWorkflowOwner(t *testing.T) { require.NotNil(t, capturedReq) assert.Equal(t, "org-123", capturedReq.OrgId) assert.Equal(t, owner, capturedReq.WorkflowOwner) + assert.Equal(t, normalizedOwner, capturedReq.Requests[0].Id.Owner) + require.Len(t, resp, 1) + require.NotNil(t, resp[0].GetError()) + assert.Contains(t, resp[0].GetError().Error, "not found") + assert.NotContains(t, resp[0].GetError().Error, "could not find response") +} + +func TestSecretsFetcher_RequiresOrgIDWhenGateEnabled(t *testing.T) { + lggr := logger.TestLogger(t) + reg := coreCap.NewRegistry(lggr) + peer := coreCap.RandomUTF8BytesWord() + + workflowEncryptionKey := workflowkey.MustNewXXXTestingOnly(big.NewInt(1)) + _, vaultPublicKey, _, err := tdh2easy.GenerateKeys(2, 3) + require.NoError(t, err) + vaultPublicKeyBytes, err := vaultPublicKey.Marshal() + require.NoError(t, err) + reg.SetLocalRegistry(CreateLocalRegistryWith1Node(t, peer, workflowEncryptionKey.PublicKey(), vaultPublicKeyBytes)) + + mc := vaultMock.Vault{ + Fn: func(ctx context.Context, req *vault.GetSecretsRequest) (*vault.GetSecretsResponse, error) { + require.Fail(t, "vault should not be called when org ID is missing") + return nil, nil + }, + } + err = reg.Add(t.Context(), mc) + require.NoError(t, err) + + sf := NewSecretsFetcher( + MetricsLabelerTest(t), + reg, + lggr, + limits.WorkflowResourcePoolLimiter[int](5), + limits.NewUpperBoundLimiter[int](5), + testVaultOrgIDAsSecretOwnerGate(t, true), + "", + "1234567890abcdef1234567890abcdef12345678", + "workflowName", + "workflowID", + "workflowExecID", + workflowEncryptionKey, + ) + + _, err = sf.GetSecrets(t.Context(), &sdkpb.GetSecretsRequest{ + Requests: []*sdkpb.SecretRequest{ + { + Id: "Foo", + Namespace: "Bar", + }, + }, + }) + require.ErrorContains(t, err, "org_id is required when VaultOrgIdAsSecretOwnerEnabled is enabled") } func TestSecretsFetcher_OmitsOrgIDAndWorkflowOwnerWhenGateDisabled(t *testing.T) { diff --git a/system-tests/lib/cre/features/vault/vault.go b/system-tests/lib/cre/features/vault/vault.go index 0a7cc67ebff..a70ac28b6af 100644 --- a/system-tests/lib/cre/features/vault/vault.go +++ b/system-tests/lib/cre/features/vault/vault.go @@ -2,7 +2,6 @@ package vault import ( "context" - "encoding/hex" "encoding/json" "fmt" "slices" @@ -21,12 +20,10 @@ import ( chainselectors "github.com/smartcontractkit/chain-selectors" "github.com/smartcontractkit/smdkg/dkgocr/dkgocrtypes" - "github.com/smartcontractkit/tdh2/go/tdh2/tdh2easy" "github.com/smartcontractkit/chainlink-testing-framework/framework" "github.com/smartcontractkit/chainlink-testing-framework/lib/utils/ptr" depcontracts "github.com/smartcontractkit/chainlink/deployment/cre/ocr3/ocr3_1/changeset/operations/contracts" - "github.com/smartcontractkit/chainlink/v2/core/capabilities/vault/vaultutils" coretoml "github.com/smartcontractkit/chainlink/v2/core/config/toml" corechainlink "github.com/smartcontractkit/chainlink/v2/core/services/chainlink" @@ -511,16 +508,3 @@ func vaultMethodConfigs() map[string]*capabilitiespb.CapabilityMethodConfig { }, } } - -func EncryptSecret(secret, masterPublicKeyStr string, owner common.Address) (string, error) { - masterPublicKey := tdh2easy.PublicKey{} - masterPublicKeyBytes, err := hex.DecodeString(masterPublicKeyStr) - if err != nil { - return "", errors.Wrap(err, "failed to decode master public key") - } - err = masterPublicKey.Unmarshal(masterPublicKeyBytes) - if err != nil { - return "", errors.Wrap(err, "failed to unmarshal master public key") - } - return vaultutils.EncryptSecretWithWorkflowOwner(secret, &masterPublicKey, owner) -} diff --git a/system-tests/lib/cre/workflow/secrets.go b/system-tests/lib/cre/workflow/secrets.go index 00b6be1ca6a..08e2c9e0c84 100644 --- a/system-tests/lib/cre/workflow/secrets.go +++ b/system-tests/lib/cre/workflow/secrets.go @@ -15,6 +15,7 @@ import ( "github.com/goccy/go-yaml" "github.com/google/uuid" "github.com/pkg/errors" + "github.com/smartcontractkit/tdh2/go/tdh2/tdh2easy" vault_helpers "github.com/smartcontractkit/chainlink-common/pkg/capabilities/actions/vault" jsonrpc "github.com/smartcontractkit/chainlink-common/pkg/jsonrpc2" @@ -22,8 +23,8 @@ import ( workflow_registry_v2_wrapper "github.com/smartcontractkit/chainlink-evm/gethwrappers/workflow/generated/workflow_registry_wrapper_v2" "github.com/smartcontractkit/chainlink-testing-framework/seth" "github.com/smartcontractkit/chainlink/system-tests/lib/cre" - crevault "github.com/smartcontractkit/chainlink/system-tests/lib/cre/features/vault" vaulttypes "github.com/smartcontractkit/chainlink/v2/core/capabilities/vault/vaulttypes" + "github.com/smartcontractkit/chainlink/v2/core/capabilities/vault/vaultutils" ) // vaultSecretsConfig defines the structure of the vault secrets YAML file. @@ -114,6 +115,15 @@ func PrepareSecrets(secretsFilePath, vaultPublicKey string, ownerAddress common. return "", errors.New("no secrets found in secrets file") } + masterPublicKeyBytes, err := hex.DecodeString(vaultPublicKey) + if err != nil { + return "", errors.Wrap(err, "failed to decode vault public key") + } + masterPublicKey := &tdh2easy.PublicKey{} + if err := masterPublicKey.Unmarshal(masterPublicKeyBytes); err != nil { + return "", errors.Wrap(err, "failed to unmarshal vault public key") + } + encryptedSecrets := make([]*vault_helpers.EncryptedSecret, 0, len(cfg.Secrets)) for _, entry := range cfg.Secrets { value := os.Getenv(entry.EnvVar) @@ -126,7 +136,7 @@ func PrepareSecrets(secretsFilePath, vaultPublicKey string, ownerAddress common. namespace = "main" } - encryptedValue, encErr := crevault.EncryptSecret(value, vaultPublicKey, ownerAddress) + encryptedValue, encErr := vaultutils.EncryptSecretWithWorkflowOwner(value, masterPublicKey, ownerAddress) if encErr != nil { return "", errors.Wrapf(encErr, "failed to encrypt secret %q", entry.Key) } diff --git a/system-tests/tests/smoke/cre/v2_vault_don_test.go b/system-tests/tests/smoke/cre/v2_vault_don_test.go index bc0630a493a..f830796dd57 100644 --- a/system-tests/tests/smoke/cre/v2_vault_don_test.go +++ b/system-tests/tests/smoke/cre/v2_vault_don_test.go @@ -3,8 +3,7 @@ package cre import ( "context" "encoding/json" - "math/rand" - "strconv" + "net/http" "strings" "testing" "time" @@ -29,13 +28,16 @@ import ( workflow_registry_v2_wrapper "github.com/smartcontractkit/chainlink-evm/gethwrappers/workflow/generated/workflow_registry_wrapper_v2" envconfig "github.com/smartcontractkit/chainlink/system-tests/lib/cre/environment/config" - crevault "github.com/smartcontractkit/chainlink/system-tests/lib/cre/features/vault" "github.com/smartcontractkit/chainlink/system-tests/lib/cre/vault" ttypes "github.com/smartcontractkit/chainlink/system-tests/tests/test-helpers/configuration" "github.com/smartcontractkit/chainlink-testing-framework/framework" ) +func uniqueVaultSecretID(prefix string) string { + return prefix + strings.ReplaceAll(uuid.NewString(), "-", "") +} + func ExecuteVaultAllowListBasedTests(t *testing.T, fixture *vaultScenarioFixture, testEnv *ttypes.TestEnvironment) { var testLogger = framework.L linkingService := fixture.LinkingService @@ -45,26 +47,32 @@ func ExecuteVaultAllowListBasedTests(t *testing.T, fixture *vaultScenarioFixture t.Run("allowlist_crud_with_workflow_owner_identity", func(t *testing.T) { sc := testEnv.CreEnvironment.Blockchains[0].(*evm.Blockchain).SethClient - owner := sc.MustGetRootKeyAddress().Hex() + workflowOwnerAddress := sc.MustGetRootKeyAddress() + owner := workflowOwnerAddress.Hex() expectedResponseOwner := owner + orgID := "" orgIDAsSecretOwnerEnabled := isVaultJWTAuthEnabledTopology(testEnv.TestConfig.EnvironmentConfigPath) if linkingService != nil { - orgID := "org" + strings.ReplaceAll(uuid.NewString(), "-", "") + orgID = "org" + strings.ReplaceAll(uuid.NewString(), "-", "") linkingService.SetOwnerOrg(owner, orgID) if orgIDAsSecretOwnerEnabled { expectedResponseOwner = orgID } } + if orgIDAsSecretOwnerEnabled { + require.NotEmpty(t, orgID, "JWT auth enabled topology must link the workflow owner to an org ID") + } wfRegAddr := crecontracts.MustGetAddressFromDataStore(testEnv.CreEnvironment.CldfEnvironment.DataStore, testEnv.CreEnvironment.Blockchains[0].ChainSelector(), keystone_changeset.WorkflowRegistry.String(), testEnv.CreEnvironment.ContractVersions[keystone_changeset.WorkflowRegistry.String()], "") wfReg, err := workflow_registry_v2_wrapper.NewWorkflowRegistry(common.HexToAddress(wfRegAddr), sc.Client) require.NoError(t, err) requireVaultLinkOwner(t, sc, common.HexToAddress(wfRegAddr), testEnv.CreEnvironment.ContractVersions[keystone_changeset.WorkflowRegistry.String()]) - secretID := strconv.Itoa(rand.Intn(10000)) + vaultParsedPublicKey := mustVaultPublicKey(t, vaultPublicKey) + secretID := uniqueVaultSecretID("allowlist") createValue := "secret-basic-create" updateValue := "secret-basic-update" - createEnc, err := crevault.EncryptSecret(createValue, vaultPublicKey, sc.MustGetRootKeyAddress()) + createEnc, err := vaultutils.EncryptSecretWithWorkflowOwner(createValue, vaultParsedPublicKey, workflowOwnerAddress) require.NoError(t, err) - updateEnc, err := crevault.EncryptSecret(updateValue, vaultPublicKey, sc.MustGetRootKeyAddress()) + updateEnc, err := vaultutils.EncryptSecretWithWorkflowOwner(updateValue, vaultParsedPublicKey, workflowOwnerAddress) require.NoError(t, err) ulCh := make(chan *workflowevents.UserLogs, 1000) bmCh := make(chan *commonevents.BaseMessage, 1000) @@ -78,15 +86,78 @@ func ExecuteVaultAllowListBasedTests(t *testing.T, fixture *vaultScenarioFixture namespaces := []string{"main", "alt"} executeVaultAllowListSecretsCreateTest(t, createEnc, secretID, owner, expectedResponseOwner, gwURL, namespaces, sc, wfReg) + var orgIDLabelSecretID string + var orgIDLabelCreateValue string + if orgIDAsSecretOwnerEnabled { + orgIDLabelSecretID = secretID + "orgidlabel" + orgIDLabelCreateValue = "secret-basic-create-org-id-label" + orgIDLabelCreateEnc, orgErr := vaultutils.EncryptSecretWithOrgID(orgIDLabelCreateValue, vaultParsedPublicKey, orgID) + require.NoError(t, orgErr) + executeVaultAllowListSecretsCreateTest(t, orgIDLabelCreateEnc, orgIDLabelSecretID, owner, expectedResponseOwner, gwURL, namespaces, sc, wfReg) + } executeVaultSecretsUpdateTest(t, updateEnc, secretID, owner, expectedResponseOwner, gwURL, namespaces, sc, wfReg) executeVaultSecretsListTest(t, secretID, owner, expectedResponseOwner, gwURL, "main", sc, wfReg) executeVaultSecretsListTest(t, secretID, owner, expectedResponseOwner, gwURL, "alt", sc, wfReg) - executeVaultSecretsDeleteTest(t, secretID, owner, expectedResponseOwner, gwURL, []string{"main"}, sc, wfReg) - executeVaultSecretsWorkflowChecksTest(t, testEnv, "allowlist-final-verify", []vaultWorkflowCheck{ + updatedChecks := []vaultWorkflowCheck{ + {Name: "allowlist-main-updated", SecretKey: secretID, SecretNamespace: "main", ExpectedValue: updateValue}, + {Name: "allowlist-alt-updated", SecretKey: secretID, SecretNamespace: "alt", ExpectedValue: updateValue}, + } + finalChecks := []vaultWorkflowCheck{ {Name: "allowlist-main-not-found", SecretKey: secretID, SecretNamespace: "main", ExpectNotFound: true}, {Name: "allowlist-alt-updated", SecretKey: secretID, SecretNamespace: "alt", ExpectedValue: updateValue}, - }, ulCh, bmCh) + } + if orgIDAsSecretOwnerEnabled { + orgIDChecks := []vaultWorkflowCheck{ + {Name: "allowlist-org-id-label-main", SecretKey: orgIDLabelSecretID, SecretNamespace: "main", ExpectedValue: orgIDLabelCreateValue}, + {Name: "allowlist-org-id-label-alt", SecretKey: orgIDLabelSecretID, SecretNamespace: "alt", ExpectedValue: orgIDLabelCreateValue}, + } + updatedChecks = append(updatedChecks, orgIDChecks...) + finalChecks = append(finalChecks, orgIDChecks...) + } + workflowID := startVaultSecretsWorkflowPhasesTest(t, testEnv, "allowlist-lifecycle", []vaultWorkflowPhase{ + {Name: "allowlist-updated", Checks: updatedChecks}, + {Name: "allowlist-final-verify", Checks: finalChecks}, + }) + waitForVaultWorkflowPhase(t, workflowID, "allowlist-updated", ulCh, bmCh) + executeVaultSecretsDeleteTest(t, secretID, owner, expectedResponseOwner, gwURL, []string{"main"}, sc, wfReg) + waitForVaultWorkflowPhase(t, workflowID, "allowlist-final-verify", ulCh, bmCh) executeVaultSecretsDeleteTest(t, secretID, owner, expectedResponseOwner, gwURL, []string{"alt"}, sc, wfReg) + if orgIDAsSecretOwnerEnabled { + executeVaultSecretsDeleteTest(t, orgIDLabelSecretID, owner, expectedResponseOwner, gwURL, namespaces, sc, wfReg) + } + }) + + if !isVaultJWTAuthEnabledTopology(testEnv.TestConfig.EnvironmentConfigPath) { + return + } + + t.Run("allowlist_crud_with_org_id_identity", func(t *testing.T) { + require.NotNil(t, linkingService, "JWT auth enabled topology must include a linking service") + + sc := testEnv.CreEnvironment.Blockchains[0].(*evm.Blockchain).SethClient + workflowOwnerAddress := sc.MustGetRootKeyAddress() + workflowOwner := workflowOwnerAddress.Hex() + orgID := "org" + strings.ReplaceAll(uuid.NewString(), "-", "") + linkingService.SetOwnerOrg(workflowOwner, orgID) + + wfRegAddr := crecontracts.MustGetAddressFromDataStore(testEnv.CreEnvironment.CldfEnvironment.DataStore, testEnv.CreEnvironment.Blockchains[0].ChainSelector(), keystone_changeset.WorkflowRegistry.String(), testEnv.CreEnvironment.ContractVersions[keystone_changeset.WorkflowRegistry.String()], "") + wfReg, err := workflow_registry_v2_wrapper.NewWorkflowRegistry(common.HexToAddress(wfRegAddr), sc.Client) + require.NoError(t, err) + requireVaultLinkOwner(t, sc, common.HexToAddress(wfRegAddr), testEnv.CreEnvironment.ContractVersions[keystone_changeset.WorkflowRegistry.String()]) + + vaultParsedPublicKey := mustVaultPublicKey(t, vaultPublicKey) + secretID := uniqueVaultSecretID("allowlistorgid") + createEnc, err := vaultutils.EncryptSecretWithOrgID("secret-org-id-owner-create", vaultParsedPublicKey, orgID) + require.NoError(t, err) + updateEnc, err := vaultutils.EncryptSecretWithOrgID("secret-org-id-owner-update", vaultParsedPublicKey, orgID) + require.NoError(t, err) + + allowlistAuth := newAllowlistVaultRequestAuth(workflowOwner, sc, wfReg) + namespaces := []string{"main"} + executeVaultSecretsCreateWithAuthExpectOwnersAndIdentifierOwner(t, allowlistAuth, orgID, createEnc, secretID, []string{orgID}, gwURL, namespaces) + executeVaultSecretsUpdateWithAuthAndIdentifierOwner(t, allowlistAuth, orgID, updateEnc, secretID, orgID, gwURL, namespaces) + executeVaultSecretsListWithAuthAndOwner(t, allowlistAuth, orgID, []string{secretID}, orgID, gwURL, "main") + executeVaultSecretsDeleteWithAuthAndIdentifierOwner(t, allowlistAuth, orgID, secretID, orgID, gwURL, namespaces) }) } @@ -131,7 +202,7 @@ func ExecuteVaultMixedAuthTest(t *testing.T, fixture *vaultScenarioFixture, test workflowOwnerAddress := common.HexToAddress(workflowOwner) t.Run("jwt_crud_with_workflow_owner", func(t *testing.T) { - secretID := strconv.Itoa(rand.Intn(10000)) + secretID := uniqueVaultSecretID("jwt") createValue := "secret-jwt-workflow-owner" enc, err := vaultutils.EncryptSecretWithOrgID(createValue, vaultParsedPublicKey, orgID) require.NoError(t, err) @@ -160,19 +231,39 @@ func ExecuteVaultMixedAuthTest(t *testing.T, fixture *vaultScenarioFixture, test waitForVaultWorkflowPhase(t, workflowID, "jwt-deleted", ulCh, bmCh) }) + t.Run("jwt_rejected_when_secret_labeled_as_workflow_owner", func(t *testing.T) { + secretID := uniqueVaultSecretID("jwtreject") + encryptedSecret, err := vaultutils.EncryptSecretWithWorkflowOwner("secret-jwt-wrong-label", vaultParsedPublicKey, workflowOwnerAddress) + require.NoError(t, err) + + uniqueRequestID := uuid.New().String() + secretsCreateRequest := vault_helpers.CreateSecretsRequest{ + RequestId: uniqueRequestID, + EncryptedSecrets: buildEncryptedSecrets(secretID, orgID, encryptedSecret, []string{"main"}), + } + jsonRequest := newVaultJSONRequest(t, uniqueRequestID, vaulttypes.MethodSecretsCreate, &secretsCreateRequest) + jwtAuth.apply(t, &jsonRequest) + + jsonResponse := sendVaultJWTRequestToGatewayExpectError(t, gwURL, jsonRequest, http.StatusBadRequest) + require.Equal(t, uniqueRequestID, jsonResponse.ID) + require.NotNil(t, jsonResponse.Error) + require.Equal(t, jsonrpc.ErrInvalidParams, jsonResponse.Error.Code) + require.Contains(t, jsonResponse.Error.Error(), "doesn't have owner as the label") + }) + t.Run("mixed_allowlist_and_jwt_auth", func(t *testing.T) { t.Run("cross_auth_create_update_list_and_delete", func(t *testing.T) { - allowlistSecretID := strconv.Itoa(rand.Intn(10000)) - jwtSecretID := strconv.Itoa(rand.Intn(10000)) + allowlistSecretID := uniqueVaultSecretID("mixedallowlist") + jwtSecretID := uniqueVaultSecretID("mixedjwt") allowlistCreateValue := "secret-mixed-allowlist-create" jwtCreateValue := "secret-mixed-jwt-create" allowlistUpdateValue := "secret-mixed-allowlist-update" jwtUpdateValue := "secret-mixed-jwt-update" - allowlistCreateEnc, err := crevault.EncryptSecret(allowlistCreateValue, vaultPublicKey, workflowOwnerAddress) + allowlistCreateEnc, err := vaultutils.EncryptSecretWithOrgID(allowlistCreateValue, vaultParsedPublicKey, orgID) require.NoError(t, err) jwtCreateEnc, err := vaultutils.EncryptSecretWithOrgID(jwtCreateValue, vaultParsedPublicKey, orgID) require.NoError(t, err) - allowlistUpdateEnc, err := crevault.EncryptSecret(allowlistUpdateValue, vaultPublicKey, workflowOwnerAddress) + allowlistUpdateEnc, err := vaultutils.EncryptSecretWithOrgID(allowlistUpdateValue, vaultParsedPublicKey, orgID) require.NoError(t, err) jwtUpdateEnc, err := vaultutils.EncryptSecretWithOrgID(jwtUpdateValue, vaultParsedPublicKey, orgID) require.NoError(t, err) @@ -217,8 +308,15 @@ func ExecuteVaultMixedAuthTest(t *testing.T, fixture *vaultScenarioFixture, test }) }) - t.Run("jwt_rejected_when_workflow_owner_missing", func(t *testing.T) { - executeVaultJWTSecretsCreateUnauthorizedTest(t, issuer, vaultPublicKey, orgID, "", gwURL, "missing workflow_owner in authorization_details") + t.Run("jwt_without_workflow_owner_uses_org_id_identity", func(t *testing.T) { + secretID := uniqueVaultSecretID("jwtorgonly") + encryptedSecret, err := vaultutils.EncryptSecretWithOrgID("secret-jwt-org-only", vaultParsedPublicKey, orgID) + require.NoError(t, err) + + orgOnlyJWTAuth := newJWTVaultRequestAuth(issuer, orgID, "") + executeVaultSecretsCreateWithAuth(t, orgOnlyJWTAuth, encryptedSecret, secretID, orgID, gwURL, []string{"main"}) + executeVaultSecretsListWithAuth(t, orgOnlyJWTAuth, []string{secretID}, orgID, gwURL, "main") + executeVaultSecretsDeleteWithAuth(t, orgOnlyJWTAuth, secretID, orgID, gwURL, []string{"main"}) }) t.Run("jwt_rejected_when_vault_secret_management_claim_false", func(t *testing.T) { diff --git a/system-tests/tests/smoke/cre/v2_vault_don_test_helpers.go b/system-tests/tests/smoke/cre/v2_vault_don_test_helpers.go index c2a122eab17..26587e961c0 100644 --- a/system-tests/tests/smoke/cre/v2_vault_don_test_helpers.go +++ b/system-tests/tests/smoke/cre/v2_vault_don_test_helpers.go @@ -391,12 +391,25 @@ func sendVaultSignedOCRRequestToGateway(t *testing.T, gatewayURL string, jsonReq func executeVaultSecretsCreateWithAuth(t *testing.T, auth vaultRequestAuth, encryptedSecret, secretID, expectedResponseOwner, gatewayURL string, namespaces []string) { t.Helper() + executeVaultSecretsCreateWithAuthExpectOwners(t, auth, encryptedSecret, secretID, []string{expectedResponseOwner}, gatewayURL, namespaces) +} + +func executeVaultSecretsCreateWithAuthExpectOwners(t *testing.T, auth vaultRequestAuth, encryptedSecret, secretID string, expectedResponseOwners []string, gatewayURL string, namespaces []string) string { + t.Helper() + + return executeVaultSecretsCreateWithAuthExpectOwnersAndIdentifierOwner(t, auth, auth.requestOwner, encryptedSecret, secretID, expectedResponseOwners, gatewayURL, namespaces) +} + +func executeVaultSecretsCreateWithAuthExpectOwnersAndIdentifierOwner(t *testing.T, auth vaultRequestAuth, identifierOwner, encryptedSecret, secretID string, expectedResponseOwners []string, gatewayURL string, namespaces []string) string { + t.Helper() + framework.L.Info().Msgf("Creating secrets (namespaces=%v)...", namespaces) + require.NotEmpty(t, expectedResponseOwners, "expected response owners must not be empty") uniqueRequestID := uuid.New().String() secretsCreateRequest := vault_helpers.CreateSecretsRequest{ RequestId: uniqueRequestID, - EncryptedSecrets: buildEncryptedSecrets(secretID, auth.requestOwner, encryptedSecret, namespaces), + EncryptedSecrets: buildEncryptedSecrets(secretID, identifierOwner, encryptedSecret, namespaces), } jsonRequest := newVaultJSONRequest(t, uniqueRequestID, vaulttypes.MethodSecretsCreate, &secretsCreateRequest) auth.apply(t, &jsonRequest) @@ -414,26 +427,40 @@ func executeVaultSecretsCreateWithAuth(t *testing.T, auth vaultRequestAuth, encr for _, r := range createSecretsResponse.GetResponses() { respByNs[r.GetId().GetNamespace()] = r } + actualResponseOwner := "" for _, namespace := range namespaces { result, ok := respByNs[namespace] require.True(t, ok, "missing response for namespace %s", namespace) require.Empty(t, result.GetError()) require.Equal(t, secretID, result.GetId().Key) - require.Equal(t, expectedResponseOwner, result.GetId().Owner) + require.Contains(t, expectedResponseOwners, result.GetId().Owner) + if actualResponseOwner == "" { + actualResponseOwner = result.GetId().Owner + continue + } + require.Equal(t, actualResponseOwner, result.GetId().Owner) } + + return actualResponseOwner } func executeVaultSecretsUpdateWithAuth(t *testing.T, auth vaultRequestAuth, encryptedSecret, secretID, expectedResponseOwner, gatewayURL string, namespaces []string) { t.Helper() + executeVaultSecretsUpdateWithAuthAndIdentifierOwner(t, auth, auth.requestOwner, encryptedSecret, secretID, expectedResponseOwner, gatewayURL, namespaces) +} + +func executeVaultSecretsUpdateWithAuthAndIdentifierOwner(t *testing.T, auth vaultRequestAuth, identifierOwner, encryptedSecret, secretID, expectedResponseOwner, gatewayURL string, namespaces []string) { + t.Helper() + framework.L.Info().Msgf("Updating secrets (namespaces=%v)...", namespaces) require.NotEmpty(t, namespaces, "namespaces must not be empty") - encryptedSecrets := buildEncryptedSecrets(secretID, auth.requestOwner, encryptedSecret, namespaces) + encryptedSecrets := buildEncryptedSecrets(secretID, identifierOwner, encryptedSecret, namespaces) encryptedSecrets = append(encryptedSecrets, &vault_helpers.EncryptedSecret{ Id: &vault_helpers.SecretIdentifier{ Key: "invalid", - Owner: auth.requestOwner, + Owner: identifierOwner, Namespace: namespaces[0], }, EncryptedValue: encryptedSecret, @@ -479,12 +506,18 @@ func executeVaultSecretsUpdateWithAuth(t *testing.T, auth vaultRequestAuth, encr func executeVaultSecretsListWithAuth(t *testing.T, auth vaultRequestAuth, expectedKeys []string, expectedOwner, gatewayURL, namespace string) { t.Helper() + executeVaultSecretsListWithAuthAndOwner(t, auth, auth.requestOwner, expectedKeys, expectedOwner, gatewayURL, namespace) +} + +func executeVaultSecretsListWithAuthAndOwner(t *testing.T, auth vaultRequestAuth, requestOwner string, expectedKeys []string, expectedOwner, gatewayURL, namespace string) { + t.Helper() + framework.L.Info().Msgf("Listing secrets (namespace=%s)...", namespace) uniqueRequestID := uuid.New().String() secretsListRequest := vault_helpers.ListSecretIdentifiersRequest{ RequestId: uniqueRequestID, - Owner: auth.requestOwner, + Owner: requestOwner, Namespace: namespace, } jsonRequest := newVaultJSONRequest(t, uniqueRequestID, vaulttypes.MethodSecretsList, &secretsListRequest) @@ -514,13 +547,19 @@ func executeVaultSecretsListWithAuth(t *testing.T, auth vaultRequestAuth, expect func executeVaultSecretsDeleteWithAuth(t *testing.T, auth vaultRequestAuth, secretID, expectedResponseOwner, gatewayURL string, namespaces []string) { t.Helper() + executeVaultSecretsDeleteWithAuthAndIdentifierOwner(t, auth, auth.requestOwner, secretID, expectedResponseOwner, gatewayURL, namespaces) +} + +func executeVaultSecretsDeleteWithAuthAndIdentifierOwner(t *testing.T, auth vaultRequestAuth, identifierOwner, secretID, expectedResponseOwner, gatewayURL string, namespaces []string) { + t.Helper() + framework.L.Info().Msgf("Deleting secrets (namespaces=%v)...", namespaces) require.NotEmpty(t, namespaces, "namespaces must not be empty") - deleteIDs := buildSecretIdentifiers(secretID, auth.requestOwner, namespaces) + deleteIDs := buildSecretIdentifiers(secretID, identifierOwner, namespaces) deleteIDs = append(deleteIDs, &vault_helpers.SecretIdentifier{ Key: "invalid", - Owner: auth.requestOwner, + Owner: identifierOwner, Namespace: namespaces[0], }) @@ -562,6 +601,8 @@ func executeVaultSecretsDeleteWithAuth(t *testing.T, auth vaultRequestAuth, secr } func executeVaultAllowListSecretsCreateTest(t *testing.T, encryptedSecret, secretID, requestOwner, expectedResponseOwner, gatewayURL string, namespaces []string, sethClient *seth.Client, wfRegistryContract *workflow_registry_v2_wrapper.WorkflowRegistry) { + t.Helper() + auth := newAllowlistVaultRequestAuth(requestOwner, sethClient, wfRegistryContract) executeVaultSecretsCreateWithAuth(t, auth, encryptedSecret, secretID, expectedResponseOwner, gatewayURL, namespaces) }