From d76eae46fc75cca156d32540d9d5c4be092e300d Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Fri, 2 Aug 2024 17:04:51 -0400 Subject: [PATCH 1/3] SSA: add integration test to exercise authz Signed-off-by: Monis Khan --- .../integration/apiserver/apply/apply_test.go | 121 +++++++++++++++++- 1 file changed, 120 insertions(+), 1 deletion(-) diff --git a/test/integration/apiserver/apply/apply_test.go b/test/integration/apiserver/apply/apply_test.go index 8fa1eb92f72..9ac0a4b1f34 100644 --- a/test/integration/apiserver/apply/apply_test.go +++ b/test/integration/apiserver/apply/apply_test.go @@ -30,14 +30,15 @@ import ( "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/require" - "sigs.k8s.io/yaml" appsv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/wait" @@ -49,7 +50,9 @@ import ( "k8s.io/client-go/kubernetes/fake" restclient "k8s.io/client-go/rest" kubeapiservertesting "k8s.io/kubernetes/cmd/kube-apiserver/app/testing" + "k8s.io/kubernetes/test/integration/authutil" "k8s.io/kubernetes/test/integration/framework" + "sigs.k8s.io/yaml" ) func setup(t testing.TB) (clientset.Interface, kubeapiservertesting.TearDownFunc) { @@ -4780,3 +4783,119 @@ func expectManagedFields(t *testing.T, managedFields []metav1.ManagedFieldsEntry t.Fatalf("Want:\n%s\nGot:\n%s\nDiff:\n%s", string(want), string(got), diff) } } + +// TestCreateOnApplyFailsWithForbidden makes sure that PATCH requests with the apply content type +// will not create the object if the user does not have both patch and create permissions. +func TestCreateOnApplyFailsWithForbidden(t *testing.T) { + // Enable RBAC so we can exercise authorization errors. + server := kubeapiservertesting.StartTestServerOrDie(t, nil, append([]string{"--authorization-mode=RBAC"}, framework.DefaultTestServerFlags()...), framework.SharedEtcd()) + t.Cleanup(server.TearDownFn) + + adminClient := clientset.NewForConfigOrDie(server.ClientConfig) + + pandaConfig := restclient.CopyConfig(server.ClientConfig) + pandaConfig.Impersonate.UserName = "panda" + pandaClient := clientset.NewForConfigOrDie(pandaConfig) + + errPatch := ssaPod(pandaClient) + + requireForbiddenPodErr(t, errPatch, `pods "test-pod" is forbidden: User "panda" cannot patch resource "pods" in API group "" in the namespace "default"`) + + createPodRBACAndWait(t, adminClient, "patch") + + errCreate := ssaPod(pandaClient) + + requireForbiddenPodErr(t, errCreate, `pods "test-pod" is forbidden: `) // TODO make this error better + + createPodRBACAndWait(t, adminClient, "create") + + errNone := ssaPod(pandaClient) + require.NoError(t, errNone, "pod create via SSA should succeed now that RBAC is correct") +} + +func requireForbiddenPodErr(t *testing.T, err error, message string) { + t.Helper() + + require.Truef(t, apierrors.IsForbidden(err), "Expected forbidden error but got: %v", err) + + wantStatusErr := &apierrors.StatusError{ErrStatus: metav1.Status{ + Status: "Failure", + Message: message, + Reason: "Forbidden", + Details: &metav1.StatusDetails{ + Name: "test-pod", + Kind: "pods", + }, + Code: http.StatusForbidden, + }} + require.Equal(t, wantStatusErr, err, "unexpected status error") +} + +func ssaPod(client *clientset.Clientset) error { + _, err := client.CoreV1().RESTClient().Patch(types.ApplyPatchType). + Namespace("default"). + Resource("pods"). + Name("test-pod"). + Param("fieldManager", "apply_test"). + Body([]byte(`{ + "apiVersion": "v1", + "kind": "Pod", + "metadata": { + "name": "test-pod" + }, + "spec": { + "containers": [{ + "name": "test-container", + "image": "test-image" + }] + } + }`)). + Do(context.TODO()). + Get() + return err +} + +func createPodRBACAndWait(t *testing.T, client *clientset.Clientset, verb string) { + t.Helper() + + _, err := client.RbacV1().ClusterRoles().Create(context.TODO(), &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("can-%s-pods", verb), + }, + Rules: []rbacv1.PolicyRule{ + { + Verbs: []string{verb}, + APIGroups: []string{""}, + Resources: []string{"pods"}, + }, + }, + }, metav1.CreateOptions{}) + require.NoError(t, err) + + _, err = client.RbacV1().RoleBindings("default").Create(context.TODO(), &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("can-%s-pods", verb), + }, + Subjects: []rbacv1.Subject{ + { + Kind: rbacv1.UserKind, + Name: "panda", + }, + }, + RoleRef: rbacv1.RoleRef{ + APIGroup: rbacv1.GroupName, + Kind: "ClusterRole", + Name: fmt.Sprintf("can-%s-pods", verb), + }, + }, metav1.CreateOptions{}) + require.NoError(t, err) + + authutil.WaitForNamedAuthorizationUpdate(t, context.TODO(), client.AuthorizationV1(), + "panda", + "default", + verb, + "", + schema.GroupResource{Resource: "pods"}, + true, + ) +} From 857127f7c44a029f6f8dd44b0b40364aa00aa13d Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Fri, 2 Aug 2024 17:20:53 -0400 Subject: [PATCH 2/3] SSA: improve create authz error message Signed-off-by: Monis Khan --- .../pkg/endpoints/handlers/responsewriters/errors.go | 12 +++++++++--- .../apiserver/pkg/endpoints/handlers/update.go | 9 ++------- test/integration/apiserver/apply/apply_test.go | 2 +- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors.go index d13bee4d223..6c0babfa9f3 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors.go @@ -34,8 +34,13 @@ var sanitizer = strings.NewReplacer(`&`, "&", `<`, "<", `>`, ">") // Forbidden renders a simple forbidden error func Forbidden(ctx context.Context, attributes authorizer.Attributes, w http.ResponseWriter, req *http.Request, reason string, s runtime.NegotiatedSerializer) { - msg := sanitizer.Replace(forbiddenMessage(attributes)) w.Header().Set("X-Content-Type-Options", "nosniff") + gv := schema.GroupVersion{Group: attributes.GetAPIGroup(), Version: attributes.GetAPIVersion()} + ErrorNegotiated(ForbiddenStatusError(attributes, reason), s, gv, w, req) +} + +func ForbiddenStatusError(attributes authorizer.Attributes, reason string) *apierrors.StatusError { + msg := sanitizer.Replace(forbiddenMessage(attributes)) var errMsg string if len(reason) == 0 { @@ -43,9 +48,10 @@ func Forbidden(ctx context.Context, attributes authorizer.Attributes, w http.Res } else { errMsg = fmt.Sprintf("%s: %s", msg, reason) } - gv := schema.GroupVersion{Group: attributes.GetAPIGroup(), Version: attributes.GetAPIVersion()} + gr := schema.GroupResource{Group: attributes.GetAPIGroup(), Resource: attributes.GetResource()} - ErrorNegotiated(apierrors.NewForbidden(gr, attributes.GetName(), fmt.Errorf(errMsg)), s, gv, w, req) + + return apierrors.NewForbidden(gr, attributes.GetName(), fmt.Errorf(errMsg)) } func forbiddenMessage(attributes authorizer.Attributes) string { diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go index 4b76ef97e07..ead2b94de60 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go @@ -39,6 +39,7 @@ import ( "k8s.io/apiserver/pkg/endpoints/handlers/finisher" requestmetrics "k8s.io/apiserver/pkg/endpoints/handlers/metrics" "k8s.io/apiserver/pkg/endpoints/handlers/negotiation" + "k8s.io/apiserver/pkg/endpoints/handlers/responsewriters" "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/rest" "k8s.io/apiserver/pkg/util/dryrun" @@ -275,13 +276,7 @@ func withAuthorization(validate rest.ValidateObjectFunc, a authorizer.Authorizer } // The user is not authorized to perform this action, so we need to build the error response - gr := schema.GroupResource{ - Group: attributes.GetAPIGroup(), - Resource: attributes.GetResource(), - } - name := attributes.GetName() - err := fmt.Errorf("%v", authorizerReason) - return errors.NewForbidden(gr, name, err) + return responsewriters.ForbiddenStatusError(attributes, authorizerReason) } } diff --git a/test/integration/apiserver/apply/apply_test.go b/test/integration/apiserver/apply/apply_test.go index 9ac0a4b1f34..b8e507b5f60 100644 --- a/test/integration/apiserver/apply/apply_test.go +++ b/test/integration/apiserver/apply/apply_test.go @@ -4805,7 +4805,7 @@ func TestCreateOnApplyFailsWithForbidden(t *testing.T) { errCreate := ssaPod(pandaClient) - requireForbiddenPodErr(t, errCreate, `pods "test-pod" is forbidden: `) // TODO make this error better + requireForbiddenPodErr(t, errCreate, `pods "test-pod" is forbidden: User "panda" cannot create resource "pods" in API group "" in the namespace "default"`) createPodRBACAndWait(t, adminClient, "create") From bff6ce4a38077c29cdf2e1ac2fce1a551082ebfe Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Mon, 5 Aug 2024 10:50:51 -0400 Subject: [PATCH 3/3] ForbiddenStatusError: make linter happy on error construction Signed-off-by: Monis Khan --- .../pkg/endpoints/handlers/responsewriters/errors.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors.go index 6c0babfa9f3..07316e802a9 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors.go @@ -42,16 +42,16 @@ func Forbidden(ctx context.Context, attributes authorizer.Attributes, w http.Res func ForbiddenStatusError(attributes authorizer.Attributes, reason string) *apierrors.StatusError { msg := sanitizer.Replace(forbiddenMessage(attributes)) - var errMsg string + var errMsg error if len(reason) == 0 { - errMsg = fmt.Sprintf("%s", msg) + errMsg = fmt.Errorf("%s", msg) } else { - errMsg = fmt.Sprintf("%s: %s", msg, reason) + errMsg = fmt.Errorf("%s: %s", msg, reason) } gr := schema.GroupResource{Group: attributes.GetAPIGroup(), Resource: attributes.GetResource()} - return apierrors.NewForbidden(gr, attributes.GetName(), fmt.Errorf(errMsg)) + return apierrors.NewForbidden(gr, attributes.GetName(), errMsg) } func forbiddenMessage(attributes authorizer.Attributes) string {