Merge pull request #17061 from juanvallejo/jvallejo/prevent-use-of-sa-long-form-names-with-z

Automatic merge from submit-queue.

return error when long-form sa name is used

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1425398

Returns an error when the long-form name of a ServiceAccount is used
with the --serviceaccount (-z) flag in `oc policy ...' commands.

/assign enj

cc @openshift/cli-review

Author: openshift-merge-robot

pkg/oc/admin/policy/modify_roles.go

@@ -4,10 +4,12 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"strings"
 
 	"github.com/spf13/cobra"
 
 	kapierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/validation"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	kapi "k8s.io/kubernetes/pkg/api"
@@ -320,6 +322,19 @@ func (o *RoleModificationOptions) CompleteUserWithSA(f *clientcmd.Factory, cmd *
 		return errors.New("you must specify at least one user or service account")
 	}
 
+	// return an error if a fully-qualified service-account name is used
+	for _, sa := range saNames {
+		if strings.HasPrefix(sa, "system:serviceaccount") {
+			return errors.New("--serviceaccount (-z) should only be used with short-form serviceaccount names (e.g. `default`)")
+		}
+
+		if errCauses := validation.ValidateServiceAccountName(sa, false); len(errCauses) > 0 {
+			message := fmt.Sprintf("%q is not a valid serviceaccount name:\n  ", sa)
+			message += strings.Join(errCauses, "\n  ")
+			return errors.New(message)
+		}
+	}
+
 	authorizationClient, err := f.OpenshiftInternalAuthorizationClient()
 	if err != nil {
 		return err

test/cmd/policy.sh

@@ -40,6 +40,11 @@ os::cmd::expect_success "oc login -u system:admin -n '${project}'"
 os::cmd::expect_success 'oc delete project policy-login'
 os::cmd::expect_failure_and_text 'oc create policybinding default -n myproject' 'error: the server does not support legacy policy resources'
 
+# validate --serviceaccount values
+os::cmd::expect_success_and_text 'oc policy add-role-to-user admin -z default' 'role "admin" added\: "default"'
+os::cmd::expect_failure_and_text 'oc policy add-role-to-user admin -z system:serviceaccount:test:default' 'should only be used with short\-form serviceaccount names'
+os::cmd::expect_failure_and_text 'oc policy add-role-to-user admin -z :invalid' '"\:invalid" is not a valid serviceaccount name'
+
 # This test validates user level policy
 os::cmd::expect_failure_and_text 'oc policy add-role-to-user' 'you must specify a role'
 os::cmd::expect_failure_and_text 'oc policy add-role-to-user -z NamespaceWithoutRole' 'you must specify a role'