From 08eaba07df10bced986b10ae5191b5aa6efbef06 Mon Sep 17 00:00:00 2001 From: rosstimothy <39066650+rosstimothy@users.noreply.github.com> Date: Tue, 21 May 2024 15:42:43 -0400 Subject: [PATCH] Close expired gRPC connections if disconnect_expired_cert is set (#41826) Updates the transport credentials used by gRPC servers that require mTLS to enforce that connections are terminated when the client certificate expires if `disconnect_expired_cert == true`. To prevent session resumption from leaving open sessions established through the Proxy gRPC server the redial mechanism was updated to inspect for certificate expired errors and abort any future reconnection attempts. Partially addresses #1199. --- lib/auth/grpcserver.go | 1 + lib/auth/middleware_test.go | 17 ++-- lib/auth/transport_credentials.go | 104 +++++++++++++++++----- lib/auth/transport_credentials_test.go | 118 ++++++++++++++++++++++++- lib/authz/permissions.go | 27 ++++++ lib/authz/permissions_test.go | 63 +++++++++++++ lib/kube/proxy/forwarder.go | 2 +- lib/resumption/client.go | 8 ++ lib/service/service.go | 2 + lib/srv/desktop/windows_server.go | 2 +- lib/srv/monitor.go | 33 +------ lib/srv/monitor_test.go | 68 -------------- 12 files changed, 315 insertions(+), 130 deletions(-) diff --git a/lib/auth/grpcserver.go b/lib/auth/grpcserver.go index f6be672cc47..c9c72679b8a 100644 --- a/lib/auth/grpcserver.go +++ b/lib/auth/grpcserver.go @@ -5040,6 +5040,7 @@ func NewGRPCServer(cfg GRPCServerConfig) (*GRPCServer, error) { creds, err := NewTransportCredentials(TransportCredentialsConfig{ TransportCredentials: &httplib.TLSCreds{Config: cfg.TLS}, UserGetter: cfg.Middleware, + GetAuthPreference: cfg.AuthServer.Cache.GetAuthPreference, }) if err != nil { return nil, trace.Wrap(err) diff --git a/lib/auth/middleware_test.go b/lib/auth/middleware_test.go index 3e080ca2c1e..fd1365fc2a6 100644 --- a/lib/auth/middleware_test.go +++ b/lib/auth/middleware_test.go @@ -29,6 +29,7 @@ import ( "net/http" "net/http/httptest" "sync" + "sync/atomic" "testing" "time" @@ -664,12 +665,18 @@ func (h *fakeHTTPHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { type fakeConn struct { net.Conn + closed atomic.Bool } -func (f fakeConn) Close() error { +func (f *fakeConn) Close() error { + f.closed.CompareAndSwap(false, true) return nil } +func (f *fakeConn) RemoteAddr() net.Addr { + return &utils.NetAddr{} +} + func TestValidateClientVersion(t *testing.T) { cases := []struct { name string @@ -732,7 +739,7 @@ func TestValidateClientVersion(t *testing.T) { ctx = metadata.NewIncomingContext(ctx, metadata.New(map[string]string{"version": tt.clientVersion})) } - tt.errAssertion(t, tt.middleware.ValidateClientVersion(ctx, IdentityInfo{Conn: fakeConn{}, IdentityGetter: TestBuiltin(types.RoleNode).I})) + tt.errAssertion(t, tt.middleware.ValidateClientVersion(ctx, IdentityInfo{Conn: &fakeConn{}, IdentityGetter: TestBuiltin(types.RoleNode).I})) }) } } @@ -751,7 +758,7 @@ func TestRejectedClientClusterAlert(t *testing.T) { ctx := metadata.NewIncomingContext(context.Background(), metadata.New(map[string]string{ "version": semver.Version{Major: teleport.SemVersion.Major - 20}.String(), })) - err := mw.ValidateClientVersion(ctx, IdentityInfo{Conn: fakeConn{}, IdentityGetter: TestBuiltin(types.RoleNode).I}) + err := mw.ValidateClientVersion(ctx, IdentityInfo{Conn: &fakeConn{}, IdentityGetter: TestBuiltin(types.RoleNode).I}) assert.Error(t, err) // Validate a client with an unknown version, which should trigger an alert, however, @@ -759,7 +766,7 @@ func TestRejectedClientClusterAlert(t *testing.T) { ctx = metadata.NewIncomingContext(context.Background(), metadata.New(map[string]string{ "version": "abcd", })) - err = mw.ValidateClientVersion(ctx, IdentityInfo{Conn: fakeConn{}, IdentityGetter: TestBuiltin(types.RoleNode).I}) + err = mw.ValidateClientVersion(ctx, IdentityInfo{Conn: &fakeConn{}, IdentityGetter: TestBuiltin(types.RoleNode).I}) assert.Error(t, err) // Assert that only a single alert was created based on the above rejections. @@ -777,7 +784,7 @@ func TestRejectedClientClusterAlert(t *testing.T) { wg.Add(1) go func() { defer wg.Done() - err := mw.ValidateClientVersion(ctx, IdentityInfo{Conn: fakeConn{}, IdentityGetter: TestBuiltin(types.RoleNode).I}) + err := mw.ValidateClientVersion(ctx, IdentityInfo{Conn: &fakeConn{}, IdentityGetter: TestBuiltin(types.RoleNode).I}) assert.Error(t, err) }() } diff --git a/lib/auth/transport_credentials.go b/lib/auth/transport_credentials.go index 082153e9dd9..8fe628f8a06 100644 --- a/lib/auth/transport_credentials.go +++ b/lib/auth/transport_credentials.go @@ -23,10 +23,13 @@ import ( "crypto/tls" "io" "net" + "time" "github.com/gravitational/trace" + "github.com/jonboulle/clockwork" "google.golang.org/grpc/credentials" + "github.com/gravitational/teleport/api/types" apievents "github.com/gravitational/teleport/api/types/events" "github.com/gravitational/teleport/lib/authz" ) @@ -78,6 +81,12 @@ type TransportCredentialsConfig struct { // of active connections is within the limit. If not set then no connection // limits are enforced. Enforcer ConnectionEnforcer + // Clock used to tell time. + Clock clockwork.Clock + // GetAuthPreference is used to retrieve the auth preference per connection + // to determine if connections should be terminated as soon as the client + // certificate has expired. + GetAuthPreference func(ctx context.Context) (types.AuthPreference, error) } // Check validates that the configuration is valid for use and @@ -105,9 +114,11 @@ func (c *TransportCredentialsConfig) Check() error { type TransportCredentials struct { credentials.TransportCredentials - userGetter UserGetter - authorizer authz.Authorizer - enforcer ConnectionEnforcer + userGetter UserGetter + authorizer authz.Authorizer + enforcer ConnectionEnforcer + getAuthPreference func(context.Context) (types.AuthPreference, error) + clock clockwork.Clock } // NewTransportCredentials returns a new TransportCredentials @@ -116,11 +127,25 @@ func NewTransportCredentials(cfg TransportCredentialsConfig) (*TransportCredenti return nil, trace.Wrap(err) } + getAuthPreference := func(context.Context) (types.AuthPreference, error) { + return types.DefaultAuthPreference(), nil + } + if cfg.GetAuthPreference != nil { + getAuthPreference = cfg.GetAuthPreference + } + + clock := clockwork.NewRealClock() + if cfg.Clock != nil { + clock = cfg.Clock + } + return &TransportCredentials{ TransportCredentials: cfg.TransportCredentials, userGetter: cfg.UserGetter, authorizer: cfg.Authorizer, enforcer: cfg.Enforcer, + getAuthPreference: getAuthPreference, + clock: clock, }, nil } @@ -143,38 +168,78 @@ type IdentityInfo struct { Conn net.Conn } -// ServerHandshake does the authentication handshake for servers. It returns -// the authenticated connection and the corresponding auth information about -// the connection. -// At minimum the TLS handshake is performed and the identity is built from +// timeoutConn wraps a connection that is to be closed when +// the timer expires. +type timeoutConn struct { + net.Conn // The underlying [net.Conn] of the gRPC connection. + timer clockwork.Timer +} + +// newTimeoutConn creates a [net.Conn] wrapper that closes the rawConn +// if the timeout is exceeded. +func newTimeoutConn(conn net.Conn, clock clockwork.Clock, expires time.Time) (net.Conn, error) { + if expires.IsZero() { + return conn, nil + } + + return &timeoutConn{ + Conn: conn, + timer: clock.AfterFunc(expires.Sub(clock.Now()), func() { conn.Close() }), + }, nil +} + +// Close closes the wrapped [net.Conn] and stops the timer +// to prevent leaking it. +func (c *timeoutConn) Close() error { + c.timer.Stop() + return trace.Wrap(c.Conn.Close()) +} + +// ServerHandshake performs the authentication handshake for servers as per +// the [credentials.TransportCredentials] interface. It returns the authenticated +// connection and the corresponding auth information about the connection. +// At minimum, the TLS handshake is performed and the identity is built from // the [tls.ConnectionState]. If the TransportCredentials is configured with -// and Authorizer and ConnectionEnforcer then additional session controls are -// applied before the handshake completes. -func (c *TransportCredentials) ServerHandshake(rawConn net.Conn) (_ net.Conn, _ credentials.AuthInfo, err error) { +// an [authz.Authorizer] and a [ConnectionEnforcer], then additional session +// controls are applied before the handshake completes. +func (c *TransportCredentials) ServerHandshake(rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) { conn, tlsInfo, err := c.performTLSHandshake(rawConn) if err != nil { return nil, nil, trace.Wrap(err) } - defer func() { - if err != nil { - conn.Close() - } - }() + validatedConn, info, err := c.validateIdentity(conn, tlsInfo) + if err != nil { + return nil, nil, trace.NewAggregate(err, conn.Close()) + } + return validatedConn, info, nil +} +// validateIdentity extracts the identity from the client certificate, +// authorizes the user, enforces any connection limits, and ensures the +// connection is terminated at expiry of the client certificate if required. +func (c *TransportCredentials) validateIdentity(conn net.Conn, tlsInfo *credentials.TLSInfo) (net.Conn, IdentityInfo, error) { identityGetter, err := c.userGetter.GetUser(tlsInfo.State) if err != nil { - return nil, nil, trace.Wrap(err) + return nil, IdentityInfo{}, trace.Wrap(err) } ctx := context.Background() authCtx, err := c.authorize(ctx, conn.RemoteAddr(), identityGetter, &tlsInfo.State) if err != nil { - return nil, nil, trace.Wrap(err) + return nil, IdentityInfo{}, trace.Wrap(err) } if err := c.enforceConnectionLimits(ctx, authCtx, conn); err != nil { - return nil, nil, trace.Wrap(err) + return nil, IdentityInfo{}, trace.Wrap(err) + } + + if authPreference, err := c.getAuthPreference(ctx); err == nil { + expiry := authCtx.GetDisconnectCertExpiry(authPreference) + conn, err = newTimeoutConn(conn, c.clock, expiry) + if err != nil { + return nil, IdentityInfo{}, trace.Wrap(err) + } } return conn, IdentityInfo{ @@ -195,8 +260,7 @@ func (c *TransportCredentials) performTLSHandshake(rawConn net.Conn) (net.Conn, tlsInfo, ok := info.(credentials.TLSInfo) if !ok { - conn.Close() - return nil, nil, trace.BadParameter("unexpected type in tls auth info %T", info) + return nil, nil, trace.NewAggregate(conn.Close(), trace.BadParameter("unexpected type in tls auth info %T", info)) } return conn, &tlsInfo, nil diff --git a/lib/auth/transport_credentials_test.go b/lib/auth/transport_credentials_test.go index f72fa529199..602eed0ddb2 100644 --- a/lib/auth/transport_credentials_test.go +++ b/lib/auth/transport_credentials_test.go @@ -28,6 +28,8 @@ import ( "time" "github.com/gravitational/trace" + "github.com/jonboulle/clockwork" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/grpc/credentials" "google.golang.org/grpc/credentials/insecure" @@ -35,6 +37,7 @@ import ( "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/lib/authz" "github.com/gravitational/teleport/lib/services" + "github.com/gravitational/teleport/lib/tlsca" ) // TestTransportCredentials_Check validates the returned values @@ -306,18 +309,121 @@ func TestTransportCredentials_ServerHandshake(t *testing.T) { } } +type fakeUserGetter struct { + identity authz.IdentityGetter +} + +func (f fakeUserGetter) GetUser(tls.ConnectionState) (authz.IdentityGetter, error) { + return f.identity, nil +} + +func TestTransportCredentialsDisconnection(t *testing.T) { + cases := []struct { + name string + expiry time.Duration + }{ + { + name: "no expiry", + }, + { + name: "closed on expiry", + expiry: time.Hour, + }, + { + name: "already expired", + expiry: -time.Hour, + }, + } + + // Assert that the connections remain open. + connectionOpenAssertion := func(t *testing.T, conn *fakeConn) { + assert.False(t, conn.closed.Load()) + } + + // Assert that the connections are eventually closed. + connectionClosedAssertion := func(t *testing.T, conn *fakeConn) { + require.EventuallyWithT(t, func(t *assert.CollectT) { + assert.True(t, conn.closed.Load()) + }, 5*time.Second, 100*time.Millisecond) + } + + pref := types.DefaultAuthPreference() + pref.SetDisconnectExpiredCert(true) + for _, test := range cases { + t.Run(test.name, func(t *testing.T) { + clock := clockwork.NewFakeClock() + conn := &fakeConn{} + + var expiry time.Time + if test.expiry != 0 { + expiry = clock.Now().Add(test.expiry) + } + identity := TestIdentity{ + I: authz.LocalUser{ + Username: "llama", + Identity: tlsca.Identity{Username: "llama", Expires: expiry}, + }, + } + + creds, err := NewTransportCredentials(TransportCredentialsConfig{ + TransportCredentials: credentials.NewTLS(&tls.Config{}), + Authorizer: &fakeAuthorizer{checker: &fakeChecker{}, identity: identity.I}, + UserGetter: fakeUserGetter{ + identity: identity.I, + }, + Clock: clock, + GetAuthPreference: func(ctx context.Context) (types.AuthPreference, error) { return pref, nil }, + }) + require.NoError(t, err, "creating transport credentials") + + validatedConn, _, err := creds.validateIdentity(conn, &credentials.TLSInfo{State: tls.ConnectionState{}}) + switch { + case test.expiry == 0: + require.NoError(t, err) + require.NotNil(t, validatedConn) + + connectionOpenAssertion(t, conn) + clock.Advance(time.Hour) + connectionOpenAssertion(t, conn) + case test.expiry < 0: + require.NoError(t, err) + require.NotNil(t, validatedConn) + + connectionClosedAssertion(t, conn) + default: + require.NoError(t, err) + require.NotNil(t, validatedConn) + + connectionOpenAssertion(t, conn) + clock.BlockUntil(1) + clock.Advance(test.expiry) + connectionClosedAssertion(t, conn) + } + }) + } +} + type fakeChecker struct { services.AccessChecker - maxConnections int64 + maxConnections int64 + disconnectExpired *bool } func (c *fakeChecker) MaxConnections() int64 { return c.maxConnections } +func (c *fakeChecker) AdjustDisconnectExpiredCert(b bool) bool { + if c.disconnectExpired == nil { + return b + } + return *c.disconnectExpired +} + type fakeAuthorizer struct { authorizeError error checker services.AccessChecker + identity authz.IdentityGetter } func (a *fakeAuthorizer) Authorize(ctx context.Context) (*authz.Context, error) { @@ -330,9 +436,15 @@ func (a *fakeAuthorizer) Authorize(ctx context.Context) (*authz.Context, error) return nil, err } + identity := a.identity + if identity == nil { + identity = TestUser(user.GetName()).I + } + return &authz.Context{ - User: user, - Checker: a.checker, + User: user, + Checker: a.checker, + Identity: identity, }, nil } diff --git a/lib/authz/permissions.go b/lib/authz/permissions.go index b2823d9b603..570f0f9d344 100644 --- a/lib/authz/permissions.go +++ b/lib/authz/permissions.go @@ -323,6 +323,33 @@ func (c *Context) GetAccessState(authPref types.AuthPreference) services.AccessS return state } +// GetDisconnectCertExpiry calculates the proper value for DisconnectExpiredCert +// based on whether a connection is set to disconnect on cert expiry, and whether +// the cert is a short-lived (<1m) one issued for an MFA verified session. If the session +// doesn't need to be disconnected on cert expiry, it will return a zero [time.Time]. +func (c *Context) GetDisconnectCertExpiry(authPref types.AuthPreference) time.Time { + // In the case where both disconnect_expired_cert and require_session_mfa are enabled, + // the PreviousIdentityExpires value of the certificate will be used, which is the + // expiry of the certificate used to issue the short-lived MFA verified certificate. + // + // See https://github.com/gravitational/teleport/issues/18544 + + // If the session doesn't need to be disconnected on cert expiry just return the default value. + if c.Checker != nil && !c.Checker.AdjustDisconnectExpiredCert(authPref.GetDisconnectExpiredCert()) { + return time.Time{} + } + + identity := c.Identity.GetIdentity() + if !identity.PreviousIdentityExpires.IsZero() { + // If this is a short-lived mfa verified cert, return the certificate extension + // that holds its issuing certificates expiry value. + return identity.PreviousIdentityExpires + } + + // Otherwise, return the current certificates expiration + return identity.Expires +} + // Authorize authorizes user based on identity supplied via context func (a *authorizer) Authorize(ctx context.Context) (authCtx *Context, err error) { defer func() { diff --git a/lib/authz/permissions_test.go b/lib/authz/permissions_test.go index 8c7c0acf903..28e268e7626 100644 --- a/lib/authz/permissions_test.go +++ b/lib/authz/permissions_test.go @@ -29,6 +29,7 @@ import ( "github.com/google/go-cmp/cmp/cmpopts" "github.com/google/uuid" "github.com/gravitational/trace" + "github.com/jonboulle/clockwork" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/grpc/metadata" @@ -52,6 +53,64 @@ const ( clusterName = "test-cluster" ) +func TestGetDisconnectExpiredCertFromIdentity(t *testing.T) { + clock := clockwork.NewFakeClock() + now := clock.Now() + inAnHour := clock.Now().Add(time.Hour) + + for _, test := range []struct { + name string + expires time.Time + previousIdentityExpires time.Time + mfaVerified bool + disconnectExpiredCert bool + expected time.Time + }{ + { + name: "mfa overrides expires when set", + expires: now, + previousIdentityExpires: inAnHour, + mfaVerified: true, + disconnectExpiredCert: true, + expected: inAnHour, + }, + { + name: "expires returned when mfa unset", + expires: now, + mfaVerified: false, + disconnectExpiredCert: true, + expected: now, + }, + { + name: "unset when disconnectExpiredCert is false", + expires: now, + previousIdentityExpires: inAnHour, + mfaVerified: true, + disconnectExpiredCert: false, + }, + } { + t.Run(test.name, func(t *testing.T) { + var mfaVerified string + if test.mfaVerified { + mfaVerified = "1234" + } + identity := tlsca.Identity{ + Expires: test.expires, + PreviousIdentityExpires: test.previousIdentityExpires, + MFAVerified: mfaVerified, + } + + authPref := types.DefaultAuthPreference() + authPref.SetDisconnectExpiredCert(test.disconnectExpiredCert) + + ctx := Context{Checker: &fakeCtxChecker{}, Identity: WrapIdentity(identity)} + + got := ctx.GetDisconnectCertExpiry(authPref) + require.Equal(t, test.expected, got) + }) + } +} + func TestContextLockTargets(t *testing.T) { t.Parallel() @@ -1035,6 +1094,10 @@ func (c *fakeCtxChecker) GetAccessState(_ types.AuthPreference) services.AccessS return c.state } +func (c *fakeCtxChecker) AdjustDisconnectExpiredCert(disconnect bool) bool { + return disconnect +} + type testClient struct { services.ClusterConfiguration services.Trust diff --git a/lib/kube/proxy/forwarder.go b/lib/kube/proxy/forwarder.go index d82483b40cd..ed9d438927f 100644 --- a/lib/kube/proxy/forwarder.go +++ b/lib/kube/proxy/forwarder.go @@ -790,7 +790,7 @@ func (f *Forwarder) setupContext( recordingConfig: recordingConfig, kubeClusterName: kubeCluster, certExpires: identity.Expires, - disconnectExpiredCert: srv.GetDisconnectExpiredCertFromIdentity(roles, authPref, &identity), + disconnectExpiredCert: authCtx.GetDisconnectCertExpiry(authPref), teleportCluster: teleportClusterClient{ name: teleportClusterName, remoteAddr: utils.NetAddr{AddrNetwork: "tcp", Addr: req.RemoteAddr}, diff --git a/lib/resumption/client.go b/lib/resumption/client.go index de44cc83fd2..11c41adf86c 100644 --- a/lib/resumption/client.go +++ b/lib/resumption/client.go @@ -26,6 +26,7 @@ import ( "net" "regexp" "strconv" + "strings" "time" "github.com/gravitational/trace" @@ -307,6 +308,13 @@ func dialResumable(ctx context.Context, token resumptionToken, hostID string, re logrus.Debug("Dialing server for connection resumption.") nc, err := redial(ctx, hostID) if err != nil { + // If connections are failing because client certificates are expired + // abandon all future connection resumption attempts. + const expiredCertError = "remote error: tls: expired certificate" + if strings.Contains(err.Error(), expiredCertError) { + return nil, nil + } + return nil, trace.Wrap(err) } diff --git a/lib/service/service.go b/lib/service/service.go index fe1d4f02da1..ac1f5407e6f 100644 --- a/lib/service/service.go +++ b/lib/service/service.go @@ -4550,6 +4550,7 @@ func (process *TeleportProcess) initProxyEndpoint(conn *Connector) error { TransportCredentials: credentials.NewTLS(tlscfg), UserGetter: authMiddleware, Authorizer: authorizer, + GetAuthPreference: accessPoint.GetAuthPreference, }) if err != nil { return trace.Wrap(err) @@ -6304,6 +6305,7 @@ func (process *TeleportProcess) initSecureGRPCServer(cfg initSecureGRPCServerCfg creds, err := auth.NewTransportCredentials(auth.TransportCredentialsConfig{ TransportCredentials: credentials.NewTLS(tlsConf), UserGetter: authMiddleware, + GetAuthPreference: cfg.accessPoint.GetAuthPreference, }) if err != nil { return nil, trace.Wrap(err) diff --git a/lib/srv/desktop/windows_server.go b/lib/srv/desktop/windows_server.go index 97dc1b85d33..e2c663a96d4 100644 --- a/lib/srv/desktop/windows_server.go +++ b/lib/srv/desktop/windows_server.go @@ -898,7 +898,7 @@ func (s *WindowsService) connectRDP(ctx context.Context, log *slog.Logger, tdpCo Conn: tdpConn, Clock: s.cfg.Clock, ClientIdleTimeout: authCtx.Checker.AdjustClientIdleTimeout(netConfig.GetClientIdleTimeout()), - DisconnectExpiredCert: srv.GetDisconnectExpiredCertFromIdentity(authCtx.Checker, authPref, &identity), + DisconnectExpiredCert: authCtx.GetDisconnectCertExpiry(authPref), Entry: logrus.NewEntry(logrus.StandardLogger()), Emitter: s.cfg.Emitter, EmitterContext: s.closeCtx, diff --git a/lib/srv/monitor.go b/lib/srv/monitor.go index 6247627e19c..cd574860dad 100644 --- a/lib/srv/monitor.go +++ b/lib/srv/monitor.go @@ -37,7 +37,6 @@ import ( "github.com/gravitational/teleport/lib/authz" "github.com/gravitational/teleport/lib/events" "github.com/gravitational/teleport/lib/services" - "github.com/gravitational/teleport/lib/tlsca" ) // ActivityTracker is a connection activity tracker, @@ -181,7 +180,7 @@ func (c *ConnectionMonitor) MonitorConn(ctx context.Context, authzCtx *authz.Con LockWatcher: c.cfg.LockWatcher, LockTargets: authzCtx.LockTargets(), LockingMode: authzCtx.Checker.LockingMode(authPref.GetLockingMode()), - DisconnectExpiredCert: GetDisconnectExpiredCertFromIdentity(checker, authPref, &identity), + DisconnectExpiredCert: authzCtx.GetDisconnectCertExpiry(authPref), ClientIdleTimeout: idleTimeout, Conn: tconn, Tracker: tconn, @@ -589,36 +588,6 @@ func (t *TrackingReadConn) UpdateClientActivity() { t.lastActive = t.cfg.Clock.Now().UTC() } -// GetDisconnectExpiredCertFromIdentity calculates the proper value for DisconnectExpiredCert -// based on whether a connection is set to disconnect on cert expiry, and whether -// the cert is a short lived (<1m) one issued for an MFA verified session. If the session -// doesn't need to be disconnected on cert expiry it will return the default value for time.Time. -func GetDisconnectExpiredCertFromIdentity( - checker services.AccessChecker, - authPref types.AuthPreference, - identity *tlsca.Identity, -) time.Time { - // In the case where both disconnect_expired_cert and require_session_mfa are enabled, - // the PreviousIdentityExpires value of the certificate will be used, which is the - // expiry of the certificate used to issue the short lived MFA verified certificate. - // - // See https://github.com/gravitational/teleport/issues/18544 - - // If the session doesn't need to be disconnected on cert expiry just return the default value. - if !checker.AdjustDisconnectExpiredCert(authPref.GetDisconnectExpiredCert()) { - return time.Time{} - } - - if !identity.PreviousIdentityExpires.IsZero() { - // If this is a short-lived mfa verified cert, return the certificate extension - // that holds its' issuing cert's expiry value. - return identity.PreviousIdentityExpires - } - - // Otherwise just return the current cert's expiration - return identity.Expires -} - // See GetDisconnectExpiredCertFromIdentity func getDisconnectExpiredCertFromIdentityContext( checker services.AccessChecker, diff --git a/lib/srv/monitor_test.go b/lib/srv/monitor_test.go index 40d3f5304d8..ad95adefef3 100644 --- a/lib/srv/monitor_test.go +++ b/lib/srv/monitor_test.go @@ -386,71 +386,3 @@ func (m mockChecker) AdjustClientIdleTimeout(ttl time.Duration) time.Duration { func (m mockChecker) LockingMode(defaultMode constants.LockingMode) constants.LockingMode { return defaultMode } - -type mockAuthPreference struct { - types.AuthPreference -} - -var disconnectExpiredCert bool - -func (m *mockAuthPreference) GetDisconnectExpiredCert() bool { - return disconnectExpiredCert -} - -func TestGetDisconnectExpiredCertFromIdentity(t *testing.T) { - clock := clockwork.NewFakeClock() - now := clock.Now() - inAnHour := clock.Now().Add(time.Hour) - var unset time.Time - checker := mockChecker{} - authPref := &mockAuthPreference{} - - for _, test := range []struct { - name string - expires time.Time - previousIdentityExpires time.Time - mfaVerified bool - disconnectExpiredCert bool - expected time.Time - }{ - { - name: "mfa overrides expires when set", - expires: now, - previousIdentityExpires: inAnHour, - mfaVerified: true, - disconnectExpiredCert: true, - expected: inAnHour, - }, - { - name: "expires returned when mfa unset", - expires: now, - previousIdentityExpires: unset, - mfaVerified: false, - disconnectExpiredCert: true, - expected: now, - }, - { - name: "unset when disconnectExpiredCert is false", - expires: now, - previousIdentityExpires: inAnHour, - mfaVerified: true, - disconnectExpiredCert: false, - expected: unset, - }, - } { - t.Run(test.name, func(t *testing.T) { - var mfaVerified string - if test.mfaVerified { - mfaVerified = "1234" - } - identity := tlsca.Identity{ - Expires: test.expires, - PreviousIdentityExpires: test.previousIdentityExpires, - MFAVerified: mfaVerified, - } - disconnectExpiredCert = test.disconnectExpiredCert - got := GetDisconnectExpiredCertFromIdentity(checker, authPref, &identity) - require.Equal(t, test.expected, got) - }) - } -}