From 5458e4de960e0883bdce09f9505793c6ba0d3b9e Mon Sep 17 00:00:00 2001 From: Sasha Klizhentas Date: Thu, 9 Jan 2020 13:52:00 -0800 Subject: [PATCH] Fix role mapping for trusted clusters 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. --- docker/docker-compose.yml | 2 +- docker/one-proxy.yaml | 4 +-- docker/one.yaml | 7 +++-- docker/two-auth.yaml | 4 +-- docker/two-node.yaml | 4 +-- docker/two-proxy.yaml | 4 +-- integration/helpers.go | 9 ++++-- integration/integration_test.go | 40 ++++++++++++++++++++----- lib/auth/auth_with_roles.go | 7 +---- lib/auth/permissions.go | 10 +++++++ lib/auth/testauthority/testauthority.go | 20 +++++++++---- lib/client/client.go | 2 +- lib/services/role.go | 3 +- 13 files changed, 80 insertions(+), 36 deletions(-) diff --git a/docker/docker-compose.yml b/docker/docker-compose.yml index 2713566da9a7f..7c72b931e5577 100644 --- 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 index d87601a7d9f98..e6f290793c9fd 100644 --- 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 index cc4db00a4121f..a3742456dd1ff 100644 --- 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 index 8c2ad047ae9f2..9cdf7292e3c45 100644 --- 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 index 7680c5c482742..123ea41629a7e 100644 --- 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 index a8d9edb67df0b..463fd704ca7f5 100644 --- 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 index c1b80e9e38378..cb9f35ebd2814 100644 --- 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 index 0a28783273e2c..3d1f39d35bb60 100644 --- 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 index 94deb661f3e23..c19a8f89ee63d 100644 --- 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 index c4958cd511f34..9bbcb7161adbc 100644 --- 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 index f2757b157b967..8cee014762d2b 100644 --- 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 index b4537886069d5..8df181f71a012 100644 --- 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 index 7443bf77ebb58..62c3c42b3a450 100644 --- 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