Merge pull request #13649 from Miciah/RestrictUsersAdmission-allow-SA-with-implicit-NS

Merged by openshift-bot

Author: OpenShift Bot

pkg/authorization/admission/restrictusers/subjectchecker.go

@@ -279,8 +279,19 @@ func (checker ServiceAccountSubjectChecker) Allowed(subject kapi.ObjectReference
 		return false, nil
 	}
 
+	subjectNamespace := subject.Namespace
+	if len(subjectNamespace) == 0 {
+		// If a RoleBinding has a subject that is a ServiceAccount with
+		// no namespace specified, the namespace will be defaulted to
+		// that of the RoleBinding.  However, admission control plug-ins
+		// execute before this happens, so in order not to reject such
+		// subjects erroneously, we copy the logic here of using the
+		// RoleBinding's namespace if the subject's is empty.
+		subjectNamespace = ctx.namespace
+	}
+
 	for _, namespace := range checker.serviceAccountRestriction.Namespaces {
-		if subject.Namespace == namespace {
+		if subjectNamespace == namespace {
 			return true, nil
 		}
 	}
@@ -292,7 +303,7 @@ func (checker ServiceAccountSubjectChecker) Allowed(subject kapi.ObjectReference
 		}
 
 		if subject.Name == serviceAccountRef.Name &&
-			subject.Namespace == serviceAccountNamespace {
+			subjectNamespace == serviceAccountNamespace {
 			return true, nil
 		}
 	}

pkg/authorization/admission/restrictusers/subjectchecker_test.go

@@ -192,7 +192,7 @@ func TestSubjectCheckers(t *testing.T) {
 			shouldAllow: true,
 		},
 		{
-			name: "allow service account by literal name match",
+			name: "allow service account with explicit namespace by match on literal name and explicit namespace",
 			checker: mustNewSubjectChecker(t,
 				&authorizationapi.RoleBindingRestrictionSpec{
 					ServiceAccountRestriction: &authorizationapi.ServiceAccountRestriction{
@@ -208,7 +208,7 @@ func TestSubjectCheckers(t *testing.T) {
 			shouldAllow: true,
 		},
 		{
-			name: "allow service account by literal name match with implicit namespace",
+			name: "allow service account with explicit namespace by match on literal name and implicit namespace",
 			checker: mustNewSubjectChecker(t,
 				&authorizationapi.RoleBindingRestrictionSpec{
 					ServiceAccountRestriction: &authorizationapi.ServiceAccountRestriction{
@@ -220,6 +220,113 @@ func TestSubjectCheckers(t *testing.T) {
 			subject:     serviceaccountRef,
 			shouldAllow: true,
 		},
+		{
+			name: "prohibit service account with explicit namespace where literal name matches but explicit namespace does not",
+			checker: mustNewSubjectChecker(t,
+				&authorizationapi.RoleBindingRestrictionSpec{
+					ServiceAccountRestriction: &authorizationapi.ServiceAccountRestriction{
+						ServiceAccounts: []authorizationapi.ServiceAccountReference{
+							{
+								Namespace: serviceaccountRef.Namespace,
+								Name:      serviceaccountRef.Name,
+							},
+						},
+					},
+				}),
+			subject: kapi.ObjectReference{
+				Kind:      authorizationapi.ServiceAccountKind,
+				Namespace: "othernamespace",
+				Name:      serviceaccountRef.Name,
+			},
+			shouldAllow: false,
+		},
+		{
+			name: "prohibit service account with explicit namespace where literal name matches but implicit namespace does not",
+			checker: mustNewSubjectChecker(t,
+				&authorizationapi.RoleBindingRestrictionSpec{
+					ServiceAccountRestriction: &authorizationapi.ServiceAccountRestriction{
+						ServiceAccounts: []authorizationapi.ServiceAccountReference{
+							{Name: serviceaccountRef.Name},
+						},
+					},
+				}),
+			subject: kapi.ObjectReference{
+				Kind:      authorizationapi.ServiceAccountKind,
+				Namespace: "othernamespace",
+				Name:      serviceaccountRef.Name,
+			},
+			shouldAllow: false,
+		},
+		{
+			name: "allow service account with implicit namespace by match on literal name and explicit namespace",
+			checker: mustNewSubjectChecker(t,
+				&authorizationapi.RoleBindingRestrictionSpec{
+					ServiceAccountRestriction: &authorizationapi.ServiceAccountRestriction{
+						ServiceAccounts: []authorizationapi.ServiceAccountReference{
+							{
+								Name:      serviceaccountRef.Name,
+								Namespace: serviceaccountRef.Namespace,
+							},
+						},
+					},
+				}),
+			subject: kapi.ObjectReference{
+				Kind: authorizationapi.ServiceAccountKind,
+				Name: serviceaccountRef.Name,
+			},
+			shouldAllow: true,
+		},
+		{
+			name: "allow service account with implicit namespace by match on literal name and implicit namespace",
+			checker: mustNewSubjectChecker(t,
+				&authorizationapi.RoleBindingRestrictionSpec{
+					ServiceAccountRestriction: &authorizationapi.ServiceAccountRestriction{
+						ServiceAccounts: []authorizationapi.ServiceAccountReference{
+							{Name: serviceaccountRef.Name},
+						},
+					},
+				}),
+			subject: kapi.ObjectReference{
+				Kind: authorizationapi.ServiceAccountKind,
+				Name: serviceaccountRef.Name,
+			},
+			shouldAllow: true,
+		},
+		{
+			name: "prohibit service account with implicit namespace where literal name matches but explicit namespace does not",
+			checker: mustNewSubjectChecker(t,
+				&authorizationapi.RoleBindingRestrictionSpec{
+					ServiceAccountRestriction: &authorizationapi.ServiceAccountRestriction{
+						ServiceAccounts: []authorizationapi.ServiceAccountReference{
+							{
+								Namespace: "othernamespace",
+								Name:      serviceaccountRef.Name,
+							},
+						},
+					},
+				}),
+			subject: kapi.ObjectReference{
+				Kind: authorizationapi.ServiceAccountKind,
+				Name: serviceaccountRef.Name,
+			},
+			shouldAllow: false,
+		},
+		{
+			name: "prohibit service account with explicit namespace where explicit namespace matches but literal name does not",
+			checker: mustNewSubjectChecker(t,
+				&authorizationapi.RoleBindingRestrictionSpec{
+					ServiceAccountRestriction: &authorizationapi.ServiceAccountRestriction{
+						ServiceAccounts: []authorizationapi.ServiceAccountReference{
+							{
+								Namespace: serviceaccountRef.Namespace,
+								Name:      "othername",
+							},
+						},
+					},
+				}),
+			subject:     serviceaccountRef,
+			shouldAllow: false,
+		},
 		{
 			name: "allow service account by match on namespace",
 			checker: mustNewSubjectChecker(t,