diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index 0e5741b3f88..f09745bdf33 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -74,6 +74,13 @@ const ( // Requires AuthorizeWithSelectors to be enabled. AuthorizeNodeWithSelectors featuregate.Feature = "AuthorizeNodeWithSelectors" + // owner: @seans3 + // kep: http://kep.k8s.io/4006 + // + // Forces authorization of the "create" verb for pod subresources like exec, attach, and portforward. + // See: https://github.com/kubernetes/kubernetes/issues/133515 + AuthorizePodWebsocketUpgradeCreatePermission featuregate.Feature = "AuthorizePodWebsocketUpgradeCreatePermission" + // owner: @szuecs // // Enable nodes to change CPUCFSQuotaPeriod @@ -1078,6 +1085,10 @@ var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate {Version: version.MustParse("1.34"), Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.37 }, + AuthorizePodWebsocketUpgradeCreatePermission: { + {Version: version.MustParse("1.35"), Default: true, PreRelease: featuregate.Beta}, + }, + CPUCFSQuotaPeriod: { {Version: version.MustParse("1.12"), Default: false, PreRelease: featuregate.Alpha}, }, @@ -2016,6 +2027,8 @@ var defaultKubernetesFeatureGateDependencies = map[featuregate.Feature][]feature AuthorizeNodeWithSelectors: {genericfeatures.AuthorizeWithSelectors}, + AuthorizePodWebsocketUpgradeCreatePermission: {}, + CPUCFSQuotaPeriod: {}, CPUManagerPolicyAlphaOptions: {}, diff --git a/pkg/registry/core/pod/rest/authorize.go b/pkg/registry/core/pod/rest/authorize.go index fac7886f480..d5b825b482a 100644 --- a/pkg/registry/core/pod/rest/authorize.go +++ b/pkg/registry/core/pod/rest/authorize.go @@ -28,6 +28,13 @@ import ( genericapirequest "k8s.io/apiserver/pkg/endpoints/request" ) +// Pod subresources differs on the REST verbs depending on the protocol used +// SPDY uses POST that at the authz layer is translated to "create". +// Websockets uses GET that is translated to "get". +// Since the defaulting to websocket for kubectl in KEP-4006 this caused an +// unexpected side effect and in order to keep existing policies backwards +// compatible we always check that the "create" verb is allowed. +// Ref: https://issues.k8s.io/133515 func ensureAuthorizedForVerb(ctx context.Context, a authorizer.Authorizer, verb string) error { requestInfo, ok := genericapirequest.RequestInfoFrom(ctx) if !ok { diff --git a/pkg/registry/core/pod/rest/authorize_test.go b/pkg/registry/core/pod/rest/authorize_test.go new file mode 100644 index 00000000000..4c5962bde82 --- /dev/null +++ b/pkg/registry/core/pod/rest/authorize_test.go @@ -0,0 +1,118 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package rest + +import ( + "context" + "errors" + "reflect" + "testing" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apiserver/pkg/authorization/authorizer" + genericapirequest "k8s.io/apiserver/pkg/endpoints/request" +) + +// mockAuthorizer provides a mock implementation of the authorizer.Interface. +type mockAuthorizer struct { + decision authorizer.Decision + reason string + err error +} + +func (a *mockAuthorizer) Authorize(ctx context.Context, attrs authorizer.Attributes) (authorized authorizer.Decision, reason string, err error) { + return a.decision, a.reason, a.err +} + +func TestEnsureAuthorizedForVerb(t *testing.T) { + tests := []struct { + name string + ctx context.Context + authorizer authorizer.Authorizer + verb string + expectErr string + expectErrType interface{} + }{ + { + name: "no request info in context", + ctx: context.Background(), + verb: "create", + expectErr: `Internal error occurred: no request info in context`, + expectErrType: &apierrors.StatusError{}, + }, + { + name: "verb already matches", + ctx: genericapirequest.WithRequestInfo(context.Background(), &genericapirequest.RequestInfo{ + Verb: "create", + }), + verb: "create", + }, + { + name: "nil authorizer", + ctx: genericapirequest.WithRequestInfo(context.Background(), &genericapirequest.RequestInfo{ + Verb: "get", + }), + authorizer: nil, + verb: "create", + expectErr: `Internal error occurred: no authorizer available`, + expectErrType: &apierrors.StatusError{}, + }, + { + name: "authorizer returns error", + ctx: genericapirequest.WithRequestInfo(context.Background(), &genericapirequest.RequestInfo{Verb: "get"}), + authorizer: &mockAuthorizer{err: errors.New("auth error")}, + verb: "create", + expectErr: "auth error", + }, + { + name: "authorizer denies", + ctx: genericapirequest.WithRequestInfo(context.Background(), &genericapirequest.RequestInfo{Verb: "get", Resource: "pods", Name: "my-pod"}), + authorizer: &mockAuthorizer{decision: authorizer.DecisionDeny, reason: "no reason"}, + verb: "create", + expectErr: `pods "my-pod" is forbidden: no reason`, + expectErrType: &apierrors.StatusError{}, + }, + { + name: "authorizer allows", + ctx: genericapirequest.WithRequestInfo(context.Background(), &genericapirequest.RequestInfo{Verb: "get"}), + authorizer: &mockAuthorizer{decision: authorizer.DecisionAllow}, + verb: "create", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := ensureAuthorizedForVerb(tc.ctx, tc.authorizer, tc.verb) + + if len(tc.expectErr) > 0 { + if err == nil { + t.Fatalf("expected error %q, got nil", tc.expectErr) + } + if err.Error() != tc.expectErr { + t.Errorf("expected error %q, got %q", tc.expectErr, err.Error()) + } + if tc.expectErrType != nil { + if reflect.TypeOf(err) != reflect.TypeOf(tc.expectErrType) { + t.Errorf("expected error type %T, got %T", tc.expectErrType, err) + } + } + } else if err != nil { + t.Fatalf("expected no error, got %v", err) + } + }) + } +} diff --git a/pkg/registry/core/pod/rest/subresources.go b/pkg/registry/core/pod/rest/subresources.go index 6ac635cc540..e6ffa3408ff 100644 --- a/pkg/registry/core/pod/rest/subresources.go +++ b/pkg/registry/core/pod/rest/subresources.go @@ -111,8 +111,12 @@ func (r *AttachREST) Destroy() { // Connect returns a handler for the pod exec proxy func (r *AttachREST) Connect(ctx context.Context, name string, opts runtime.Object, responder rest.Responder) (http.Handler, error) { - if err := ensureAuthorizedForVerb(ctx, r.Authorizer, "create"); err != nil { - return nil, err + // Forces a authz check for "create", if feature gate enabled. + // See: https://github.com/kubernetes/kubernetes/issues/133515 + if utilfeature.DefaultFeatureGate.Enabled(features.AuthorizePodWebsocketUpgradeCreatePermission) { + if err := ensureAuthorizedForVerb(ctx, r.Authorizer, "create"); err != nil { + return nil, err + } } attachOpts, ok := opts.(*api.PodAttachOptions) @@ -173,8 +177,12 @@ func (r *ExecREST) Destroy() { // Connect returns a handler for the pod exec proxy func (r *ExecREST) Connect(ctx context.Context, name string, opts runtime.Object, responder rest.Responder) (http.Handler, error) { - if err := ensureAuthorizedForVerb(ctx, r.Authorizer, "create"); err != nil { - return nil, err + // Forces a authz check for "create", if feature gate enabled. + // See: https://github.com/kubernetes/kubernetes/issues/133515 + if utilfeature.DefaultFeatureGate.Enabled(features.AuthorizePodWebsocketUpgradeCreatePermission) { + if err := ensureAuthorizedForVerb(ctx, r.Authorizer, "create"); err != nil { + return nil, err + } } execOpts, ok := opts.(*api.PodExecOptions) @@ -246,8 +254,12 @@ func (r *PortForwardREST) ConnectMethods() []string { // Connect returns a handler for the pod portforward proxy func (r *PortForwardREST) Connect(ctx context.Context, name string, opts runtime.Object, responder rest.Responder) (http.Handler, error) { - if err := ensureAuthorizedForVerb(ctx, r.Authorizer, "create"); err != nil { - return nil, err + // Forces a authz check for "create", if feature gate enabled. + // See: https://github.com/kubernetes/kubernetes/issues/133515 + if utilfeature.DefaultFeatureGate.Enabled(features.AuthorizePodWebsocketUpgradeCreatePermission) { + if err := ensureAuthorizedForVerb(ctx, r.Authorizer, "create"); err != nil { + return nil, err + } } portForwardOpts, ok := opts.(*api.PodPortForwardOptions) diff --git a/test/compatibility_lifecycle/reference/versioned_feature_list.yaml b/test/compatibility_lifecycle/reference/versioned_feature_list.yaml index b5e765af876..b67b37b5dcc 100644 --- a/test/compatibility_lifecycle/reference/versioned_feature_list.yaml +++ b/test/compatibility_lifecycle/reference/versioned_feature_list.yaml @@ -153,6 +153,12 @@ lockToDefault: true preRelease: GA version: "1.34" +- name: AuthorizePodWebsocketUpgradeCreatePermission + versionedSpecs: + - default: true + lockToDefault: false + preRelease: Beta + version: "1.35" - name: AuthorizeWithSelectors versionedSpecs: - default: false diff --git a/test/integration/apiserver/subresource_auth_test.go b/test/integration/apiserver/subresource_auth_test.go new file mode 100644 index 00000000000..285343bfb6f --- /dev/null +++ b/test/integration/apiserver/subresource_auth_test.go @@ -0,0 +1,177 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package apiserver + +import ( + "context" + "fmt" + "testing" + + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/rest" + "k8s.io/kubernetes/cmd/kube-apiserver/app/options" + "k8s.io/kubernetes/test/integration/framework" + "k8s.io/kubernetes/test/utils/ktesting" +) + +// TestPodSubresourceAuth tests that the synthetic authorization check for pod subresources is working correctly. +func TestPodSubresourceAuth(t *testing.T) { + tCtx := ktesting.Init(t) + _, clientConfig, tearDownFn := framework.StartTestServer(tCtx, t, framework.TestServerSetup{ + ModifyServerRunOptions: func(opts *options.ServerRunOptions) { + opts.Authorization.Modes = []string{"RBAC"} + }, + }) + defer tearDownFn() + + adminConfig := rest.CopyConfig(clientConfig) + adminClientset, err := kubernetes.NewForConfig(adminConfig) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + ns := "test-pod-subresource-auth" + if _, err := adminClientset.CoreV1().Namespaces().Create(context.TODO(), &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ns}}, metav1.CreateOptions{}); err != nil { + t.Fatal(err) + } + + sa := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: "default"}} + if _, err := adminClientset.CoreV1().ServiceAccounts(ns).Create(context.TODO(), sa, metav1.CreateOptions{}); err != nil { + t.Fatal(err) + } + + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "test-pod"}, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "test-container", + Image: "test-image", + }, + }, + }, + } + if _, err := adminClientset.CoreV1().Pods(ns).Create(context.TODO(), pod, metav1.CreateOptions{}); err != nil { + t.Fatal(err) + } + + // User with only 'get' permissions + podGetterUsername := "pod-getter" + podGetterRole := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{Name: "pod-getter-role"}, + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{""}, + Resources: []string{"pods/exec", "pods/attach", "pods/portforward"}, + Verbs: []string{"get"}, + }, + }, + } + if _, err := adminClientset.RbacV1().Roles(ns).Create(context.TODO(), podGetterRole, metav1.CreateOptions{}); err != nil { + t.Fatal(err) + } + podGetterRoleBinding := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "pod-getter-binding"}, + Subjects: []rbacv1.Subject{{Kind: "User", Name: podGetterUsername}}, + RoleRef: rbacv1.RoleRef{Kind: "Role", Name: "pod-getter-role"}, + } + if _, err := adminClientset.RbacV1().RoleBindings(ns).Create(context.TODO(), podGetterRoleBinding, metav1.CreateOptions{}); err != nil { + t.Fatal(err) + } + + podGetterConfig := rest.CopyConfig(clientConfig) + podGetterConfig.Impersonate = rest.ImpersonationConfig{UserName: podGetterUsername} + podGetterClient, err := kubernetes.NewForConfig(podGetterConfig) + if err != nil { + t.Fatal(err) + } + + // User with 'get' and 'create' permissions on pods subresources. + podCreatorUsername := "pod-creator" + podCreatorRole := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{Name: "pod-creator-role"}, + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{""}, + Resources: []string{"pods/exec", "pods/attach", "pods/portforward"}, + Verbs: []string{"get"}, + }, + { + APIGroups: []string{""}, + Resources: []string{"pods/exec", "pods/attach", "pods/portforward"}, + Verbs: []string{"create"}, + }, + }, + } + if _, err := adminClientset.RbacV1().Roles(ns).Create(context.TODO(), podCreatorRole, metav1.CreateOptions{}); err != nil { + t.Fatal(err) + } + podCreatorRoleBinding := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "pod-creator-binding"}, + Subjects: []rbacv1.Subject{{Kind: "User", Name: podCreatorUsername}}, + RoleRef: rbacv1.RoleRef{Kind: "Role", Name: "pod-creator-role"}, + } + if _, err := adminClientset.RbacV1().RoleBindings(ns).Create(context.TODO(), podCreatorRoleBinding, metav1.CreateOptions{}); err != nil { + t.Fatal(err) + } + + podCreatorConfig := rest.CopyConfig(clientConfig) + podCreatorConfig.Impersonate = rest.ImpersonationConfig{UserName: podCreatorUsername} + podCreatorClient, err := kubernetes.NewForConfig(podCreatorConfig) + if err != nil { + t.Fatal(err) + } + + subresources := []string{"exec", "attach", "portforward"} + for _, subresource := range subresources { + t.Run(fmt.Sprintf("subresource=%s", subresource), func(t *testing.T) { + // User with only 'get' permissions should be denied. + // GET method, since that is the method for WebSocket upgrade requests. + err := podGetterClient.CoreV1().RESTClient().Get(). + Namespace(ns). + Resource("pods"). + Name("test-pod"). + SubResource(subresource). + Do(context.TODO()). + Error() + if !errors.IsForbidden(err) { + t.Errorf("expected forbidden error for user with only 'get' permissions, but got: %v", err) + } + + // User with 'get' and 'create' permissions should be allowed. + // GET method, since that is the method for WebSocket upgrade requests. + err = podCreatorClient.CoreV1().RESTClient().Get(). + Namespace(ns). + Resource("pods"). + Name("test-pod"). + SubResource(subresource). + Do(context.TODO()). + Error() + // Absence of "Forbidden" is proof of success; the integration test + // server doesn't have a real Kubelet running for the pod, so the + // proxying/streaming connection ultimately fails after the authorization + // has already succeeded (hence "Bad Request" error). + if err != nil && !errors.IsBadRequest(err) { + t.Errorf("expected nil error for user with 'create' permissions, but got: %v", err) + } + }) + } +}