From 2b05ab252dcd417e71b0182f7e6563a87e1b1655 Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Wed, 6 May 2026 11:20:41 +0200 Subject: [PATCH] feat: use resource-based RBAC for lifecycle-server auth Replace the controller-runtime metrics auth filter with a custom resource-based authorization middleware. The metrics filter performed nonResourceURL-based SubjectAccessReviews, which meant the default system:discovery ClusterRole (granting GET on /api/*) allowed any authenticated user to access the lifecycle-server API. The new middleware creates SubjectAccessReviews with ResourceAttributes (apiGroup: lifecycle.olm.openshift.io, resource: lifecycles, verb: get) so access requires an explicit ClusterRoleBinding to the new lifecycle-server-consumer ClusterRole. Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Per G. da Silva --- cmd/lifecycle-server/start.go | 11 +- go.mod | 2 +- pkg/lifecycle-server/auth.go | 107 +++++++++++++++++ pkg/lifecycle-server/auth_test.go | 191 ++++++++++++++++++++++++++++++ 4 files changed, 302 insertions(+), 9 deletions(-) create mode 100644 pkg/lifecycle-server/auth.go create mode 100644 pkg/lifecycle-server/auth_test.go diff --git a/cmd/lifecycle-server/start.go b/cmd/lifecycle-server/start.go index d9d31010b3..ced3f97467 100644 --- a/cmd/lifecycle-server/start.go +++ b/cmd/lifecycle-server/start.go @@ -13,7 +13,6 @@ import ( "golang.org/x/sync/errgroup" "k8s.io/client-go/rest" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/metrics/filters" "k8s.io/klog/v2" @@ -126,9 +125,9 @@ func run(_ *cobra.Command, _ []string) error { return err } - authnzFilter, err := filters.WithAuthenticationAndAuthorization(restCfg, httpClient) + authFilter, err := server.NewAuthFilter(restCfg, httpClient, log) if err != nil { - log.Error(err, "failed to create authorization filter") + log.Error(err, "failed to create auth filter") return err } @@ -147,11 +146,7 @@ func run(_ *cobra.Command, _ []string) error { // Create HTTP apiHandler with authn/authz middleware baseHandler := server.NewHandler(data, log) - apiHandler, err := authnzFilter(log, baseHandler) - if err != nil { - log.Error(err, "failed to create api handler") - return err - } + apiHandler := authFilter(baseHandler) // Create health handler (no auth required) healthHandler := server.NewHealthHandler(loadErr == nil) diff --git a/go.mod b/go.mod index e6f878c5f4..57dfc0ba67 100644 --- a/go.mod +++ b/go.mod @@ -26,6 +26,7 @@ require ( gopkg.in/yaml.v2 v2.4.0 k8s.io/api v0.35.4 k8s.io/apimachinery v0.35.4 + k8s.io/apiserver v0.35.4 k8s.io/client-go v0.35.4 k8s.io/code-generator v0.35.4 k8s.io/klog/v2 v2.140.0 @@ -228,7 +229,6 @@ require ( gopkg.in/warnings.v0 v0.1.2 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect k8s.io/apiextensions-apiserver v0.35.4 // indirect - k8s.io/apiserver v0.35.4 // indirect k8s.io/cli-runtime v0.35.0 // indirect k8s.io/component-base v0.35.4 // indirect k8s.io/gengo/v2 v2.0.0-20250922181213-ec3ebc5fd46b // indirect diff --git a/pkg/lifecycle-server/auth.go b/pkg/lifecycle-server/auth.go new file mode 100644 index 0000000000..4f48f7e7ed --- /dev/null +++ b/pkg/lifecycle-server/auth.go @@ -0,0 +1,107 @@ +package server + +import ( + "fmt" + "net/http" + "time" + + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/apiserver/pkg/apis/apiserver" + "k8s.io/apiserver/pkg/authentication/authenticator" + "k8s.io/apiserver/pkg/authentication/authenticatorfactory" + "k8s.io/apiserver/pkg/authorization/authorizer" + "k8s.io/apiserver/pkg/authorization/authorizerfactory" + authenticationv1 "k8s.io/client-go/kubernetes/typed/authentication/v1" + authorizationv1 "k8s.io/client-go/kubernetes/typed/authorization/v1" + "k8s.io/client-go/rest" + + "github.com/go-logr/logr" +) + +const ( + apiGroup = "lifecycle.olm.openshift.io" + resource = "lifecycles" +) + +// NewAuthFilter creates HTTP middleware that authenticates requests via +// TokenReview and authorizes them via SubjectAccessReview using resource-based +// RBAC. Callers must have a ClusterRole granting "get" on the "lifecycles" +// resource in the "lifecycle.olm.openshift.io" API group. +func NewAuthFilter(cfg *rest.Config, httpClient *http.Client, log logr.Logger) (func(http.Handler) http.Handler, error) { + authenticationClient, err := authenticationv1.NewForConfigAndClient(cfg, httpClient) + if err != nil { + return nil, fmt.Errorf("failed to create authentication client: %w", err) + } + authorizationClient, err := authorizationv1.NewForConfigAndClient(cfg, httpClient) + if err != nil { + return nil, fmt.Errorf("failed to create authorization client: %w", err) + } + + retryBackoff := &wait.Backoff{ + Duration: 500 * time.Millisecond, + Factor: 1.5, + Jitter: 0.2, + Steps: 5, + } + + authn, _, err := authenticatorfactory.DelegatingAuthenticatorConfig{ + Anonymous: &apiserver.AnonymousAuthConfig{Enabled: false}, + CacheTTL: 1 * time.Minute, + TokenAccessReviewClient: authenticationClient, + TokenAccessReviewTimeout: 10 * time.Second, + WebhookRetryBackoff: retryBackoff, + }.New() + if err != nil { + return nil, fmt.Errorf("failed to create authenticator: %w", err) + } + + authz, err := authorizerfactory.DelegatingAuthorizerConfig{ + SubjectAccessReviewClient: authorizationClient, + AllowCacheTTL: 5 * time.Minute, + DenyCacheTTL: 30 * time.Second, + WebhookRetryBackoff: retryBackoff, + }.New() + if err != nil { + return nil, fmt.Errorf("failed to create authorizer: %w", err) + } + + return newAuthMiddleware(authn, authz, log), nil +} + +func newAuthMiddleware(authn authenticator.Request, authz authorizer.Authorizer, log logr.Logger) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + res, ok, err := authn.AuthenticateRequest(req) + if err != nil { + log.Error(err, "authentication failed") + http.Error(w, "Authentication failed", http.StatusInternalServerError) + return + } + if !ok { + log.V(4).Info("authentication failed") + http.Error(w, "Unauthorized", http.StatusUnauthorized) + return + } + + decision, reason, err := authz.Authorize(req.Context(), authorizer.AttributesRecord{ + User: res.User, + Verb: "get", + APIGroup: apiGroup, + Resource: resource, + ResourceRequest: true, + }) + if err != nil { + log.Error(err, "authorization failed", "user", res.User.GetName()) + http.Error(w, "Authorization failed", http.StatusInternalServerError) + return + } + if decision != authorizer.DecisionAllow { + log.V(4).Info("authorization denied", "user", res.User.GetName(), "reason", reason) + http.Error(w, "Forbidden", http.StatusForbidden) + return + } + + next.ServeHTTP(w, req) + }) + } +} diff --git a/pkg/lifecycle-server/auth_test.go b/pkg/lifecycle-server/auth_test.go new file mode 100644 index 0000000000..e8eb156d46 --- /dev/null +++ b/pkg/lifecycle-server/auth_test.go @@ -0,0 +1,191 @@ +package server + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/require" + "k8s.io/apiserver/pkg/authentication/authenticator" + "k8s.io/apiserver/pkg/authentication/user" + "k8s.io/apiserver/pkg/authorization/authorizer" +) + +type fakeAuthenticator struct { + resp *authenticator.Response + ok bool + err error +} + +func (f *fakeAuthenticator) AuthenticateRequest(_ *http.Request) (*authenticator.Response, bool, error) { + return f.resp, f.ok, f.err +} + +type fakeAuthorizer struct { + decision authorizer.Decision + reason string + err error + attrs authorizer.Attributes +} + +func (f *fakeAuthorizer) Authorize(_ context.Context, a authorizer.Attributes) (authorizer.Decision, string, error) { + f.attrs = a + return f.decision, f.reason, f.err +} + +func TestAuthMiddleware_Unauthenticated(t *testing.T) { + authn := &fakeAuthenticator{ok: false} + authz := &fakeAuthorizer{decision: authorizer.DecisionAllow} + middleware := newAuthMiddleware(authn, authz, logr.Discard()) + + var handlerCalled bool + handler := middleware(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + handlerCalled = true + })) + + req := httptest.NewRequest(http.MethodGet, "/api/v1alpha1/lifecycles/my-operator", nil) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + require.False(t, handlerCalled, "inner handler should not have been called") + require.Equal(t, http.StatusUnauthorized, rec.Code) +} + +func TestAuthMiddleware_AuthenticationError(t *testing.T) { + authn := &fakeAuthenticator{err: http.ErrAbortHandler} + authz := &fakeAuthorizer{decision: authorizer.DecisionAllow} + middleware := newAuthMiddleware(authn, authz, logr.Discard()) + + var handlerCalled bool + handler := middleware(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + handlerCalled = true + })) + + req := httptest.NewRequest(http.MethodGet, "/api/v1alpha1/lifecycles/my-operator", nil) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + require.False(t, handlerCalled, "inner handler should not have been called") + require.Equal(t, http.StatusInternalServerError, rec.Code) +} + +func TestAuthMiddleware_Forbidden(t *testing.T) { + authn := &fakeAuthenticator{ + resp: &authenticator.Response{User: &user.DefaultInfo{Name: "test-user"}}, + ok: true, + } + authz := &fakeAuthorizer{decision: authorizer.DecisionDeny, reason: "no binding"} + middleware := newAuthMiddleware(authn, authz, logr.Discard()) + + var handlerCalled bool + handler := middleware(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + handlerCalled = true + })) + + req := httptest.NewRequest(http.MethodGet, "/api/v1alpha1/lifecycles/my-operator", nil) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + require.False(t, handlerCalled, "inner handler should not have been called") + require.Equal(t, http.StatusForbidden, rec.Code) + require.NotContains(t, rec.Body.String(), "no binding", "response must not leak authorization reason") +} + +func TestAuthMiddleware_NoOpinion(t *testing.T) { + authn := &fakeAuthenticator{ + resp: &authenticator.Response{User: &user.DefaultInfo{Name: "test-user"}}, + ok: true, + } + authz := &fakeAuthorizer{decision: authorizer.DecisionNoOpinion, reason: "no RBAC policy matched"} + middleware := newAuthMiddleware(authn, authz, logr.Discard()) + + var handlerCalled bool + handler := middleware(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + handlerCalled = true + })) + + req := httptest.NewRequest(http.MethodGet, "/api/v1alpha1/lifecycles/my-operator", nil) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + require.False(t, handlerCalled, "inner handler should not have been called") + require.Equal(t, http.StatusForbidden, rec.Code) +} + +func TestAuthMiddleware_Allowed(t *testing.T) { + authn := &fakeAuthenticator{ + resp: &authenticator.Response{User: &user.DefaultInfo{Name: "test-user"}}, + ok: true, + } + authz := &fakeAuthorizer{decision: authorizer.DecisionAllow} + middleware := newAuthMiddleware(authn, authz, logr.Discard()) + + var handlerCalled bool + handler := middleware(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + handlerCalled = true + w.WriteHeader(http.StatusOK) + })) + + req := httptest.NewRequest(http.MethodGet, "/api/v1alpha1/lifecycles/my-operator", nil) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + require.True(t, handlerCalled, "inner handler should have been called") + require.Equal(t, http.StatusOK, rec.Code) + +} + +func TestAuthMiddleware_AttributesRecord(t *testing.T) { + authn := &fakeAuthenticator{ + resp: &authenticator.Response{User: &user.DefaultInfo{Name: "test-user", Groups: []string{"system:authenticated"}}}, + ok: true, + } + authz := &fakeAuthorizer{decision: authorizer.DecisionAllow} + middleware := newAuthMiddleware(authn, authz, logr.Discard()) + + var handlerCalled bool + handler := middleware(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + handlerCalled = true + })) + + req := httptest.NewRequest(http.MethodGet, "/api/v1alpha1/lifecycles/my-operator", nil) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + require.True(t, handlerCalled, "inner handler should have been called") + require.Equal(t, http.StatusOK, rec.Code) + require.True(t, authz.attrs.IsResourceRequest()) + require.Equal(t, "get", authz.attrs.GetVerb()) + require.Equal(t, apiGroup, authz.attrs.GetAPIGroup()) + require.Equal(t, resource, authz.attrs.GetResource()) + require.Equal(t, "test-user", authz.attrs.GetUser().GetName()) +} + +func TestAuthMiddleware_AuthorizationError(t *testing.T) { + authn := &fakeAuthenticator{ + resp: &authenticator.Response{User: &user.DefaultInfo{Name: "test-user"}}, + ok: true, + } + authz := &fakeAuthorizer{err: http.ErrAbortHandler} + middleware := newAuthMiddleware(authn, authz, logr.Discard()) + + var handlerCalled bool + handler := middleware(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + handlerCalled = true + })) + + req := httptest.NewRequest(http.MethodGet, "/api/v1alpha1/lifecycles/my-operator", nil) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + require.False(t, handlerCalled, "inner handler should not have been called") + require.Equal(t, http.StatusInternalServerError, rec.Code) +}