Add validation for broker spec to SAR admission controller (openshift…

…#1605) This changes the admission controller to validate that the secret reference for both basic and bearer authentication isn't nil. Closes openshift#1600
jboyd01 · Dec 1, 2017 · 3cdd556 · 3cdd556
1 parent a3408ce
commit 3cdd556
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 5 deletions.
diff --git a/plugin/pkg/admission/broker/authsarcheck/admission.go b/plugin/pkg/admission/broker/authsarcheck/admission.go
@@ -90,7 +90,11 @@ func (s *sarcheck) Admit(a admission.Attributes) error {
 	} else if clusterServiceBroker.Spec.AuthInfo.Bearer != nil {
 		secretRef = clusterServiceBroker.Spec.AuthInfo.Bearer.SecretRef
 	}
-	glog.V(5).Infof("ClusterServiceBroker %q: evaluating auth secret ref", clusterServiceBroker)
+
+	if secretRef == nil {
+		return nil
+	}
+	glog.V(5).Infof("ClusterServiceBroker %+v: evaluating auth secret ref, with authInfo %q", clusterServiceBroker, secretRef)
 	userInfo := a.GetUserInfo()
 
 	sar := &authorizationapi.SubjectAccessReview{

diff --git a/plugin/pkg/admission/broker/authsarcheck/admission_test.go b/plugin/pkg/admission/broker/authsarcheck/admission_test.go
@@ -89,7 +89,8 @@ func TestAdmissionBroker(t *testing.T) {
 					Name: "test-broker",
 				},
 				Spec: servicecatalog.ClusterServiceBrokerSpec{
-					URL: "http://example.com",
+					URL:            "http://example.com",
+					RelistBehavior: "Manual",
 				},
 			},
 			userInfo: &user.DefaultInfo{
@@ -105,7 +106,8 @@ func TestAdmissionBroker(t *testing.T) {
 					Name: "test-broker",
 				},
 				Spec: servicecatalog.ClusterServiceBrokerSpec{
-					URL: "http://example.com",
+					URL:            "http://example.com",
+					RelistBehavior: "Manual",
 					AuthInfo: &servicecatalog.ServiceBrokerAuthInfo{
 						Basic: &servicecatalog.BasicAuthConfig{
 							SecretRef: &servicecatalog.ObjectReference{
@@ -129,7 +131,8 @@ func TestAdmissionBroker(t *testing.T) {
 					Name: "test-broker",
 				},
 				Spec: servicecatalog.ClusterServiceBrokerSpec{
-					URL: "http://example.com",
+					URL:            "http://example.com",
+					RelistBehavior: "Manual",
 					AuthInfo: &servicecatalog.ServiceBrokerAuthInfo{
 						Bearer: &servicecatalog.BearerTokenAuthConfig{
 							SecretRef: &servicecatalog.ObjectReference{
@@ -153,7 +156,8 @@ func TestAdmissionBroker(t *testing.T) {
 					Name: "test-broker",
 				},
 				Spec: servicecatalog.ClusterServiceBrokerSpec{
-					URL: "http://example.com",
+					URL:            "http://example.com",
+					RelistBehavior: "Manual",
 					AuthInfo: &servicecatalog.ServiceBrokerAuthInfo{
 						Bearer: &servicecatalog.BearerTokenAuthConfig{
 							SecretRef: &servicecatalog.ObjectReference{
@@ -170,6 +174,49 @@ func TestAdmissionBroker(t *testing.T) {
 			},
 			allowed: false,
 		},
+		{
+			name: "broker with empty authInfo",
+			broker: &servicecatalog.ClusterServiceBroker{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-broker",
+				},
+				Spec: servicecatalog.ClusterServiceBrokerSpec{
+					URL:            "http://example.com",
+					RelistBehavior: "Manual",
+					AuthInfo:       &servicecatalog.ServiceBrokerAuthInfo{},
+				},
+			},
+			userInfo: &user.DefaultInfo{
+				Name:   "system:serviceaccount:test-ns:forbidden",
+				Groups: []string{"system:serviceaccount", "system:serviceaccounts:test-ns"},
+			},
+			allowed: true,
+		},
+		{
+			name: "broker with authInfo, empty strings for Namespace/Name",
+			broker: &servicecatalog.ClusterServiceBroker{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-broker",
+				},
+				Spec: servicecatalog.ClusterServiceBrokerSpec{
+					URL:            "http://example.com",
+					RelistBehavior: "Manual",
+					AuthInfo: &servicecatalog.ServiceBrokerAuthInfo{
+						Bearer: &servicecatalog.BearerTokenAuthConfig{
+							SecretRef: &servicecatalog.ObjectReference{
+								Namespace: "",
+								Name:      "",
+							},
+						},
+					},
+				},
+			},
+			userInfo: &user.DefaultInfo{
+				Name:   "system:serviceaccount:test-ns:catalog",
+				Groups: []string{"system:serviceaccount", "system:serviceaccounts:test-ns"},
+			},
+			allowed: true,
+		},
 	}
 
 	for _, tc := range cases {