Fix role mapping for trusted clusters (#3726)

This commit fixes #3252 Security patches 4.2 introduced a regression - leaf clusters ignore role mapping and attempt to use role names coming from identity of the root cluster whenever GetNodes method was used. This commit reverts back the logic, however it ensures that the original fix is preserved - traits and groups are updated on the user object. Integration test has been extended to avoid the regression in the future. Co-authored-by: Sasha Klizhentas <[email protected]>
gravitational · May 18, 2020 · 5b6732a · 5b6732a
1 parent 50a1f1c
commit 5b6732a
Show file tree

Hide file tree

Showing 13 changed files with 80 additions and 36 deletions.
diff --git a/docker/docker-compose.yml b/docker/docker-compose.yml
@@ -41,7 +41,7 @@ services:
         ipv4_address: 172.10.1.20
 
   #
-  # one-node is a single-node Teleport cluster called "one" (runs all 3 roles: proxy, auth and node)
+  # one-sshd is a single-node Teleport cluster called "one" (runs all 3 roles: proxy, auth and node)
   #
   one-sshd:
     image: teleport:latest

diff --git a/docker/one-proxy.yaml b/docker/one-proxy.yaml
@@ -4,8 +4,8 @@ teleport:
   nodename: one-proxy
   advertise_ip: 172.10.1.10
   log:
-    output: /var/lib/teleport/teleport.log
-    severity: INFO
+    output: stdout
+    severity: DEBUG
   auth_servers:
     - one:3025
   data_dir: /var/lib/teleport

diff --git a/docker/one.yaml b/docker/one.yaml
@@ -3,8 +3,8 @@ teleport:
   nodename: one
   advertise_ip: 172.10.1.1
   log:
-    output: /var/lib/teleport/teleport.log
-    severity: INFO
+    output: stdout
+    severity: DEBUG
 
   data_dir: /var/lib/teleport
   storage:
@@ -30,7 +30,10 @@ ssh_service:
       - name: kernel
         command: [/bin/uname, -r]
         period: 5m
+  public_addr: ['localhost']
 
 proxy_service:
   enabled: yes
+  public_addr: ['localhost:3080']
+
 
diff --git a/docker/two-auth.yaml b/docker/two-auth.yaml
@@ -2,8 +2,8 @@
 teleport:
   nodename: two-auth
   log:
-    output: /var/lib/teleport/teleport.log
-    severity: INFO
+    output: stdout
+    severity: DEBUG
 
   data_dir: /var/lib/teleport
   storage:

diff --git a/docker/two-node.yaml b/docker/two-node.yaml
@@ -5,8 +5,8 @@ teleport:
   auth_token: foo
   advertise_ip: 172.10.1.4
   log:
-    output: /var/lib/teleport/teleport.log
-    severity: INFO
+    output: stdout
+    severity: DEBUG
   data_dir: /var/lib/teleport
   storage:
       path: /var/lib/teleport/backend

diff --git a/docker/two-proxy.yaml b/docker/two-proxy.yaml
@@ -4,8 +4,8 @@ teleport:
   auth_servers: ["two-auth"]
   auth_token: foo
   log:
-    output: /var/lib/teleport/teleport.log
-    severity: INFO
+    output: stdout
+    severity: DEBUG
   data_dir: /var/lib/teleport
   storage:
       path: /var/lib/teleport/backend

diff --git a/integration/helpers.go b/integration/helpers.go
@@ -507,6 +507,8 @@ func (i *TeleInstance) CreateEx(trustedSecrets []*InstanceSecrets, tconf *servic
 		if err != nil {
 			return trace.Wrap(err)
 		}
+		// set hardcode traits to trigger new style certificates
+		teleUser.SetTraits(map[string][]string{"testing": []string{"integration"}})
 		var roles []services.Role
 		if len(user.Roles) == 0 {
 			role := services.RoleForUser(teleUser)
@@ -737,12 +739,13 @@ func (i *TeleInstance) Reset() (err error) {
 	return nil
 }
 
-// AddUserUserWithRole adds user with assigned role
-func (i *TeleInstance) AddUserWithRole(username string, role services.Role) *User {
+// AddUserUserWithRole adds user with one or many assigned roles
+func (i *TeleInstance) AddUserWithRole(username string, roles ...services.Role) *User {
 	user := &User{
 		Username: username,
-		Roles:    []services.Role{role},
+		Roles:    make([]services.Role, len(roles)),
 	}
+	copy(user.Roles, roles)
 	i.Secrets.Users[username] = user
 	return user
 }

diff --git a/integration/integration_test.go b/integration/integration_test.go
@@ -1428,15 +1428,23 @@ func (s *IntSuite) trustedClusters(c *check.C, multiplex bool) {
 	main := NewInstance(InstanceConfig{ClusterName: clusterMain, HostID: HostID, NodeName: Host, Ports: s.getPorts(5), Priv: s.priv, Pub: s.pub, MultiplexProxy: multiplex})
 	aux := NewInstance(InstanceConfig{ClusterName: clusterAux, HostID: HostID, NodeName: Host, Ports: s.getPorts(5), Priv: s.priv, Pub: s.pub})
 
-	// main cluster has a local user and belongs to role "main-devs"
+	// main cluster has a local user and belongs to role "main-devs" and "main-admins"
 	mainDevs := "main-devs"
-	role, err := services.NewRole(mainDevs, services.RoleSpecV3{
+	devsRole, err := services.NewRole(mainDevs, services.RoleSpecV3{
 		Allow: services.RoleConditions{
 			Logins: []string{username},
 		},
 	})
 	c.Assert(err, check.IsNil)
-	main.AddUserWithRole(username, role)
+
+	mainAdmins := "main-admins"
+	adminsRole, err := services.NewRole(mainAdmins, services.RoleSpecV3{
+		Allow: services.RoleConditions{
+			Logins: []string{"superuser"},
+		},
+	})
+
+	main.AddUserWithRole(username, devsRole, adminsRole)
 
 	// for role mapping test we turn on Web API on the main cluster
 	// as it's used
@@ -1454,22 +1462,24 @@ func (s *IntSuite) trustedClusters(c *check.C, multiplex bool) {
 	c.Assert(main.CreateEx(makeConfig(false)), check.IsNil)
 	c.Assert(aux.CreateEx(makeConfig(true)), check.IsNil)
 
-	// auxiliary cluster has a role aux-devs
+	// auxiliary cluster has only a role aux-devs
 	// connect aux cluster to main cluster
 	// using trusted clusters, so remote user will be allowed to assume
 	// role specified by mapping remote role "devs" to local role "local-devs"
 	auxDevs := "aux-devs"
-	role, err = services.NewRole(auxDevs, services.RoleSpecV3{
+	auxRole, err := services.NewRole(auxDevs, services.RoleSpecV3{
 		Allow: services.RoleConditions{
 			Logins: []string{username},
 		},
 	})
 	c.Assert(err, check.IsNil)
-	err = aux.Process.GetAuthServer().UpsertRole(role, backend.Forever)
+	err = aux.Process.GetAuthServer().UpsertRole(auxRole, backend.Forever)
 	c.Assert(err, check.IsNil)
-	trustedClusterToken := "trusted-clsuter-token"
+	trustedClusterToken := "trusted-cluster-token"
 	err = main.Process.GetAuthServer().UpsertToken(trustedClusterToken, []teleport.Role{teleport.RoleTrustedCluster}, backend.Forever)
 	c.Assert(err, check.IsNil)
+	// Note that the mapping omits admins role, this is to cover the scenario
+	// when root cluster and leaf clusters have different role sets
 	trustedCluster := main.Secrets.AsTrustedCluster(trustedClusterToken, services.RoleMap{
 		{Remote: mainDevs, Local: []string{auxDevs}},
 	})
@@ -1518,6 +1528,15 @@ func (s *IntSuite) trustedClusters(c *check.C, multiplex bool) {
 	cmd := []string{"echo", "hello world"}
 	tc, err := main.NewClient(ClientConfig{Login: username, Cluster: clusterAux, Host: "127.0.0.1", Port: sshPort})
 	c.Assert(err, check.IsNil)
+
+	// tell the client to trust aux cluster CAs (from secrets). this is the
+	// equivalent of 'known hosts' in openssh
+	auxCAS := aux.Secrets.GetCAs()
+	for i := range auxCAS {
+		err = tc.AddTrustedCA(auxCAS[i])
+		c.Assert(err, check.IsNil)
+	}
+
 	output := &bytes.Buffer{}
 	tc.Stdout = output
 	c.Assert(err, check.IsNil)
@@ -1534,6 +1553,13 @@ func (s *IntSuite) trustedClusters(c *check.C, multiplex bool) {
 	c.Assert(err, check.IsNil)
 	c.Assert(output.String(), check.Equals, "hello world\n")
 
+	// ListNodes expect labels as a value of host
+	tc.Host = ""
+	servers, err := tc.ListNodes(context.TODO())
+	c.Assert(err, check.IsNil)
+	c.Assert(servers, check.HasLen, 2)
+	tc.Host = "127.0.0.1"
+
 	// check that remote cluster has been provisioned
 	remoteClusters, err := main.Process.GetAuthServer().GetRemoteClusters()
 	c.Assert(err, check.IsNil)

diff --git a/lib/auth/auth_with_roles.go b/lib/auth/auth_with_roles.go
@@ -356,12 +356,7 @@ func (a *AuthWithRoles) filterNodes(nodes []services.Server) ([]services.Server,
 		return nodes, nil
 	}
 
-	// Fetch services.RoleSet for the identity of the logged in user.
-	roles, traits, err := services.ExtractFromIdentity(a.authServer, &a.identity)
-	if err != nil {
-		return nil, trace.Wrap(err)
-	}
-	roleset, err := services.FetchRoles(roles, a.authServer, traits)
+	roleset, err := services.FetchRoles(a.user.GetRoles(), a.authServer, a.user.GetTraits())
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}

diff --git a/lib/auth/permissions.go b/lib/auth/permissions.go
@@ -449,6 +449,7 @@ func contextForBuiltinRole(clusterName string, clusterConfig services.ClusterCon
 }
 
 func contextForLocalUser(u LocalUser, identity services.UserGetter, access services.Access) (*AuthContext, error) {
+	// User has to be fetched to check if it's a blocked username
 	user, err := identity.GetUser(u.Username)
 	if err != nil {
 		return nil, trace.Wrap(err)
@@ -461,6 +462,15 @@ func contextForLocalUser(u LocalUser, identity services.UserGetter, access servi
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
+	// Override roles and traits from the local user based on the identity roles
+	// and traits, this is done to prevent potential conflict. Imagine a scenairo
+	// when SSO user has left the company, but local user entry remained with old
+	// privileged roles. New user with the same name has been onboarded and would
+	// have derived the roles from the stale user entry. This code prevents
+	// that by extracting up to date identity traits and roles from the user's
+	// certificate metadata.
+	user.SetRoles(roles)
+	user.SetTraits(traits)
 
 	return &AuthContext{
 		User:    user,

diff --git a/lib/auth/testauthority/testauthority.go b/lib/auth/testauthority/testauthority.go
@@ -25,6 +25,7 @@ import (
 	"github.com/gravitational/teleport/lib/auth/native"
 	"github.com/gravitational/teleport/lib/services"
 	"github.com/gravitational/teleport/lib/utils"
+	"github.com/gravitational/teleport/lib/wrappers"
 
 	"github.com/gravitational/trace"
 	"golang.org/x/crypto/ssh"
@@ -110,12 +111,19 @@ func (n *Keygen) GenerateUserCert(c services.UserCertParams) ([]byte, error) {
 	if !c.PermitPortForwarding {
 		delete(cert.Permissions.Extensions, teleport.CertExtensionPermitPortForwarding)
 	}
-	if len(c.Roles) != 0 {
-		// only add roles to the certificate extensions if the standard format was
-		// requested. we allow the option to omit this to support older versions of
-		// OpenSSH due to a bug in <= OpenSSH 7.1
-		// https://bugzilla.mindrot.org/show_bug.cgi?id=2387
-		if c.CertificateFormat == teleport.CertificateFormatStandard {
+	// Add roles, traits, and route to cluster in the certificate extensions if
+	// the standard format was requested. Certificate extensions are not included
+	// legacy SSH certificates due to a bug in OpenSSH <= OpenSSH 7.1:
+	// https://bugzilla.mindrot.org/show_bug.cgi?id=2387
+	if c.CertificateFormat == teleport.CertificateFormatStandard {
+		traits, err := wrappers.MarshalTraits(&c.Traits)
+		if err != nil {
+			return nil, trace.Wrap(err)
+		}
+		if len(traits) > 0 {
+			cert.Permissions.Extensions[teleport.CertExtensionTeleportTraits] = string(traits)
+		}
+		if len(c.Roles) != 0 {
 			roles, err := services.MarshalCertRoles(c.Roles)
 			if err != nil {
 				return nil, trace.Wrap(err)

diff --git a/lib/client/client.go b/lib/client/client.go
@@ -75,7 +75,7 @@ func (proxy *ProxyClient) GetSites() ([]services.Site, error) {
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
-
+	defer proxySession.Close()
 	stdout := &bytes.Buffer{}
 	reader, err := proxySession.StdoutPipe()
 	if err != nil {

diff --git a/lib/services/role.go b/lib/services/role.go
@@ -1357,7 +1357,6 @@ func ExtractFromCertificate(access UserGetter, cert *ssh.Certificate) ([]string,
 		if err != nil {
 			return nil, nil, trace.Wrap(err)
 		}
-
 		log.Warnf("User %v using old style SSH certificate, fetching roles and traits "+
 			"from backend. If the identity provider allows username changes, this can "+
 			"potentially allow an attacker to change the role of the existing user. "+
@@ -1423,7 +1422,7 @@ func isFormatOld(cert *ssh.Certificate) bool {
 	_, hasRoles := cert.Extensions[teleport.CertExtensionTeleportRoles]
 	_, hasTraits := cert.Extensions[teleport.CertExtensionTeleportTraits]
 
-	if hasRoles && hasTraits {
+	if hasRoles || hasTraits {
 		return false
 	}
 	return true