Skip to content

Commit

Permalink
mfa: per-session MFA certs for SSH and Kubernetes
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Andrew Lytvynov committed Mar 10, 2021
1 parent eee051c commit 79a49d4
Show file tree
Hide file tree
Showing 13 changed files with 992 additions and 337 deletions.
774 changes: 495 additions & 279 deletions api/client/proto/authservice.pb.go

Large diffs are not rendered by default.

7 changes: 7 additions & 0 deletions api/client/proto/authservice.proto
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,7 @@ message UserSingleUseCertsResponse {
oneof Response {
MFAAuthenticateChallenge MFAChallenge = 1;
SingleUseUserCert Cert = 2;
SingleUseUserCertNotNeeded NotNeeded = 3;
}
}

Expand All @@ -719,6 +720,12 @@ message SingleUseUserCert {
}
}

// SingleUseUserCertNotNeeded is a flag message to indicate that the user can
// access a resource (ssh/kube/db/app) without the MFA challenge.
//
// It is used to avoid checking whether MFA is needed client-side.
message SingleUseUserCertNotNeeded {}

// AuthService is authentication/authorization service implementation
service AuthService {
// SendKeepAlives allows node to send a stream of keep alive requests
Expand Down
117 changes: 111 additions & 6 deletions lib/auth/grpcserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package auth
import (
"context"
"crypto/tls"
"errors"
"io"
"net"
"time"
Expand All @@ -30,6 +31,7 @@ import (
"github.com/gravitational/teleport/api/types"
apievents "github.com/gravitational/teleport/api/types/events"
"github.com/gravitational/teleport/lib/auth/u2f"
"github.com/gravitational/teleport/lib/defaults"
"github.com/gravitational/teleport/lib/events"
"github.com/gravitational/teleport/lib/httplib"
"github.com/gravitational/teleport/lib/jwt"
Expand All @@ -40,7 +42,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"
Expand Down Expand Up @@ -1622,7 +1623,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)
}
Expand All @@ -1644,7 +1646,18 @@ 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 {
err = validateUserSingleUseCertRequest(ctx, actx, initReq)
if err == errUserSingleUseUserCertNotNeeded {
// Tell the client that MFA is not needed and they can use their
// regular login cert.
if err := stream.Send(&proto.UserSingleUseCertsResponse{
Response: &proto.UserSingleUseCertsResponse_NotNeeded{NotNeeded: &proto.SingleUseUserCertNotNeeded{}},
}); err != nil {
return trail.ToGRPC(err)
}
return nil
}
if err != nil {
return trail.ToGRPC(err)
}

Expand All @@ -1656,7 +1669,7 @@ func (g *GRPCServer) GenerateUserSingleUseCerts(stream proto.AuthService_Generat
}

// Generate the cert.
respCert, err := userSingleUseCertsGenerate(stream.Context(), actx, *initReq, mfaDev)
respCert, err := userSingleUseCertsGenerate(ctx, actx, *initReq, mfaDev)
if err != nil {
return trail.ToGRPC(err)
}
Expand All @@ -1670,26 +1683,115 @@ func (g *GRPCServer) GenerateUserSingleUseCerts(stream proto.AuthService_Generat
return nil
}

func validateUserSingleUseCertRequest(req *proto.UserCertsRequest, clock clockwork.Clock) error {
var errUserSingleUseUserCertNotNeeded = errors.New("single-use user certificate not needed to access this resource")

// validateUserSingleUseCertRequest validates the request for a single-use user
// cert. Returns nil if validation succeeds and MFA check is required, or
// errUserSingleUseUserCertNotNeeded if validation succeeds and MFA
// authentication is not required for this session. Any other error means
// failed validation.
func validateUserSingleUseCertRequest(ctx context.Context, actx *grpcContext, req *proto.UserCertsRequest) error {
if req.Username != actx.User.GetName() {
return trace.BadParameter("cannot request a single-use certificate for a different user")
}

notFoundErr := trace.NotFound("target resource not found")
var noMFAAccessErr error
switch req.Usage {
case proto.UserCertsRequest_SSH:
if req.NodeName == "" {
return trace.BadParameter("missing NodeName field in a ssh-only UserCertsRequest")
}
// Find the target node and check whether MFA is required.
nodes, err := actx.authServer.GetNodes(defaults.Namespace, services.SkipValidation())
if err != nil {
return trace.Wrap(err)
}
var node 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() == req.NodeName || n.GetHostname() == req.NodeName || addr == req.NodeName {
node = n
break
}
}
if node == nil {
return notFoundErr
}
noMFAAccessErr = actx.context.Checker.CheckAccessToServer(req.Username, node, false)

case proto.UserCertsRequest_Kubernetes:
if req.KubernetesCluster == "" {
return trace.BadParameter("missing KubernetesCluster field in a kubernetes-only UserCertsRequest")
}
// Find the target cluster and check whether MFA is required.
svcs, err := actx.authServer.GetKubeServices(ctx)
if err != nil {
return trace.Wrap(err)
}
var cluster *types.KubernetesCluster
outer:
for _, svc := range svcs {
for _, c := range svc.GetKubernetesClusters() {
if c.Name == req.KubernetesCluster {
cluster = c
break outer
}
}
}
if cluster == nil {
return notFoundErr
}
noMFAAccessErr = actx.context.Checker.CheckAccessToKubernetes(defaults.Namespace, cluster, false)

case proto.UserCertsRequest_Database:
if req.RouteToDatabase.ServiceName == "" {
return trace.BadParameter("missing ServiceName field in a database-only UserCertsRequest")
}
dbs, err := actx.authServer.GetDatabaseServers(ctx, defaults.Namespace, services.SkipValidation())
if err != nil {
return trace.Wrap(err)
}
var db types.DatabaseServer
for _, d := range dbs {
if d.GetName() == req.RouteToDatabase.ServiceName {
db = d
break
}
}
if db == nil {
return notFoundErr
}
noMFAAccessErr = actx.context.Checker.CheckAccessToDatabase(db, false)

case proto.UserCertsRequest_All:
return trace.BadParameter("must specify a concrete Usage in UserCertsRequest, one of SSH, Kubernetes or Database")
default:
return trace.BadParameter("unknown certificate Usage %q", req.Usage)
}
maxExpiry := clock.Now().Add(teleport.UserSingleUseCertTTL)
// No error means that MFA is not required for this resource by
// AccessChecker.
if noMFAAccessErr == nil {
return errUserSingleUseUserCertNotNeeded
}
// Errors other than ErrSessionMFARequired mean something else is wrong,
// most likely access denied.
if noMFAAccessErr != services.ErrSessionMFARequired {
if trace.IsAccessDenied(noMFAAccessErr) {
// Mask access denied errors to prevent resource name oracles.
return notFoundErr
}
return trace.Wrap(noMFAAccessErr)
}
// If we reach here, the error from AccessChecker was
// ErrSessionMFARequired, proceed with MFA challenge.

maxExpiry := actx.authServer.GetClock().Now().Add(teleport.UserSingleUseCertTTL)
if req.Expires.After(maxExpiry) {
req.Expires = maxExpiry
}
Expand All @@ -1709,6 +1811,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 {
Expand Down
99 changes: 96 additions & 3 deletions lib/auth/grpcserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -550,12 +550,24 @@ func TestGenerateUserSingleUseCert(t *testing.T) {
U2F: &types.U2F{
AppID: "teleport",
Facets: []string{"teleport"},
},
})
}})
require.NoError(t, err)
err = srv.Auth().SetAuthPreference(authPref)
require.NoError(t, err)

// Register an SSH node.
node := &types.ServerV2{
Kind: types.KindKubeService,
Version: types.V2,
Metadata: types.Metadata{
Name: "node-a",
},
Spec: types.ServerSpecV2{
Hostname: "node-a",
},
}
_, err = srv.Auth().UpsertNode(node)
require.NoError(t, err)
// Register a k8s cluster.
k8sSrv := &types.ServerV2{
Kind: types.KindKubeService,
Expand All @@ -569,9 +581,24 @@ func TestGenerateUserSingleUseCert(t *testing.T) {
}
err = srv.Auth().UpsertKubeService(ctx, k8sSrv)
require.NoError(t, err)
// Register a database.
db := types.NewDatabaseServerV3("db-a", nil, types.DatabaseServerSpecV3{
Protocol: "postgres",
URI: "localhost",
Hostname: "localhost",
HostID: "localhost",
})
_, err = srv.Auth().UpsertDatabaseServer(ctx, db)
require.NoError(t, err)

// Create a fake user.
user, _, err := CreateUserAndRole(srv.Auth(), "mfa-user", []string{"role"})
user, role, err := CreateUserAndRole(srv.Auth(), "mfa-user", []string{"role"})
require.NoError(t, err)
// Make sure MFA is required for this user.
roleOpt := role.GetOptions()
roleOpt.RequireSessionMFA = true
role.SetOptions(roleOpt)
err = srv.Auth().UpsertRole(ctx, role)
require.NoError(t, err)
cl, err := srv.NewClient(TestUser(user.GetName()))
require.NoError(t, err)
Expand Down Expand Up @@ -836,3 +863,69 @@ func testGenerateUserSingleUseCert(ctx context.Context, t *testing.T, cl *Client

require.NoError(t, stream.CloseSend())
}

func TestGenerateUserSingleUseCertMFANotRequired(t *testing.T) {
ctx := context.Background()
srv := newTestTLSServer(t)
clock := srv.Clock()

// Enable MFA support.
authPref, err := services.NewAuthPreference(types.AuthPreferenceSpecV2{
Type: teleport.Local,
SecondFactor: constants.SecondFactorOptional,
U2F: &types.U2F{
AppID: "teleport",
Facets: []string{"teleport"},
},
})
require.NoError(t, err)
err = srv.Auth().SetAuthPreference(authPref)
require.NoError(t, err)

// Register an SSH node.
node := &types.ServerV2{
Kind: types.KindKubeService,
Version: types.V2,
Metadata: types.Metadata{
Name: "node-a",
},
Spec: types.ServerSpecV2{
Hostname: "node-a",
},
}
_, err = srv.Auth().UpsertNode(node)
require.NoError(t, err)

// Create a fake user.
user, role, err := CreateUserAndRole(srv.Auth(), "mfa-user", []string{"role"})
require.NoError(t, err)
// Make sure MFA is NOT required for this user.
roleOpt := role.GetOptions()
roleOpt.RequireSessionMFA = false
role.SetOptions(roleOpt)
err = srv.Auth().UpsertRole(ctx, role)
require.NoError(t, err)

cl, err := srv.NewClient(TestUser(user.GetName()))
require.NoError(t, err)

_, pub, err := srv.Auth().GenerateKeyPair("")
require.NoError(t, err)

stream, err := cl.GenerateUserSingleUseCerts(ctx)
require.NoError(t, err)
err = stream.Send(&proto.UserSingleUseCertsRequest{Request: &proto.UserSingleUseCertsRequest_Init{
Init: &proto.UserCertsRequest{
PublicKey: pub,
Username: user.GetName(),
Expires: clock.Now().Add(teleport.UserSingleUseCertTTL),
Usage: proto.UserCertsRequest_SSH,
NodeName: "node-a",
},
}})
require.NoError(t, err)

resp, err := stream.Recv()
require.NoError(t, err)
require.NotNil(t, resp.GetNotNeeded())
}
2 changes: 1 addition & 1 deletion lib/auth/native/native.go
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ func (k *Keygen) GenerateHostCertWithoutValidation(c services.HostCertParams) ([
// GenerateUserCert generates a user certificate with the passed in parameters.
// The private key of the CA to sign the certificate must be provided.
func (k *Keygen) GenerateUserCert(c services.UserCertParams) ([]byte, error) {
if err := c.Check(); err != nil {
if err := c.CheckAndSetDefaults(); err != nil {
return nil, trace.Wrap(err, "error validating UserCertParams")
}
return k.GenerateUserCertWithoutValidation(c)
Expand Down
Loading

0 comments on commit 79a49d4

Please sign in to comment.