From 9aea342be1764c33033aa1717242829970d5f5be Mon Sep 17 00:00:00 2001 From: Matheus Macabu Date: Fri, 16 May 2025 11:32:20 +0200 Subject: [PATCH] SecretsManager: Update decrypt authorization with service identity --- pkg/registry/apis/secret/contracts/decrypt.go | 2 +- .../apis/secret/decrypt/authorizer.go | 104 ++++--- .../apis/secret/decrypt/authorizer_test.go | 294 +++++++++++++++--- .../apis/secret/decrypt/noop_authorizer.go | 2 +- 4 files changed, 304 insertions(+), 98 deletions(-) diff --git a/pkg/registry/apis/secret/contracts/decrypt.go b/pkg/registry/apis/secret/contracts/decrypt.go index a636f350efd..73ff2248e7a 100644 --- a/pkg/registry/apis/secret/contracts/decrypt.go +++ b/pkg/registry/apis/secret/contracts/decrypt.go @@ -21,7 +21,7 @@ type DecryptStorage interface { // DecryptAuthorizer is the interface for authorizing decryption requests. type DecryptAuthorizer interface { - Authorize(ctx context.Context, secureValueDecrypters []string) (identity string, allowed bool) + Authorize(ctx context.Context, secureValueName string, secureValueDecrypters []string) (identity string, allowed bool) } // TEMPORARY: Needed to pass it with wire. diff --git a/pkg/registry/apis/secret/decrypt/authorizer.go b/pkg/registry/apis/secret/decrypt/authorizer.go index 9e12552bba1..de76ca759f6 100644 --- a/pkg/registry/apis/secret/decrypt/authorizer.go +++ b/pkg/registry/apis/secret/decrypt/authorizer.go @@ -4,6 +4,7 @@ import ( "context" "strings" + "github.com/grafana/authlib/authn" claims "github.com/grafana/authlib/types" secretv0alpha1 "github.com/grafana/grafana/pkg/apis/secret/v0alpha1" @@ -22,70 +23,79 @@ func ProvideDecryptAuthorizer(allowList contracts.DecryptAllowList) contracts.De } // authorize checks whether the auth info token has the right permissions to decrypt the secure value. -func (a *decryptAuthorizer) Authorize(ctx context.Context, secureValueDecrypters []string) (string, bool) { +func (a *decryptAuthorizer) Authorize(ctx context.Context, secureValueName string, secureValueDecrypters []string) (string, bool) { authInfo, ok := claims.AuthInfoFrom(ctx) if !ok { return "", false } - tokenPermissions := authInfo.GetTokenPermissions() - - tokenActors := make(map[string]struct{}, 0) - for _, permission := range tokenPermissions { - // Will look like `secret.grafana.app/securevalues/:decrypt` for now. - gr, verb, found := strings.Cut(permission, ":") - if !found { - continue - } - - // If it isn't decrypt, then we don't care to check. - if verb != "decrypt" { - continue - } - - parts := strings.Split(gr, "/") - if len(parts) != 3 { - continue - } + serviceIdentityList, ok := authInfo.GetExtra()[authn.ServiceIdentityKey] + if !ok { + return "", false + } - group, resource, actor := parts[0], parts[1], parts[2] - if group != secretv0alpha1.GROUP || resource != secretv0alpha1.SecureValuesResourceInfo.GetName() || actor == "" { - continue - } + // If there's more than one service identity, something is suspicious and we reject it. + if len(serviceIdentityList) != 1 { + return "", false + } - // TEMPORARY: while we can't onboard every app into secrets, we can block them from decrypting - // securevalues preemptively here before even reaching out to the database. - // This check can be removed once we open the gates for any service to use secrets. - if _, exists := a.allowList[actor]; !exists { - continue - } + serviceIdentity := serviceIdentityList[0] - tokenActors[actor] = struct{}{} + // TEMPORARY: while we can't onboard every app into secrets, we can block them from decrypting + // securevalues preemptively here before even reaching out to the database. + // This check can be removed once we open the gates for any service to use secrets. + if _, exists := a.allowList[serviceIdentity]; !exists || serviceIdentity == "" { + return serviceIdentity, false } - // If we arrived here and the token actors is empty, it means the permissions either have an invalid format, - // or it didn't pass the allow list, meaning no allowed decryptor. - if len(tokenActors) == 0 { - return "", false + // Checks whether the token has the permission to decrypt secure values. + if !hasPermissionInToken(authInfo.GetTokenPermissions(), secureValueName) { + return serviceIdentity, false } - // TEMPORARY: while we still need to mix permission and identity, we can use this - // to decide whether the SecureValue can be decrypted or not. - // Once we have an `actor` field in the JWT claims, we can have a properly formatted permission, - // like `secret.grafana.app/securevalues{/}:decrypt` and do regular access control eval, - // and for the `decrypters` part here, we can just check it against the `actor` field, which at - // that point will have a different format, depending on how the `actor` will be formatted. - // Check whether at least one of declared token actors matches the allowed decrypters from the SecureValue. + // Finally check whether the service identity is allowed to decrypt this secure value. allowed := false - - var identity string for _, decrypter := range secureValueDecrypters { - if _, exists := tokenActors[decrypter]; exists { + if decrypter == serviceIdentity { allowed = true - identity = decrypter break } } - return identity, allowed + return serviceIdentity, allowed +} + +// Adapted from https://github.com/grafana/authlib/blob/1492b99410603ca15730a1805a9220ce48232bc3/authz/client.go#L138 +// Changes: 1) we don't support `*` for verbs; 2) we support specific names in the permission. +func hasPermissionInToken(tokenPermissions []string, name string) bool { + var ( + group = secretv0alpha1.GROUP + resource = secretv0alpha1.SecureValuesResourceInfo.GetName() + verb = "decrypt" + ) + + for _, p := range tokenPermissions { + tokenGR, tokenVerb, found := strings.Cut(p, ":") + if !found || tokenVerb != verb { + continue + } + + parts := strings.SplitN(tokenGR, "/", 3) + + switch len(parts) { + // secret.grafana.app/securevalues:decrypt + case 2: + if parts[0] == group && parts[1] == resource { + return true + } + + // secret.grafana.app/securevalues/:decrypt + case 3: + if parts[0] == group && parts[1] == resource && parts[2] == name && name != "" { + return true + } + } + } + + return false } diff --git a/pkg/registry/apis/secret/decrypt/authorizer_test.go b/pkg/registry/apis/secret/decrypt/authorizer_test.go index 9859f7a64c9..bd808a1740c 100644 --- a/pkg/registry/apis/secret/decrypt/authorizer_test.go +++ b/pkg/registry/apis/secret/decrypt/authorizer_test.go @@ -16,127 +16,323 @@ func TestDecryptAuthorizer(t *testing.T) { ctx := context.Background() authorizer := ProvideDecryptAuthorizer(nil) - identity, allowed := authorizer.Authorize(ctx, nil) + identity, allowed := authorizer.Authorize(ctx, "", nil) require.Empty(t, identity) require.False(t, allowed) }) t.Run("when token permissions are empty, it returns false", func(t *testing.T) { - ctx := createAuthContext(context.Background(), []string{}) + ctx := createAuthContext(context.Background(), "identity", []string{}) authorizer := ProvideDecryptAuthorizer(nil) - identity, allowed := authorizer.Authorize(ctx, nil) + identity, allowed := authorizer.Authorize(ctx, "", nil) + require.NotEmpty(t, identity) + require.False(t, allowed) + }) + + t.Run("when service identity is empty, it returns false", func(t *testing.T) { + ctx := createAuthContext(context.Background(), "", []string{}) + authorizer := ProvideDecryptAuthorizer(nil) + + identity, allowed := authorizer.Authorize(ctx, "", nil) require.Empty(t, identity) require.False(t, allowed) }) t.Run("when permission format is malformed (missing verb), it returns false", func(t *testing.T) { - ctx := createAuthContext(context.Background(), []string{"secret.grafana.app/securevalues/group1"}) authorizer := ProvideDecryptAuthorizer(nil) - identity, allowed := authorizer.Authorize(ctx, nil) - require.Empty(t, identity) + // nameless + ctx := createAuthContext(context.Background(), "identity", []string{"secret.grafana.app/securevalues"}) + identity, allowed := authorizer.Authorize(ctx, "", nil) + require.NotEmpty(t, identity) + require.False(t, allowed) + + // named + ctx = createAuthContext(context.Background(), "identity", []string{"secret.grafana.app/securevalues/name"}) + identity, allowed = authorizer.Authorize(ctx, "", nil) + require.NotEmpty(t, identity) require.False(t, allowed) }) t.Run("when permission verb is not exactly `decrypt`, it returns false", func(t *testing.T) { - ctx := createAuthContext(context.Background(), []string{"secret.grafana.app/securevalues/group1:something"}) authorizer := ProvideDecryptAuthorizer(nil) - identity, allowed := authorizer.Authorize(ctx, nil) - require.Empty(t, identity) + // nameless + ctx := createAuthContext(context.Background(), "identity", []string{"secret.grafana.app/securevalues:*"}) + identity, allowed := authorizer.Authorize(ctx, "", nil) + require.NotEmpty(t, identity) + require.False(t, allowed) + + // named + ctx = createAuthContext(context.Background(), "identity", []string{"secret.grafana.app/securevalues/name:something"}) + identity, allowed = authorizer.Authorize(ctx, "", nil) + require.NotEmpty(t, identity) require.False(t, allowed) }) - t.Run("when permission does not have 3 parts, it returns false", func(t *testing.T) { - ctx := createAuthContext(context.Background(), []string{"secret.grafana.app/securevalues:decrypt"}) + t.Run("when permission does not have 2 or 3 parts, it returns false", func(t *testing.T) { + ctx := createAuthContext(context.Background(), "identity", []string{"secret.grafana.app:decrypt"}) authorizer := ProvideDecryptAuthorizer(nil) - identity, allowed := authorizer.Authorize(ctx, nil) - require.Empty(t, identity) + identity, allowed := authorizer.Authorize(ctx, "", nil) + require.NotEmpty(t, identity) require.False(t, allowed) }) t.Run("when permission has group that is not `secret.grafana.app`, it returns false", func(t *testing.T) { - ctx := createAuthContext(context.Background(), []string{"wrong.group/securevalues/invalid:decrypt"}) + ctx := createAuthContext(context.Background(), "identity", []string{"wrong.group/securevalues/invalid:decrypt"}) authorizer := ProvideDecryptAuthorizer(nil) - identity, allowed := authorizer.Authorize(ctx, nil) - require.Empty(t, identity) + identity, allowed := authorizer.Authorize(ctx, "", nil) + require.NotEmpty(t, identity) require.False(t, allowed) }) t.Run("when permission has resource that is not `securevalues`, it returns false", func(t *testing.T) { - ctx := createAuthContext(context.Background(), []string{"secret.grafana.app/invalid-resource/invalid:decrypt"}) authorizer := ProvideDecryptAuthorizer(nil) - identity, allowed := authorizer.Authorize(ctx, nil) - require.Empty(t, identity) + // nameless + ctx := createAuthContext(context.Background(), "identity", []string{"secret.grafana.app/invalid-resource:decrypt"}) + identity, allowed := authorizer.Authorize(ctx, "", nil) + require.NotEmpty(t, identity) + require.False(t, allowed) + + // named + ctx = createAuthContext(context.Background(), "identity", []string{"secret.grafana.app/invalid-resource/name:decrypt"}) + identity, allowed = authorizer.Authorize(ctx, "", nil) + require.NotEmpty(t, identity) require.False(t, allowed) }) - t.Run("when the actor is not in the allow list, it returns false", func(t *testing.T) { - ctx := createAuthContext(context.Background(), []string{"secret.grafana.app/securevalues/allowed2:decrypt"}) + t.Run("when the identity is not in the allow list, it returns false", func(t *testing.T) { + ctx := createAuthContext(context.Background(), "identity", []string{"secret.grafana.app/securevalues:decrypt"}) authorizer := ProvideDecryptAuthorizer(map[string]struct{}{"allowed1": {}}) - identity, allowed := authorizer.Authorize(ctx, nil) - require.Empty(t, identity) + identity, allowed := authorizer.Authorize(ctx, "", nil) + require.NotEmpty(t, identity) require.False(t, allowed) }) - t.Run("when the actor doesn't match any allowed decrypters, it returns false", func(t *testing.T) { - ctx := createAuthContext(context.Background(), []string{"secret.grafana.app/securevalues/group1:decrypt"}) - authorizer := ProvideDecryptAuthorizer(map[string]struct{}{"group1": {}}) + t.Run("when the identity doesn't match any allowed decrypters, it returns false", func(t *testing.T) { + authorizer := ProvideDecryptAuthorizer(map[string]struct{}{"identity": {}}) - identity, allowed := authorizer.Authorize(ctx, []string{"group2"}) - require.Empty(t, identity) + // nameless + ctx := createAuthContext(context.Background(), "identity", []string{"secret.grafana.app/securevalues:decrypt"}) + identity, allowed := authorizer.Authorize(ctx, "", []string{"group2"}) + require.NotEmpty(t, identity) + require.False(t, allowed) + + // named + ctx = createAuthContext(context.Background(), "identity", []string{"secret.grafana.app/securevalues/name:decrypt"}) + identity, allowed = authorizer.Authorize(ctx, "", []string{"group2"}) + require.NotEmpty(t, identity) require.False(t, allowed) }) - t.Run("when the actor matches an allowed decrypter, it returns true", func(t *testing.T) { - ctx := createAuthContext(context.Background(), []string{"secret.grafana.app/securevalues/group1:decrypt"}) - authorizer := ProvideDecryptAuthorizer(map[string]struct{}{"group1": {}}) + t.Run("when the identity matches an allowed decrypter, it returns true", func(t *testing.T) { + authorizer := ProvideDecryptAuthorizer(map[string]struct{}{"identity": {}}) + + // nameless + ctx := createAuthContext(context.Background(), "identity", []string{"secret.grafana.app/securevalues:decrypt"}) + identity, allowed := authorizer.Authorize(ctx, "", []string{"identity"}) + require.True(t, allowed) + require.Equal(t, "identity", identity) - identity, allowed := authorizer.Authorize(ctx, []string{"group1"}) + // named + ctx = createAuthContext(context.Background(), "identity", []string{"secret.grafana.app/securevalues/name:decrypt"}) + identity, allowed = authorizer.Authorize(ctx, "name", []string{"identity"}) require.True(t, allowed) - require.Equal(t, "group1", identity) + require.Equal(t, "identity", identity) }) t.Run("when there are multiple permissions, some invalid, only valid ones are considered", func(t *testing.T) { - ctx := createAuthContext(context.Background(), []string{ - "secret.grafana.app/securevalues/group1:decrypt", + ctx := createAuthContext(context.Background(), "identity", []string{ + "secret.grafana.app/securevalues/name1:decrypt", + "secret.grafana.app/securevalues/name2:decrypt", "secret.grafana.app/securevalues/invalid:read", "wrong.group/securevalues/group2:decrypt", - "secret.grafana.app/securevalues/group2:decrypt", + "secret.grafana.app/securevalues/identity:decrypt", // old style of identity+permission }) - authorizer := ProvideDecryptAuthorizer(map[string]struct{}{"group1": {}, "group2": {}}) + authorizer := ProvideDecryptAuthorizer(map[string]struct{}{"identity": {}}) - identity, allowed := authorizer.Authorize(ctx, []string{"group2", "group3"}) + identity, allowed := authorizer.Authorize(ctx, "name1", []string{"identity"}) require.True(t, allowed) - require.Equal(t, "group2", identity) + require.Equal(t, "identity", identity) + + identity, allowed = authorizer.Authorize(ctx, "name2", []string{"identity"}) + require.True(t, allowed) + require.Equal(t, "identity", identity) }) - t.Run("when multiple valid actors match decrypters, the first match already returns true", func(t *testing.T) { - ctx := createAuthContext(context.Background(), []string{ - "secret.grafana.app/securevalues/group1:decrypt", - "secret.grafana.app/securevalues/group2:decrypt", - }) - authorizer := ProvideDecryptAuthorizer(map[string]struct{}{"group1": {}, "group2": {}}) + t.Run("when empty secure value name with specific permission, it returns false", func(t *testing.T) { + ctx := createAuthContext(context.Background(), "identity", []string{"secret.grafana.app/securevalues/name:decrypt"}) + authorizer := ProvideDecryptAuthorizer(map[string]struct{}{"identity": {}}) + + identity, allowed := authorizer.Authorize(ctx, "", []string{"identity"}) + require.Equal(t, "identity", identity) + require.False(t, allowed) + }) + + t.Run("when permission has an extra / but no name, it returns false", func(t *testing.T) { + ctx := createAuthContext(context.Background(), "identity", []string{"secret.grafana.app/securevalues/:decrypt"}) + authorizer := ProvideDecryptAuthorizer(map[string]struct{}{"identity": {}}) + + identity, allowed := authorizer.Authorize(ctx, "", []string{"identity"}) + require.Equal(t, "identity", identity) + require.False(t, allowed) + }) + + t.Run("when the decrypters list is empty, meaning nothing can decrypt the secure value, it returns false", func(t *testing.T) { + ctx := createAuthContext(context.Background(), "identity", []string{"secret.grafana.app/securevalues:decrypt"}) + authorizer := ProvideDecryptAuthorizer(map[string]struct{}{"identity": {}}) + + identity, allowed := authorizer.Authorize(ctx, "name", []string{}) + require.Equal(t, "identity", identity) + require.False(t, allowed) + }) + + t.Run("when one of decrypters matches the identity, it returns true", func(t *testing.T) { + ctx := createAuthContext(context.Background(), "identity1", []string{"secret.grafana.app/securevalues:decrypt"}) + authorizer := ProvideDecryptAuthorizer(map[string]struct{}{"identity1": {}, "identity2": {}}) - identity, allowed := authorizer.Authorize(ctx, []string{"group2", "group1"}) + identity, allowed := authorizer.Authorize(ctx, "", []string{"identity1", "identity2", "identity3"}) + require.Equal(t, "identity1", identity) require.True(t, allowed) - require.Equal(t, "group2", identity) + }) + + t.Run("permissions must be case-sensitive and return false", func(t *testing.T) { + authorizer := ProvideDecryptAuthorizer(map[string]struct{}{"identity": {}}) + + ctx := createAuthContext(context.Background(), "identity", []string{"SECRET.grafana.app/securevalues:decrypt"}) + identity, allowed := authorizer.Authorize(ctx, "", []string{"identity"}) + require.Equal(t, "identity", identity) + require.False(t, allowed) + + ctx = createAuthContext(context.Background(), "identity", []string{"secret.grafana.app/SECUREVALUES:decrypt"}) + identity, allowed = authorizer.Authorize(ctx, "", []string{"identity"}) + require.Equal(t, "identity", identity) + require.False(t, allowed) + + ctx = createAuthContext(context.Background(), "identity", []string{"secret.grafana.app/securevalues:DECRYPT"}) + identity, allowed = authorizer.Authorize(ctx, "", []string{"identity"}) + require.Equal(t, "identity", identity) + require.False(t, allowed) }) } -func createAuthContext(ctx context.Context, permissions []string) context.Context { +func createAuthContext(ctx context.Context, serviceIdentity string, permissions []string) context.Context { requester := &identity.StaticRequester{ AccessTokenClaims: &authn.Claims[authn.AccessTokenClaims]{ Rest: authn.AccessTokenClaims{ - Permissions: permissions, + Permissions: permissions, + ServiceIdentity: serviceIdentity, }, }, } return types.WithAuthInfo(ctx, requester) } + +// Adapted from https://github.com/grafana/authlib/blob/1492b99410603ca15730a1805a9220ce48232bc3/authz/client_test.go#L18 +func TestHasPermissionInToken(t *testing.T) { + t.Parallel() + + tests := []struct { + test string + tokenPermissions []string + name string + want bool + }{ + { + test: "Permission matches group/resource", + tokenPermissions: []string{"secret.grafana.app/securevalues:decrypt"}, + want: true, + }, + { + test: "Permission does not match verb", + tokenPermissions: []string{"secret.grafana.app/securevalues:create"}, + want: false, + }, + { + test: "Permission does not have support for wildcard verb", + tokenPermissions: []string{"secret.grafana.app/securevalues:*"}, + want: false, + }, + { + test: "Invalid permission missing verb", + tokenPermissions: []string{"secret.grafana.app/securevalues"}, + want: false, + }, + { + test: "Permission on the wrong group", + tokenPermissions: []string{"other-group.grafana.app/securevalues:decrypt"}, + want: false, + }, + { + test: "Permission on the wrong resource", + tokenPermissions: []string{"secret.grafana.app/other-resource:decrypt"}, + want: false, + }, + { + test: "Permission without group are skipped", + tokenPermissions: []string{":decrypt"}, + want: false, + }, + { + test: "Group level permission is not supported", + tokenPermissions: []string{"secret.grafana.app:decrypt"}, + want: false, + }, + { + test: "Permission with name matches group/resource/name", + tokenPermissions: []string{"secret.grafana.app/securevalues/name:decrypt"}, + name: "name", + want: true, + }, + { + test: "Permission with name2 does not matche group/resource/name1", + tokenPermissions: []string{"secret.grafana.app/securevalues/name1:decrypt"}, + name: "name2", + want: false, + }, + { + test: "Parts need an exact match", + tokenPermissions: []string{"secret.grafana.app/secure:*"}, + want: false, + }, + { + test: "Resource specific permission should not allow access to all resources", + tokenPermissions: []string{"secret.grafana.app/securevalues/name:decrypt"}, + name: "", + want: false, + }, + { + test: "Permission at group/resource should allow access to all resources", + tokenPermissions: []string{"secret.grafana.app/securevalues:decrypt"}, + name: "name", + want: true, + }, + { + test: "Empty name trying to match everything is not allowed", + tokenPermissions: []string{"secret.grafana.app/securevalues/:decrypt"}, + name: "", + want: false, + }, + { + test: "Empty name trying to match a specific name is not allowed", + tokenPermissions: []string{"secret.grafana.app/securevalues/:decrypt"}, + name: "name", + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.test, func(t *testing.T) { + t.Parallel() + + got := hasPermissionInToken(tt.tokenPermissions, tt.name) + require.Equal(t, tt.want, got) + }) + } +} diff --git a/pkg/registry/apis/secret/decrypt/noop_authorizer.go b/pkg/registry/apis/secret/decrypt/noop_authorizer.go index d62af2f78b8..079d49b67ab 100644 --- a/pkg/registry/apis/secret/decrypt/noop_authorizer.go +++ b/pkg/registry/apis/secret/decrypt/noop_authorizer.go @@ -11,6 +11,6 @@ type NoopAlwaysAllowedAuthorizer struct{} var _ contracts.DecryptAuthorizer = &NoopAlwaysAllowedAuthorizer{} -func (a *NoopAlwaysAllowedAuthorizer) Authorize(ctx context.Context, secureValueDecrypters []string) (identity string, allowed bool) { +func (a *NoopAlwaysAllowedAuthorizer) Authorize(ctx context.Context, secureValueName string, secureValueDecrypters []string) (identity string, allowed bool) { return "", true }