Merge pull request #38981 from liggitt/remove-rbac-user-all

Automatic merge from submit-queue (batch tested with PRs 39408, 38981)

Remove RBAC UserAll

* Removes special handling of User * subjects in rolebinding matching evaluation
* Converts v1alpha1 rolebindings to `User *` subjects to `Group system:authenticated` subjects for backwards compatibility

```release-note
RBAC's special handling of the User subject named "*" in RoleBinding and ClusterRoleBinding objects is being deprecated and will be removed in v1beta1. Existing v1alpha1 role bindings to User "*" will be converted to the group "system:authenticated". To match unauthenticated requests, RBAC role bindings must explicitly bind to the group "system:unauthenticated".
```

hack/.linted_packages

@@ -94,6 +94,7 @@ pkg/apis/imagepolicy/install
 pkg/apis/meta/v1/unstructured
 pkg/apis/policy/install
 pkg/apis/rbac/install
+pkg/apis/rbac/v1alpha1
 pkg/apis/storage/install
 pkg/apis/storage/validation
 pkg/auth/authenticator

pkg/apis/rbac/types.go

@@ -36,8 +36,6 @@ const (
 	GroupKind          = "Group"
 	ServiceAccountKind = "ServiceAccount"
 	UserKind           = "User"
-
-	UserAll = "*"
 )
 
 // PolicyRule holds information that describes a policy rule, but does not contain information

pkg/apis/rbac/v1alpha1/BUILD

@@ -5,11 +5,13 @@ licenses(["notice"])
 load(
     "@io_bazel_rules_go//go:def.bzl",
     "go_library",
+    "go_test",
 )
 
 go_library(
     name = "go_default_library",
     srcs = [
+        "conversion.go",
         "defaults.go",
         "doc.go",
         "generated.pb.go",
@@ -34,5 +36,18 @@ go_library(
         "//pkg/watch/versioned:go_default_library",
         "//vendor:github.com/gogo/protobuf/proto",
         "//vendor:github.com/ugorji/go/codec",
+        "//vendor:k8s.io/apiserver/pkg/authentication/user",
+    ],
+)
+
+go_test(
+    name = "go_default_xtest",
+    srcs = ["conversion_test.go"],
+    tags = ["automanaged"],
+    deps = [
+        "//pkg/api:go_default_library",
+        "//pkg/apis/rbac:go_default_library",
+        "//pkg/apis/rbac/install:go_default_library",
+        "//pkg/apis/rbac/v1alpha1:go_default_library",
     ],
 )

pkg/apis/rbac/v1alpha1/conversion.go

@@ -0,0 +1,39 @@
+/*
+Copyright 2017 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 v1alpha1
+
+import (
+	"k8s.io/apiserver/pkg/authentication/user"
+	api "k8s.io/kubernetes/pkg/apis/rbac"
+	"k8s.io/kubernetes/pkg/conversion"
+)
+
+func Convert_v1alpha1_Subject_To_rbac_Subject(in *Subject, out *api.Subject, s conversion.Scope) error {
+	if err := autoConvert_v1alpha1_Subject_To_rbac_Subject(in, out, s); err != nil {
+		return err
+	}
+
+	// User * in v1alpha1 will only match all authenticated users
+	// This is only for compatibility with old RBAC bindings
+	// Special treatment for * should not be included in v1beta1
+	if out.Kind == UserKind && out.Name == "*" {
+		out.Kind = GroupKind
+		out.Name = user.AllAuthenticated
+	}
+
+	return nil
+}

pkg/apis/rbac/v1alpha1/conversion_test.go

@@ -0,0 +1,64 @@
+/*
+Copyright 2017 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 v1alpha1_test
+
+import (
+	"reflect"
+	"testing"
+
+	"k8s.io/kubernetes/pkg/api"
+	rbacapi "k8s.io/kubernetes/pkg/apis/rbac"
+	_ "k8s.io/kubernetes/pkg/apis/rbac/install"
+	"k8s.io/kubernetes/pkg/apis/rbac/v1alpha1"
+)
+
+func TestConversion(t *testing.T) {
+	testcases := map[string]struct {
+		old      *v1alpha1.RoleBinding
+		expected *rbacapi.RoleBinding
+	}{
+		"specific user": {
+			old: &v1alpha1.RoleBinding{
+				RoleRef:  v1alpha1.RoleRef{Name: "foo", APIGroup: v1alpha1.GroupName},
+				Subjects: []v1alpha1.Subject{{Kind: "User", Name: "bob"}},
+			},
+			expected: &rbacapi.RoleBinding{
+				RoleRef:  rbacapi.RoleRef{Name: "foo", APIGroup: v1alpha1.GroupName},
+				Subjects: []rbacapi.Subject{{Kind: "User", Name: "bob"}},
+			},
+		},
+		"wildcard user matches authenticated": {
+			old: &v1alpha1.RoleBinding{
+				RoleRef:  v1alpha1.RoleRef{Name: "foo", APIGroup: v1alpha1.GroupName},
+				Subjects: []v1alpha1.Subject{{Kind: "User", Name: "*"}},
+			},
+			expected: &rbacapi.RoleBinding{
+				RoleRef:  rbacapi.RoleRef{Name: "foo", APIGroup: v1alpha1.GroupName},
+				Subjects: []rbacapi.Subject{{Kind: "Group", Name: "system:authenticated"}},
+			},
+		},
+	}
+	for k, tc := range testcases {
+		internal := &rbacapi.RoleBinding{}
+		if err := api.Scheme.Convert(tc.old, internal, nil); err != nil {
+			t.Errorf("%s: unexpected error: %v", k, err)
+		}
+		if !reflect.DeepEqual(internal, tc.expected) {
+			t.Errorf("%s: expected\n\t%#v, got \n\t%#v", k, tc.expected, internal)
+		}
+	}
+}

pkg/apis/rbac/v1alpha1/types.go

@@ -36,8 +36,6 @@ const (
 	GroupKind          = "Group"
 	ServiceAccountKind = "ServiceAccount"
 	UserKind           = "User"
-
-	UserAll = "*"
 )
 
 // Authorization is calculated against

pkg/apis/rbac/v1alpha1/zz_generated.conversion.go

@@ -115,7 +115,17 @@ func autoConvert_v1alpha1_ClusterRoleBinding_To_rbac_ClusterRoleBinding(in *Clus
 	if err := s.Convert(&in.ObjectMeta, &out.ObjectMeta, 0); err != nil {
 		return err
 	}
-	out.Subjects = *(*[]rbac.Subject)(unsafe.Pointer(&in.Subjects))
+	if in.Subjects != nil {
+		in, out := &in.Subjects, &out.Subjects
+		*out = make([]rbac.Subject, len(*in))
+		for i := range *in {
+			if err := Convert_v1alpha1_Subject_To_rbac_Subject(&(*in)[i], &(*out)[i], s); err != nil {
+				return err
+			}
+		}
+	} else {
+		out.Subjects = nil
+	}
 	if err := Convert_v1alpha1_RoleRef_To_rbac_RoleRef(&in.RoleRef, &out.RoleRef, s); err != nil {
 		return err
 	}
@@ -131,7 +141,17 @@ func autoConvert_rbac_ClusterRoleBinding_To_v1alpha1_ClusterRoleBinding(in *rbac
 	if err := s.Convert(&in.ObjectMeta, &out.ObjectMeta, 0); err != nil {
 		return err
 	}
-	out.Subjects = *(*[]Subject)(unsafe.Pointer(&in.Subjects))
+	if in.Subjects != nil {
+		in, out := &in.Subjects, &out.Subjects
+		*out = make([]Subject, len(*in))
+		for i := range *in {
+			if err := Convert_rbac_Subject_To_v1alpha1_Subject(&(*in)[i], &(*out)[i], s); err != nil {
+				return err
+			}
+		}
+	} else {
+		out.Subjects = nil
+	}
 	if err := Convert_rbac_RoleRef_To_v1alpha1_RoleRef(&in.RoleRef, &out.RoleRef, s); err != nil {
 		return err
 	}
@@ -166,7 +186,17 @@ func Convert_rbac_ClusterRoleBindingBuilder_To_v1alpha1_ClusterRoleBindingBuilde
 
 func autoConvert_v1alpha1_ClusterRoleBindingList_To_rbac_ClusterRoleBindingList(in *ClusterRoleBindingList, out *rbac.ClusterRoleBindingList, s conversion.Scope) error {
 	out.ListMeta = in.ListMeta
-	out.Items = *(*[]rbac.ClusterRoleBinding)(unsafe.Pointer(&in.Items))
+	if in.Items != nil {
+		in, out := &in.Items, &out.Items
+		*out = make([]rbac.ClusterRoleBinding, len(*in))
+		for i := range *in {
+			if err := Convert_v1alpha1_ClusterRoleBinding_To_rbac_ClusterRoleBinding(&(*in)[i], &(*out)[i], s); err != nil {
+				return err
+			}
+		}
+	} else {
+		out.Items = nil
+	}
 	return nil
 }
 
@@ -176,7 +206,17 @@ func Convert_v1alpha1_ClusterRoleBindingList_To_rbac_ClusterRoleBindingList(in *
 
 func autoConvert_rbac_ClusterRoleBindingList_To_v1alpha1_ClusterRoleBindingList(in *rbac.ClusterRoleBindingList, out *ClusterRoleBindingList, s conversion.Scope) error {
 	out.ListMeta = in.ListMeta
-	out.Items = *(*[]ClusterRoleBinding)(unsafe.Pointer(&in.Items))
+	if in.Items != nil {
+		in, out := &in.Items, &out.Items
+		*out = make([]ClusterRoleBinding, len(*in))
+		for i := range *in {
+			if err := Convert_rbac_ClusterRoleBinding_To_v1alpha1_ClusterRoleBinding(&(*in)[i], &(*out)[i], s); err != nil {
+				return err
+			}
+		}
+	} else {
+		out.Items = nil
+	}
 	return nil
 }
 
@@ -329,7 +369,17 @@ func autoConvert_v1alpha1_RoleBinding_To_rbac_RoleBinding(in *RoleBinding, out *
 	if err := s.Convert(&in.ObjectMeta, &out.ObjectMeta, 0); err != nil {
 		return err
 	}
-	out.Subjects = *(*[]rbac.Subject)(unsafe.Pointer(&in.Subjects))
+	if in.Subjects != nil {
+		in, out := &in.Subjects, &out.Subjects
+		*out = make([]rbac.Subject, len(*in))
+		for i := range *in {
+			if err := Convert_v1alpha1_Subject_To_rbac_Subject(&(*in)[i], &(*out)[i], s); err != nil {
+				return err
+			}
+		}
+	} else {
+		out.Subjects = nil
+	}
 	if err := Convert_v1alpha1_RoleRef_To_rbac_RoleRef(&in.RoleRef, &out.RoleRef, s); err != nil {
 		return err
 	}
@@ -345,7 +395,17 @@ func autoConvert_rbac_RoleBinding_To_v1alpha1_RoleBinding(in *rbac.RoleBinding,
 	if err := s.Convert(&in.ObjectMeta, &out.ObjectMeta, 0); err != nil {
 		return err
 	}
-	out.Subjects = *(*[]Subject)(unsafe.Pointer(&in.Subjects))
+	if in.Subjects != nil {
+		in, out := &in.Subjects, &out.Subjects
+		*out = make([]Subject, len(*in))
+		for i := range *in {
+			if err := Convert_rbac_Subject_To_v1alpha1_Subject(&(*in)[i], &(*out)[i], s); err != nil {
+				return err
+			}
+		}
+	} else {
+		out.Subjects = nil
+	}
 	if err := Convert_rbac_RoleRef_To_v1alpha1_RoleRef(&in.RoleRef, &out.RoleRef, s); err != nil {
 		return err
 	}
@@ -358,7 +418,17 @@ func Convert_rbac_RoleBinding_To_v1alpha1_RoleBinding(in *rbac.RoleBinding, out
 
 func autoConvert_v1alpha1_RoleBindingList_To_rbac_RoleBindingList(in *RoleBindingList, out *rbac.RoleBindingList, s conversion.Scope) error {
 	out.ListMeta = in.ListMeta
-	out.Items = *(*[]rbac.RoleBinding)(unsafe.Pointer(&in.Items))
+	if in.Items != nil {
+		in, out := &in.Items, &out.Items
+		*out = make([]rbac.RoleBinding, len(*in))
+		for i := range *in {
+			if err := Convert_v1alpha1_RoleBinding_To_rbac_RoleBinding(&(*in)[i], &(*out)[i], s); err != nil {
+				return err
+			}
+		}
+	} else {
+		out.Items = nil
+	}
 	return nil
 }
 
@@ -368,7 +438,17 @@ func Convert_v1alpha1_RoleBindingList_To_rbac_RoleBindingList(in *RoleBindingLis
 
 func autoConvert_rbac_RoleBindingList_To_v1alpha1_RoleBindingList(in *rbac.RoleBindingList, out *RoleBindingList, s conversion.Scope) error {
 	out.ListMeta = in.ListMeta
-	out.Items = *(*[]RoleBinding)(unsafe.Pointer(&in.Items))
+	if in.Items != nil {
+		in, out := &in.Items, &out.Items
+		*out = make([]RoleBinding, len(*in))
+		for i := range *in {
+			if err := Convert_rbac_RoleBinding_To_v1alpha1_RoleBinding(&(*in)[i], &(*out)[i], s); err != nil {
+				return err
+			}
+		}
+	} else {
+		out.Items = nil
+	}
 	return nil
 }
 
@@ -446,10 +526,6 @@ func autoConvert_v1alpha1_Subject_To_rbac_Subject(in *Subject, out *rbac.Subject
 	return nil
 }
 
-func Convert_v1alpha1_Subject_To_rbac_Subject(in *Subject, out *rbac.Subject, s conversion.Scope) error {
-	return autoConvert_v1alpha1_Subject_To_rbac_Subject(in, out, s)
-}
-
 func autoConvert_rbac_Subject_To_v1alpha1_Subject(in *rbac.Subject, out *Subject, s conversion.Scope) error {
 	out.Kind = in.Kind
 	out.APIVersion = in.APIVersion

pkg/apis/rbac/validation/rulevalidation.go

@@ -168,7 +168,7 @@ func appliesTo(user user.Info, bindingSubjects []rbac.Subject, namespace string)
 func appliesToUser(user user.Info, subject rbac.Subject, namespace string) bool {
 	switch subject.Kind {
 	case rbac.UserKind:
-		return subject.Name == rbac.UserAll || user.GetName() == subject.Name
+		return user.GetName() == subject.Name
 
 	case rbac.GroupKind:
 		return has(user.GetGroups(), subject.Name)

pkg/apis/rbac/validation/rulevalidation_test.go

@@ -232,8 +232,28 @@ func TestAppliesTo(t *testing.T) {
 			},
 			user:      &user.DefaultInfo{Name: "foobar"},
 			namespace: "default",
+			appliesTo: false,
+			testCase:  "* user subject name doesn't match all users",
+		},
+		{
+			subjects: []rbac.Subject{
+				{Kind: rbac.GroupKind, Name: user.AllAuthenticated},
+				{Kind: rbac.GroupKind, Name: user.AllUnauthenticated},
+			},
+			user:      &user.DefaultInfo{Name: "foobar", Groups: []string{user.AllAuthenticated}},
+			namespace: "default",
 			appliesTo: true,
-			testCase:  "multiple subjects with a service account that matches",
+			testCase:  "binding to all authenticated and unauthenticated subjects matches authenticated user",
+		},
+		{
+			subjects: []rbac.Subject{
+				{Kind: rbac.GroupKind, Name: user.AllAuthenticated},
+				{Kind: rbac.GroupKind, Name: user.AllUnauthenticated},
+			},
+			user:      &user.DefaultInfo{Name: "system:anonymous", Groups: []string{user.AllUnauthenticated}},
+			namespace: "default",
+			appliesTo: true,
+			testCase:  "binding to all authenticated and unauthenticated subjects matches anonymous user",
 		},
 	}
 

test/test_owners.csv

@@ -585,6 +585,7 @@ k8s.io/kubernetes/pkg/apis/meta/v1,sttts,0
 k8s.io/kubernetes/pkg/apis/meta/v1/unstructured,smarterclayton,0
 k8s.io/kubernetes/pkg/apis/meta/v1/validation,smarterclayton,0
 k8s.io/kubernetes/pkg/apis/policy/validation,deads2k,1
+k8s.io/kubernetes/pkg/apis/rbac/v1alpha1,liggitt,0
 k8s.io/kubernetes/pkg/apis/rbac/validation,erictune,0
 k8s.io/kubernetes/pkg/apis/storage/validation,caesarxuchao,1
 k8s.io/kubernetes/pkg/genericapiserver/api,nikhiljindal,0