Input validation / Authentication context integrity

Go PoC (repro harness): - Setup an authnserver with a mock client that would be invoked if a namespace is forwarded. - Call the Authenticate RPC with a nil request to simulate a missing namespace: // Pseudo-setup mirroring test harness in this repo svc := NewService(tracing.InitializeTracerForTest()) client := &mockClient{name: "should-not-run", testResult: true} svc.RegisterClient(client) resp, err := svc.Authenticate(context.Background(), nil) // Expected prior to fix (exploitable scenario): the server may proceed to dispatch to a client with an empty/missing namespace. // After the fix: err should be non-nil and equal to errExpectedNamespace, resp.Code == AUTHENTICATE_CODE_FAILED, and the downstream client must not be invoked. fmt.Printf("resp=%v, err=%v\n", resp, err) The repository includes corresponding tests (nil request returns FAILED with errExpectedNamespace) that demonstrate this behavior. The PoC above follows that repro and shows the exploit surface prior to the fix and the hardened behavior after the fix.

diff --git a/pkg/services/authn/authnserver/service.go b/pkg/services/authn/authnserver/service.go index 437a29edfa6ad..46b597413920f 100644 --- a/pkg/services/authn/authnserver/service.go +++ b/pkg/services/authn/authnserver/service.go @@ -2,11 +2,13 @@ package authnserver import ( "context" + "errors" "slices" "strings" grpclog "github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/logging" "go.opentelemetry.io/otel/attribute" + "k8s.io/apiserver/pkg/endpoints/request" authnv1 "github.com/grafana/authlib/authn/proto/v1" @@ -14,6 +16,8 @@ import ( "github.com/grafana/grafana/pkg/infra/tracing" ) +var errExpectedNamespace = errors.New("expected namespace") + // Client is the interface that MT auth clients implement. // This is the MT equivalent of authn.ContextAwareClient, but operating // on proto types instead of authn.Request/Identity. @@ -54,6 +58,16 @@ func (s *Service) Authenticate(ctx context.Context, req *authnv1.AuthenticateReq ctx, span := s.tracer.Start(ctx, "authnserver.Authenticate") defer span.End() + if req == nil || req.Namespace == "" { + s.log.Error("Authenticate request error", "error", errExpectedNamespace) + return &authnv1.AuthenticateResponse{ + Code: authnv1.AuthenticateCode_AUTHENTICATE_CODE_FAILED, + }, errExpectedNamespace + } + + ctx = request.WithNamespace(ctx, req.Namespace) + span.SetAttributes(attribute.String("authn.namespace", req.Namespace)) + grpclog.AddFields(ctx, grpclog.Fields{"authn.headers", headerNames(req.GetHttpHeaders())}) for _, c := range s.clients { diff --git a/pkg/services/authn/authnserver/service_test.go b/pkg/services/authn/authnserver/service_test.go index 40602b120d52d..45561aef50952 100644 --- a/pkg/services/authn/authnserver/service_test.go +++ b/pkg/services/authn/authnserver/service_test.go @@ -2,12 +2,14 @@ package authnserver import ( "context" + "errors" "fmt" "testing" grpclog "github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/logging" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "k8s.io/apiserver/pkg/endpoints/request" authnv1 "github.com/grafana/authlib/authn/proto/v1" @@ -19,15 +21,20 @@ type mockClient struct { testResult bool authResponse *authnv1.AuthenticateResponse authError error + + gotTestCtx context.Context + gotAuthCtx context.Context } func (m *mockClient) Name() string { return m.name } -func (m *mockClient) Test(_ context.Context, _ *authnv1.AuthenticateRequest) bool { +func (m *mockClient) Test(ctx context.Context, _ *authnv1.AuthenticateRequest) bool { + m.gotTestCtx = ctx return m.testResult } -func (m *mockClient) Authenticate(_ context.Context, _ *authnv1.AuthenticateRequest) (*authnv1.AuthenticateResponse, error) { +func (m *mockClient) Authenticate(ctx context.Context, _ *authnv1.AuthenticateRequest) (*authnv1.AuthenticateResponse, error) { + m.gotAuthCtx = ctx return m.authResponse, m.authError } @@ -164,6 +171,64 @@ func TestAuthenticate(t *testing.T) { assert.Contains(t, err.Error(), "internal failure") }) + t.Run("nil request returns FAILED with errExpectedNamespace", func(t *testing.T) { + svc := NewService(tracing.InitializeTracerForTest()) + client := &mockClient{name: "should-not-run", testResult: true} + svc.RegisterClient(client) + + resp, err := svc.Authenticate(context.Background(), nil) + require.Error(t, err) + assert.True(t, errors.Is(err, errExpectedNamespace)) + require.NotNil(t, resp) + assert.Equal(t, authnv1.AuthenticateCode_AUTHENTICATE_CODE_FAILED, resp.Code) + assert.Nil(t, client.gotTestCtx, "clients must not be dispatched when namespace is missing") + assert.Nil(t, client.gotAuthCtx) + }) + + t.Run("empty namespace returns FAILED with errExpectedNamespace", func(t *testing.T) { + svc := NewService(tracing.InitializeTracerForTest()) + client := &mockClient{name: "should-not-run", testResult: true} + svc.RegisterClient(client) + + emptyNS := &authnv1.AuthenticateRequest{ + Namespace: "", + HttpHeaders: map[string]string{"X-Access-Token": "some-token"}, + } + resp, err := svc.Authenticate(context.Background(), emptyNS) + require.Error(t, err) + assert.True(t, errors.Is(err, errExpectedNamespace)) + require.NotNil(t, resp) + assert.Equal(t, authnv1.AuthenticateCode_AUTHENTICATE_CODE_FAILED, resp.Code) + assert.Nil(t, client.gotTestCtx, "clients must not be dispatched when namespace is empty") + assert.Nil(t, client.gotAuthCtx) + }) + + t.Run("namespace from request is propagated into client context", func(t *testing.T) { + svc := NewService(tracing.InitializeTracerForTest()) + client := &mockClient{ + name: "ns-capture", + testResult: true, + authResponse: &authnv1.AuthenticateResponse{ + Code: authnv1.AuthenticateCode_AUTHENTICATE_CODE_OK, + Token: "ok", + }, + } + svc.RegisterClient(client) + + _, err := svc.Authenticate(context.Background(), req) + require.NoError(t, err) + + require.NotNil(t, client.gotTestCtx) + gotTestNS, ok := request.NamespaceFrom(client.gotTestCtx) + require.True(t, ok, "namespace must be set on Test ctx") + assert.Equal(t, "stacks-1234", gotTestNS) + + require.NotNil(t, client.gotAuthCtx) + gotAuthNS, ok := request.NamespaceFrom(client.gotAuthCtx) + require.True(t, ok, "namespace must be set on Authenticate ctx") + assert.Equal(t, "stacks-1234", gotAuthNS) + }) + t.Run("all clients decline via NOT_HANDLED returns NOT_HANDLED", func(t *testing.T) { svc := NewService(tracing.InitializeTracerForTest()) svc.RegisterClient(&mockClient{