From 1f637d07ebdd4b6101ebde4bc3d01e4d31a8370a Mon Sep 17 00:00:00 2001 From: Georges Chaudy Date: Wed, 12 Mar 2025 14:37:17 +0100 Subject: [PATCH] unistore: check namespace (#102020) * check namespace in unistore * fix tests * fix trace status * Use capital letter --------- Co-authored-by: Karl Persson <23356117+kalleep@users.noreply.github.com> --- .../authn/grpcutils/grpc_authenticator.go | 6 +- pkg/storage/unified/resource/access.go | 47 ++++- pkg/storage/unified/resource/access_test.go | 174 +++++++++++++++++- 3 files changed, 215 insertions(+), 12 deletions(-) diff --git a/pkg/services/authn/grpcutils/grpc_authenticator.go b/pkg/services/authn/grpcutils/grpc_authenticator.go index 542a5268eee..050b89820dc 100644 --- a/pkg/services/authn/grpcutils/grpc_authenticator.go +++ b/pkg/services/authn/grpcutils/grpc_authenticator.go @@ -105,6 +105,10 @@ func FallbackUsed(ctx context.Context) bool { return ctx.Value(contextFallbackKey{}) != nil } +func WithFallback(ctx context.Context) context.Context { + return context.WithValue(ctx, contextFallbackKey{}, true) +} + func (f *authenticatorWithFallback) Authenticate(ctx context.Context) (context.Context, error) { ctx, span := f.tracer.Start(ctx, "grpcutils.AuthenticatorWithFallback.Authenticate") defer span.End() @@ -122,7 +126,7 @@ func (f *authenticatorWithFallback) Authenticate(ctx context.Context) (context.C span.SetAttributes(attribute.Bool("fallback_used", true)) newCtx, err = f.fallback.Authenticate(ctx) if newCtx != nil { - newCtx = context.WithValue(newCtx, contextFallbackKey{}, true) + newCtx = WithFallback(newCtx) } f.metrics.requestsTotal.WithLabelValues("true", fmt.Sprintf("%t", err == nil)).Inc() return newCtx, err diff --git a/pkg/storage/unified/resource/access.go b/pkg/storage/unified/resource/access.go index 16dc0d9d170..5f8d184f57a 100644 --- a/pkg/storage/unified/resource/access.go +++ b/pkg/storage/unified/resource/access.go @@ -9,6 +9,7 @@ import ( "github.com/prometheus/client_golang/prometheus" "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/codes" "go.opentelemetry.io/otel/trace" "go.opentelemetry.io/otel/trace/noop" @@ -120,10 +121,28 @@ func (c authzLimitedClient) Check(ctx context.Context, id claims.AuthInfo, req c attribute.Bool("fallback_used", grpcutils.FallbackUsed(ctx)), )) defer span.End() + if grpcutils.FallbackUsed(ctx) { + if req.Namespace == "" { + // cross namespace queries are not allowed when fallback is used + span.SetAttributes(attribute.Bool("allowed", false)) + span.SetStatus(codes.Error, "Namespace empty") + err := fmt.Errorf("namespace empty") + span.RecordError(err) + return claims.CheckResponse{Allowed: false}, err + } + span.SetAttributes(attribute.Bool("allowed", true)) return claims.CheckResponse{Allowed: true}, nil } + + if !claims.NamespaceMatches(id.GetNamespace(), req.Namespace) { + span.SetAttributes(attribute.Bool("allowed", false)) + span.SetStatus(codes.Error, "Namespace missmatch") + span.RecordError(claims.ErrNamespaceMissmatch) + return claims.CheckResponse{Allowed: false}, claims.ErrNamespaceMissmatch + } + if !c.IsCompatibleWithRBAC(req.Group, req.Resource) { span.SetAttributes(attribute.Bool("allowed", true)) return claims.CheckResponse{Allowed: true}, nil @@ -132,7 +151,8 @@ func (c authzLimitedClient) Check(ctx context.Context, id claims.AuthInfo, req c if err != nil { c.logger.Error("Check", "group", req.Group, "resource", req.Resource, "error", err, "duration", time.Since(t), "traceid", tracing.TraceIDFromContext(ctx, false)) c.metrics.errorsTotal.WithLabelValues(req.Group, req.Resource, req.Verb).Inc() - span.SetAttributes(attribute.String("error", err.Error())) + span.SetStatus(codes.Error, fmt.Sprintf("check failed: %v", err)) + span.RecordError(err) return resp, err } span.SetAttributes(attribute.Bool("allowed", resp.Allowed)) @@ -152,7 +172,27 @@ func (c authzLimitedClient) Compile(ctx context.Context, id claims.AuthInfo, req attribute.Bool("fallback_used", fallbackUsed), )) defer span.End() - if fallbackUsed || !c.IsCompatibleWithRBAC(req.Group, req.Resource) { + if fallbackUsed { + if req.Namespace == "" { + // cross namespace queries are not allowed when fallback is used + span.SetAttributes(attribute.Bool("allowed", false)) + span.SetStatus(codes.Error, "Namespace empty") + err := fmt.Errorf("namespace empty") + span.RecordError(err) + return nil, err + } + return func(name, folder string) bool { + return true + }, nil + } + if !claims.NamespaceMatches(id.GetNamespace(), req.Namespace) { + span.SetAttributes(attribute.Bool("allowed", false)) + span.SetStatus(codes.Error, "Namespace missmatch") + span.RecordError(claims.ErrNamespaceMissmatch) + return nil, claims.ErrNamespaceMissmatch + } + + if !c.IsCompatibleWithRBAC(req.Group, req.Resource) { return func(name, folder string) bool { return true }, nil @@ -161,7 +201,8 @@ func (c authzLimitedClient) Compile(ctx context.Context, id claims.AuthInfo, req if err != nil { c.logger.Error("Compile", "group", req.Group, "resource", req.Resource, "error", err, "traceid", tracing.TraceIDFromContext(ctx, false)) c.metrics.errorsTotal.WithLabelValues(req.Group, req.Resource, req.Verb).Inc() - span.SetAttributes(attribute.String("error", err.Error())) + span.SetStatus(codes.Error, fmt.Sprintf("compile failed: %v", err)) + span.RecordError(err) return nil, err } c.metrics.compileDuration.WithLabelValues(req.Group, req.Resource, req.Verb).Observe(time.Since(t).Seconds()) diff --git a/pkg/storage/unified/resource/access_test.go b/pkg/storage/unified/resource/access_test.go index a056099ec36..9fc83c6dd62 100644 --- a/pkg/storage/unified/resource/access_test.go +++ b/pkg/storage/unified/resource/access_test.go @@ -5,9 +5,12 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" authlib "github.com/grafana/authlib/types" + "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/apimachinery/utils" + "github.com/grafana/grafana/pkg/services/authn/grpcutils" ) func TestAuthzLimitedClient_Check(t *testing.T) { @@ -26,11 +29,12 @@ func TestAuthzLimitedClient_Check(t *testing.T) { for _, test := range tests { req := authlib.CheckRequest{ - Group: test.group, - Resource: test.resource, - Verb: utils.VerbGet, + Group: test.group, + Resource: test.resource, + Verb: utils.VerbGet, + Namespace: "stacks-1", } - resp, err := client.Check(context.Background(), nil, req) + resp, err := client.Check(context.Background(), &identity.StaticRequester{Namespace: "stacks-1"}, req) assert.NoError(t, err) assert.Equal(t, test.expected, resp.Allowed) } @@ -52,11 +56,12 @@ func TestAuthzLimitedClient_Compile(t *testing.T) { for _, test := range tests { req := authlib.ListRequest{ - Group: test.group, - Resource: test.resource, - Verb: utils.VerbGet, + Group: test.group, + Resource: test.resource, + Verb: utils.VerbGet, + Namespace: "stacks-1", } - checker, err := client.Compile(context.Background(), nil, req) + checker, err := client.Compile(context.Background(), &identity.StaticRequester{Namespace: "stacks-1"}, req) assert.NoError(t, err) assert.NotNil(t, checker) @@ -64,3 +69,156 @@ func TestAuthzLimitedClient_Compile(t *testing.T) { assert.Equal(t, test.expected, result) } } + +// TestNamespaceMatching tests namespace matching in Check and Compile methods +func TestNamespaceMatching(t *testing.T) { + // Create a mock client that always returns allowed=true + mockClient := authlib.FixedAccessClient(true) + client := NewAuthzLimitedClient(mockClient, AuthzOptions{}) + + // Create a context with fallback disabled + ctx := context.Background() + + tests := []struct { + name string + authNamespace string + reqNamespace string + expectError bool + }{ + { + name: "matching namespaces", + authNamespace: "ns1", + reqNamespace: "ns1", + expectError: false, + }, + { + name: "mismatched namespaces", + authNamespace: "ns1", + reqNamespace: "ns2", + expectError: true, + }, + { + name: "empty request namespace", + authNamespace: "ns1", + reqNamespace: "", + expectError: true, + }, + { + name: "empty auth namespace", + authNamespace: "", + reqNamespace: "ns1", + expectError: true, + }, + { + name: "wildcard auth namespace", + authNamespace: "*", + reqNamespace: "ns1", + expectError: false, + }, + { + name: "both empty namespaces", + authNamespace: "", + reqNamespace: "", + expectError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Test Check method with namespace matching + checkReq := authlib.CheckRequest{ + Group: "unknown.group", // Use unknown group to bypass RBAC check + Resource: "unknown.resource", + Verb: utils.VerbGet, + Namespace: tt.reqNamespace, + } + // Create a mock auth info with the specified namespace + // Test Check method + user := &identity.StaticRequester{Namespace: tt.authNamespace} + _, checkErr := client.Check(ctx, user, checkReq) + + // Test Compile method + compileReq := authlib.ListRequest{ + Group: "unknown.group", // Use unknown group to bypass RBAC check + Resource: "unknown.resource", + Verb: utils.VerbGet, + Namespace: tt.reqNamespace, + } + _, compileErr := client.Compile(ctx, user, compileReq) + + if tt.expectError { + require.Error(t, checkErr, "Check should return error") + require.Error(t, compileErr, "Compile should return error") + assert.ErrorIs(t, checkErr, authlib.ErrNamespaceMissmatch, "Check should return namespace mismatch error") + assert.ErrorIs(t, compileErr, authlib.ErrNamespaceMissmatch, "Compile should return namespace mismatch error") + } else { + assert.NoError(t, checkErr, "Check should not return error when namespaces match") + assert.NoError(t, compileErr, "Compile should not return error when namespaces match") + } + }) + } +} + +// TestNamespaceMatchingFallback tests namespace matching in Check and Compile methods when fallback is used +func TestNamespaceMatchingFallback(t *testing.T) { + // Create a mock client that always returns allowed=true + mockClient := authlib.FixedAccessClient(true) + client := NewAuthzLimitedClient(mockClient, AuthzOptions{}) + + // Create a context with fallback disabled + ctx := context.Background() + + tests := []struct { + name string + authNamespace string + reqNamespace string + expectError bool + }{ + { + name: "with namespace fallback", + reqNamespace: "ns1", + expectError: false, + }, + { + name: "empty request namespace with fallback", + reqNamespace: "", + expectError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Test Check method with namespace matching + checkReq := authlib.CheckRequest{ + Group: "unknown.group", // Use unknown group to bypass RBAC check + Resource: "unknown.resource", + Verb: utils.VerbGet, + Namespace: tt.reqNamespace, + } + ctx = grpcutils.WithFallback(ctx) + // Create a mock auth info with the specified namespace + // Test Check method + user := &identity.StaticRequester{Namespace: tt.authNamespace} + _, checkErr := client.Check(ctx, user, checkReq) + + // Test Compile method + compileReq := authlib.ListRequest{ + Group: "unknown.group", // Use unknown group to bypass RBAC check + Resource: "unknown.resource", + Verb: utils.VerbGet, + Namespace: tt.reqNamespace, + } + _, compileErr := client.Compile(ctx, user, compileReq) + + if tt.expectError { + require.Error(t, checkErr, "Check should return error") + require.Error(t, compileErr, "Compile should return error") + assert.ErrorContains(t, checkErr, "namespace empty", "Check should return namespace mismatch error") + assert.ErrorContains(t, compileErr, "namespace empty", "Compile should return namespace mismatch error") + } else { + assert.NoError(t, checkErr, "Check should not return error when namespaces match") + assert.NoError(t, compileErr, "Compile should not return error when namespaces match") + } + }) + } +}