mfa: per-session MFA certs for SSH and Kubernetes (#5564)

* mfa: per-session MFA certs for SSH and Kubernetes

This is client-side support for requesting single-use certs with an MFA
check.

The client doesn't know whether they need MFA check when accessing a
resource, this is decided during an RBAC check on the server. So a
client will always try to get a single-use cert, and the server will
respond with NotNeeded if MFA is not required. This is an extra
round-trip for every session which causes ~20% slowdown in SSH logins:

```
$ hyperfine '/tmp/tsh-old ssh talos date' '/tmp/tsh-new ssh talos date'
Benchmark #1: /tmp/tsh-old ssh talos date
  Time (mean ± σ):      49.9 ms ±   1.0 ms    [User: 15.1 ms, System: 7.4 ms]
  Range (min … max):    48.4 ms …  54.1 ms    59 runs

Benchmark #2: /tmp/tsh-new ssh talos date
  Time (mean ± σ):      60.2 ms ±   1.6 ms    [User: 19.1 ms, System: 8.3 ms]
  Range (min … max):    59.0 ms …  69.7 ms    50 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet PC without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

Summary
  '/tmp/tsh-old ssh talos date' ran
    1.21 ± 0.04 times faster than '/tmp/tsh-new ssh talos date'
```

Another few other internal changes:

- client.LocalKeyAgent will now always have a non-nil LocalKeyStore.
  Previously, it would be nil (e.g. in a web UI handler or when using an
  identity file) which easily causes panics. I added a noLocalKeyStore
  type instead that returns errors from all methods.

- requesting a user cert with a TTL < 1min will now succeed and return a
  1min cert instead of failing

* Capture access approvals on MFA-issued certs

* Address review feedback

* Address review feedback

* mfa: accept unknown nodes during short-term MFA cert creation

An unknown node could be an OpenSSH node set up via
https://goteleport.com/teleport/docs/openssh-teleport/

In this case, we shouldn't prevent the user from connecting.

There's a small risk of authz bypass - an attacker might know a
different name/IP for a registered node which Teleport doesn't know
about. But a Teleport node will still check RBAC and reject the
connection.

* Validate username against unmapped user identity

IssueUserCertsWithMFA is called on the leaf auth server in case of
trusted clusters. Username in the request object will be that of the
original unmapped caller.

* mfa: add IsMFARequired RPC

This RPC is ran before every connection to check whether MFA is
required. If a connection is against the leaf cluster, this request is
forwarded from root to leaf for evaluation.

* Fix integration tests

* Correctly treat "Username" as login name in IsMFARequired

Also, move the logic into auth.Server out of ServerWithRoles.

* Fix TestHA

* Address review feedback

api/client/client.go

@@ -117,7 +117,7 @@ func (c *Client) grpcDialer() grpcDialer {
 		}
 		conn, err := c.dialer.DialContext(ctx, "tcp", addr)
 		if err != nil {
-			return nil, trace.ConnectionProblem(err, "failed to dial")
+			return nil, trace.ConnectionProblem(err, "failed to dial: %v", err)
 		}
 		return conn, nil
 	}
@@ -1063,3 +1063,11 @@ func (c *Client) DeleteToken(ctx context.Context, name string) error {
 	_, err := c.grpc.DeleteToken(ctx, &types.ResourceRequest{Name: name})
 	return trail.FromGRPC(err)
 }
+
+func (c *Client) IsMFARequired(ctx context.Context, req *proto.IsMFARequiredRequest) (*proto.IsMFARequiredResponse, error) {
+	resp, err := c.grpc.IsMFARequired(ctx, req)
+	if err != nil {
+		return nil, trail.FromGRPC(err)
+	}
+	return resp, nil
+}

api/client/proto/authservice.proto

@@ -745,6 +745,30 @@ message UserSingleUseCertsResponse {
     }
 }
 
+// IsMFARequiredRequest is a request to check whether MFA is required to access
+// the Target.
+message IsMFARequiredRequest {
+    oneof Target {
+        // KubernetesCluster specifies the target kubernetes cluster.
+        string KubernetesCluster = 1;
+        // RouteToDatabase specifies the target database proxy name.
+        RouteToDatabase Database = 2;
+        // Node specifies the target SSH node.
+        NodeLogin Node = 3;
+    }
+}
+
+// NodeLogin specifies an SSH node and OS login.
+message NodeLogin {
+    // Node can be node's hostname or UUID.
+    string Node = 1;
+    // Login is the OS login name.
+    string Login = 2;
+}
+
+// IsMFARequiredResponse is a response for MFA requirement check.
+message IsMFARequiredResponse { bool Required = 1; }
+
 // SingleUseUserCert is a single-use user certificate, either SSH or TLS.
 message SingleUseUserCert {
     oneof Cert {
@@ -767,7 +791,9 @@ service AuthService {
     // certificates.
     rpc GenerateUserSingleUseCerts(stream UserSingleUseCertsRequest)
         returns (stream UserSingleUseCertsResponse);
-
+    // IsMFARequired checks whether MFA is required to access the specified
+    // target.
+    rpc IsMFARequired(IsMFARequiredRequest) returns (IsMFARequiredResponse);
     // GetAccessRequests gets all pending access requests.
     rpc GetAccessRequests(types.AccessRequestFilter) returns (AccessRequests);
     // CreateAccessRequest creates a new access request.

integration/integration_test.go

@@ -1576,8 +1576,19 @@ func (s *IntSuite) TestHA(c *check.C) {
 	c.Assert(err, check.IsNil)
 	c.Assert(output.String(), check.Equals, "hello world\n")
 
-	// stop auth server a now
+	// Stop cluster "a" to force existing tunnels to close.
 	c.Assert(a.StopAuth(true), check.IsNil)
+	// Restart cluster "a".
+	c.Assert(a.Reset(), check.IsNil)
+	c.Assert(a.Start(), check.IsNil)
+	// Wait for the tunnels to reconnect.
+	abortTime = time.Now().Add(time.Second * 10)
+	for len(checkGetClusters(c, a.Tunnel)) < 2 && len(checkGetClusters(c, b.Tunnel)) < 2 {
+		time.Sleep(time.Millisecond * 2000)
+		if time.Now().After(abortTime) {
+			c.Fatalf("two sites do not see each other: tunnels are not working")
+		}
+	}
 
 	// try to execute an SSH command using the same old client to site-B
 	// "site-A" and "site-B" reverse tunnels are supposed to reconnect,

lib/auth/auth.go

@@ -28,8 +28,10 @@ import (
 	"crypto"
 	"crypto/subtle"
 	"encoding/base64"
+	"errors"
 	"fmt"
 	"math/rand"
+	"net"
 	"net/url"
 	"strings"
 	"sync"
@@ -1993,6 +1995,131 @@ func (a *Server) GetDatabaseServers(ctx context.Context, namespace string, opts
 	return a.GetCache().GetDatabaseServers(ctx, namespace, opts...)
 }
 
+func (a *Server) isMFARequired(ctx context.Context, checker services.AccessChecker, req *proto.IsMFARequiredRequest) (*proto.IsMFARequiredResponse, error) {
+	var noMFAAccessErr, notFoundErr error
+	switch t := req.Target.(type) {
+	case *proto.IsMFARequiredRequest_Node:
+		notFoundErr = trace.NotFound("node %q not found", t.Node)
+		if t.Node.Node == "" {
+			return nil, trace.BadParameter("empty Node field")
+		}
+		if t.Node.Login == "" {
+			return nil, trace.BadParameter("empty Login field")
+		}
+		// Find the target node and check whether MFA is required.
+		nodes, err := a.GetNodes(defaults.Namespace, services.SkipValidation())
+		if err != nil {
+			return nil, trace.Wrap(err)
+		}
+		var matches []types.Server
+		for _, n := range nodes {
+			// Get the server address without port number.
+			addr, _, err := net.SplitHostPort(n.GetAddr())
+			if err != nil {
+				addr = n.GetAddr()
+			}
+			// Match NodeName to UUID, hostname or self-reported server address.
+			if n.GetName() == t.Node.Node || n.GetHostname() == t.Node.Node || addr == t.Node.Node {
+				matches = append(matches, n)
+			}
+		}
+		if len(matches) == 0 {
+			// If t.Node.Node is not a known registered node, it may be an
+			// unregistered host running OpenSSH with a certificate created via
+			// `tctl auth sign`. In these cases, let the user through without
+			// extra checks.
+			//
+			// If t.Node.Node turns out to be an alias for a real node (e.g.
+			// private network IP), and MFA check was actually required, the
+			// Node itself will check the cert extensions and reject the
+			// connection.
+			return &proto.IsMFARequiredResponse{Required: false}, nil
+		}
+		// Check RBAC against all matching nodes and return the first error.
+		// If at least one node requires MFA, we'll catch it.
+		for _, n := range matches {
+			err := checker.CheckAccessToServer(t.Node.Login, n, false)
+			if err != nil {
+				noMFAAccessErr = err
+				break
+			}
+		}
+
+	case *proto.IsMFARequiredRequest_KubernetesCluster:
+		notFoundErr = trace.NotFound("kubernetes cluster %q not found", t.KubernetesCluster)
+		if t.KubernetesCluster == "" {
+			return nil, trace.BadParameter("missing KubernetesCluster field in a kubernetes-only UserCertsRequest")
+		}
+		// Find the target cluster and check whether MFA is required.
+		svcs, err := a.GetKubeServices(ctx)
+		if err != nil {
+			return nil, trace.Wrap(err)
+		}
+		var cluster *types.KubernetesCluster
+	outer:
+		for _, svc := range svcs {
+			for _, c := range svc.GetKubernetesClusters() {
+				if c.Name == t.KubernetesCluster {
+					cluster = c
+					break outer
+				}
+			}
+		}
+		if cluster == nil {
+			return nil, trace.Wrap(notFoundErr)
+		}
+		noMFAAccessErr = checker.CheckAccessToKubernetes(defaults.Namespace, cluster, false)
+
+	case *proto.IsMFARequiredRequest_Database:
+		notFoundErr = trace.NotFound("database service %q not found", t.Database.ServiceName)
+		if t.Database.ServiceName == "" {
+			return nil, trace.BadParameter("missing ServiceName field in a database-only UserCertsRequest")
+		}
+		dbs, err := a.GetDatabaseServers(ctx, defaults.Namespace, services.SkipValidation())
+		if err != nil {
+			return nil, trace.Wrap(err)
+		}
+		var db types.DatabaseServer
+		for _, d := range dbs {
+			if d.GetName() == t.Database.ServiceName {
+				db = d
+				break
+			}
+		}
+		if db == nil {
+			return nil, trace.Wrap(notFoundErr)
+		}
+		noMFAAccessErr = checker.CheckAccessToDatabase(db, false)
+
+	default:
+		return nil, trace.BadParameter("unknown Target %T", req.Target)
+	}
+	// No error means that MFA is not required for this resource by
+	// AccessChecker.
+	if noMFAAccessErr == nil {
+		return &proto.IsMFARequiredResponse{Required: false}, nil
+	}
+	// Errors other than ErrSessionMFARequired mean something else is wrong,
+	// most likely access denied.
+	if !errors.Is(noMFAAccessErr, services.ErrSessionMFARequired) {
+		if trace.IsAccessDenied(noMFAAccessErr) {
+			log.Infof("Access to resource denied: %v", noMFAAccessErr)
+			// notFoundErr should always be set by this point, but check it
+			// just in case.
+			if notFoundErr == nil {
+				notFoundErr = trace.NotFound("target resource not found")
+			}
+			// Mask access denied errors to prevent resource name oracles.
+			return nil, trace.Wrap(notFoundErr)
+		}
+		return nil, trace.Wrap(noMFAAccessErr)
+	}
+	// If we reach here, the error from AccessChecker was
+	// ErrSessionMFARequired.
+
+	return &proto.IsMFARequiredResponse{Required: true}, nil
+}
+
 // mfaAuthChallenge constructs an MFAAuthenticateChallenge for all MFA devices
 // registered by the user.
 func (a *Server) mfaAuthChallenge(ctx context.Context, user string, u2fStorage u2f.AuthenticationStorage) (*proto.MFAAuthenticateChallenge, error) {

lib/auth/auth_with_roles.go

@@ -66,7 +66,7 @@ func (a *ServerWithRoles) action(namespace string, resource string, action strin
 // even if they are not admins, e.g. update their own passwords,
 // or generate certificates, otherwise it will require admin privileges
 func (a *ServerWithRoles) currentUserAction(username string) error {
-	if a.hasLocalUserRole(a.context.Checker) && username == a.context.User.GetName() {
+	if hasLocalUserRole(a.context.Checker) && username == a.context.User.GetName() {
 		return nil
 	}
 	return utils.OpaqueAccessDenied(
@@ -120,12 +120,17 @@ func (a *ServerWithRoles) hasRemoteBuiltinRole(name string) bool {
 	return true
 }
 
+// hasRemoteUserRole checks if the type of the role set is a remote user or
+// not.
+func hasRemoteUserRole(checker services.AccessChecker) bool {
+	_, ok := checker.(RemoteUserRoleSet)
+	return ok
+}
+
 // hasLocalUserRole checks if the type of the role set is a local user or not.
-func (a *ServerWithRoles) hasLocalUserRole(checker services.AccessChecker) bool {
-	if _, ok := checker.(LocalUserRoleSet); !ok {
-		return false
-	}
-	return true
+func hasLocalUserRole(checker services.AccessChecker) bool {
+	_, ok := checker.(LocalUserRoleSet)
+	return ok
 }
 
 // AuthenticateWebUser authenticates web user, creates and returns a web session
@@ -2680,6 +2685,13 @@ func (a *ServerWithRoles) GenerateUserSingleUseCerts(ctx context.Context) (proto
 	return nil, trace.NotImplemented("bug: GenerateUserSingleUseCerts must not be called on auth.ServerWithRoles")
 }
 
+func (a *ServerWithRoles) IsMFARequired(ctx context.Context, req *proto.IsMFARequiredRequest) (*proto.IsMFARequiredResponse, error) {
+	if !hasLocalUserRole(a.context.Checker) && !hasRemoteUserRole(a.context.Checker) {
+		return nil, trace.AccessDenied("only a user role can call IsMFARequired, got %T", a.context.Checker)
+	}
+	return a.authServer.isMFARequired(ctx, a.context.Checker, req)
+}
+
 // NewAdminAuthServer returns auth server authorized as admin,
 // used for auth server cached access
 func NewAdminAuthServer(authServer *Server, sessions session.Service, alog events.IAuditLog) (ClientI, error) {

lib/auth/clt.go

@@ -2524,6 +2524,10 @@ type IdentityService interface {
 	// (https://github.com/gravitational/teleport/blob/3a1cf9111c2698aede2056513337f32bfc16f1f1/rfd/0014-session-2FA.md#sessions).
 	GenerateUserSingleUseCerts(ctx context.Context) (proto.AuthService_GenerateUserSingleUseCertsClient, error)
 
+	// IsMFARequiredRequest is a request to check whether MFA is required to
+	// access the Target.
+	IsMFARequired(ctx context.Context, req *proto.IsMFARequiredRequest) (*proto.IsMFARequiredResponse, error)
+
 	// DeleteAllUsers deletes all users
 	DeleteAllUsers() error
 

lib/auth/grpcserver.go

@@ -40,7 +40,6 @@ import (
 	"github.com/golang/protobuf/ptypes/empty"
 	"github.com/gravitational/trace"
 	"github.com/gravitational/trace/trail"
-	"github.com/jonboulle/clockwork"
 	"github.com/sirupsen/logrus"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
@@ -1695,7 +1694,8 @@ func (g *GRPCServer) GetMFADevices(ctx context.Context, req *proto.GetMFADevices
 }
 
 func (g *GRPCServer) GenerateUserSingleUseCerts(stream proto.AuthService_GenerateUserSingleUseCertsServer) error {
-	actx, err := g.authenticate(stream.Context())
+	ctx := stream.Context()
+	actx, err := g.authenticate(ctx)
 	if err != nil {
 		return trail.ToGRPC(err)
 	}
@@ -1717,20 +1717,23 @@ func (g *GRPCServer) GenerateUserSingleUseCerts(stream proto.AuthService_Generat
 	if initReq == nil {
 		return trail.ToGRPC(trace.BadParameter("expected UserCertsRequest, got %T", req.Request))
 	}
-	if err := validateUserSingleUseCertRequest(initReq, g.AuthServer.GetClock()); err != nil {
+	if err := validateUserSingleUseCertRequest(ctx, actx, initReq); err != nil {
+		g.Entry.Debugf("Validation of single-use cert request failed: %v", err)
 		return trail.ToGRPC(err)
 	}
 
 	// 2. send MFAChallenge
 	// 3. receive and validate MFAResponse
 	mfaDev, err := userSingleUseCertsAuthChallenge(actx, stream)
 	if err != nil {
+		g.Entry.Debugf("Failed to perform single-use cert challenge: %v", err)
 		return trail.ToGRPC(err)
 	}
 
 	// Generate the cert.
-	respCert, err := userSingleUseCertsGenerate(stream.Context(), actx, *initReq, mfaDev)
+	respCert, err := userSingleUseCertsGenerate(ctx, actx, *initReq, mfaDev)
 	if err != nil {
+		g.Entry.Warningf("Failed to generate single-use cert: %v", err)
 		return trail.ToGRPC(err)
 	}
 
@@ -1743,7 +1746,13 @@ func (g *GRPCServer) GenerateUserSingleUseCerts(stream proto.AuthService_Generat
 	return nil
 }
 
-func validateUserSingleUseCertRequest(req *proto.UserCertsRequest, clock clockwork.Clock) error {
+// validateUserSingleUseCertRequest validates the request for a single-use user
+// cert.
+func validateUserSingleUseCertRequest(ctx context.Context, actx *grpcContext, req *proto.UserCertsRequest) error {
+	if err := actx.currentUserAction(req.Username); err != nil {
+		return trace.Wrap(err)
+	}
+
 	switch req.Usage {
 	case proto.UserCertsRequest_SSH:
 		if req.NodeName == "" {
@@ -1762,7 +1771,8 @@ func validateUserSingleUseCertRequest(req *proto.UserCertsRequest, clock clockwo
 	default:
 		return trace.BadParameter("unknown certificate Usage %q", req.Usage)
 	}
-	maxExpiry := clock.Now().Add(teleport.UserSingleUseCertTTL)
+
+	maxExpiry := actx.authServer.GetClock().Now().Add(teleport.UserSingleUseCertTTL)
 	if req.Expires.After(maxExpiry) {
 		req.Expires = maxExpiry
 	}
@@ -1782,6 +1792,9 @@ func userSingleUseCertsAuthChallenge(gctx *grpcContext, stream proto.AuthService
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
+	if authChallenge.TOTP == nil && len(authChallenge.U2F) == 0 {
+		return nil, trace.AccessDenied("MFA is required to access this resource but user has no MFA devices; use 'tsh mfa add' to register MFA devices")
+	}
 	if err := stream.Send(&proto.UserSingleUseCertsResponse{
 		Response: &proto.UserSingleUseCertsResponse_MFAChallenge{MFAChallenge: authChallenge},
 	}); err != nil {
@@ -2167,6 +2180,18 @@ func (g *GRPCServer) DeleteToken(ctx context.Context, req *types.ResourceRequest
 	return &empty.Empty{}, nil
 }
 
+func (g *GRPCServer) IsMFARequired(ctx context.Context, req *proto.IsMFARequiredRequest) (*proto.IsMFARequiredResponse, error) {
+	actx, err := g.authenticate(ctx)
+	if err != nil {
+		return nil, trail.ToGRPC(err)
+	}
+	resp, err := actx.IsMFARequired(ctx, req)
+	if err != nil {
+		return nil, trail.ToGRPC(err)
+	}
+	return resp, nil
+}
+
 type grpcContext struct {
 	*Context
 	*ServerWithRoles