Preserve roles from user cert when forwarding k8s requests

When a proxy forwards a k8s request to another proxy in a trusted leaf
cluster, it dynamically generates a new client key/cert to impersonate
the user. This key/cert is presented to the leaf proxy as if a user was
directly making a request to it.

Auth server, when processing the CSR, would load user identity from the
backend. If this user had temporary role grants via workflow API, they
would not be loaded from the backend.

This means that if a user accesses k8s through:
```
kubectl -> root proxy -> leaf proxy -> k8s
```
the second hop would drop their temporary role grants.

To fix this, preserve full user identity from the original client cert
presented to root proxy, as encoded in CSR Subject. The auth server
implicitly trusts the CSR Subject that the proxy presents.
This also requires fixing the Subject encoding on the proxy side, which
was inconsistent with how auth server does it.

One downside here is: a compromised proxy can mint k8s client certs for
known users with arbitrary roles.

lib/auth/kube.go

@@ -17,7 +17,8 @@ limitations under the License.
 package auth
 
 import (
-	"github.com/gravitational/teleport"
+	"time"
+
 	"github.com/gravitational/teleport/lib/defaults"
 	"github.com/gravitational/teleport/lib/modules"
 	"github.com/gravitational/teleport/lib/services"
@@ -84,30 +85,24 @@ func (s *AuthServer) ProcessKubeCSR(req KubeCSR) (*KubeCSRResponse, error) {
 		return nil, trace.Wrap(err)
 	}
 
-	user, err := s.GetUser(req.Username, false)
-	if err != nil {
-		return nil, trace.Wrap(err)
-	}
-
-	roles, err := services.FetchRoles(user.GetRoles(), s, user.GetTraits())
-	if err != nil {
-		return nil, trace.Wrap(err)
-	}
-
-	ttl := roles.AdjustSessionTTL(defaults.CertDuration)
-
-	// extract and encode the kubernetes groups of the authenticated
-	// user in the newly issued certificate
-	kubernetesGroups, kubernetesUsers, err := roles.CheckKubeGroupsAndUsers(0)
-	if err != nil {
-		return nil, trace.Wrap(err)
-	}
-
 	csr, err := tlsca.ParseCertificateRequestPEM(req.CSR)
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
 
+	// Extract user roles from the CSR. Pass zero time for id.Expiry, it won't
+	// be used here.
+	id, err := tlsca.FromSubject(csr.Subject, time.Time{})
+	if err != nil {
+		return nil, trace.Wrap(err)
+	}
+	roles, err := services.FetchRoles(id.Groups, s, id.Traits)
+	if err != nil {
+		return nil, trace.Wrap(err)
+	}
+	// Get the correct cert TTL based on roles.
+	ttl := roles.AdjustSessionTTL(defaults.CertDuration)
+
 	userCA, err := s.Trust.GetCertAuthority(services.CertAuthID{
 		Type:       services.UserCA,
 		DomainName: clusterName.GetClusterName(),
@@ -121,25 +116,14 @@ func (s *AuthServer) ProcessKubeCSR(req KubeCSR) (*KubeCSRResponse, error) {
 		return nil, trace.Wrap(err)
 	}
 
-	identity := tlsca.Identity{
-		Username: user.GetName(),
-		Groups:   roles.RoleNames(),
-		// Generate a certificate restricted for
-		// use against a kubernetes endpoint, and not the API server endpoint
-		// otherwise proxies can generate certs for any user.
-		Usage:            []string{teleport.UsageKubeOnly},
-		KubernetesGroups: kubernetesGroups,
-		KubernetesUsers:  kubernetesUsers,
-	}
-	subject, err := identity.Subject()
-	if err != nil {
-		return nil, trace.Wrap(err)
-	}
 	certRequest := tlsca.CertificateRequest{
 		Clock:     s.clock,
 		PublicKey: csr.PublicKey,
-		Subject:   subject,
-		NotAfter:  s.clock.Now().UTC().Add(ttl),
+		// Always trust the Subject sent by the proxy. A user may have received
+		// temporary extra roles via workflow API, we must preserve those. The
+		// storage backend doesn't record temporary granted roles.
+		Subject:  csr.Subject,
+		NotAfter: s.clock.Now().UTC().Add(ttl),
 	}
 	tlsCert, err := tlsAuthority.GenerateCertificate(certRequest)
 	if err != nil {

lib/auth/kube_test.go

@@ -0,0 +1,89 @@
+package auth
+
+import (
+	"crypto/rsa"
+	"crypto/x509"
+	"crypto/x509/pkix"
+	"encoding/pem"
+	"math/rand"
+	"time"
+
+	"github.com/gravitational/teleport/lib/services"
+	"github.com/gravitational/teleport/lib/services/suite"
+	"github.com/gravitational/teleport/lib/tlsca"
+	"github.com/gravitational/trace"
+	"gopkg.in/check.v1"
+)
+
+func (s *AuthSuite) TestProcessKubeCSR(c *check.C) {
+	const (
+		username    = "bob"
+		roleA       = "user:bob"
+		roleB       = "requestable"
+		clusterName = "me.localhost"
+	)
+	c.Assert(s.a.UpsertCertAuthority(suite.NewTestCA(services.UserCA, clusterName)), check.IsNil)
+	c.Assert(s.a.UpsertCertAuthority(suite.NewTestCA(services.HostCA, clusterName)), check.IsNil)
+
+	// Requested user identity, presented in CSR Subject.
+	userID := tlsca.Identity{
+		Username:         username,
+		Groups:           []string{roleA, roleB},
+		Usage:            []string{"usage a", "usage b"},
+		Principals:       []string{"principal a", "principal b"},
+		KubernetesGroups: []string{"k8s group a", "k8s group b"},
+		Traits:           map[string][]string{"trait a": []string{"b", "c"}},
+	}
+	subj, err := userID.Subject()
+	c.Assert(err, check.IsNil)
+
+	pemCSR, err := newTestCSR(subj)
+	c.Assert(err, check.IsNil)
+	csr := KubeCSR{
+		Username:    username,
+		ClusterName: clusterName,
+		CSR:         pemCSR,
+	}
+
+	// CSR with unknown roles.
+	_, err = s.a.ProcessKubeCSR(csr)
+	c.Assert(err, check.NotNil)
+	c.Assert(trace.IsNotFound(err), check.Equals, true)
+
+	// Create the user and allow it to request the additional role.
+	_, err = CreateUserRoleAndRequestable(s.a, username, roleB)
+	c.Assert(err, check.IsNil)
+
+	// CSR with allowed, known roles.
+	resp, err := s.a.ProcessKubeCSR(csr)
+	c.Assert(err, check.IsNil)
+
+	cert, err := tlsca.ParseCertificatePEM(resp.Cert)
+	c.Assert(err, check.IsNil)
+	// Note: we could compare cert.Subject with subj here directly.
+	// However, because pkix.Name encoding/decoding isn't symmetric (ExtraNames
+	// before encoding becomes Names after decoding), they wouldn't match.
+	// Therefore, convert back to Identity, which handles this oddity and
+	// should match.
+	gotUserID, err := tlsca.FromSubject(cert.Subject, time.Time{})
+	c.Assert(err, check.IsNil)
+	c.Assert(*gotUserID, check.DeepEquals, userID)
+}
+
+// newTestCSR creates and PEM-encodes an x509 CSR with given subject.
+func newTestCSR(subj pkix.Name) ([]byte, error) {
+	// Use math/rand to avoid blocking on system entropy.
+	rng := rand.New(rand.NewSource(0))
+	priv, err := rsa.GenerateKey(rng, 2048)
+	if err != nil {
+		return nil, err
+	}
+	x509CSR := &x509.CertificateRequest{
+		Subject: subj,
+	}
+	derCSR, err := x509.CreateCertificateRequest(rng, x509CSR, priv)
+	if err != nil {
+		return nil, err
+	}
+	return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE REQUEST", Bytes: derCSR}), nil
+}

lib/kube/proxy/forwarder.go

@@ -21,7 +21,6 @@ import (
 	"crypto/rand"
 	"crypto/tls"
 	"crypto/x509"
-	"crypto/x509/pkix"
 	"encoding/pem"
 	"fmt"
 	"net"
@@ -1060,11 +1059,15 @@ func (f *Forwarder) requestCertificate(ctx authContext) (*tls.Config, error) {
 		return nil, trace.Wrap(err, "failed to parse private key")
 	}
 
+	// Note: ctx.Identity can potentially have temporary roles granted via
+	// workflow API. Always use the Subject() method to preserve the roles from
+	// caller's certificate.
+	subject, err := ctx.Identity.Subject()
+	if err != nil {
+		return nil, trace.Wrap(err)
+	}
 	csr := &x509.CertificateRequest{
-		Subject: pkix.Name{
-			CommonName:   ctx.User.GetName(),
-			Organization: utils.StringsSliceFromSet(ctx.kubeGroups),
-		},
+		Subject: subject,
 	}
 	csrBytes, err := x509.CreateCertificateRequest(rand.Reader, csr, privateKey)
 	if err != nil {

lib/kube/proxy/forwarder_test.go

@@ -0,0 +1,98 @@
+package proxy
+
+import (
+	"crypto/x509"
+	"encoding/pem"
+	"time"
+
+	"github.com/gravitational/teleport/lib/auth"
+	"github.com/gravitational/teleport/lib/auth/testauthority"
+	"github.com/gravitational/teleport/lib/reversetunnel"
+	"github.com/gravitational/teleport/lib/services"
+	"github.com/gravitational/teleport/lib/tlsca"
+	"gopkg.in/check.v1"
+)
+
+type ForwarderSuite struct{}
+
+var _ = check.Suite(ForwarderSuite{})
+
+func (s ForwarderSuite) TestRequestCertificate(c *check.C) {
+	cl := &mockClient{
+		csrResp: auth.KubeCSRResponse{
+			Cert:            []byte("mock cert"),
+			CertAuthorities: [][]byte{[]byte("mock CA")},
+			TargetAddr:      "mock addr",
+		},
+	}
+	f := &Forwarder{
+		ForwarderConfig: ForwarderConfig{
+			Keygen: testauthority.New(),
+			Client: cl,
+		},
+	}
+	user, err := services.NewUser("bob")
+	c.Assert(err, check.IsNil)
+	ctx := authContext{
+		cluster: cluster{
+			RemoteSite: mockRemoteSite{name: "site a"},
+		},
+		AuthContext: auth.AuthContext{
+			User: user,
+			Identity: tlsca.Identity{
+				Username:         "bob",
+				Groups:           []string{"group a", "group b"},
+				Usage:            []string{"usage a", "usage b"},
+				Principals:       []string{"principal a", "principal b"},
+				KubernetesGroups: []string{"k8s group a", "k8s group b"},
+				Traits:           map[string][]string{"trait a": []string{"b", "c"}},
+			},
+		},
+	}
+
+	b, err := f.requestCertificate(ctx)
+	c.Assert(err, check.IsNil)
+	// All fields except b.key are predictable.
+	c.Assert(b.cert, check.DeepEquals, cl.csrResp.Cert)
+	c.Assert(b.certAuthorities, check.DeepEquals, cl.csrResp.CertAuthorities)
+	c.Assert(b.targetAddr, check.DeepEquals, cl.csrResp.TargetAddr)
+
+	// Check the KubeCSR fields.
+	c.Assert(cl.gotCSR.Username, check.DeepEquals, ctx.User.GetName())
+	c.Assert(cl.gotCSR.ClusterName, check.DeepEquals, ctx.cluster.GetName())
+
+	// Parse x509 CSR and check the subject.
+	csrBlock, _ := pem.Decode(cl.gotCSR.CSR)
+	c.Assert(csrBlock, check.NotNil)
+	csr, err := x509.ParseCertificateRequest(csrBlock.Bytes)
+	c.Assert(err, check.IsNil)
+	idFromCSR, err := tlsca.FromSubject(csr.Subject, time.Time{})
+	c.Assert(err, check.IsNil)
+	c.Assert(idFromCSR, check.DeepEquals, ctx.Identity)
+}
+
+// mockClient to intercept ProcessKubeCSR requests, record them and return a
+// stub response.
+type mockClient struct {
+	auth.ClientI
+
+	csrResp auth.KubeCSRResponse
+	gotCSR  auth.KubeCSR
+}
+
+func (c *mockClient) ProcessKubeCSR(csr auth.KubeCSR) (*auth.KubeCSRResponse, error) {
+	c.gotCSR = csr
+	return &c.csrResp, nil
+}
+
+// mockRemoteSite is a reversetunnel.RemoteSite implementation with hardcoded
+// name, because there's no easy way to construct a real
+// reversetunnel.RemoteSite.
+type mockRemoteSite struct {
+	reversetunnel.RemoteSite
+	name string
+}
+
+func (s mockRemoteSite) GetName() string {
+	return s.name
+}