Clean up system role parsing

api/types/system_role.go

@@ -17,7 +17,6 @@ limitations under the License.
 package types
 
 import (
-	"fmt"
 	"strings"
 
 	"github.com/gravitational/trace"
@@ -61,8 +60,26 @@ const (
 	RoleWindowsDesktop SystemRole = "WindowsDesktop"
 )
 
-// LegacyClusterTokenType exists for backwards compatibility reasons, needed to upgrade to 2.3
-const LegacyClusterTokenType SystemRole = "Trustedcluster"
+// roleMappings maps a set of allowed lowercase system role names
+// to the proper system role
+var roleMappings = map[string]SystemRole{
+	"auth":            RoleAuth,
+	"node":            RoleNode,
+	"proxy":           RoleProxy,
+	"admin":           RoleAdmin,
+	"provisiontoken":  RoleProvisionToken,
+	"trusted_cluster": RoleTrustedCluster,
+	"trustedcluster":  RoleTrustedCluster,
+	"signup":          RoleSignup,
+	"nop":             RoleNop,
+	"remoteproxy":     RoleRemoteProxy,
+	"remote_proxy":    RoleRemoteProxy,
+	"kube":            RoleKube,
+	"app":             RoleApp,
+	"db":              RoleDatabase,
+	"windowsdesktop":  RoleWindowsDesktop,
+	"windows_desktop": RoleWindowsDesktop,
+}
 
 // NewTeleportRoles return a list of teleport roles from slice of strings
 func NewTeleportRoles(in []string) (SystemRoles, error) {
@@ -78,15 +95,17 @@ func NewTeleportRoles(in []string) (SystemRoles, error) {
 func ParseTeleportRoles(str string) (SystemRoles, error) {
 	var roles SystemRoles
 	for _, s := range strings.Split(str, ",") {
-		s = strings.TrimSpace(s)
-		if r := SystemRole(s); r.Check() == nil {
+		cleaned := strings.ToLower(strings.TrimSpace(s))
+		if r, ok := roleMappings[cleaned]; ok && r.Check() == nil {
 			roles = append(roles, r)
 			continue
 		}
-		// If role is not valid as-is, attempt to canonicalize the format.
-		r := SystemRole(strings.Title(strings.ToLower(s)))
-		roles = append(roles, r)
+		return nil, trace.BadParameter("invalid role %q", s)
+	}
+	if len(roles) == 0 {
+		return nil, trace.BadParameter("no valid roles in $%q", str)
 	}
+
 	return roles, roles.Check()
 }
 
@@ -178,22 +197,22 @@ func (r *SystemRole) String() string {
 	switch *r {
 	case RoleSignup:
 		return "Password"
-	case RoleTrustedCluster, LegacyClusterTokenType:
+	case RoleTrustedCluster:
 		return "trusted_cluster"
 	default:
-		return fmt.Sprintf("%v", string(*r))
+		return string(*r)
 	}
 }
 
+// Check checks if this a a valid teleport role value, returns nil
+// if it's ok, false otherwise
 // Check checks if this a a valid teleport role value, returns nil
 // if it's ok, false otherwise
 func (r *SystemRole) Check() error {
-	switch *r {
-	case RoleAuth, RoleNode, RoleApp, RoleDatabase,
-		RoleAdmin, RoleProvisionToken,
-		RoleTrustedCluster, LegacyClusterTokenType,
-		RoleSignup, RoleProxy, RoleNop, RoleKube, RoleWindowsDesktop:
+	sr, ok := roleMappings[strings.ToLower(string(*r))]
+	if ok && string(*r) == string(sr) {
 		return nil
 	}
+
 	return trace.BadParameter("role %v is not registered", *r)
 }

api/types/system_role_test.go

@@ -17,6 +17,8 @@ package types
 
 import (
 	"testing"
+
+	"github.com/stretchr/testify/require"
 )
 
 func TestRolesCheck(t *testing.T) {
@@ -26,7 +28,7 @@ func TestRolesCheck(t *testing.T) {
 	}{
 		{roles: []SystemRole{}, wantErr: false},
 		{roles: []SystemRole{RoleAuth}, wantErr: false},
-		{roles: []SystemRole{RoleAuth, RoleNode, RoleProxy, RoleAdmin, RoleProvisionToken, RoleTrustedCluster, LegacyClusterTokenType, RoleSignup, RoleNop}, wantErr: false},
+		{roles: []SystemRole{RoleAuth, RoleNode, RoleProxy, RoleAdmin, RoleProvisionToken, RoleTrustedCluster, RoleSignup, RoleNop}, wantErr: false},
 		{roles: []SystemRole{RoleAuth, RoleNode, RoleAuth}, wantErr: true},
 		{roles: []SystemRole{SystemRole("unknown")}, wantErr: true},
 	}
@@ -62,3 +64,77 @@ func TestRolesEqual(t *testing.T) {
 		}
 	}
 }
+
+func TestParseTeleportRoles(t *testing.T) {
+	t.Parallel()
+	for _, test := range []struct {
+		in      string
+		out     SystemRoles
+		wantErr bool
+	}{
+		{
+			// system role constant
+			in:      string(RoleNode),
+			out:     SystemRoles{RoleNode},
+			wantErr: false,
+		},
+		{
+			// alternate capitalization
+			in:      "node",
+			out:     SystemRoles{RoleNode},
+			wantErr: false,
+		},
+		{
+			in:      "windowsdesktop",
+			out:     SystemRoles{RoleWindowsDesktop},
+			wantErr: false,
+		},
+		{
+			// allow underscores for camelcase roles
+			in:      "windows_desktop,trusted_cluster,remote_proxy",
+			out:     SystemRoles{RoleWindowsDesktop, RoleTrustedCluster, RoleRemoteProxy},
+			wantErr: false,
+		},
+		{
+			// multiple comma-separated roles
+			in:      "Auth,Proxy",
+			out:     SystemRoles{RoleAuth, RoleProxy},
+			wantErr: false,
+		},
+		{
+			// extra whitespace handled properly
+			in:      "Auth , Proxy,WindowsDesktop",
+			out:     SystemRoles{RoleAuth, RoleProxy, RoleWindowsDesktop},
+			wantErr: false,
+		},
+		{
+			// duplicate roles is an error
+			in:      "Auth,Auth",
+			wantErr: true,
+		},
+		{
+			in:      "Auth, auth",
+			wantErr: true,
+		},
+		{
+			// invalid role errors
+			in:      "invalidrole",
+			wantErr: true,
+		},
+		{
+			// valid + invalid role errors
+			in:      "Admin,invalidrole",
+			wantErr: true,
+		},
+	} {
+		t.Run(test.in, func(t *testing.T) {
+			roles, err := ParseTeleportRoles(test.in)
+			if test.wantErr {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, test.out, roles)
+			}
+		})
+	}
+}

lib/auth/trustedcluster.go

@@ -538,7 +538,7 @@ func (a *Server) validateTrustedClusterToken(token string) (map[string]string, e
 		return nil, trace.AccessDenied("the remote server denied access: invalid cluster token")
 	}
 
-	if !roles.Include(types.RoleTrustedCluster) && !roles.Include(types.LegacyClusterTokenType) {
+	if !roles.Include(types.RoleTrustedCluster) {
 		return nil, trace.AccessDenied("role does not match")
 	}
 

tool/tctl/common/node_command.go

@@ -144,9 +144,9 @@ func (c *NodeCommand) Invite(client auth.ClientI) error {
 		return trace.Errorf("This cluster does not have any auth servers running.")
 	}
 
-	// output format swtich:
+	// output format switch:
 	if c.format == "text" {
-		if roles.Include(types.RoleTrustedCluster) || roles.Include(types.LegacyClusterTokenType) {
+		if roles.Include(types.RoleTrustedCluster) {
 			fmt.Printf(trustedClusterMessage, token, int(c.ttl.Minutes()))
 		} else {
 			return nodeMessageTemplate.Execute(os.Stdout, map[string]interface{}{

tool/tctl/common/token_command.go

@@ -213,7 +213,7 @@ func (c *TokenCommand) Add(client auth.ClientI) error {
 				"db_protocol": c.dbProtocol,
 				"db_uri":      c.dbURI,
 			})
-	case roles.Include(types.RoleTrustedCluster), roles.Include(types.LegacyClusterTokenType):
+	case roles.Include(types.RoleTrustedCluster):
 		fmt.Printf(trustedClusterMessage,
 			token,
 			int(c.ttl.Minutes()))