MWI: Add certificate expiry check leeway for tbot (#64293)

* MWI: Add certificate expiry check leeway for app tunnel service

This adds some leeway to the application-tunnel service's certificate
expiration check.

For context, the app tunnel service does not follow Machine ID's
usual certificate renewal cycle and instead opts to renew
certificates just-in-time before completing a request if the
certificate has expired per the local clock.

Unfortunately, the local clock is not always accurate, and if the
certificate or underlying app session has already expired from the
server's perspective, client requests can still fail until the local
clock catches up and the certificate is refreshed.

To mitigate this, this change adds a `leeway` parameter to the
service, configurable via YAML, with a default value of 1m. This is
added to the current time when certificate validity is checked. This
means that, in the worst case, certificates will be refreshed to early
rather than too late.

See also: #64284

* Remove duration pointer and ignore leeway if greater than cert TTL

* Make tbot's leeway parameter global

This makes the leeway parameter global, and additionally uses it in
the main renewal loop (for the expired bot internal identity
detection) and in the database tunnel service.

Also adds some test coverage for app tunnel cert renewals.

* Tweak doc comment for clarity

* Remove unused code

* Ignore excessive leeway values in identity service

* Honor effective lifetime in leeway check

* Fix leeway var reference

* Update golden tests

* Codex reviewer appeasement

Factor in actual cert TTL in the leeway cap.

* Fix failing test

* Fix imports

Author: timothyb89

lib/srv/alpnproxy/local_proxy.go

@@ -30,6 +30,7 @@ import (
 	"net/http/httputil"
 	"strings"
 	"sync"
+	"time"
 
 	"github.com/gravitational/trace"
 	"github.com/jackc/pgproto3/v2"
@@ -434,8 +435,11 @@ func (l *LocalProxy) getCert() tls.Certificate {
 	return l.cfg.Cert
 }
 
-// CheckDBCert checks the proxy certificates for expiration and that the cert subject matches a database route.
-func (l *LocalProxy) CheckDBCert(ctx context.Context, dbRoute tlsca.RouteToDatabase) error {
+// CheckDBCertWithLeeway checks the proxy certificates for expiration and that
+// the cert subject matches a database route. The provided leeway value is added
+// to the current time when checking certificate expiration and can be used to
+// account for potential client-side clock drift.
+func (l *LocalProxy) CheckDBCertWithLeeway(ctx context.Context, dbRoute tlsca.RouteToDatabase, leeway time.Duration) error {
 	l.cfg.Log.DebugContext(ctx, "checking local proxy database certs")
 	l.certMu.RLock()
 	defer l.certMu.RUnlock()
@@ -450,15 +454,22 @@ func (l *LocalProxy) CheckDBCert(ctx context.Context, dbRoute tlsca.RouteToDatab
 	}
 
 	// Check for cert expiration.
-	if err := utils.VerifyCertificateExpiry(cert, l.cfg.Clock); err != nil {
+	if err := utils.VerifyCertificateExpiryWithLeeway(cert, l.cfg.Clock, leeway); err != nil {
 		return trace.Wrap(err)
 	}
 
 	return trace.Wrap(CheckDBCertSubject(cert, dbRoute))
 }
 
-// CheckCertExpiry checks the proxy certificates for expiration.
-func (l *LocalProxy) CheckCertExpiry(ctx context.Context) error {
+// CheckDBCert checks the proxy certificates for expiration and that the cert subject matches a database route.
+func (l *LocalProxy) CheckDBCert(ctx context.Context, dbRoute tlsca.RouteToDatabase) error {
+	return l.CheckDBCertWithLeeway(ctx, dbRoute, 0)
+}
+
+// CheckCertExpiryWithLeeway checks the proxy certificates for expiration. The
+// provided leeway value is added to the current time when compared to the cert
+// expiration and can be used to account for potential client-side clock drift.
+func (l *LocalProxy) CheckCertExpiryWithLeeway(ctx context.Context, leeway time.Duration) error {
 	l.cfg.Log.DebugContext(ctx, "checking local proxy certs")
 	l.certMu.RLock()
 	defer l.certMu.RUnlock()
@@ -472,7 +483,12 @@ func (l *LocalProxy) CheckCertExpiry(ctx context.Context) error {
 		return trace.Wrap(err)
 	}
 
-	return trace.Wrap(utils.VerifyCertificateExpiry(cert, l.cfg.Clock))
+	return trace.Wrap(utils.VerifyCertificateExpiryWithLeeway(cert, l.cfg.Clock, leeway))
+}
+
+// CheckCertExpiry checks the proxy certificates for expiration.
+func (l *LocalProxy) CheckCertExpiry(ctx context.Context) error {
+	return trace.Wrap(l.CheckCertExpiryWithLeeway(ctx, 0))
 }
 
 // CheckDBCertSubject checks if the route to the database from the cert matches the provided route in

lib/tbot/bot/bot.go

@@ -355,6 +355,7 @@ func (b *Bot) buildIdentityService(
 		Destination:     b.cfg.InternalStorage,
 		TTL:             b.cfg.CredentialLifetime.TTL,
 		RenewalInterval: b.cfg.CredentialLifetime.RenewalInterval,
+		Leeway:          b.cfg.Leeway,
 		FIPS:            b.cfg.FIPS,
 		Logger: b.cfg.Logger.With(
 			teleport.ComponentKey,

lib/tbot/bot/config.go

@@ -20,6 +20,7 @@ package bot
 
 import (
 	"log/slog"
+	"time"
 
 	"github.com/gravitational/trace"
 	"github.com/grpc-ecosystem/go-grpc-middleware/providers/prometheus"
@@ -52,6 +53,12 @@ type Config struct {
 	// internal credentials.
 	CredentialLifetime CredentialLifetime
 
+	// Leeway is a duration added to local system time when checking for expired
+	// certificates in certain cases, particularly with app and database
+	// tunnels. It can be useful to account for clock drift, or if a negative
+	// duration is provided, to simulate clock drift.
+	Leeway time.Duration `yaml:"leeway,omitempty"`
+
 	// FIPS controls whether the bot will run in a mode designed to comply with
 	// Federal Information Processing Standards.
 	FIPS bool

lib/tbot/config/config.go

@@ -56,6 +56,7 @@ import (
 const (
 	DefaultCertificateTTL = 60 * time.Minute
 	DefaultRenewInterval  = 20 * time.Minute
+	DefaultLeeway         = 1 * time.Minute
 )
 
 var log = logutils.NewPackageLogger(teleport.ComponentKey, teleport.ComponentTBot)
@@ -87,7 +88,15 @@ type BotConfig struct {
 	JoinURI string `yaml:"join_uri,omitempty"`
 
 	CredentialLifetime bot.CredentialLifetime `yaml:",inline"`
-	Oneshot            bool                   `yaml:"oneshot"`
+
+	// Leeway is a duration added to local system time when checking for expired
+	// certificates in certain cases, particularly with app and database
+	// tunnels. It can be useful to account for clock drift, or if a negative
+	// duration is provided, to simulate clock drift.
+	Leeway time.Duration `yaml:"leeway,omitempty"`
+
+	Oneshot bool `yaml:"oneshot"`
+
 	// FIPS instructs `tbot` to run in a mode designed to comply with FIPS
 	// regulations. This means the bot should:
 	// - Refuse to run if not compiled with boringcrypto
@@ -259,6 +268,10 @@ func (conf *BotConfig) CheckAndSetDefaults() error {
 		conf.CredentialLifetime.RenewalInterval = DefaultRenewInterval
 	}
 
+	if conf.Leeway == 0 {
+		conf.Leeway = DefaultLeeway
+	}
+
 	// We require the join method for `configure` and `start` but not for `init`
 	// Therefore, we need to check its valid here, but enforce its presence
 	// elsewhere.

lib/tbot/internal/identity/service.go

@@ -63,6 +63,7 @@ type Config struct {
 
 	TTL             time.Duration
 	RenewalInterval time.Duration
+	Leeway          time.Duration
 
 	FIPS bool
 
@@ -543,13 +544,29 @@ func renewIdentity(
 		return newIdentity, nil
 	}
 
+	// Note: This leeway cap check is simpler than app/db tunnel services as the
+	// main renewal loop is, well, a loop, and will not attempt to renew more
+	// often than the configured renewal interval.
+	leeway := cfg.Leeway
+	if leeway >= cfg.TTL {
+		log.WarnContext(ctx, "leeway is greater than credential lifetime and "+
+			"will be ignored, be aware of potential failures due to clock drift",
+			"credential_ttl", cfg.TTL,
+			"configured_leeway", cfg.Leeway,
+		)
+		leeway = 0
+	}
+
 	// Note: This simple expiration check is probably not the best possible
 	// solution to determine when to discard an existing identity: the client
 	// could have severe clock drift, or there could be non-expiry related
 	// reasons that an identity should be thrown out. We may improve this
-	// discard logic in the future if we determine we're still creating  excess
+	// discard logic in the future if we determine we're still creating excess
 	// bot instances.
-	now := time.Now()
+	// To allow users to manually compensate for clock drift if e.g. using very
+	// tight renewal/TTL values, we expose a configurable leeway to trigger
+	// modestly early renewal.
+	now := time.Now().Add(leeway)
 	if expiry, ok := facade.Expiry(); !ok || now.After(expiry) {
 		slog.WarnContext(
 			ctx,
@@ -564,6 +581,7 @@ func renewIdentity(
 			"expiry", expiry,
 			"ttl", cfg.TTL,
 			"renewal_interval", cfg.RenewalInterval,
+			"leeway", cfg.Leeway,
 		)
 
 		newIdentity, err := botIdentityFromToken(ctx, log, cfg, nil)

lib/tbot/services/application/tunnel_config.go

@@ -23,6 +23,7 @@ import (
 	"net/url"
 
 	"github.com/gravitational/trace"
+	"github.com/jonboulle/clockwork"
 	"gopkg.in/yaml.v3"
 
 	"github.com/gravitational/teleport/lib/tbot/bot"
@@ -54,6 +55,14 @@ type TunnelConfig struct {
 	// Listener overrides "listen" and directly provides an opened listener to
 	// use.
 	Listener net.Listener `yaml:"-"`
+
+	// Clock is a clock. If unset, the standard system clock is used. Used in
+	// tests.
+	clock clockwork.Clock `yaml:"-"`
+
+	// certIssuedHook is an optional hook called when the tunnel requests a new
+	// certificate. Used in tests.
+	certIssuedHook func() `yaml:"-"`
 }
 
 // GetName returns the user-given name of the service, used for validation purposes.
@@ -94,6 +103,10 @@ func (s *TunnelConfig) CheckAndSetDefaults() error {
 	if _, err := url.Parse(s.Listen); err != nil {
 		return trace.Wrap(err, "parsing listen")
 	}
+	if s.clock == nil {
+		s.clock = clockwork.NewRealClock()
+	}
+
 	return nil
 }
 

lib/tbot/services/application/tunnel_config_test.go

@@ -22,6 +22,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/jonboulle/clockwork"
+
 	"github.com/gravitational/teleport/lib/tbot/bot"
 )
 
@@ -47,6 +49,8 @@ func TestApplicationTunnelService_YAML(t *testing.T) {
 func TestApplicationTunnelService_CheckAndSetDefaults(t *testing.T) {
 	t.Parallel()
 
+	clock := clockwork.NewFakeClock()
+
 	tests := []testCheckAndSetDefaultsCase[*TunnelConfig]{
 		{
 			name: "valid",
@@ -55,8 +59,15 @@ func TestApplicationTunnelService_CheckAndSetDefaults(t *testing.T) {
 					Listen:  "tcp://0.0.0.0:3621",
 					Roles:   []string{"role1", "role2"},
 					AppName: "my-app",
+					clock:   clock,
 				}
 			},
+			want: &TunnelConfig{
+				Listen:  "tcp://0.0.0.0:3621",
+				Roles:   []string{"role1", "role2"},
+				AppName: "my-app",
+				clock:   clock,
+			},
 			wantErr: "",
 		},
 		{

lib/tbot/services/application/tunnel_service.go

@@ -24,6 +24,7 @@ import (
 	"crypto/tls"
 	"fmt"
 	"log/slog"
+	"time"
 
 	"github.com/gravitational/trace"
 
@@ -44,6 +45,7 @@ func TunnelServiceBuilder(
 	cfg *TunnelConfig,
 	connCfg connection.Config,
 	defaultCredentialLifetime bot.CredentialLifetime,
+	leeway time.Duration,
 ) bot.ServiceBuilder {
 	buildFn := func(deps bot.ServiceDependencies) (bot.Service, error) {
 		if err := cfg.CheckAndSetDefaults(); err != nil {
@@ -52,6 +54,7 @@ func TunnelServiceBuilder(
 		svc := &TunnelService{
 			connCfg:                   connCfg,
 			defaultCredentialLifetime: defaultCredentialLifetime,
+			leeway:                    leeway,
 			getBotIdentity:            deps.BotIdentity,
 			botIdentityReadyCh:        deps.BotIdentityReadyCh,
 			proxyPinger:               deps.ProxyPinger,
@@ -74,6 +77,7 @@ func TunnelServiceBuilder(
 type TunnelService struct {
 	connCfg                   connection.Config
 	defaultCredentialLifetime bot.CredentialLifetime
+	leeway                    time.Duration
 	cfg                       *TunnelConfig
 	proxyPinger               connection.ProxyPinger
 	log                       *slog.Logger
@@ -183,13 +187,40 @@ func (s *TunnelService) buildLocalProxyConfig(ctx context.Context) (lpCfg alpnpr
 	}
 	s.log.DebugContext(ctx, "Issued initial certificate for local proxy.")
 
+	// Attempt to provide a sensible cap for leeway values based on the
+	// configured and actual cert TTL to guard against rapid renewals. This
+	// doesn't attempt to be perfect, and it's still possible to force a new
+	// cert to be issued on every connection with the right set of unreasonable
+	// values. However, we want it to be possible to specify  nearly-too-large
+	// values for testing purposes (i.e. using negative leeway to simulate clock
+	// drift) and do not want to be in the business of calculating leeway-leeway
+	// so we'll keep the check somewhat simple.
+	issuedCertLifetime := appCert.Leaf.NotAfter.Sub(appCert.Leaf.NotBefore)
+	effectiveLifetime := cmp.Or(s.cfg.CredentialLifetime, s.defaultCredentialLifetime)
+	effectiveTTL := min(issuedCertLifetime, effectiveLifetime.TTL)
+
+	leeway := s.leeway
+	if leeway >= effectiveTTL {
+		s.log.WarnContext(ctx,
+			"leeway is greater than the credential lifetime and will be "+
+				"ignored, be aware of potential failures due to clock drift",
+			"configured_ttl", effectiveLifetime.TTL,
+			"configured_leeway", leeway,
+			"issued_cert_ttl", issuedCertLifetime,
+		)
+		leeway = 0
+	}
+
 	middleware := internal.ALPNProxyMiddleware{
 		OnNewConnectionFunc: func(ctx context.Context, lp *alpnproxy.LocalProxy) error {
 			ctx, span := tracer.Start(ctx, "TunnelService/OnNewConnection")
 			defer span.End()
 
-			if err := lp.CheckCertExpiry(ctx); err != nil {
-				s.log.InfoContext(ctx, "Certificate for tunnel needs reissuing.", "reason", err.Error())
+			if err := lp.CheckCertExpiryWithLeeway(ctx, leeway); err != nil {
+				s.log.InfoContext(ctx, "Certificate for tunnel needs reissuing.",
+					"reason", err.Error(),
+					"leeway", leeway,
+				)
 				cert, _, err := s.issueCert(ctx)
 				if err != nil {
 					return trace.Wrap(err, "issuing cert")
@@ -208,6 +239,7 @@ func (s *TunnelService) buildLocalProxyConfig(ctx context.Context) (lpCfg alpnpr
 		Protocols:          []common.Protocol{alpnProtocolForApp(app)},
 		Cert:               *appCert,
 		InsecureSkipVerify: s.connCfg.Insecure,
+		Clock:              s.cfg.clock,
 	}
 	if apiclient.IsALPNConnUpgradeRequired(
 		ctx,
@@ -264,7 +296,17 @@ func (s *TunnelService) issueCert(
 	}
 	s.log.InfoContext(ctx, "Certificate issued for tunnel proxy.")
 
-	return routedIdent.TLSCert, app, nil
+	// In tests, notify the test that a cert has been issued.
+	if s.cfg.certIssuedHook != nil {
+		s.cfg.certIssuedHook()
+	}
+
+	// The leaf isn't appended by the stdlib, so add it here so we can inspect
+	// the TTL downstream.
+	cert := routedIdent.TLSCert
+	cert.Leaf = routedIdent.X509Cert
+
+	return cert, app, nil
 }
 
 // String returns a human-readable string that can uniquely identify the