From ce5239b9862105e5fd13a015d31df42c5cabb18a Mon Sep 17 00:00:00 2001 From: Clement Tee Date: Wed, 29 Jul 2026 18:51:08 +0800 Subject: [PATCH 01/11] feat: support dynamic downstream TLS certificate minting Add a `tls.dynamic` mode that mints short-lived downstream leaf certificates from a configured CA, so operators no longer maintain static SANs for every resource hostname. Both modes now sit behind a `CertProvider` interface (Run, GetCertificate, GetCertificateForHost) selected by `newCertProviderFromConfig`, mirroring the existing sshhandler caProvider. Static mode keeps the file-watching CertReloader; the inner post-CONNECT handshake pins the certificate for the CONNECT-requested host in both modes. Co-Authored-By: Claude Fable 5 --- .golangci.yml | 1 + internal/config/config.go | 172 ++++++++++++- internal/config/config_test.go | 205 ++++++++++++++- internal/connect/cert.go | 195 ++++++++++++++ internal/connect/cert_provider.go | 46 ++++ internal/connect/cert_provider_test.go | 78 ++++++ internal/connect/cert_reloader.go | 6 + internal/connect/cert_test.go | 284 +++++++++++++++++++++ internal/connect/conn.go | 51 +++- internal/connect/conn_test.go | 237 +++++++++++++++++ internal/connect/connect.go | 18 +- internal/connect/listener.go | 15 +- internal/connect/listener_test.go | 6 +- internal/httpproxy/proxy_test.go | 4 +- internal/kuberneteshandler/handler_test.go | 2 +- internal/sshhandler/proxy_test.go | 2 +- internal/webapphandler/handler_test.go | 12 +- 17 files changed, 1292 insertions(+), 42 deletions(-) create mode 100644 internal/connect/cert.go create mode 100644 internal/connect/cert_provider.go create mode 100644 internal/connect/cert_provider_test.go create mode 100644 internal/connect/cert_test.go diff --git a/.golangci.yml b/.golangci.yml index e9c38d29..5ad96838 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -48,6 +48,7 @@ linters: - Recorder - ca - caProvider + - CertProvider # golang.org/x/crypto/ssh - Signer - PublicKey diff --git a/internal/config/config.go b/internal/config/config.go index 7eebd6d7..71223a4c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -48,6 +48,8 @@ const ( defaultMetricsPort = 9090 defaultAuditLogFlushInterval = time.Minute * 10 defaultAuditLogFlushSizeThreshold = 1_000_000 // 1MB in bytes + defaultTLSCertDuration = 24 * time.Hour + defaultTLSCertRenewBefore = 8 * time.Hour ) type Config struct { @@ -85,9 +87,11 @@ type AuditLogConfig struct { FlushSizeThreshold int `yaml:"flushSizeThreshold"` // bytes } -// TLSConfig represents the downstream TLS configuration. Static must be set. +// TLSConfig represents the downstream TLS configuration. Exactly one of +// Static or Dynamic must be set. type TLSConfig struct { - Static *TLSStaticConfig `yaml:"static,omitempty"` + Static *TLSStaticConfig `yaml:"static,omitempty"` + Dynamic *TLSDynamicConfig `yaml:"dynamic,omitempty"` } type TLSStaticConfig struct { @@ -95,6 +99,31 @@ type TLSStaticConfig struct { PrivateKeyFile string `yaml:"privateKeyFile"` } +// TLSDynamicConfig configures on-demand minting of downstream leaf certificates. +type TLSDynamicConfig struct { + CA TLSDynamicCAConfig `yaml:"ca"` + Cert TLSDynamicCertConfig `yaml:"cert"` +} + +// TLSDynamicCAConfig represents the signing CA configuration. SelfSign must be set. +type TLSDynamicCAConfig struct { + SelfSign *TLSSelfSignCAConfig `yaml:"selfSign,omitempty"` +} + +// TLSSelfSignCAConfig configures a signing CA loaded from certificate and key files. +type TLSSelfSignCAConfig struct { + CertificateFile string `yaml:"certificateFile"` + PrivateKeyFile string `yaml:"privateKeyFile"` +} + +// TLSDynamicCertConfig controls the leaf certificates minted by the dynamic CA. +type TLSDynamicCertConfig struct { + Duration time.Duration `yaml:"duration"` // Leaf certificate lifetime. Defaults to 24h. + RenewBefore time.Duration `yaml:"renewBefore"` // Window before expiry in which a fresh leaf is minted. Defaults to 8h. + KeyType string `yaml:"keyType"` // ecdsa or rsa. Defaults to ecdsa. + KeyBits int `yaml:"keyBits"` // ECDSA: 256/384/521, RSA: 2048/3072/4096. Defaults to 256 for ECDSA, 2048 for RSA. +} + type KubernetesConfig struct { Upstreams []KubernetesUpstream `yaml:"upstreams"` } @@ -341,19 +370,29 @@ func (c *Config) Validate() error { } func (t *TLSConfig) Validate() error { - if t.Static == nil { + if t.Static == nil && t.Dynamic == nil { return ErrMissingTLSConfig } - if err := t.Static.Validate(); err != nil { - return fmt.Errorf("static: %w", err) + if t.Static != nil && t.Dynamic != nil { + return ErrConflictingTLSConfig + } + + if t.Static != nil { + if err := t.Static.Validate(); err != nil { + return fmt.Errorf("static: %w", err) + } + } + + if t.Dynamic != nil { + if err := t.Dynamic.Validate(); err != nil { + return fmt.Errorf("dynamic: %w", err) + } } return nil } -var ErrMissingTLSConfig = errors.New("'static' must be specified for TLS config") - func (s *TLSStaticConfig) Validate() error { if s.CertificateFile == "" { return fmt.Errorf("%w: certificateFile", ErrRequired) @@ -366,6 +405,125 @@ func (s *TLSStaticConfig) Validate() error { return nil } +var ( + ErrMissingTLSConfig = errors.New("either 'static' or 'dynamic' must be specified for TLS config") + ErrConflictingTLSConfig = errors.New("only one of 'static' or 'dynamic' can be specified for TLS config") + ErrMissingTLSCAConfig = errors.New("'selfSign' must be specified for dynamic CA config") + ErrInvalidTLSKeyType = errors.New("invalid TLS key type") + ErrInvalidTLSKeyBits = errors.New("invalid TLS key bits") + ErrNegativeDuration = errors.New("duration must be non-negative") + ErrRenewBeforeTooLong = errors.New("'renewBefore' must be shorter than 'duration'") +) + +func (d *TLSDynamicConfig) Validate() error { + if err := d.CA.Validate(); err != nil { + return fmt.Errorf("ca: %w", err) + } + + if err := d.Cert.Validate(); err != nil { + return fmt.Errorf("cert: %w", err) + } + + return nil +} + +func (c *TLSDynamicCAConfig) Validate() error { + if c.SelfSign == nil { + return ErrMissingTLSCAConfig + } + + if err := c.SelfSign.Validate(); err != nil { + return fmt.Errorf("selfSign: %w", err) + } + + return nil +} + +func (s *TLSSelfSignCAConfig) Validate() error { + if s.CertificateFile == "" { + return fmt.Errorf("%w: certificateFile", ErrRequired) + } + + if s.PrivateKeyFile == "" { + return fmt.Errorf("%w: privateKeyFile", ErrRequired) + } + + return nil +} + +func (c *TLSDynamicCertConfig) Validate() error { + if c.Duration < 0 { + return fmt.Errorf("%w: duration", ErrNegativeDuration) + } + + if c.RenewBefore < 0 { + return fmt.Errorf("%w: renewBefore", ErrNegativeDuration) + } + + if c.GetRenewBefore() >= c.GetDuration() { + return ErrRenewBeforeTooLong + } + + switch c.GetKeyType() { + case "ecdsa": + switch c.GetKeyBits() { + case 256, 384, 521: + default: + return fmt.Errorf("%w: ECDSA %d", ErrInvalidTLSKeyBits, c.GetKeyBits()) + } + case "rsa": + switch c.GetKeyBits() { + case 2048, 3072, 4096: + default: + return fmt.Errorf("%w: RSA %d", ErrInvalidTLSKeyBits, c.GetKeyBits()) + } + default: + return fmt.Errorf("%w: %q", ErrInvalidTLSKeyType, c.KeyType) + } + + return nil +} + +// GetDuration returns the leaf certificate lifetime, defaulting to 24h. +func (c *TLSDynamicCertConfig) GetDuration() time.Duration { + if c.Duration == 0 { + return defaultTLSCertDuration + } + + return c.Duration +} + +// GetRenewBefore returns the re-mint window before expiry, defaulting to 8h. +func (c *TLSDynamicCertConfig) GetRenewBefore() time.Duration { + if c.RenewBefore == 0 { + return defaultTLSCertRenewBefore + } + + return c.RenewBefore +} + +// GetKeyType returns the leaf key type, defaulting to ecdsa. +func (c *TLSDynamicCertConfig) GetKeyType() string { + if c.KeyType == "" { + return "ecdsa" + } + + return c.KeyType +} + +// GetKeyBits returns the leaf key size, defaulting to 256 for ECDSA and 2048 for RSA. +func (c *TLSDynamicCertConfig) GetKeyBits() int { + if c.KeyBits == 0 { + if c.GetKeyType() == "rsa" { + return 2048 + } + + return 256 + } + + return c.KeyBits +} + func (k *KubernetesConfig) Validate() error { upstreamNames := make(map[string]struct{}) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 2a92b5ac..40962ad4 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -139,6 +139,45 @@ func TestResolveTwingateHostname(t *testing.T) { }) } +func TestLoad_TLSDynamic(t *testing.T) { + yaml := ` +twingate: + network: "acme" +port: 8443 +metricsPort: 9090 +tls: + dynamic: + ca: + selfSign: + certificateFile: "ca.crt" + privateKeyFile: "ca.key" + cert: + duration: "48h" + renewBefore: "12h" + keyType: "ecdsa" + keyBits: 384 +webApp: {} +` + + tmpFile := filepath.Join(t.TempDir(), "config.yaml") + err := os.WriteFile(tmpFile, []byte(yaml), 0600) + require.NoError(t, err) + + cfg, err := Load(tmpFile) + require.NoError(t, err) + require.NotNil(t, cfg.TLS.Dynamic) + + require.NotNil(t, cfg.TLS.Dynamic.CA.SelfSign) + assert.Equal(t, "ca.crt", cfg.TLS.Dynamic.CA.SelfSign.CertificateFile) + assert.Equal(t, "ca.key", cfg.TLS.Dynamic.CA.SelfSign.PrivateKeyFile) + assert.Equal(t, 48*time.Hour, cfg.TLS.Dynamic.Cert.GetDuration()) + assert.Equal(t, 12*time.Hour, cfg.TLS.Dynamic.Cert.GetRenewBefore()) + assert.Equal(t, "ecdsa", cfg.TLS.Dynamic.Cert.GetKeyType()) + assert.Equal(t, 384, cfg.TLS.Dynamic.Cert.GetKeyBits()) + + assert.NoError(t, cfg.Validate()) +} + func TestLoad_Kubernetes(t *testing.T) { yaml := ` twingate: @@ -595,10 +634,34 @@ func TestTLSConfig_Validate(t *testing.T) { wantErr: false, }, { - name: "missing static", + name: "valid with dynamic", + tls: TLSConfig{ + Dynamic: &TLSDynamicConfig{ + CA: TLSDynamicCAConfig{ + SelfSign: &TLSSelfSignCAConfig{CertificateFile: "ca.crt", PrivateKeyFile: "ca.key"}, + }, + }, + }, + wantErr: false, + }, + { + name: "missing static and dynamic", tls: TLSConfig{}, wantErr: true, - errContains: "'static' must be specified", + errContains: "either 'static' or 'dynamic' must be specified", + }, + { + name: "conflicting static and dynamic", + tls: TLSConfig{ + Static: &TLSStaticConfig{CertificateFile: "tls.crt", PrivateKeyFile: "tls.key"}, + Dynamic: &TLSDynamicConfig{ + CA: TLSDynamicCAConfig{ + SelfSign: &TLSSelfSignCAConfig{CertificateFile: "ca.crt", PrivateKeyFile: "ca.key"}, + }, + }, + }, + wantErr: true, + errContains: "only one of 'static' or 'dynamic' can be specified", }, { name: "static missing certificate", @@ -612,6 +675,12 @@ func TestTLSConfig_Validate(t *testing.T) { wantErr: true, errContains: "static: required field is missing: privateKeyFile", }, + { + name: "invalid dynamic", + tls: TLSConfig{Dynamic: &TLSDynamicConfig{}}, + wantErr: true, + errContains: "dynamic: ca: 'selfSign' must be specified", + }, } for _, tt := range tests { @@ -627,6 +696,138 @@ func TestTLSConfig_Validate(t *testing.T) { } } +func TestTLSDynamicConfig_Validate(t *testing.T) { + validCA := TLSDynamicCAConfig{ + SelfSign: &TLSSelfSignCAConfig{CertificateFile: "ca.crt", PrivateKeyFile: "ca.key"}, + } + + tests := []struct { + name string + dynamic TLSDynamicConfig + wantErr bool + errContains string + }{ + { + name: "valid with defaults", + dynamic: TLSDynamicConfig{CA: validCA}, + wantErr: false, + }, + { + name: "valid with full cert config", + dynamic: TLSDynamicConfig{ + CA: validCA, + Cert: TLSDynamicCertConfig{ + Duration: 48 * time.Hour, + RenewBefore: 12 * time.Hour, + KeyType: "rsa", + KeyBits: 4096, + }, + }, + wantErr: false, + }, + { + name: "missing selfSign", + dynamic: TLSDynamicConfig{}, + wantErr: true, + errContains: "'selfSign' must be specified", + }, + { + name: "selfSign missing certificate", + dynamic: TLSDynamicConfig{ + CA: TLSDynamicCAConfig{SelfSign: &TLSSelfSignCAConfig{PrivateKeyFile: "ca.key"}}, + }, + wantErr: true, + errContains: "ca: selfSign: required field is missing: certificateFile", + }, + { + name: "selfSign missing private key", + dynamic: TLSDynamicConfig{ + CA: TLSDynamicCAConfig{SelfSign: &TLSSelfSignCAConfig{CertificateFile: "ca.crt"}}, + }, + wantErr: true, + errContains: "ca: selfSign: required field is missing: privateKeyFile", + }, + { + name: "negative duration", + dynamic: TLSDynamicConfig{ + CA: validCA, + Cert: TLSDynamicCertConfig{Duration: -time.Hour}, + }, + wantErr: true, + errContains: "cert: duration must be non-negative: duration", + }, + { + name: "negative renewBefore", + dynamic: TLSDynamicConfig{ + CA: validCA, + Cert: TLSDynamicCertConfig{RenewBefore: -time.Hour}, + }, + wantErr: true, + errContains: "cert: duration must be non-negative: renewBefore", + }, + { + name: "renewBefore not shorter than duration", + dynamic: TLSDynamicConfig{ + CA: validCA, + Cert: TLSDynamicCertConfig{Duration: 8 * time.Hour, RenewBefore: 8 * time.Hour}, + }, + wantErr: true, + errContains: "'renewBefore' must be shorter than 'duration'", + }, + { + name: "invalid key type", + dynamic: TLSDynamicConfig{ + CA: validCA, + Cert: TLSDynamicCertConfig{KeyType: "ed25519"}, + }, + wantErr: true, + errContains: "invalid TLS key type", + }, + { + name: "invalid ecdsa key bits", + dynamic: TLSDynamicConfig{ + CA: validCA, + Cert: TLSDynamicCertConfig{KeyType: "ecdsa", KeyBits: 2048}, + }, + wantErr: true, + errContains: "invalid TLS key bits: ECDSA 2048", + }, + { + name: "invalid rsa key bits", + dynamic: TLSDynamicConfig{ + CA: validCA, + Cert: TLSDynamicCertConfig{KeyType: "rsa", KeyBits: 256}, + }, + wantErr: true, + errContains: "invalid TLS key bits: RSA 256", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.dynamic.Validate() + if tt.wantErr { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.errContains) + } else { + require.NoError(t, err) + } + }) + } +} + +func TestTLSDynamicCertConfig_Defaults(t *testing.T) { + var cert TLSDynamicCertConfig + + assert.Equal(t, 24*time.Hour, cert.GetDuration()) + assert.Equal(t, 8*time.Hour, cert.GetRenewBefore()) + assert.Equal(t, "ecdsa", cert.GetKeyType()) + assert.Equal(t, 256, cert.GetKeyBits()) + + rsaCert := TLSDynamicCertConfig{KeyType: "rsa"} + assert.Equal(t, 2048, rsaCert.GetKeyBits()) +} + func TestKubernetesConfig_Validate(t *testing.T) { tests := []struct { name string diff --git a/internal/connect/cert.go b/internal/connect/cert.go new file mode 100644 index 00000000..6c402650 --- /dev/null +++ b/internal/connect/cert.go @@ -0,0 +1,195 @@ +// Copyright (c) Twingate Inc. +// SPDX-License-Identifier: MPL-2.0 + +package connect + +import ( + "context" + "crypto" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/rsa" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "errors" + "fmt" + "math/big" + "net" + "sync" + "time" + + "go.uber.org/zap" + + "gateway/internal/config" +) + +var ( + errNotCACertificate = errors.New("certificate is not a certificate authority") + errCAKeyNotSigner = errors.New("CA private key does not implement crypto.Signer") + errUnsupportedKeyType = errors.New("unsupported key type") + errUnsupportedKeyBits = errors.New("unsupported key bits") +) + +// clockSkewBuffer backdates minted certificates to tolerate downstream clients +// with slightly-behind clocks. +const clockSkewBuffer = 30 * time.Second + +var serialNumberLimit = new(big.Int).Lsh(big.NewInt(1), 128) + +// DynamicCert mints short-lived leaf certificates signed by the configured +// CA, caching one certificate per requested host and re-minting a fresh one +// once the cached certificate enters the renewal window. +type DynamicCert struct { + caCert *x509.Certificate + caKey crypto.Signer + cert config.TLSDynamicCertConfig + logger *zap.Logger + + mu sync.Mutex + cache map[string]*tls.Certificate +} + +func NewDynamicCert(cfg config.TLSDynamicConfig, logger *zap.Logger) (*DynamicCert, error) { + if cfg.CA.SelfSign == nil { + return nil, config.ErrMissingTLSCAConfig + } + + pair, err := tls.LoadX509KeyPair(cfg.CA.SelfSign.CertificateFile, cfg.CA.SelfSign.PrivateKeyFile) + if err != nil { + return nil, fmt.Errorf("failed to load CA key pair: %w", err) + } + + caCert, err := x509.ParseCertificate(pair.Certificate[0]) + if err != nil { + return nil, fmt.Errorf("failed to parse CA certificate: %w", err) + } + + if !caCert.IsCA { + return nil, errNotCACertificate + } + + caKey, ok := pair.PrivateKey.(crypto.Signer) + if !ok { + return nil, errCAKeyNotSigner + } + + return &DynamicCert{ + caCert: caCert, + caKey: caKey, + cert: cfg.Cert, + logger: logger, + cache: make(map[string]*tls.Certificate), + }, nil +} + +// Run implements CertProvider; dynamic mode has no background maintenance. +func (c *DynamicCert) Run(_ context.Context) {} + +// GetCertificate implements tls.Config.GetCertificate for handshakes that +// happen before the CONNECT target is known. It mints for the SNI host when +// the client sends one, falling back to the connection's local IP for clients +// that don't (IP-dialed clients and health probes). +func (c *DynamicCert) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certificate, error) { + host := hello.ServerName + if host == "" { + var err error + + host, _, err = net.SplitHostPort(hello.Conn.LocalAddr().String()) + if err != nil { + return nil, fmt.Errorf("failed to parse local address: %w", err) + } + } + + return c.GetCertificateForHost(host) +} + +// GetCertificateForHost returns a certificate for the requested host, minting +// a new one when none is cached or the cached one is inside the renewal window. +func (c *DynamicCert) GetCertificateForHost(host string) (*tls.Certificate, error) { + c.mu.Lock() + defer c.mu.Unlock() + + if cert, ok := c.cache[host]; ok && time.Now().Before(cert.Leaf.NotAfter.Add(-c.cert.GetRenewBefore())) { + return cert, nil + } + + cert, err := c.mint(host) + if err != nil { + return nil, err + } + + c.cache[host] = cert + + return cert, nil +} + +func (c *DynamicCert) mint(host string) (*tls.Certificate, error) { + key, err := c.generateKey() + if err != nil { + return nil, fmt.Errorf("failed to generate leaf key: %w", err) + } + + serial, err := rand.Int(rand.Reader, serialNumberLimit) + if err != nil { + return nil, fmt.Errorf("failed to generate serial number: %w", err) + } + + now := time.Now() + template := &x509.Certificate{ + SerialNumber: serial, + Subject: pkix.Name{CommonName: host}, + NotBefore: now.Add(-clockSkewBuffer), + NotAfter: now.Add(c.cert.GetDuration()), + KeyUsage: x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + } + + if ip := net.ParseIP(host); ip != nil { + template.IPAddresses = []net.IP{ip} + } else { + template.DNSNames = []string{host} + } + + leafDER, err := x509.CreateCertificate(rand.Reader, template, c.caCert, key.Public(), c.caKey) + if err != nil { + return nil, fmt.Errorf("failed to sign leaf certificate: %w", err) + } + + leaf, err := x509.ParseCertificate(leafDER) + if err != nil { + return nil, fmt.Errorf("failed to parse leaf certificate: %w", err) + } + + c.logger.Info("Minted downstream certificate", + zap.String("host", host), + zap.Time("not_after", leaf.NotAfter), + ) + + return &tls.Certificate{ + Certificate: [][]byte{leafDER, c.caCert.Raw}, + PrivateKey: key, + Leaf: leaf, + }, nil +} + +func (c *DynamicCert) generateKey() (crypto.Signer, error) { + switch c.cert.GetKeyType() { + case "ecdsa": + switch c.cert.GetKeyBits() { + case 256: + return ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + case 384: + return ecdsa.GenerateKey(elliptic.P384(), rand.Reader) + case 521: + return ecdsa.GenerateKey(elliptic.P521(), rand.Reader) + default: + return nil, fmt.Errorf("%w: ECDSA %d", errUnsupportedKeyBits, c.cert.GetKeyBits()) + } + case "rsa": + return rsa.GenerateKey(rand.Reader, c.cert.GetKeyBits()) + default: + return nil, fmt.Errorf("%w: %s", errUnsupportedKeyType, c.cert.GetKeyType()) + } +} diff --git a/internal/connect/cert_provider.go b/internal/connect/cert_provider.go new file mode 100644 index 00000000..de866d06 --- /dev/null +++ b/internal/connect/cert_provider.go @@ -0,0 +1,46 @@ +// Copyright (c) Twingate Inc. +// SPDX-License-Identifier: MPL-2.0 + +package connect + +import ( + "context" + "crypto/tls" + "fmt" + + "go.uber.org/zap" + + "gateway/internal/config" +) + +// CertProvider supplies the downstream serving certificates and runs any +// background maintenance (file watching in static mode). +type CertProvider interface { + // Run runs background maintenance until the context is canceled. + Run(ctx context.Context) + + // GetCertificate serves the outer, pre-CONNECT handshake. + GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certificate, error) + + // GetCertificateForHost returns the certificate presented for the + // validated CONNECT host on the inner handshake. + GetCertificateForHost(host string) (*tls.Certificate, error) +} + +// newCertProviderFromConfig creates a CertProvider based on the provided +// configuration. +func newCertProviderFromConfig(tlsCfg config.TLSConfig, logger *zap.Logger) (CertProvider, error) { + switch { + case tlsCfg.Static != nil: + return NewCertReloader(tlsCfg.Static.CertificateFile, tlsCfg.Static.PrivateKeyFile, logger), nil + case tlsCfg.Dynamic != nil: + dynamicCert, err := NewDynamicCert(*tlsCfg.Dynamic, logger) + if err != nil { + return nil, fmt.Errorf("failed to create dynamic cert: %w", err) + } + + return dynamicCert, nil + default: + return nil, config.ErrMissingTLSConfig + } +} diff --git a/internal/connect/cert_provider_test.go b/internal/connect/cert_provider_test.go new file mode 100644 index 00000000..7be0e76e --- /dev/null +++ b/internal/connect/cert_provider_test.go @@ -0,0 +1,78 @@ +// Copyright (c) Twingate Inc. +// SPDX-License-Identifier: MPL-2.0 + +package connect + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "gateway/internal/config" +) + +func TestNewCertProviderFromConfig(t *testing.T) { + ca := generateCACert(t) + caFile, caKeyFile := createCertFiles(t, ca) + + tests := []struct { + name string + tlsCfg config.TLSConfig + wantType any + wantErr error + errContains string + }{ + { + name: "static", + tlsCfg: config.TLSConfig{Static: &config.TLSStaticConfig{CertificateFile: caFile, PrivateKeyFile: caKeyFile}}, + wantType: &CertReloader{}, + }, + { + name: "dynamic", + tlsCfg: config.TLSConfig{Dynamic: &config.TLSDynamicConfig{ + CA: config.TLSDynamicCAConfig{ + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: caFile, PrivateKeyFile: caKeyFile}, + }, + }}, + wantType: &DynamicCert{}, + }, + { + name: "dynamic with missing CA files", + tlsCfg: config.TLSConfig{Dynamic: &config.TLSDynamicConfig{ + CA: config.TLSDynamicCAConfig{ + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "missing.crt", PrivateKeyFile: "missing.key"}, + }, + }}, + errContains: "failed to create dynamic cert", + }, + { + name: "neither static nor dynamic", + wantErr: config.ErrMissingTLSConfig, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + provider, err := newCertProviderFromConfig(tt.tlsCfg, zap.NewNop()) + + if tt.wantErr != nil { + assert.ErrorIs(t, err, tt.wantErr) + + return + } + + if tt.errContains != "" { + assert.ErrorContains(t, err, tt.errContains) + + return + } + + require.NoError(t, err) + assert.IsType(t, tt.wantType, provider) + + provider.Run(t.Context()) + }) + } +} diff --git a/internal/connect/cert_reloader.go b/internal/connect/cert_reloader.go index 8fcf912d..59264658 100644 --- a/internal/connect/cert_reloader.go +++ b/internal/connect/cert_reloader.go @@ -47,6 +47,12 @@ func (cr *CertReloader) GetCertificate(_ *tls.ClientHelloInfo) (*tls.Certificate return cr.cert, nil } +// GetCertificateForHost implements CertProvider, the configured certificate is +// served regardless of the requested host. +func (cr *CertReloader) GetCertificateForHost(_ string) (*tls.Certificate, error) { + return cr.GetCertificate(nil) +} + func (cr *CertReloader) load() error { cert, err := tls.LoadX509KeyPair(cr.certFile, cr.keyFile) if err != nil { diff --git a/internal/connect/cert_test.go b/internal/connect/cert_test.go new file mode 100644 index 00000000..fbb041c1 --- /dev/null +++ b/internal/connect/cert_test.go @@ -0,0 +1,284 @@ +// Copyright (c) Twingate Inc. +// SPDX-License-Identifier: MPL-2.0 + +package connect + +import ( + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/rsa" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "net" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "gateway/internal/config" + "gateway/test/data" +) + +func generateCACert(t *testing.T) tls.Certificate { + t.Helper() + + privateKey, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + + template := x509.Certificate{ + Subject: pkix.Name{CommonName: "test-ca"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(24 * time.Hour), + KeyUsage: x509.KeyUsageCertSign, + IsCA: true, + BasicConstraintsValid: true, + } + + derBytes, err := x509.CreateCertificate(rand.Reader, &template, &template, &privateKey.PublicKey, privateKey) + require.NoError(t, err) + + return tls.Certificate{ + Certificate: [][]byte{derBytes}, + PrivateKey: privateKey, + } +} + +func newTestCert(t *testing.T, certCfg config.TLSDynamicCertConfig) (*DynamicCert, *x509.CertPool) { + t.Helper() + + ca := generateCACert(t) + certFile, keyFile := createCertFiles(t, ca) + + cert, err := NewDynamicCert(config.TLSDynamicConfig{ + CA: config.TLSDynamicCAConfig{ + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: certFile, PrivateKeyFile: keyFile}, + }, + Cert: certCfg, + }, zap.NewNop()) + require.NoError(t, err) + + caCert, err := x509.ParseCertificate(ca.Certificate[0]) + require.NoError(t, err) + + pool := x509.NewCertPool() + pool.AddCert(caCert) + + return cert, pool +} + +func TestDynamicCert_GetCertificate(t *testing.T) { + cert, pool := newTestCert(t, config.TLSDynamicCertConfig{}) + + minted, err := cert.GetCertificateForHost("app.internal") + require.NoError(t, err) + + assert.Equal(t, "app.internal", minted.Leaf.Subject.CommonName) + assert.Equal(t, []string{"app.internal"}, minted.Leaf.DNSNames) + assert.WithinDuration(t, time.Now().Add(24*time.Hour), minted.Leaf.NotAfter, time.Minute) + + key, ok := minted.PrivateKey.(*ecdsa.PrivateKey) + require.True(t, ok, "expected an ECDSA leaf key by default") + assert.Equal(t, elliptic.P256(), key.Curve) + + _, err = minted.Leaf.Verify(x509.VerifyOptions{ + DNSName: "app.internal", + Roots: pool, + KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + }) + assert.NoError(t, err, "leaf should verify against the CA for the requested host") +} + +func TestDynamicCert_GetCertificate_IPHost(t *testing.T) { + cert, _ := newTestCert(t, config.TLSDynamicCertConfig{}) + + minted, err := cert.GetCertificateForHost("10.0.0.5") + require.NoError(t, err) + + require.Len(t, minted.Leaf.IPAddresses, 1) + assert.True(t, minted.Leaf.IPAddresses[0].Equal(net.ParseIP("10.0.0.5"))) + assert.Empty(t, minted.Leaf.DNSNames) +} + +type fakeAddrConn struct { + net.Conn + + addr net.Addr +} + +func (c fakeAddrConn) LocalAddr() net.Addr { return c.addr } + +func TestDynamicCert_GetCertificate_ClientHelloSNI(t *testing.T) { + cert, _ := newTestCert(t, config.TLSDynamicCertConfig{}) + + minted, err := cert.GetCertificate(&tls.ClientHelloInfo{ServerName: "app.internal"}) + require.NoError(t, err) + + assert.Equal(t, []string{"app.internal"}, minted.Leaf.DNSNames) +} + +func TestDynamicCert_GetCertificate_ClientHelloNoSNIFallsBackToLocalAddr(t *testing.T) { + cert, _ := newTestCert(t, config.TLSDynamicCertConfig{}) + + hello := &tls.ClientHelloInfo{ + Conn: fakeAddrConn{addr: &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 8443}}, + } + + minted, err := cert.GetCertificate(hello) + require.NoError(t, err) + + require.Len(t, minted.Leaf.IPAddresses, 1) + assert.True(t, minted.Leaf.IPAddresses[0].Equal(net.ParseIP("127.0.0.1"))) +} + +func TestDynamicCert_GetCertificate_CachesPerHost(t *testing.T) { + cert, _ := newTestCert(t, config.TLSDynamicCertConfig{}) + + first, err := cert.GetCertificateForHost("app.internal") + require.NoError(t, err) + + second, err := cert.GetCertificateForHost("app.internal") + require.NoError(t, err) + assert.Same(t, first, second) + + other, err := cert.GetCertificateForHost("other.internal") + require.NoError(t, err) + assert.NotSame(t, first, other) +} + +func TestDynamicCert_GetCertificate_RenewsInsideWindow(t *testing.T) { + // renewBefore longer than duration puts a freshly minted certificate + // inside the renewal window immediately, forcing a re-mint on every call. + cert, _ := newTestCert(t, config.TLSDynamicCertConfig{ + Duration: time.Hour, + RenewBefore: 2 * time.Hour, + }) + + first, err := cert.GetCertificateForHost("app.internal") + require.NoError(t, err) + + second, err := cert.GetCertificateForHost("app.internal") + require.NoError(t, err) + + assert.NotSame(t, first, second) + assert.NotEqual(t, first.Leaf.SerialNumber, second.Leaf.SerialNumber) +} + +func TestDynamicCert_GetCertificate_RSAKey(t *testing.T) { + cert, _ := newTestCert(t, config.TLSDynamicCertConfig{KeyType: "rsa", KeyBits: 2048}) + + minted, err := cert.GetCertificateForHost("app.internal") + require.NoError(t, err) + + _, ok := minted.PrivateKey.(*rsa.PrivateKey) + assert.True(t, ok, "expected an RSA leaf key") +} + +func TestDynamicCert_GetCertificate_UnsupportedKeyConfig(t *testing.T) { + tests := []struct { + name string + certCfg config.TLSDynamicCertConfig + wantErr error + }{ + { + name: "unsupported key type", + certCfg: config.TLSDynamicCertConfig{KeyType: "ed25519"}, + wantErr: errUnsupportedKeyType, + }, + { + name: "unsupported ecdsa key bits", + certCfg: config.TLSDynamicCertConfig{KeyType: "ecdsa", KeyBits: 512}, + wantErr: errUnsupportedKeyBits, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cert, _ := newTestCert(t, tt.certCfg) + + _, err := cert.GetCertificateForHost("app.internal") + assert.ErrorIs(t, err, tt.wantErr) + }) + } +} + +// TestNewDynamicCert_ProxyFixture guards the test/data/proxy fixture staying a CA +// certificate — tools/local uses it as the dynamic signing CA. +func TestNewDynamicCert_ProxyFixture(t *testing.T) { + cert, err := NewDynamicCert(config.TLSDynamicConfig{ + CA: config.TLSDynamicCAConfig{ + SelfSign: &config.TLSSelfSignCAConfig{ + CertificateFile: "../../test/data/proxy/tls.crt", + PrivateKeyFile: "../../test/data/proxy/tls.key", + }, + }, + }, zap.NewNop()) + require.NoError(t, err) + + minted, err := cert.GetCertificateForHost("127.0.0.1") + require.NoError(t, err) + + pool := x509.NewCertPool() + pool.AppendCertsFromPEM(data.ProxyCert) + + _, err = minted.Leaf.Verify(x509.VerifyOptions{ + DNSName: "127.0.0.1", + Roots: pool, + KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + }) + assert.NoError(t, err, "minted leaf should verify against the proxy fixture CA") +} + +func TestNewDynamicCert_Errors(t *testing.T) { + caFile, _ := createCertFiles(t, generateCACert(t)) + nonCAFile, nonCAKeyFile := createCertFiles(t, generateCert(t)) + _, otherKeyFile := createCertFiles(t, generateCACert(t)) + + tests := []struct { + name string + selfSign *config.TLSSelfSignCAConfig + wantErr error + errContains string + }{ + { + name: "missing selfSign", + wantErr: config.ErrMissingTLSCAConfig, + }, + { + name: "missing files", + selfSign: &config.TLSSelfSignCAConfig{CertificateFile: "missing.crt", PrivateKeyFile: "missing.key"}, + errContains: "failed to load CA key pair", + }, + { + name: "mismatched certificate and key", + selfSign: &config.TLSSelfSignCAConfig{CertificateFile: caFile, PrivateKeyFile: otherKeyFile}, + errContains: "failed to load CA key pair", + }, + { + name: "certificate is not a CA", + selfSign: &config.TLSSelfSignCAConfig{CertificateFile: nonCAFile, PrivateKeyFile: nonCAKeyFile}, + wantErr: errNotCACertificate, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := NewDynamicCert(config.TLSDynamicConfig{ + CA: config.TLSDynamicCAConfig{SelfSign: tt.selfSign}, + }, zap.NewNop()) + + require.Error(t, err) + + if tt.wantErr != nil { + require.ErrorIs(t, err, tt.wantErr) + } + + if tt.errContains != "" { + assert.Contains(t, err.Error(), tt.errContains) + } + }) + } +} diff --git a/internal/connect/conn.go b/internal/connect/conn.go index 88d915b2..8a3e85aa 100644 --- a/internal/connect/conn.go +++ b/internal/connect/conn.go @@ -51,13 +51,15 @@ type ProxyConn struct { net.Conn TLSConfig *tls.Config + CertProvider CertProvider ConnectValidator Validator Logger *zap.Logger - ID string - Address string - Claims *token.GATClaims - Token string + ID string + Address string + DownstreamAddress string + Claims *token.GATClaims + Token string Timer *time.Timer Mu sync.Mutex @@ -66,10 +68,18 @@ type ProxyConn struct { once sync.Once } -func NewProxyConn(conn net.Conn, tlsConfig *tls.Config, validator Validator, logger *zap.Logger, metrics *ProxyConnMetrics) *ProxyConn { +func NewProxyConn( + conn net.Conn, + tlsConfig *tls.Config, + certProvider CertProvider, + validator Validator, + logger *zap.Logger, + metrics *ProxyConnMetrics, +) *ProxyConn { return &ProxyConn{ Conn: conn, TLSConfig: tlsConfig, + CertProvider: certProvider, ConnectValidator: validator, Logger: logger, tracker: NewProxyConnMetricsTracker(ConnCategoryUnknown, metrics), @@ -225,7 +235,14 @@ func (p *ProxyConn) Authenticate() error { } func (p *ProxyConn) UpgradeToTLS() error { - tlsConn := tls.Server(p.Conn, p.TLSConfig) + tlsConfig, err := p.getTLSConfig() + if err != nil { + p.Logger.Error("failed to prepare TLS config for upgrade", zap.Error(err)) + + return err + } + + tlsConn := tls.Server(p.Conn, tlsConfig) if err := tlsConn.Handshake(); err != nil { p.Logger.Error("failed to upgrade TLS", zap.Error(err)) @@ -238,9 +255,31 @@ func (p *ProxyConn) UpgradeToTLS() error { return nil } +// getTLSConfig pins the certificate presented to the downstream client +// for the CONNECT-requested host: minted in dynamic mode, the configured +// certificate in static mode. +func (p *ProxyConn) getTLSConfig() (*tls.Config, error) { + host, _, err := net.SplitHostPort(p.DownstreamAddress) + if err != nil { + return nil, fmt.Errorf("failed to parse downstream address %q: %w", p.DownstreamAddress, err) + } + + cert, err := p.CertProvider.GetCertificateForHost(host) + if err != nil { + return nil, fmt.Errorf("failed to get certificate for %q: %w", host, err) + } + + tlsConfig := p.TLSConfig.Clone() + tlsConfig.GetCertificate = nil + tlsConfig.Certificates = []tls.Certificate{*cert} + + return tlsConfig, nil +} + func (p *ProxyConn) setConnectInfo(connectInfo Info) { p.ID = connectInfo.ConnID p.Address = connectInfo.Address + p.DownstreamAddress = connectInfo.DownstreamAddress p.Claims = connectInfo.Claims p.Token = connectInfo.Token p.Timer = time.AfterFunc(time.Until(connectInfo.Claims.ExpiresAt.Time), func() { diff --git a/internal/connect/conn_test.go b/internal/connect/conn_test.go index 0bc06c34..2519527b 100644 --- a/internal/connect/conn_test.go +++ b/internal/connect/conn_test.go @@ -25,6 +25,7 @@ import ( promtestutil "github.com/prometheus/client_golang/prometheus/testutil" + "gateway/internal/config" "gateway/internal/token" "gateway/test/data" ) @@ -479,6 +480,242 @@ func TestProxyConn_Authenticate_FailedValidation(t *testing.T) { <-done } +// upgradeToTLSHandshake runs proxyConn.UpgradeToTLS against a TLS client +// connected over TCP and returns the leaf certificate the client saw. +func upgradeToTLSHandshake(t *testing.T, proxyConn *ProxyConn, clientTLSConfig *tls.Config) (*x509.Certificate, error) { + t.Helper() + + listener, addr := startMockListener(t) + defer listener.Close() + + type clientResult struct { + leaf *x509.Certificate + err error + } + + clientCh := make(chan clientResult, 1) + + go func() { + conn, err := net.Dial("tcp", addr) + if err != nil { + clientCh <- clientResult{nil, err} + + return + } + + defer conn.Close() + + tlsConn := tls.Client(conn, clientTLSConfig) + if err := tlsConn.Handshake(); err != nil { + clientCh <- clientResult{nil, err} + + return + } + + clientCh <- clientResult{tlsConn.ConnectionState().PeerCertificates[0], nil} + }() + + conn, err := listener.Accept() + require.NoError(t, err) + + proxyConn.Conn = conn + + serverErr := proxyConn.UpgradeToTLS() + if serverErr != nil { + _ = conn.Close() + + <-clientCh + + return nil, serverErr + } + + result := <-clientCh + require.NoError(t, result.err) + + return result.leaf, nil +} + +func staticServerTLSConfig(t *testing.T) (*tls.Config, tls.Certificate) { + t.Helper() + + serverCert, err := tls.X509KeyPair(data.ProxyCert, data.ProxyKey) + require.NoError(t, err) + + return &tls.Config{ + Certificates: []tls.Certificate{serverCert}, + MinVersion: tls.VersionTLS13, + }, serverCert +} + +func TestProxyConn_UpgradeToTLS_KubernetesMintedCert(t *testing.T) { + cert, caPool := newTestCert(t, config.TLSDynamicCertConfig{}) + serverTLSConfig, _ := staticServerTLSConfig(t) + + proxyConn := &ProxyConn{ + TLSConfig: serverTLSConfig, + CertProvider: cert, + DownstreamAddress: "k8s.internal:443", + Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeKubernetes}}, + Logger: zap.NewNop(), + } + + leaf, err := upgradeToTLSHandshake(t, proxyConn, &tls.Config{ + ServerName: "k8s.internal", + RootCAs: caPool, + MinVersion: tls.VersionTLS13, + }) + require.NoError(t, err) + + assert.Equal(t, []string{"k8s.internal"}, leaf.DNSNames) +} + +func TestProxyConn_UpgradeToTLS_StaticPinsStaticCert(t *testing.T) { + serverTLSConfig, serverCert := staticServerTLSConfig(t) + + certReloader := NewCertReloader("../../test/data/proxy/tls.crt", "../../test/data/proxy/tls.key", zap.NewNop()) + certReloader.Run(t.Context()) + requireCertReloader(t, certReloader, serverCert) + + caCertPool := x509.NewCertPool() + caCertPool.AppendCertsFromPEM(data.ProxyCert) + + proxyConn := &ProxyConn{ + TLSConfig: serverTLSConfig, + CertProvider: certReloader, + DownstreamAddress: "app.internal:443", + Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp}}, + Logger: zap.NewNop(), + } + + leaf, err := upgradeToTLSHandshake(t, proxyConn, &tls.Config{ + ServerName: "127.0.0.1", + RootCAs: caCertPool, + MinVersion: tls.VersionTLS13, + }) + require.NoError(t, err) + + assert.Equal(t, serverCert.Certificate[0], leaf.Raw) +} + +func TestProxyConn_UpgradeToTLS_TerminatesTLSAtGateway(t *testing.T) { + cert, caPool := newTestCert(t, config.TLSDynamicCertConfig{}) + serverTLSConfig, _ := staticServerTLSConfig(t) + + listener, addr := startMockListener(t) + defer listener.Close() + + const request = "GET / HTTP/1.1\r\n\r\n" + + done := make(chan struct{}) + + go func() { + defer close(done) + + conn, err := net.Dial("tcp", addr) + assert.NoError(t, err) + + defer conn.Close() + + tlsConn := tls.Client(conn, &tls.Config{ + ServerName: "app.internal", + RootCAs: caPool, + MinVersion: tls.VersionTLS13, + }) + assert.NoError(t, tlsConn.Handshake()) + + _, err = tlsConn.Write([]byte(request)) + assert.NoError(t, err) + }() + + conn, err := listener.Accept() + require.NoError(t, err) + + proxyConn := &ProxyConn{ + Conn: conn, + TLSConfig: serverTLSConfig, + CertProvider: cert, + DownstreamAddress: "app.internal:443", + Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp}}, + Logger: zap.NewNop(), + } + + require.NoError(t, proxyConn.UpgradeToTLS()) + + // The gateway reads the decrypted plaintext through the upgraded connection. + buf := make([]byte, len(request)) + _, err = io.ReadFull(proxyConn, buf) + require.NoError(t, err) + assert.Equal(t, request, string(buf)) + + <-done +} + +func TestProxyConn_UpgradeToTLS_HandshakeError(t *testing.T) { + cert, _ := newTestCert(t, config.TLSDynamicCertConfig{}) + serverTLSConfig, _ := staticServerTLSConfig(t) + + // The client trusts a different CA, so it rejects the minted certificate + // and the server-side handshake fails. + wrongPool := x509.NewCertPool() + wrongPool.AppendCertsFromPEM(data.ProxyCert) + + proxyConn := &ProxyConn{ + TLSConfig: serverTLSConfig, + CertProvider: cert, + DownstreamAddress: "app.internal:443", + Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp}}, + Logger: zap.NewNop(), + } + + _, err := upgradeToTLSHandshake(t, proxyConn, &tls.Config{ + ServerName: "app.internal", + RootCAs: wrongPool, + MinVersion: tls.VersionTLS13, + }) + + require.Error(t, err) +} + +func TestProxyConn_UpgradeToTLS_MintError(t *testing.T) { + cert, caPool := newTestCert(t, config.TLSDynamicCertConfig{KeyType: "ed25519"}) + serverTLSConfig, _ := staticServerTLSConfig(t) + + proxyConn := &ProxyConn{ + TLSConfig: serverTLSConfig, + CertProvider: cert, + DownstreamAddress: "app.internal:443", + Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp}}, + Logger: zap.NewNop(), + } + + _, err := upgradeToTLSHandshake(t, proxyConn, &tls.Config{ + ServerName: "app.internal", + RootCAs: caPool, + MinVersion: tls.VersionTLS13, + }) + + require.Error(t, err) + assert.ErrorIs(t, err, errUnsupportedKeyType) +} + +func TestProxyConn_UpgradeToTLS_MalformedDownstreamAddress(t *testing.T) { + cert, _ := newTestCert(t, config.TLSDynamicCertConfig{}) + serverTLSConfig, _ := staticServerTLSConfig(t) + + proxyConn := &ProxyConn{ + TLSConfig: serverTLSConfig, + CertProvider: cert, + DownstreamAddress: "app.internal", + Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp}}, + Logger: zap.NewNop(), + } + + err := proxyConn.UpgradeToTLS() + + require.Error(t, err) + assert.ErrorContains(t, err, `failed to parse downstream address "app.internal"`) +} + func TestIsHealthCheckRequest(t *testing.T) { testCases := []struct { name string diff --git a/internal/connect/connect.go b/internal/connect/connect.go index ad724b45..335265c8 100644 --- a/internal/connect/connect.go +++ b/internal/connect/connect.go @@ -28,10 +28,11 @@ const AuthSignatureHeaderKey string = "X-Token-Signature" const ConnIDHeaderKey string = "X-Connection-Id" type Info struct { - Address string - Claims *token.GATClaims - ConnID string - Token string + Address string + DownstreamAddress string + Claims *token.GATClaims + ConnID string + Token string } type HTTPError struct { @@ -140,10 +141,11 @@ func (v *MessageValidator) ParseConnect(req *http.Request, ekm []byte) (connectI } return Info{ - Address: address, - Claims: gatClaims, - ConnID: connID, - Token: bearerToken, + Address: address, + DownstreamAddress: req.RequestURI, + Claims: gatClaims, + ConnID: connID, + Token: bearerToken, }, nil } diff --git a/internal/connect/listener.go b/internal/connect/listener.go index d7974407..302ba55a 100644 --- a/internal/connect/listener.go +++ b/internal/connect/listener.go @@ -73,7 +73,7 @@ type Listener struct { channels map[token.ResourceType]chan<- Conn tokenParser *token.Parser - certReloader *CertReloader + certProvider CertProvider tlsConfig *tls.Config connectValidator Validator logger *zap.Logger @@ -101,12 +101,15 @@ func NewListener( return nil, fmt.Errorf("failed to create token parser: %w", err) } - certReloader := NewCertReloader(tlsCfg.Static.CertificateFile, tlsCfg.Static.PrivateKeyFile, logger) + certProvider, err := newCertProviderFromConfig(tlsCfg, logger) + if err != nil { + return nil, err + } tlsConfig := &tls.Config{ MinVersion: tls.VersionTLS13, MaxVersion: tls.VersionTLS13, - GetCertificate: certReloader.GetCertificate, + GetCertificate: certProvider.GetCertificate, } connectValidator := &MessageValidator{ @@ -118,13 +121,13 @@ func NewListener( l := &Listener{ channels: channels, tokenParser: tokenParser, - certReloader: certReloader, + certProvider: certProvider, tlsConfig: tlsConfig, connectValidator: connectValidator, logger: logger, metrics: metrics, proxyConnFactory: func(conn net.Conn, tlsConfig *tls.Config, connectValidator Validator, logger *zap.Logger) Conn { - return NewProxyConn(conn, tlsConfig, connectValidator, logger, metrics) + return NewProxyConn(conn, tlsConfig, certProvider, connectValidator, logger, metrics) }, } @@ -135,7 +138,7 @@ func NewListener( // The caller owns the listener and is responsible for closing it. // Serve closes the channels when it returns. func (l *Listener) Serve(ctx context.Context, listener net.Listener) error { - l.certReloader.Run(ctx) + l.certProvider.Run(ctx) var wg sync.WaitGroup diff --git a/internal/connect/listener_test.go b/internal/connect/listener_test.go index 0044ba88..d54f955d 100644 --- a/internal/connect/listener_test.go +++ b/internal/connect/listener_test.go @@ -134,7 +134,7 @@ func createTestListenerWithChannels(t *testing.T) *testListenerFixtures { channels: channels, logger: logger, metrics: CreateProxyConnMetrics(registry), - certReloader: certReloader, + certProvider: certReloader, } return &testListenerFixtures{ @@ -342,7 +342,7 @@ func TestListener_UnsupportedResourceType(t *testing.T) { channels: channels, logger: logger, metrics: CreateProxyConnMetrics(registry), - certReloader: certReloader, + certProvider: certReloader, } sshClaims := createClaims(t, token.ResourceTypeSSH) @@ -399,7 +399,7 @@ func TestListener_Serve_GracefulShutdown(t *testing.T) { channels: channels, logger: logger, metrics: CreateProxyConnMetrics(prometheus.NewRegistry()), - certReloader: NewCertReloader("../../test/data/proxy/tls.crt", "../../test/data/proxy/tls.key", logger), + certProvider: NewCertReloader("../../test/data/proxy/tls.crt", "../../test/data/proxy/tls.key", logger), } listener.proxyConnFactory = func(conn net.Conn, _ *tls.Config, _ Validator, _ *zap.Logger) Conn { diff --git a/internal/httpproxy/proxy_test.go b/internal/httpproxy/proxy_test.go index 5ede9e01..b1c80b37 100644 --- a/internal/httpproxy/proxy_test.go +++ b/internal/httpproxy/proxy_test.go @@ -31,7 +31,7 @@ func (l *mockConnListener) Accept() (net.Conn, error) { } connMetrics := connect.CreateProxyConnMetrics(prometheus.NewRegistry()) - proxyConn := connect.NewProxyConn(conn, nil, nil, zap.NewNop(), connMetrics) + proxyConn := connect.NewProxyConn(conn, nil, nil, nil, zap.NewNop(), connMetrics) proxyConn.ID = "test-conn" proxyConn.Address = "localhost" proxyConn.Claims = &token.GATClaims{ @@ -45,7 +45,7 @@ func (l *mockConnListener) Accept() (net.Conn, error) { func TestProxyConnFromContext(t *testing.T) { t.Run("Returns ProxyConn from context", func(t *testing.T) { connMetrics := connect.CreateProxyConnMetrics(prometheus.NewRegistry()) - expected := connect.NewProxyConn(nil, nil, nil, zap.NewNop(), connMetrics) + expected := connect.NewProxyConn(nil, nil, nil, nil, zap.NewNop(), connMetrics) ctx := context.WithValue(t.Context(), ConnContextKey{}, expected) diff --git a/internal/kuberneteshandler/handler_test.go b/internal/kuberneteshandler/handler_test.go index f03dd802..b19c609a 100644 --- a/internal/kuberneteshandler/handler_test.go +++ b/internal/kuberneteshandler/handler_test.go @@ -20,7 +20,7 @@ import ( func TestRewrite(t *testing.T) { connMetrics := connect.CreateProxyConnMetrics(prometheus.NewRegistry()) - conn := connect.NewProxyConn(nil, nil, nil, zap.NewNop(), connMetrics) + conn := connect.NewProxyConn(nil, nil, nil, nil, zap.NewNop(), connMetrics) conn.Address = "kubernetes.default.svc" conn.Claims = &token.GATClaims{ User: token.User{ diff --git a/internal/sshhandler/proxy_test.go b/internal/sshhandler/proxy_test.go index 7cf13ec7..e6ee33ae 100644 --- a/internal/sshhandler/proxy_test.go +++ b/internal/sshhandler/proxy_test.go @@ -636,7 +636,7 @@ func (s *echoServer) identity() (username string, userCert *ssh.Certificate) { // newProxyConn wraps conn in the connect.ProxyConn the proxy serves, with test GAT claims and // the address of the upstream the proxy dials. func newProxyConn(conn net.Conn, upstreamAddr string) *connect.ProxyConn { - proxyConn := connect.NewProxyConn(conn, nil, nil, zap.NewNop(), + proxyConn := connect.NewProxyConn(conn, nil, nil, nil, zap.NewNop(), connect.CreateProxyConnMetrics(prometheus.NewRegistry())) proxyConn.Claims = &token.GATClaims{} proxyConn.Address = upstreamAddr diff --git a/internal/webapphandler/handler_test.go b/internal/webapphandler/handler_test.go index e6d4fe86..e22e3f8f 100644 --- a/internal/webapphandler/handler_test.go +++ b/internal/webapphandler/handler_test.go @@ -41,7 +41,7 @@ func mustParse(t *testing.T, templates map[string]string) map[string]*template.T func TestNewHandler_PanicsOnRewriteError(t *testing.T) { connMetrics := connect.CreateProxyConnMetrics(prometheus.NewRegistry()) - conn := connect.NewProxyConn(nil, nil, nil, zap.NewNop(), connMetrics) + conn := connect.NewProxyConn(nil, nil, nil, nil, zap.NewNop(), connMetrics) conn.Claims = &token.GATClaims{ User: token.User{Username: "alice@acme.com"}, } @@ -198,7 +198,7 @@ func TestRewrite(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { connMetrics := connect.CreateProxyConnMetrics(prometheus.NewRegistry()) - conn := connect.NewProxyConn(nil, nil, nil, zap.NewNop(), connMetrics) + conn := connect.NewProxyConn(nil, nil, nil, nil, zap.NewNop(), connMetrics) conn.Address = tt.address conn.Token = tt.jwtToken conn.Claims = tt.claims @@ -224,7 +224,7 @@ func TestRewrite(t *testing.T) { func TestRewrite_PreservesClientHost(t *testing.T) { connMetrics := connect.CreateProxyConnMetrics(prometheus.NewRegistry()) - conn := connect.NewProxyConn(nil, nil, nil, zap.NewNop(), connMetrics) + conn := connect.NewProxyConn(nil, nil, nil, nil, zap.NewNop(), connMetrics) conn.Address = "admin.example.int:80" conn.Claims = &token.GATClaims{} @@ -242,7 +242,7 @@ func TestRewrite_PreservesClientHost(t *testing.T) { func TestRewrite_StripsClientIdentityHeaders(t *testing.T) { connMetrics := connect.CreateProxyConnMetrics(prometheus.NewRegistry()) - conn := connect.NewProxyConn(nil, nil, nil, zap.NewNop(), connMetrics) + conn := connect.NewProxyConn(nil, nil, nil, nil, zap.NewNop(), connMetrics) conn.Address = "admin.example.int:80" conn.Claims = &token.GATClaims{} @@ -270,7 +270,7 @@ func TestRewrite_SkipsInvalidGATHeaders(t *testing.T) { } connMetrics := connect.CreateProxyConnMetrics(prometheus.NewRegistry()) - conn := connect.NewProxyConn(nil, nil, nil, zap.NewNop(), connMetrics) + conn := connect.NewProxyConn(nil, nil, nil, nil, zap.NewNop(), connMetrics) conn.Token = "test-token" conn.Claims = withRequestHeaderRewrites(baseClaims, map[string]string{ "X-Malformed": "{{unclosed", @@ -291,7 +291,7 @@ func TestRewrite_SkipsInvalidGATHeaders(t *testing.T) { func TestBuildVariables_CoversAllowedKeys(t *testing.T) { connMetrics := connect.CreateProxyConnMetrics(prometheus.NewRegistry()) - conn := connect.NewProxyConn(nil, nil, nil, zap.NewNop(), connMetrics) + conn := connect.NewProxyConn(nil, nil, nil, nil, zap.NewNop(), connMetrics) conn.Claims = &token.GATClaims{} got := slices.Sorted(maps.Keys(buildVariables(conn))) From 4b6bf9ba022679bacc6bbcad0696b9ad1566110b Mon Sep 17 00:00:00 2001 From: Clement Tee Date: Wed, 29 Jul 2026 22:48:08 +0800 Subject: [PATCH 02/11] fix: bound the dynamic certificate cache and narrow the mint lock GetCertificate serves the pre-CONNECT handshake, so the certificate cache was keyed by client-controlled SNI with no eviction: distinct SNI values per connection grew it without limit. Cap it at 1024 entries (~2 MB) with an LRU backed by hashicorp/golang-lru, sized in line with comparable certificate caches (mitmproxy 100, Caddy 10000). The mutex also wrapped the whole lookup, so every caller queued behind an in-flight mint even on a cache hit -- 349 ms for an rsa-4096 leaf. Take the lock only on the miss path and re-check the cache under it, which keeps hits lock-free while still collapsing concurrent cold misses for the same host to a single mint. Co-Authored-By: Claude Fable 5 --- go.mod | 1 + go.sum | 2 + internal/connect/cert.go | 60 +++++++++++------ internal/connect/cert_test.go | 122 ++++++++++++++++++++++++++++++++++ 4 files changed, 166 insertions(+), 19 deletions(-) diff --git a/go.mod b/go.mod index ecf5debd..94c9a140 100644 --- a/go.mod +++ b/go.mod @@ -9,6 +9,7 @@ require ( github.com/golang-jwt/jwt/v5 v5.3.1 github.com/google/uuid v1.6.0 github.com/hashicorp/go-retryablehttp v0.7.8 + github.com/hashicorp/golang-lru/v2 v2.0.7 github.com/hashicorp/vault/api v1.23.0 github.com/hashicorp/vault/api/auth/approle v0.12.0 github.com/hashicorp/vault/api/auth/aws v0.12.0 diff --git a/go.sum b/go.sum index 6c7bd8b5..e0408585 100644 --- a/go.sum +++ b/go.sum @@ -122,6 +122,8 @@ github.com/hashicorp/go-sockaddr v1.0.7 h1:G+pTkSO01HpR5qCxg7lxfsFEZaG+C0VssTy/9 github.com/hashicorp/go-sockaddr v1.0.7/go.mod h1:FZQbEYa1pxkQ7WLpyXJ6cbjpT8q0YgQaK/JakXqGyWw= github.com/hashicorp/go-uuid v1.0.2 h1:cfejS+Tpcp13yd5nYHWDI6qVCny6wyX2Mt5SGur2IGE= github.com/hashicorp/go-uuid v1.0.2/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro= +github.com/hashicorp/golang-lru/v2 v2.0.7 h1:a+bsQ5rvGLjzHuww6tVxozPZFVghXaHOwFs4luLUK2k= +github.com/hashicorp/golang-lru/v2 v2.0.7/go.mod h1:QeFd9opnmA6QUJc5vARoKUSoFhyfM2/ZepoAG6RGpeM= github.com/hashicorp/hcl v1.0.1-vault-7 h1:ag5OxFVy3QYTFTJODRzTKVZ6xvdfLLCA1cy/Y6xGI0I= github.com/hashicorp/hcl v1.0.1-vault-7/go.mod h1:XYhtn6ijBSAj6n4YqAaf7RBPS4I06AItNorpy+MoQNM= github.com/hashicorp/vault/api v1.23.0 h1:gXgluBsSECfRWTSW9niY2jwg2e9mMJc4WoHNv4g3h6A= diff --git a/internal/connect/cert.go b/internal/connect/cert.go index 6c402650..ea3dbd0b 100644 --- a/internal/connect/cert.go +++ b/internal/connect/cert.go @@ -22,9 +22,16 @@ import ( "go.uber.org/zap" + lru "github.com/hashicorp/golang-lru/v2" + "gateway/internal/config" ) +const ( + clockSkewBuffer = 30 * time.Second + maxCachedCerts = 1024 +) + var ( errNotCACertificate = errors.New("certificate is not a certificate authority") errCAKeyNotSigner = errors.New("CA private key does not implement crypto.Signer") @@ -32,10 +39,6 @@ var ( errUnsupportedKeyBits = errors.New("unsupported key bits") ) -// clockSkewBuffer backdates minted certificates to tolerate downstream clients -// with slightly-behind clocks. -const clockSkewBuffer = 30 * time.Second - var serialNumberLimit = new(big.Int).Lsh(big.NewInt(1), 128) // DynamicCert mints short-lived leaf certificates signed by the configured @@ -48,7 +51,7 @@ type DynamicCert struct { logger *zap.Logger mu sync.Mutex - cache map[string]*tls.Certificate + cache *lru.Cache[string, *tls.Certificate] } func NewDynamicCert(cfg config.TLSDynamicConfig, logger *zap.Logger) (*DynamicCert, error) { @@ -75,31 +78,33 @@ func NewDynamicCert(cfg config.TLSDynamicConfig, logger *zap.Logger) (*DynamicCe return nil, errCAKeyNotSigner } + cache, err := lru.New[string, *tls.Certificate](maxCachedCerts) + if err != nil { + return nil, fmt.Errorf("failed to create certificate cache: %w", err) + } + return &DynamicCert{ caCert: caCert, caKey: caKey, cert: cfg.Cert, logger: logger, - cache: make(map[string]*tls.Certificate), + cache: cache, }, nil } // Run implements CertProvider; dynamic mode has no background maintenance. func (c *DynamicCert) Run(_ context.Context) {} -// GetCertificate implements tls.Config.GetCertificate for handshakes that -// happen before the CONNECT target is known. It mints for the SNI host when -// the client sends one, falling back to the connection's local IP for clients -// that don't (IP-dialed clients and health probes). +// GetCertificate mints for the SNI host, falling back to the connection's +// local IP for clients that send none (IP-dialed clients and health probes). func (c *DynamicCert) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certificate, error) { - host := hello.ServerName - if host == "" { - var err error + if hello.ServerName != "" { + return c.GetCertificateForHost(hello.ServerName) + } - host, _, err = net.SplitHostPort(hello.Conn.LocalAddr().String()) - if err != nil { - return nil, fmt.Errorf("failed to parse local address: %w", err) - } + host, _, err := net.SplitHostPort(hello.Conn.LocalAddr().String()) + if err != nil { + return nil, fmt.Errorf("failed to parse local address: %w", err) } return c.GetCertificateForHost(host) @@ -108,10 +113,16 @@ func (c *DynamicCert) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certifica // GetCertificateForHost returns a certificate for the requested host, minting // a new one when none is cached or the cached one is inside the renewal window. func (c *DynamicCert) GetCertificateForHost(host string) (*tls.Certificate, error) { + if cert, ok := c.cachedCert(host); ok { + return cert, nil + } + c.mu.Lock() defer c.mu.Unlock() - if cert, ok := c.cache[host]; ok && time.Now().Before(cert.Leaf.NotAfter.Add(-c.cert.GetRenewBefore())) { + // Re-check under the lock: a caller ahead in the queue may have minted + // this host already, which keeps concurrent cold misses to one mint. + if cert, ok := c.cachedCert(host); ok { return cert, nil } @@ -120,11 +131,22 @@ func (c *DynamicCert) GetCertificateForHost(host string) (*tls.Certificate, erro return nil, err } - c.cache[host] = cert + c.cache.Add(host, cert) return cert, nil } +// cachedCert returns the cached certificate for the given host +// while it is outside the renewal window. +func (c *DynamicCert) cachedCert(host string) (*tls.Certificate, bool) { + cert, ok := c.cache.Get(host) + if !ok { + return nil, false + } + + return cert, time.Now().Before(cert.Leaf.NotAfter.Add(-c.cert.GetRenewBefore())) +} + func (c *DynamicCert) mint(host string) (*tls.Certificate, error) { key, err := c.generateKey() if err != nil { diff --git a/internal/connect/cert_test.go b/internal/connect/cert_test.go index fbb041c1..66a05202 100644 --- a/internal/connect/cert_test.go +++ b/internal/connect/cert_test.go @@ -12,6 +12,7 @@ import ( "crypto/x509" "crypto/x509/pkix" "net" + "sync" "testing" "time" @@ -134,6 +135,18 @@ func TestDynamicCert_GetCertificate_ClientHelloNoSNIFallsBackToLocalAddr(t *test assert.True(t, minted.Leaf.IPAddresses[0].Equal(net.ParseIP("127.0.0.1"))) } +func TestDynamicCert_GetCertificate_UnparsableLocalAddr(t *testing.T) { + cert, _ := newTestCert(t, config.TLSDynamicCertConfig{}) + + hello := &tls.ClientHelloInfo{ + Conn: fakeAddrConn{addr: &net.UnixAddr{Name: "/tmp/gateway.sock", Net: "unix"}}, + } + + _, err := cert.GetCertificate(hello) + + assert.ErrorContains(t, err, "failed to parse local address") +} + func TestDynamicCert_GetCertificate_CachesPerHost(t *testing.T) { cert, _ := newTestCert(t, config.TLSDynamicCertConfig{}) @@ -149,6 +162,112 @@ func TestDynamicCert_GetCertificate_CachesPerHost(t *testing.T) { assert.NotSame(t, first, other) } +func TestDynamicCert_GetCertificate_BoundsCacheSize(t *testing.T) { + cert, _ := newTestCert(t, config.TLSDynamicCertConfig{}) + cert.cache.Resize(3) + + first, err := cert.GetCertificateForHost("first.internal") + require.NoError(t, err) + + for _, host := range []string{"second.internal", "third.internal", "fourth.internal"} { + _, err := cert.GetCertificateForHost(host) + require.NoError(t, err) + } + + assert.Equal(t, 3, cert.cache.Len(), "cache should stay at the cap") + assert.False(t, cert.cache.Contains("first.internal"), "the least recently used host should be evicted") + + // The evicted host is re-minted rather than served stale. + refreshed, err := cert.GetCertificateForHost("first.internal") + require.NoError(t, err) + assert.NotEqual(t, first.Leaf.SerialNumber, refreshed.Leaf.SerialNumber) +} + +func TestDynamicCert_GetCertificate_EvictsByRecency(t *testing.T) { + cert, _ := newTestCert(t, config.TLSDynamicCertConfig{}) + cert.cache.Resize(3) + + for _, host := range []string{"first.internal", "second.internal", "third.internal"} { + _, err := cert.GetCertificateForHost(host) + require.NoError(t, err) + } + + // Touching the oldest host makes the next-oldest the eviction candidate. + touched, err := cert.GetCertificateForHost("first.internal") + require.NoError(t, err) + + _, err = cert.GetCertificateForHost("fourth.internal") + require.NoError(t, err) + + assert.False(t, cert.cache.Contains("second.internal"), "the untouched host should be evicted") + assert.True(t, cert.cache.Contains("first.internal"), "the touched host should survive") + + cached, err := cert.GetCertificateForHost("first.internal") + require.NoError(t, err) + assert.Same(t, touched, cached, "the surviving host should still be served from cache") +} + +func TestDynamicCert_GetCertificate_RenewFailureKeepsError(t *testing.T) { + cert, _ := newTestCert(t, config.TLSDynamicCertConfig{ + Duration: time.Hour, + RenewBefore: 2 * time.Hour, + }) + + _, err := cert.GetCertificateForHost("app.internal") + require.NoError(t, err) + + // The cached entry is already inside the renewal window, so the next call + // re-mints — and now minting fails. + cert.cert.KeyType = "ed25519" + + _, err = cert.GetCertificateForHost("app.internal") + assert.ErrorIs(t, err, errUnsupportedKeyType) +} + +// Concurrent cold misses for one host must mint once. This is what the +// re-check inside the lock in GetCertificateForHost buys; without it every +// caller mints its own certificate. +func TestDynamicCert_GetCertificate_ConcurrentColdMissMintsOnce(t *testing.T) { + const callers = 50 + + cert, _ := newTestCert(t, config.TLSDynamicCertConfig{}) + + var ( + mu sync.Mutex + wg sync.WaitGroup + mintErr error + serials = map[string]struct{}{} + ) + + start := make(chan struct{}) + + for range callers { + wg.Go(func() { + <-start + + got, err := cert.GetCertificateForHost("cold.internal") + + mu.Lock() + defer mu.Unlock() + + if err != nil { + mintErr = err + + return + } + + serials[got.Leaf.SerialNumber.String()] = struct{}{} + }) + } + + close(start) + wg.Wait() + + require.NoError(t, mintErr) + assert.Len(t, serials, 1, "concurrent cold misses should mint exactly once") + assert.Equal(t, 1, cert.cache.Len()) +} + func TestDynamicCert_GetCertificate_RenewsInsideWindow(t *testing.T) { // renewBefore longer than duration puts a freshly minted certificate // inside the renewal window immediately, forcing a re-mint on every call. @@ -165,6 +284,9 @@ func TestDynamicCert_GetCertificate_RenewsInsideWindow(t *testing.T) { assert.NotSame(t, first, second) assert.NotEqual(t, first.Leaf.SerialNumber, second.Leaf.SerialNumber) + + // Re-minting replaces the cached entry rather than adding another one. + assert.Equal(t, 1, cert.cache.Len()) } func TestDynamicCert_GetCertificate_RSAKey(t *testing.T) { From 4178249964414b19a2255481568282eb64774954 Mon Sep 17 00:00:00 2001 From: Clement Tee Date: Thu, 30 Jul 2026 00:12:17 +0800 Subject: [PATCH 03/11] test: rework the dynamic certificate tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a checked-in CA at test/data/ca, embedded as data.CACert/data.CAKey and produced the same way as the existing proxy and api_server fixtures, so the tests stop generating an RSA CA per call. Drop the tests that provoked mint failures through configuration TLSDynamicCertConfig.Validate rejects — unsupported key types and sizes, and a renewBefore longer than duration. The remaining renewal test reaches the renewal window by expiring the cached certificate instead. Cover the provider failure that getTLSConfig wraps with a stub CertProvider, which needs no invalid configuration. Inline the certificate construction each test needs in place of a shared helper, name each test after the method it exercises, and group them by that method. CertReloader.GetCertificate now delegates to GetCertificateForHost so the read lock sits with the implementation. Co-Authored-By: Claude Fable 5 --- internal/connect/cert_provider_test.go | 7 +- internal/connect/cert_reloader.go | 10 +- internal/connect/cert_test.go | 404 ++++++++++--------------- internal/connect/conn.go | 2 + internal/connect/conn_test.go | 81 +++-- test/data/ca/README.md | 8 + test/data/ca/tls.crt | 19 ++ test/data/ca/tls.key | 28 ++ test/data/data.go | 6 + 9 files changed, 297 insertions(+), 268 deletions(-) create mode 100644 test/data/ca/README.md create mode 100644 test/data/ca/tls.crt create mode 100644 test/data/ca/tls.key diff --git a/internal/connect/cert_provider_test.go b/internal/connect/cert_provider_test.go index 7be0e76e..b3a4c6f2 100644 --- a/internal/connect/cert_provider_test.go +++ b/internal/connect/cert_provider_test.go @@ -14,9 +14,6 @@ import ( ) func TestNewCertProviderFromConfig(t *testing.T) { - ca := generateCACert(t) - caFile, caKeyFile := createCertFiles(t, ca) - tests := []struct { name string tlsCfg config.TLSConfig @@ -26,14 +23,14 @@ func TestNewCertProviderFromConfig(t *testing.T) { }{ { name: "static", - tlsCfg: config.TLSConfig{Static: &config.TLSStaticConfig{CertificateFile: caFile, PrivateKeyFile: caKeyFile}}, + tlsCfg: config.TLSConfig{Static: &config.TLSStaticConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}}, wantType: &CertReloader{}, }, { name: "dynamic", tlsCfg: config.TLSConfig{Dynamic: &config.TLSDynamicConfig{ CA: config.TLSDynamicCAConfig{ - SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: caFile, PrivateKeyFile: caKeyFile}, + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, }, }}, wantType: &DynamicCert{}, diff --git a/internal/connect/cert_reloader.go b/internal/connect/cert_reloader.go index 59264658..d169005a 100644 --- a/internal/connect/cert_reloader.go +++ b/internal/connect/cert_reloader.go @@ -41,16 +41,16 @@ func (cr *CertReloader) Run(ctx context.Context) { } func (cr *CertReloader) GetCertificate(_ *tls.ClientHelloInfo) (*tls.Certificate, error) { - cr.mu.RLock() - defer cr.mu.RUnlock() - - return cr.cert, nil + return cr.GetCertificateForHost("") } // GetCertificateForHost implements CertProvider, the configured certificate is // served regardless of the requested host. func (cr *CertReloader) GetCertificateForHost(_ string) (*tls.Certificate, error) { - return cr.GetCertificate(nil) + cr.mu.RLock() + defer cr.mu.RUnlock() + + return cr.cert, nil } func (cr *CertReloader) load() error { diff --git a/internal/connect/cert_test.go b/internal/connect/cert_test.go index 66a05202..b7489eb5 100644 --- a/internal/connect/cert_test.go +++ b/internal/connect/cert_test.go @@ -6,11 +6,9 @@ package connect import ( "crypto/ecdsa" "crypto/elliptic" - "crypto/rand" "crypto/rsa" "crypto/tls" "crypto/x509" - "crypto/x509/pkix" "net" "sync" "testing" @@ -24,57 +22,72 @@ import ( "gateway/test/data" ) -func generateCACert(t *testing.T) tls.Certificate { - t.Helper() +type fakeAddrConn struct { + net.Conn - privateKey, err := rsa.GenerateKey(rand.Reader, 2048) - require.NoError(t, err) + addr net.Addr +} - template := x509.Certificate{ - Subject: pkix.Name{CommonName: "test-ca"}, - NotBefore: time.Now().Add(-time.Hour), - NotAfter: time.Now().Add(24 * time.Hour), - KeyUsage: x509.KeyUsageCertSign, - IsCA: true, - BasicConstraintsValid: true, - } +func (c fakeAddrConn) LocalAddr() net.Addr { return c.addr } - derBytes, err := x509.CreateCertificate(rand.Reader, &template, &template, &privateKey.PublicKey, privateKey) - require.NoError(t, err) +func TestNewDynamicCert_Errors(t *testing.T) { + nonCAFile, nonCAKeyFile := createCertFiles(t, generateCert(t)) - return tls.Certificate{ - Certificate: [][]byte{derBytes}, - PrivateKey: privateKey, + tests := []struct { + name string + selfSign *config.TLSSelfSignCAConfig + wantErr error + errContains string + }{ + { + name: "missing selfSign", + wantErr: config.ErrMissingTLSCAConfig, + }, + { + name: "missing files", + selfSign: &config.TLSSelfSignCAConfig{CertificateFile: "missing.crt", PrivateKeyFile: "missing.key"}, + errContains: "failed to load CA key pair", + }, + { + name: "mismatched certificate and key", + selfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/proxy/tls.key"}, + errContains: "failed to load CA key pair", + }, + { + name: "certificate is not a CA", + selfSign: &config.TLSSelfSignCAConfig{CertificateFile: nonCAFile, PrivateKeyFile: nonCAKeyFile}, + wantErr: errNotCACertificate, + }, } -} -func newTestCert(t *testing.T, certCfg config.TLSDynamicCertConfig) (*DynamicCert, *x509.CertPool) { - t.Helper() + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := NewDynamicCert(config.TLSDynamicConfig{ + CA: config.TLSDynamicCAConfig{SelfSign: tt.selfSign}, + }, zap.NewNop()) + + require.Error(t, err) + + if tt.wantErr != nil { + require.ErrorIs(t, err, tt.wantErr) + } - ca := generateCACert(t) - certFile, keyFile := createCertFiles(t, ca) + if tt.errContains != "" { + assert.Contains(t, err.Error(), tt.errContains) + } + }) + } +} +func TestDynamicCert_GetCertificate_ClientHelloSNI(t *testing.T) { cert, err := NewDynamicCert(config.TLSDynamicConfig{ CA: config.TLSDynamicCAConfig{ - SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: certFile, PrivateKeyFile: keyFile}, + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, }, - Cert: certCfg, }, zap.NewNop()) require.NoError(t, err) - caCert, err := x509.ParseCertificate(ca.Certificate[0]) - require.NoError(t, err) - - pool := x509.NewCertPool() - pool.AddCert(caCert) - - return cert, pool -} - -func TestDynamicCert_GetCertificate(t *testing.T) { - cert, pool := newTestCert(t, config.TLSDynamicCertConfig{}) - - minted, err := cert.GetCertificateForHost("app.internal") + minted, err := cert.GetCertificate(&tls.ClientHelloInfo{ServerName: "app.internal"}) require.NoError(t, err) assert.Equal(t, "app.internal", minted.Leaf.Subject.CommonName) @@ -85,6 +98,9 @@ func TestDynamicCert_GetCertificate(t *testing.T) { require.True(t, ok, "expected an ECDSA leaf key by default") assert.Equal(t, elliptic.P256(), key.Curve) + pool := x509.NewCertPool() + pool.AppendCertsFromPEM(data.CACert) + _, err = minted.Leaf.Verify(x509.VerifyOptions{ DNSName: "app.internal", Roots: pool, @@ -93,36 +109,13 @@ func TestDynamicCert_GetCertificate(t *testing.T) { assert.NoError(t, err, "leaf should verify against the CA for the requested host") } -func TestDynamicCert_GetCertificate_IPHost(t *testing.T) { - cert, _ := newTestCert(t, config.TLSDynamicCertConfig{}) - - minted, err := cert.GetCertificateForHost("10.0.0.5") - require.NoError(t, err) - - require.Len(t, minted.Leaf.IPAddresses, 1) - assert.True(t, minted.Leaf.IPAddresses[0].Equal(net.ParseIP("10.0.0.5"))) - assert.Empty(t, minted.Leaf.DNSNames) -} - -type fakeAddrConn struct { - net.Conn - - addr net.Addr -} - -func (c fakeAddrConn) LocalAddr() net.Addr { return c.addr } - -func TestDynamicCert_GetCertificate_ClientHelloSNI(t *testing.T) { - cert, _ := newTestCert(t, config.TLSDynamicCertConfig{}) - - minted, err := cert.GetCertificate(&tls.ClientHelloInfo{ServerName: "app.internal"}) - require.NoError(t, err) - - assert.Equal(t, []string{"app.internal"}, minted.Leaf.DNSNames) -} - func TestDynamicCert_GetCertificate_ClientHelloNoSNIFallsBackToLocalAddr(t *testing.T) { - cert, _ := newTestCert(t, config.TLSDynamicCertConfig{}) + cert, err := NewDynamicCert(config.TLSDynamicConfig{ + CA: config.TLSDynamicCAConfig{ + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, + }, + }, zap.NewNop()) + require.NoError(t, err) hello := &tls.ClientHelloInfo{ Conn: fakeAddrConn{addr: &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 8443}}, @@ -136,101 +129,116 @@ func TestDynamicCert_GetCertificate_ClientHelloNoSNIFallsBackToLocalAddr(t *test } func TestDynamicCert_GetCertificate_UnparsableLocalAddr(t *testing.T) { - cert, _ := newTestCert(t, config.TLSDynamicCertConfig{}) + cert, err := NewDynamicCert(config.TLSDynamicConfig{ + CA: config.TLSDynamicCAConfig{ + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, + }, + }, zap.NewNop()) + require.NoError(t, err) hello := &tls.ClientHelloInfo{ Conn: fakeAddrConn{addr: &net.UnixAddr{Name: "/tmp/gateway.sock", Net: "unix"}}, } - _, err := cert.GetCertificate(hello) + _, err = cert.GetCertificate(hello) assert.ErrorContains(t, err, "failed to parse local address") } -func TestDynamicCert_GetCertificate_CachesPerHost(t *testing.T) { - cert, _ := newTestCert(t, config.TLSDynamicCertConfig{}) - - first, err := cert.GetCertificateForHost("app.internal") +func TestDynamicCert_GetCertificateForHost_IPHost(t *testing.T) { + cert, err := NewDynamicCert(config.TLSDynamicConfig{ + CA: config.TLSDynamicCAConfig{ + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, + }, + }, zap.NewNop()) require.NoError(t, err) - second, err := cert.GetCertificateForHost("app.internal") + minted, err := cert.GetCertificateForHost("10.0.0.5") require.NoError(t, err) - assert.Same(t, first, second) - other, err := cert.GetCertificateForHost("other.internal") - require.NoError(t, err) - assert.NotSame(t, first, other) + require.Len(t, minted.Leaf.IPAddresses, 1) + assert.True(t, minted.Leaf.IPAddresses[0].Equal(net.ParseIP("10.0.0.5"))) + assert.Empty(t, minted.Leaf.DNSNames) } -func TestDynamicCert_GetCertificate_BoundsCacheSize(t *testing.T) { - cert, _ := newTestCert(t, config.TLSDynamicCertConfig{}) - cert.cache.Resize(3) - - first, err := cert.GetCertificateForHost("first.internal") +func TestDynamicCert_GetCertificateForHost_RSAKey(t *testing.T) { + cert, err := NewDynamicCert(config.TLSDynamicConfig{ + CA: config.TLSDynamicCAConfig{ + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, + }, + Cert: config.TLSDynamicCertConfig{KeyType: "rsa", KeyBits: 2048}, + }, zap.NewNop()) require.NoError(t, err) - for _, host := range []string{"second.internal", "third.internal", "fourth.internal"} { - _, err := cert.GetCertificateForHost(host) - require.NoError(t, err) - } - - assert.Equal(t, 3, cert.cache.Len(), "cache should stay at the cap") - assert.False(t, cert.cache.Contains("first.internal"), "the least recently used host should be evicted") - - // The evicted host is re-minted rather than served stale. - refreshed, err := cert.GetCertificateForHost("first.internal") + minted, err := cert.GetCertificateForHost("app.internal") require.NoError(t, err) - assert.NotEqual(t, first.Leaf.SerialNumber, refreshed.Leaf.SerialNumber) + + _, ok := minted.PrivateKey.(*rsa.PrivateKey) + assert.True(t, ok, "expected an RSA leaf key") } -func TestDynamicCert_GetCertificate_EvictsByRecency(t *testing.T) { - cert, _ := newTestCert(t, config.TLSDynamicCertConfig{}) - cert.cache.Resize(3) +func TestDynamicCert_GetCertificateForHost_CachesPerHost(t *testing.T) { + cert, err := NewDynamicCert(config.TLSDynamicConfig{ + CA: config.TLSDynamicCAConfig{ + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, + }, + }, zap.NewNop()) + require.NoError(t, err) - for _, host := range []string{"first.internal", "second.internal", "third.internal"} { - _, err := cert.GetCertificateForHost(host) - require.NoError(t, err) - } + first, err := cert.GetCertificateForHost("app.internal") + require.NoError(t, err) - // Touching the oldest host makes the next-oldest the eviction candidate. - touched, err := cert.GetCertificateForHost("first.internal") + second, err := cert.GetCertificateForHost("app.internal") require.NoError(t, err) + assert.Same(t, first, second) - _, err = cert.GetCertificateForHost("fourth.internal") + other, err := cert.GetCertificateForHost("other.internal") require.NoError(t, err) + assert.NotSame(t, first, other) +} - assert.False(t, cert.cache.Contains("second.internal"), "the untouched host should be evicted") - assert.True(t, cert.cache.Contains("first.internal"), "the touched host should survive") +func TestDynamicCert_GetCertificateForHost_RenewsInsideWindow(t *testing.T) { + cert, err := NewDynamicCert(config.TLSDynamicConfig{ + CA: config.TLSDynamicCAConfig{ + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, + }, + Cert: config.TLSDynamicCertConfig{ + Duration: 2 * time.Hour, + RenewBefore: time.Hour, + }, + }, zap.NewNop()) + require.NoError(t, err) - cached, err := cert.GetCertificateForHost("first.internal") + first, err := cert.GetCertificateForHost("app.internal") require.NoError(t, err) - assert.Same(t, touched, cached, "the surviving host should still be served from cache") -} -func TestDynamicCert_GetCertificate_RenewFailureKeepsError(t *testing.T) { - cert, _ := newTestCert(t, config.TLSDynamicCertConfig{ - Duration: time.Hour, - RenewBefore: 2 * time.Hour, - }) + // Expire the cached certificate into its renewal window. + cached, ok := cert.cache.Get("app.internal") + require.True(t, ok) - _, err := cert.GetCertificateForHost("app.internal") + cached.Leaf.NotAfter = time.Now().Add(30 * time.Minute) + + second, err := cert.GetCertificateForHost("app.internal") require.NoError(t, err) - // The cached entry is already inside the renewal window, so the next call - // re-mints — and now minting fails. - cert.cert.KeyType = "ed25519" + assert.NotEqual(t, first.Leaf.SerialNumber, second.Leaf.SerialNumber) - _, err = cert.GetCertificateForHost("app.internal") - assert.ErrorIs(t, err, errUnsupportedKeyType) + // Re-minting replaces the cached entry rather than adding another one. + assert.Equal(t, 1, cert.cache.Len()) } // Concurrent cold misses for one host must mint once. This is what the // re-check inside the lock in GetCertificateForHost buys; without it every // caller mints its own certificate. -func TestDynamicCert_GetCertificate_ConcurrentColdMissMintsOnce(t *testing.T) { - const callers = 50 +func TestDynamicCert_GetCertificateForHost_ConcurrentColdMissMintsOnce(t *testing.T) { + const callers = 10 - cert, _ := newTestCert(t, config.TLSDynamicCertConfig{}) + cert, err := NewDynamicCert(config.TLSDynamicConfig{ + CA: config.TLSDynamicCAConfig{ + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, + }, + }, zap.NewNop()) + require.NoError(t, err) var ( mu sync.Mutex @@ -268,139 +276,57 @@ func TestDynamicCert_GetCertificate_ConcurrentColdMissMintsOnce(t *testing.T) { assert.Equal(t, 1, cert.cache.Len()) } -func TestDynamicCert_GetCertificate_RenewsInsideWindow(t *testing.T) { - // renewBefore longer than duration puts a freshly minted certificate - // inside the renewal window immediately, forcing a re-mint on every call. - cert, _ := newTestCert(t, config.TLSDynamicCertConfig{ - Duration: time.Hour, - RenewBefore: 2 * time.Hour, - }) - - first, err := cert.GetCertificateForHost("app.internal") - require.NoError(t, err) - - second, err := cert.GetCertificateForHost("app.internal") +func TestDynamicCert_GetCertificateForHost_BoundsCacheSize(t *testing.T) { + cert, err := NewDynamicCert(config.TLSDynamicConfig{ + CA: config.TLSDynamicCAConfig{ + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, + }, + }, zap.NewNop()) require.NoError(t, err) + cert.cache.Resize(3) - assert.NotSame(t, first, second) - assert.NotEqual(t, first.Leaf.SerialNumber, second.Leaf.SerialNumber) - - // Re-minting replaces the cached entry rather than adding another one. - assert.Equal(t, 1, cert.cache.Len()) -} - -func TestDynamicCert_GetCertificate_RSAKey(t *testing.T) { - cert, _ := newTestCert(t, config.TLSDynamicCertConfig{KeyType: "rsa", KeyBits: 2048}) - - minted, err := cert.GetCertificateForHost("app.internal") + first, err := cert.GetCertificateForHost("first.internal") require.NoError(t, err) - _, ok := minted.PrivateKey.(*rsa.PrivateKey) - assert.True(t, ok, "expected an RSA leaf key") -} - -func TestDynamicCert_GetCertificate_UnsupportedKeyConfig(t *testing.T) { - tests := []struct { - name string - certCfg config.TLSDynamicCertConfig - wantErr error - }{ - { - name: "unsupported key type", - certCfg: config.TLSDynamicCertConfig{KeyType: "ed25519"}, - wantErr: errUnsupportedKeyType, - }, - { - name: "unsupported ecdsa key bits", - certCfg: config.TLSDynamicCertConfig{KeyType: "ecdsa", KeyBits: 512}, - wantErr: errUnsupportedKeyBits, - }, + for _, host := range []string{"second.internal", "third.internal", "fourth.internal"} { + _, err := cert.GetCertificateForHost(host) + require.NoError(t, err) } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - cert, _ := newTestCert(t, tt.certCfg) + assert.Equal(t, 3, cert.cache.Len(), "cache should stay at the cap") + assert.False(t, cert.cache.Contains("first.internal"), "the least recently used host should be evicted") - _, err := cert.GetCertificateForHost("app.internal") - assert.ErrorIs(t, err, tt.wantErr) - }) - } + // The evicted host is re-minted rather than served stale. + refreshed, err := cert.GetCertificateForHost("first.internal") + require.NoError(t, err) + assert.NotEqual(t, first.Leaf.SerialNumber, refreshed.Leaf.SerialNumber) } -// TestNewDynamicCert_ProxyFixture guards the test/data/proxy fixture staying a CA -// certificate — tools/local uses it as the dynamic signing CA. -func TestNewDynamicCert_ProxyFixture(t *testing.T) { +func TestDynamicCert_GetCertificateForHost_EvictsByRecency(t *testing.T) { cert, err := NewDynamicCert(config.TLSDynamicConfig{ CA: config.TLSDynamicCAConfig{ - SelfSign: &config.TLSSelfSignCAConfig{ - CertificateFile: "../../test/data/proxy/tls.crt", - PrivateKeyFile: "../../test/data/proxy/tls.key", - }, + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, }, }, zap.NewNop()) require.NoError(t, err) + cert.cache.Resize(3) - minted, err := cert.GetCertificateForHost("127.0.0.1") - require.NoError(t, err) - - pool := x509.NewCertPool() - pool.AppendCertsFromPEM(data.ProxyCert) - - _, err = minted.Leaf.Verify(x509.VerifyOptions{ - DNSName: "127.0.0.1", - Roots: pool, - KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, - }) - assert.NoError(t, err, "minted leaf should verify against the proxy fixture CA") -} - -func TestNewDynamicCert_Errors(t *testing.T) { - caFile, _ := createCertFiles(t, generateCACert(t)) - nonCAFile, nonCAKeyFile := createCertFiles(t, generateCert(t)) - _, otherKeyFile := createCertFiles(t, generateCACert(t)) - - tests := []struct { - name string - selfSign *config.TLSSelfSignCAConfig - wantErr error - errContains string - }{ - { - name: "missing selfSign", - wantErr: config.ErrMissingTLSCAConfig, - }, - { - name: "missing files", - selfSign: &config.TLSSelfSignCAConfig{CertificateFile: "missing.crt", PrivateKeyFile: "missing.key"}, - errContains: "failed to load CA key pair", - }, - { - name: "mismatched certificate and key", - selfSign: &config.TLSSelfSignCAConfig{CertificateFile: caFile, PrivateKeyFile: otherKeyFile}, - errContains: "failed to load CA key pair", - }, - { - name: "certificate is not a CA", - selfSign: &config.TLSSelfSignCAConfig{CertificateFile: nonCAFile, PrivateKeyFile: nonCAKeyFile}, - wantErr: errNotCACertificate, - }, + for _, host := range []string{"first.internal", "second.internal", "third.internal"} { + _, err := cert.GetCertificateForHost(host) + require.NoError(t, err) } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - _, err := NewDynamicCert(config.TLSDynamicConfig{ - CA: config.TLSDynamicCAConfig{SelfSign: tt.selfSign}, - }, zap.NewNop()) + // Touching the oldest host makes the next-oldest the eviction candidate. + touched, err := cert.GetCertificateForHost("first.internal") + require.NoError(t, err) - require.Error(t, err) + _, err = cert.GetCertificateForHost("fourth.internal") + require.NoError(t, err) - if tt.wantErr != nil { - require.ErrorIs(t, err, tt.wantErr) - } + assert.False(t, cert.cache.Contains("second.internal"), "the untouched host should be evicted") + assert.True(t, cert.cache.Contains("first.internal"), "the touched host should survive") - if tt.errContains != "" { - assert.Contains(t, err.Error(), tt.errContains) - } - }) - } + cached, err := cert.GetCertificateForHost("first.internal") + require.NoError(t, err) + assert.Same(t, touched, cached, "the surviving host should still be served from cache") } diff --git a/internal/connect/conn.go b/internal/connect/conn.go index 8a3e85aa..ee8e4488 100644 --- a/internal/connect/conn.go +++ b/internal/connect/conn.go @@ -269,6 +269,8 @@ func (p *ProxyConn) getTLSConfig() (*tls.Config, error) { return nil, fmt.Errorf("failed to get certificate for %q: %w", host, err) } + // GetCertificate takes precedence when SNI is present and SNI cannot carry an IP address (RFC 6066 § 3) + // So we need to pin the TLS cert for IP address. tlsConfig := p.TLSConfig.Clone() tlsConfig.GetCertificate = nil tlsConfig.Certificates = []tls.Certificate{*cert} diff --git a/internal/connect/conn_test.go b/internal/connect/conn_test.go index 2519527b..7f610741 100644 --- a/internal/connect/conn_test.go +++ b/internal/connect/conn_test.go @@ -5,8 +5,10 @@ package connect import ( "bufio" + "context" "crypto/tls" "crypto/x509" + "errors" "fmt" "io" "net" @@ -548,7 +550,17 @@ func staticServerTLSConfig(t *testing.T) (*tls.Config, tls.Certificate) { } func TestProxyConn_UpgradeToTLS_KubernetesMintedCert(t *testing.T) { - cert, caPool := newTestCert(t, config.TLSDynamicCertConfig{}) + cert, err := NewDynamicCert(config.TLSDynamicConfig{ + CA: config.TLSDynamicCAConfig{ + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, + }, + Cert: config.TLSDynamicCertConfig{}, + }, zap.NewNop()) + require.NoError(t, err) + + caPool := x509.NewCertPool() + caPool.AppendCertsFromPEM(data.CACert) + serverTLSConfig, _ := staticServerTLSConfig(t) proxyConn := &ProxyConn{ @@ -598,7 +610,17 @@ func TestProxyConn_UpgradeToTLS_StaticPinsStaticCert(t *testing.T) { } func TestProxyConn_UpgradeToTLS_TerminatesTLSAtGateway(t *testing.T) { - cert, caPool := newTestCert(t, config.TLSDynamicCertConfig{}) + cert, err := NewDynamicCert(config.TLSDynamicConfig{ + CA: config.TLSDynamicCAConfig{ + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, + }, + Cert: config.TLSDynamicCertConfig{}, + }, zap.NewNop()) + require.NoError(t, err) + + caPool := x509.NewCertPool() + caPool.AppendCertsFromPEM(data.CACert) + serverTLSConfig, _ := staticServerTLSConfig(t) listener, addr := startMockListener(t) @@ -651,7 +673,13 @@ func TestProxyConn_UpgradeToTLS_TerminatesTLSAtGateway(t *testing.T) { } func TestProxyConn_UpgradeToTLS_HandshakeError(t *testing.T) { - cert, _ := newTestCert(t, config.TLSDynamicCertConfig{}) + cert, err := NewDynamicCert(config.TLSDynamicConfig{ + CA: config.TLSDynamicCAConfig{ + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, + }, + Cert: config.TLSDynamicCertConfig{}, + }, zap.NewNop()) + require.NoError(t, err) serverTLSConfig, _ := staticServerTLSConfig(t) // The client trusts a different CA, so it rejects the minted certificate @@ -667,7 +695,7 @@ func TestProxyConn_UpgradeToTLS_HandshakeError(t *testing.T) { Logger: zap.NewNop(), } - _, err := upgradeToTLSHandshake(t, proxyConn, &tls.Config{ + _, err = upgradeToTLSHandshake(t, proxyConn, &tls.Config{ ServerName: "app.internal", RootCAs: wrongPool, MinVersion: tls.VersionTLS13, @@ -676,44 +704,59 @@ func TestProxyConn_UpgradeToTLS_HandshakeError(t *testing.T) { require.Error(t, err) } -func TestProxyConn_UpgradeToTLS_MintError(t *testing.T) { - cert, caPool := newTestCert(t, config.TLSDynamicCertConfig{KeyType: "ed25519"}) +func TestProxyConn_UpgradeToTLS_MalformedDownstreamAddress(t *testing.T) { + cert, err := NewDynamicCert(config.TLSDynamicConfig{ + CA: config.TLSDynamicCAConfig{ + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, + }, + Cert: config.TLSDynamicCertConfig{}, + }, zap.NewNop()) + require.NoError(t, err) serverTLSConfig, _ := staticServerTLSConfig(t) proxyConn := &ProxyConn{ TLSConfig: serverTLSConfig, CertProvider: cert, - DownstreamAddress: "app.internal:443", + DownstreamAddress: "app.internal", Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp}}, Logger: zap.NewNop(), } - _, err := upgradeToTLSHandshake(t, proxyConn, &tls.Config{ - ServerName: "app.internal", - RootCAs: caPool, - MinVersion: tls.VersionTLS13, - }) + err = proxyConn.UpgradeToTLS() require.Error(t, err) - assert.ErrorIs(t, err, errUnsupportedKeyType) + assert.ErrorContains(t, err, `failed to parse downstream address "app.internal"`) } -func TestProxyConn_UpgradeToTLS_MalformedDownstreamAddress(t *testing.T) { - cert, _ := newTestCert(t, config.TLSDynamicCertConfig{}) +var errCertProviderFailed = errors.New("cert provider failed") + +type failingCertProvider struct{} + +func (failingCertProvider) Run(_ context.Context) {} + +func (failingCertProvider) GetCertificate(_ *tls.ClientHelloInfo) (*tls.Certificate, error) { + return nil, errCertProviderFailed +} + +func (failingCertProvider) GetCertificateForHost(_ string) (*tls.Certificate, error) { + return nil, errCertProviderFailed +} + +func TestProxyConn_UpgradeToTLS_CertProviderError(t *testing.T) { serverTLSConfig, _ := staticServerTLSConfig(t) proxyConn := &ProxyConn{ TLSConfig: serverTLSConfig, - CertProvider: cert, - DownstreamAddress: "app.internal", + CertProvider: failingCertProvider{}, + DownstreamAddress: "app.internal:443", Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp}}, Logger: zap.NewNop(), } err := proxyConn.UpgradeToTLS() - require.Error(t, err) - assert.ErrorContains(t, err, `failed to parse downstream address "app.internal"`) + require.ErrorIs(t, err, errCertProviderFailed) + assert.ErrorContains(t, err, `failed to get certificate for "app.internal"`) } func TestIsHealthCheckRequest(t *testing.T) { diff --git a/test/data/ca/README.md b/test/data/ca/README.md new file mode 100644 index 00000000..041383dd --- /dev/null +++ b/test/data/ca/README.md @@ -0,0 +1,8 @@ +Self-signed CA certificate and key for signing dynamically minted downstream certificates + +```bash +openssl req -x509 -newkey rsa:2048 -keyout tls.key -out tls.crt -sha256 -days 18250 -nodes \ + -subj "/CN=test-ca" \ + -addext "basicConstraints=critical,CA:TRUE" \ + -addext "keyUsage=critical,keyCertSign,cRLSign" +``` diff --git a/test/data/ca/tls.crt b/test/data/ca/tls.crt new file mode 100644 index 00000000..22859998 --- /dev/null +++ b/test/data/ca/tls.crt @@ -0,0 +1,19 @@ +-----BEGIN CERTIFICATE----- +MIIDFzCCAf+gAwIBAgIUf1FvlFCEMDMoIHx76ZX6ffy6TXowDQYJKoZIhvcNAQEL +BQAwEjEQMA4GA1UEAwwHdGVzdC1jYTAgFw0yNjA3MjkxNDQ4NTVaGA8yMDc2MDcx +NjE0NDg1NVowEjEQMA4GA1UEAwwHdGVzdC1jYTCCASIwDQYJKoZIhvcNAQEBBQAD +ggEPADCCAQoCggEBAJ65vCzrSnwqL9t7qGoprh4KEkZTQgZZ5nDpYVZ1Ivnh7s80 +TvGBpY59t70dEgtKU7TCP1PWJR3LilKn2K9XuxACXcsmFx7U6UUT7ZbbsL7zGsJB +F2wGwhwFy0ScfwPQgxSXCNg/4rhEhj9+GSNjtV9DQ3PIbfw/LeKOMAoGLOpXSjK/ +/OMK0SOlzaYKHIbBe7q81dBHnJbFUnwwNvBHIxHgkhoZ1CtkBwhvCS5zLahZybaR +gHq/daL8+DBApiKw9tmTBos53hQsPKzZOcQNgUOiHXKP8O0vbiepuzGWULNtj86X +KxDOFfi8+9850CApjLiPTG8TxOD1Wh2MghRT3iECAwEAAaNjMGEwHQYDVR0OBBYE +FFQTnqsrlcUztbh2xj+YT9o+HAkGMB8GA1UdIwQYMBaAFFQTnqsrlcUztbh2xj+Y +T9o+HAkGMA8GA1UdEwEB/wQFMAMBAf8wDgYDVR0PAQH/BAQDAgEGMA0GCSqGSIb3 +DQEBCwUAA4IBAQBwqMUGqljGTo5SrCe5+7yCmce7kaVtDTWxtUcNq/DUE9RwRLbf +51lePZSGzZR2FFD4+GGG2p9zlFZ3Cbs+pekA/0cTpg1rGv277lQvNzW+TUm5t1xx +vSC+Xb8xhiTXLAuhmvfm+O3svIeroBpwAXqGYNzVMeW5i+Q+m/Rhxn4pXWSY10Or +b3kv6zb2RzLy+pk3F+zgwvMqW27MabewDnk7jRffuC797f5L4/xhIj20DdKmECVv +LTuismQpC38lbtIAXpgbz0L4LE5QkIlLsR7mtXuMmtnRDEgsbsTgr6H3kLJ4j8ZV +jGtrUIEKCP1YiAGxPyTwf8i/i8qBHFqUTFZN +-----END CERTIFICATE----- diff --git a/test/data/ca/tls.key b/test/data/ca/tls.key new file mode 100644 index 00000000..89d2b4f9 --- /dev/null +++ b/test/data/ca/tls.key @@ -0,0 +1,28 @@ +-----BEGIN PRIVATE KEY----- +MIIEvwIBADANBgkqhkiG9w0BAQEFAASCBKkwggSlAgEAAoIBAQCeubws60p8Ki/b +e6hqKa4eChJGU0IGWeZw6WFWdSL54e7PNE7xgaWOfbe9HRILSlO0wj9T1iUdy4pS +p9ivV7sQAl3LJhce1OlFE+2W27C+8xrCQRdsBsIcBctEnH8D0IMUlwjYP+K4RIY/ +fhkjY7VfQ0NzyG38Py3ijjAKBizqV0oyv/zjCtEjpc2mChyGwXu6vNXQR5yWxVJ8 +MDbwRyMR4JIaGdQrZAcIbwkucy2oWcm2kYB6v3Wi/PgwQKYisPbZkwaLOd4ULDys +2TnEDYFDoh1yj/DtL24nqbsxllCzbY/OlysQzhX4vPvfOdAgKYy4j0xvE8Tg9Vod +jIIUU94hAgMBAAECggEAAIGH5vPBZcfDpcqNNPjDiunOI5vAgOEt/HZTVAbPu6YG +UHJH/c9Dk21Kqzy/EeVPgtVrWsLQFPomYNDoqQHEVqQkn5EURJ9PTA6oyfuSzWpc +ejniD3suXg+rACG96j7yqrDi5hTpYzF9ZRFA7Bg1W+OiFinRfJrzSZQu9S/jr5k3 +hKHUye8ht8e/CSm4qNLyCfrE1zshLUSFNgtjbpwK0Df9xYUJx2UFpthxQ4kE4dgz +rfs/s6Uy7sE+hF64jJlLzHMQciasycYivKPTTRrQL31XMS7yRPhBOEj6+p4V2vvi +c9SEKiFQ0PqcXXYYQrBidCTs/7hM1fQ55CqrU9l9DQKBgQDO5FJFdCyG9/rzNlx3 +rtHeR93OCNvaDjB63foN16IOEjWrcJtVGaFdDvvA3eqmp/Je3d4/DwAhnuOgqWmh +znowo+3i692KoHhlAB5JZIrV8pnEcU+CttlzLQ5+1g980epQnNX+BX/Xa1D9mOxK +PtRBSG+ffiE08s+QWK4orQzILQKBgQDEZpwsVb5Ykw0Bjhl9XzXvb6J+IR0KchQ0 +XDndiMwvIGEVnwFY2+JDNKnHk3rX1r2SckPuU99L50pUbOrtEEseVuXn036189jL +Y76VyAmDORseQol/+xR/RRvHubL5X7zqyguFf/GxRiIm0+HTwHqYf7DO+IvEaWzy +cIzeiYHSRQKBgQCInrNJU+73bZfUtWgYTGQgAZsRrD3p34baC+1M6MEFw7aXMHQS +nSs2dWd/s8t5I9xkEuTpBTHUAcVU2ap8hAmiRw4W1CRmn5MBNzPIyVD4+QvbcevM +aIxthypUTWzKwx+U/gw9g0opaZ+A51PZr2WgeoHjc3ngKhswua7AA442PQKBgQCY +CtOWIt7TlSt5a7dh7kZD0QlbWWkw//WSP19wmAlx5kAiS9DwKHE2E9vnq81qsExb +xee+5eE00p4hU5xe38E3gJBE8t7iHx9S/Sm6rHxowNm0iFAH6qkIiciiyqi6Exqe +LMHUnKLP2PpxUpA9rIF780Y+Q/13lSIJYYY8dPkyfQKBgQCGOdSzcFB3+7R2ShQe +Tl5sswacq7KD+V4xNo6xpfWYhQBcmeQPPOzWrLm7hkqofSz4ATwzJw7kA5SYiV4i +XyMTLBr7y1LOiLbT+FU03Wq0yrrWxyEjkQkcYAq2IjMsSnwKHYZgVWXdY2esDxtV +HBZmoRA4KE4OhhXyTjkmkhzwnw== +-----END PRIVATE KEY----- diff --git a/test/data/data.go b/test/data/data.go index 7e229194..ddd14421 100644 --- a/test/data/data.go +++ b/test/data/data.go @@ -7,6 +7,12 @@ import ( _ "embed" ) +//go:embed ca/tls.crt +var CACert []byte + +//go:embed ca/tls.key +var CAKey []byte + //go:embed client/key.pem var ClientKey []byte From a95ee96d8e8d98156218479a4bba810d28d38198 Mon Sep 17 00:00:00 2001 From: Clement Tee Date: Thu, 30 Jul 2026 10:56:38 +0800 Subject: [PATCH 04/11] refactor: resolve the downstream certificate through GetCertificate CertProvider only supplied a certificate per host; deriving that host from the ClientHello was a second method every provider had to implement, and the static reloader had to ignore its argument. Move the derivation into getCertificateForHello, wired by the listener, and drop GetCertificate from the interface. The inner upgrade now resolves through a GetCertificate closure rather than pinning Certificates. The closure ignores the ClientHello, so the served certificate still follows the validated CONNECT address and not client SNI. getTLSConfig can no longer fail, so it returns a config alone. Drop the unused data.CAKey embed; tests reference the CA key by path. Co-Authored-By: Claude Fable 5 --- internal/connect/cert.go | 15 ------ internal/connect/cert_provider.go | 28 ++++++++-- internal/connect/cert_provider_test.go | 65 ++++++++++++++++++++++ internal/connect/cert_reloader.go | 4 -- internal/connect/cert_reloader_test.go | 16 ++---- internal/connect/cert_test.go | 49 +---------------- internal/connect/conn.go | 42 ++++++--------- internal/connect/conn_test.go | 75 +++++++++++++++++--------- internal/connect/listener.go | 8 +-- test/data/data.go | 3 -- 10 files changed, 165 insertions(+), 140 deletions(-) diff --git a/internal/connect/cert.go b/internal/connect/cert.go index ea3dbd0b..29244c7a 100644 --- a/internal/connect/cert.go +++ b/internal/connect/cert.go @@ -95,21 +95,6 @@ func NewDynamicCert(cfg config.TLSDynamicConfig, logger *zap.Logger) (*DynamicCe // Run implements CertProvider; dynamic mode has no background maintenance. func (c *DynamicCert) Run(_ context.Context) {} -// GetCertificate mints for the SNI host, falling back to the connection's -// local IP for clients that send none (IP-dialed clients and health probes). -func (c *DynamicCert) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certificate, error) { - if hello.ServerName != "" { - return c.GetCertificateForHost(hello.ServerName) - } - - host, _, err := net.SplitHostPort(hello.Conn.LocalAddr().String()) - if err != nil { - return nil, fmt.Errorf("failed to parse local address: %w", err) - } - - return c.GetCertificateForHost(host) -} - // GetCertificateForHost returns a certificate for the requested host, minting // a new one when none is cached or the cached one is inside the renewal window. func (c *DynamicCert) GetCertificateForHost(host string) (*tls.Certificate, error) { diff --git a/internal/connect/cert_provider.go b/internal/connect/cert_provider.go index de866d06..0603b844 100644 --- a/internal/connect/cert_provider.go +++ b/internal/connect/cert_provider.go @@ -7,6 +7,7 @@ import ( "context" "crypto/tls" "fmt" + "net" "go.uber.org/zap" @@ -19,14 +20,31 @@ type CertProvider interface { // Run runs background maintenance until the context is canceled. Run(ctx context.Context) - // GetCertificate serves the outer, pre-CONNECT handshake. - GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certificate, error) - - // GetCertificateForHost returns the certificate presented for the - // validated CONNECT host on the inner handshake. + // GetCertificateForHost returns the certificate presented for the given + // host: the SNI host on the outer TLS, the validated CONNECT host + // on the inner TLS. + // + // Note: SNI cannot carry an IP address (see RFC 6066 § 3) GetCertificateForHost(host string) (*tls.Certificate, error) } +// getCertificateForHello serves the outer TLS handshake when the SNI +// host is present, falling back to the connection's local IP for clients +// that send none (e.g. IP-dialed clients and health probes). +func getCertificateForHello(provider CertProvider, hello *tls.ClientHelloInfo) (*tls.Certificate, error) { + host := hello.ServerName + if host == "" { + var err error + + host, _, err = net.SplitHostPort(hello.Conn.LocalAddr().String()) + if err != nil { + return nil, fmt.Errorf("failed to parse local address: %w", err) + } + } + + return provider.GetCertificateForHost(host) +} + // newCertProviderFromConfig creates a CertProvider based on the provided // configuration. func newCertProviderFromConfig(tlsCfg config.TLSConfig, logger *zap.Logger) (CertProvider, error) { diff --git a/internal/connect/cert_provider_test.go b/internal/connect/cert_provider_test.go index b3a4c6f2..be494623 100644 --- a/internal/connect/cert_provider_test.go +++ b/internal/connect/cert_provider_test.go @@ -4,6 +4,9 @@ package connect import ( + "context" + "crypto/tls" + "net" "testing" "github.com/stretchr/testify/assert" @@ -13,6 +16,68 @@ import ( "gateway/internal/config" ) +type fakeAddrConn struct { + net.Conn + + addr net.Addr +} + +func (c fakeAddrConn) LocalAddr() net.Addr { return c.addr } + +type recordingCertProvider struct { + host string +} + +func (p *recordingCertProvider) Run(_ context.Context) {} + +func (p *recordingCertProvider) GetCertificateForHost(host string) (*tls.Certificate, error) { + p.host = host + + return &tls.Certificate{}, nil +} + +func TestGetCertificateForHello(t *testing.T) { + tests := []struct { + name string + hello *tls.ClientHelloInfo + wantHost string + wantErr string + }{ + { + name: "SNI host", + hello: &tls.ClientHelloInfo{ServerName: "app.internal"}, + wantHost: "app.internal", + }, + { + name: "no SNI falls back to local IP", + hello: &tls.ClientHelloInfo{Conn: fakeAddrConn{addr: &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 8443}}}, + wantHost: "127.0.0.1", + }, + { + name: "unparsable local address", + hello: &tls.ClientHelloInfo{Conn: fakeAddrConn{addr: &net.UnixAddr{Name: "/tmp/gateway.sock", Net: "unix"}}}, + wantErr: "failed to parse local address", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + provider := &recordingCertProvider{} + + _, err := getCertificateForHello(provider, tt.hello) + + if tt.wantErr != "" { + assert.ErrorContains(t, err, tt.wantErr) + + return + } + + require.NoError(t, err) + assert.Equal(t, tt.wantHost, provider.host) + }) + } +} + func TestNewCertProviderFromConfig(t *testing.T) { tests := []struct { name string diff --git a/internal/connect/cert_reloader.go b/internal/connect/cert_reloader.go index d169005a..6bbce400 100644 --- a/internal/connect/cert_reloader.go +++ b/internal/connect/cert_reloader.go @@ -40,10 +40,6 @@ func (cr *CertReloader) Run(ctx context.Context) { }, time.Minute, ctx.Done()) } -func (cr *CertReloader) GetCertificate(_ *tls.ClientHelloInfo) (*tls.Certificate, error) { - return cr.GetCertificateForHost("") -} - // GetCertificateForHost implements CertProvider, the configured certificate is // served regardless of the requested host. func (cr *CertReloader) GetCertificateForHost(_ string) (*tls.Certificate, error) { diff --git a/internal/connect/cert_reloader_test.go b/internal/connect/cert_reloader_test.go index 1dd9f9e5..590cdef3 100644 --- a/internal/connect/cert_reloader_test.go +++ b/internal/connect/cert_reloader_test.go @@ -34,10 +34,8 @@ func TestReloadWhenFileChanged(t *testing.T) { newCert := generateCert(t) replaceCertFiles(t, certFile, keyFile, newCert) - hello := &tls.ClientHelloInfo{} - assert.EventuallyWithT(t, func(c *assert.CollectT) { - cert, err := certReloader.GetCertificate(hello) + cert, err := certReloader.GetCertificateForHost("") assert.NoError(t, err) assert.Equal(c, newCert.Certificate, cert.Certificate) @@ -64,9 +62,7 @@ func TestDontReloadWhenMismatchedKeyAndCertificate(t *testing.T) { replaceCertFiles(t, certFile, keyFile, invalidCert) time.Sleep(5 * time.Millisecond) - hello := &tls.ClientHelloInfo{} - - cert, err := certReloader.GetCertificate(hello) + cert, err := certReloader.GetCertificateForHost("") require.NoError(t, err) // Ensure certificate is unchanged @@ -92,9 +88,7 @@ func TestDontReloadWhenContextIsCanceled(t *testing.T) { replaceCertFiles(t, certFile, keyFile, newCert) time.Sleep(5 * time.Millisecond) - hello := &tls.ClientHelloInfo{} - - cert, err := certReloader.GetCertificate(hello) + cert, err := certReloader.GetCertificateForHost("") require.NoError(t, err) assert.Equal(t, expectedCert.Certificate, cert.Certificate) @@ -149,10 +143,8 @@ func TestErrorInitializeCertReloader(t *testing.T) { func requireCertReloader(t *testing.T, certReloader *CertReloader, expectedCert tls.Certificate) { t.Helper() - hello := &tls.ClientHelloInfo{} - require.EventuallyWithT(t, func(c *assert.CollectT) { - existingCert, err := certReloader.GetCertificate(hello) + existingCert, err := certReloader.GetCertificateForHost("") require.NoError(c, err) require.NotNil(c, existingCert) diff --git a/internal/connect/cert_test.go b/internal/connect/cert_test.go index b7489eb5..05b77c57 100644 --- a/internal/connect/cert_test.go +++ b/internal/connect/cert_test.go @@ -7,7 +7,6 @@ import ( "crypto/ecdsa" "crypto/elliptic" "crypto/rsa" - "crypto/tls" "crypto/x509" "net" "sync" @@ -22,14 +21,6 @@ import ( "gateway/test/data" ) -type fakeAddrConn struct { - net.Conn - - addr net.Addr -} - -func (c fakeAddrConn) LocalAddr() net.Addr { return c.addr } - func TestNewDynamicCert_Errors(t *testing.T) { nonCAFile, nonCAKeyFile := createCertFiles(t, generateCert(t)) @@ -79,7 +70,7 @@ func TestNewDynamicCert_Errors(t *testing.T) { } } -func TestDynamicCert_GetCertificate_ClientHelloSNI(t *testing.T) { +func TestDynamicCert_GetCertificateForHost_DNSHost(t *testing.T) { cert, err := NewDynamicCert(config.TLSDynamicConfig{ CA: config.TLSDynamicCAConfig{ SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, @@ -87,7 +78,7 @@ func TestDynamicCert_GetCertificate_ClientHelloSNI(t *testing.T) { }, zap.NewNop()) require.NoError(t, err) - minted, err := cert.GetCertificate(&tls.ClientHelloInfo{ServerName: "app.internal"}) + minted, err := cert.GetCertificateForHost("app.internal") require.NoError(t, err) assert.Equal(t, "app.internal", minted.Leaf.Subject.CommonName) @@ -109,42 +100,6 @@ func TestDynamicCert_GetCertificate_ClientHelloSNI(t *testing.T) { assert.NoError(t, err, "leaf should verify against the CA for the requested host") } -func TestDynamicCert_GetCertificate_ClientHelloNoSNIFallsBackToLocalAddr(t *testing.T) { - cert, err := NewDynamicCert(config.TLSDynamicConfig{ - CA: config.TLSDynamicCAConfig{ - SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, - }, - }, zap.NewNop()) - require.NoError(t, err) - - hello := &tls.ClientHelloInfo{ - Conn: fakeAddrConn{addr: &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 8443}}, - } - - minted, err := cert.GetCertificate(hello) - require.NoError(t, err) - - require.Len(t, minted.Leaf.IPAddresses, 1) - assert.True(t, minted.Leaf.IPAddresses[0].Equal(net.ParseIP("127.0.0.1"))) -} - -func TestDynamicCert_GetCertificate_UnparsableLocalAddr(t *testing.T) { - cert, err := NewDynamicCert(config.TLSDynamicConfig{ - CA: config.TLSDynamicCAConfig{ - SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, - }, - }, zap.NewNop()) - require.NoError(t, err) - - hello := &tls.ClientHelloInfo{ - Conn: fakeAddrConn{addr: &net.UnixAddr{Name: "/tmp/gateway.sock", Net: "unix"}}, - } - - _, err = cert.GetCertificate(hello) - - assert.ErrorContains(t, err, "failed to parse local address") -} - func TestDynamicCert_GetCertificateForHost_IPHost(t *testing.T) { cert, err := NewDynamicCert(config.TLSDynamicConfig{ CA: config.TLSDynamicCAConfig{ diff --git a/internal/connect/conn.go b/internal/connect/conn.go index ee8e4488..ca6888c8 100644 --- a/internal/connect/conn.go +++ b/internal/connect/conn.go @@ -235,14 +235,7 @@ func (p *ProxyConn) Authenticate() error { } func (p *ProxyConn) UpgradeToTLS() error { - tlsConfig, err := p.getTLSConfig() - if err != nil { - p.Logger.Error("failed to prepare TLS config for upgrade", zap.Error(err)) - - return err - } - - tlsConn := tls.Server(p.Conn, tlsConfig) + tlsConn := tls.Server(p.Conn, p.getTLSConfig()) if err := tlsConn.Handshake(); err != nil { p.Logger.Error("failed to upgrade TLS", zap.Error(err)) @@ -255,27 +248,24 @@ func (p *ProxyConn) UpgradeToTLS() error { return nil } -// getTLSConfig pins the certificate presented to the downstream client -// for the CONNECT-requested host: minted in dynamic mode, the configured -// certificate in static mode. -func (p *ProxyConn) getTLSConfig() (*tls.Config, error) { - host, _, err := net.SplitHostPort(p.DownstreamAddress) - if err != nil { - return nil, fmt.Errorf("failed to parse downstream address %q: %w", p.DownstreamAddress, err) - } +// getTLSConfig pins the served certificate to the CONNECT host. +func (p *ProxyConn) getTLSConfig() *tls.Config { + tlsConfig := p.TLSConfig.Clone() + tlsConfig.GetCertificate = func(_ *tls.ClientHelloInfo) (*tls.Certificate, error) { + host, _, err := net.SplitHostPort(p.DownstreamAddress) + if err != nil { + return nil, fmt.Errorf("failed to parse downstream address %q: %w", p.DownstreamAddress, err) + } - cert, err := p.CertProvider.GetCertificateForHost(host) - if err != nil { - return nil, fmt.Errorf("failed to get certificate for %q: %w", host, err) - } + cert, err := p.CertProvider.GetCertificateForHost(host) + if err != nil { + return nil, fmt.Errorf("failed to get certificate for %q: %w", host, err) + } - // GetCertificate takes precedence when SNI is present and SNI cannot carry an IP address (RFC 6066 § 3) - // So we need to pin the TLS cert for IP address. - tlsConfig := p.TLSConfig.Clone() - tlsConfig.GetCertificate = nil - tlsConfig.Certificates = []tls.Certificate{*cert} + return cert, nil + } - return tlsConfig, nil + return tlsConfig } func (p *ProxyConn) setConnectInfo(connectInfo Info) { diff --git a/internal/connect/conn_test.go b/internal/connect/conn_test.go index 7f610741..7046402c 100644 --- a/internal/connect/conn_test.go +++ b/internal/connect/conn_test.go @@ -549,7 +549,7 @@ func staticServerTLSConfig(t *testing.T) (*tls.Config, tls.Certificate) { }, serverCert } -func TestProxyConn_UpgradeToTLS_KubernetesMintedCert(t *testing.T) { +func TestProxyConn_UpgradeToTLS_WebAppResourceMintedCert(t *testing.T) { cert, err := NewDynamicCert(config.TLSDynamicConfig{ CA: config.TLSDynamicCAConfig{ SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, @@ -561,24 +561,53 @@ func TestProxyConn_UpgradeToTLS_KubernetesMintedCert(t *testing.T) { caPool := x509.NewCertPool() caPool.AppendCertsFromPEM(data.CACert) - serverTLSConfig, _ := staticServerTLSConfig(t) + proxyConn := &ProxyConn{ + TLSConfig: &tls.Config{}, + CertProvider: cert, + DownstreamAddress: "grafana.internal:443", + Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp}}, + Logger: zap.NewNop(), + } + + leaf, err := upgradeToTLSHandshake(t, proxyConn, &tls.Config{ + ServerName: "grafana.internal", + RootCAs: caPool, + MinVersion: tls.VersionTLS13, + }) + require.NoError(t, err) + + assert.Equal(t, []string{"grafana.internal"}, leaf.DNSNames) +} + +func TestProxyConn_UpgradeToTLS_IPResourceMintedCert(t *testing.T) { + cert, err := NewDynamicCert(config.TLSDynamicConfig{ + CA: config.TLSDynamicCAConfig{ + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, + }, + Cert: config.TLSDynamicCertConfig{}, + }, zap.NewNop()) + require.NoError(t, err) + + caPool := x509.NewCertPool() + caPool.AppendCertsFromPEM(data.CACert) proxyConn := &ProxyConn{ - TLSConfig: serverTLSConfig, + TLSConfig: &tls.Config{}, CertProvider: cert, - DownstreamAddress: "k8s.internal:443", - Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeKubernetes}}, + DownstreamAddress: "10.0.0.5:443", + Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp}}, Logger: zap.NewNop(), } leaf, err := upgradeToTLSHandshake(t, proxyConn, &tls.Config{ - ServerName: "k8s.internal", + ServerName: "10.0.0.5", RootCAs: caPool, MinVersion: tls.VersionTLS13, }) require.NoError(t, err) - assert.Equal(t, []string{"k8s.internal"}, leaf.DNSNames) + require.Len(t, leaf.IPAddresses, 1) + assert.True(t, leaf.IPAddresses[0].Equal(net.ParseIP("10.0.0.5"))) } func TestProxyConn_UpgradeToTLS_StaticPinsStaticCert(t *testing.T) { @@ -621,8 +650,6 @@ func TestProxyConn_UpgradeToTLS_TerminatesTLSAtGateway(t *testing.T) { caPool := x509.NewCertPool() caPool.AppendCertsFromPEM(data.CACert) - serverTLSConfig, _ := staticServerTLSConfig(t) - listener, addr := startMockListener(t) defer listener.Close() @@ -654,7 +681,7 @@ func TestProxyConn_UpgradeToTLS_TerminatesTLSAtGateway(t *testing.T) { proxyConn := &ProxyConn{ Conn: conn, - TLSConfig: serverTLSConfig, + TLSConfig: &tls.Config{}, CertProvider: cert, DownstreamAddress: "app.internal:443", Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp}}, @@ -680,7 +707,6 @@ func TestProxyConn_UpgradeToTLS_HandshakeError(t *testing.T) { Cert: config.TLSDynamicCertConfig{}, }, zap.NewNop()) require.NoError(t, err) - serverTLSConfig, _ := staticServerTLSConfig(t) // The client trusts a different CA, so it rejects the minted certificate // and the server-side handshake fails. @@ -688,7 +714,7 @@ func TestProxyConn_UpgradeToTLS_HandshakeError(t *testing.T) { wrongPool.AppendCertsFromPEM(data.ProxyCert) proxyConn := &ProxyConn{ - TLSConfig: serverTLSConfig, + TLSConfig: &tls.Config{}, CertProvider: cert, DownstreamAddress: "app.internal:443", Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp}}, @@ -712,20 +738,22 @@ func TestProxyConn_UpgradeToTLS_MalformedDownstreamAddress(t *testing.T) { Cert: config.TLSDynamicCertConfig{}, }, zap.NewNop()) require.NoError(t, err) - serverTLSConfig, _ := staticServerTLSConfig(t) proxyConn := &ProxyConn{ - TLSConfig: serverTLSConfig, + TLSConfig: &tls.Config{}, CertProvider: cert, - DownstreamAddress: "app.internal", + DownstreamAddress: "garbage", Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp}}, Logger: zap.NewNop(), } - err = proxyConn.UpgradeToTLS() + _, err = upgradeToTLSHandshake(t, proxyConn, &tls.Config{ + ServerName: "garbage", + MinVersion: tls.VersionTLS13, + }) require.Error(t, err) - assert.ErrorContains(t, err, `failed to parse downstream address "app.internal"`) + assert.ErrorContains(t, err, `failed to parse downstream address "garbage"`) } var errCertProviderFailed = errors.New("cert provider failed") @@ -734,26 +762,23 @@ type failingCertProvider struct{} func (failingCertProvider) Run(_ context.Context) {} -func (failingCertProvider) GetCertificate(_ *tls.ClientHelloInfo) (*tls.Certificate, error) { - return nil, errCertProviderFailed -} - func (failingCertProvider) GetCertificateForHost(_ string) (*tls.Certificate, error) { return nil, errCertProviderFailed } func TestProxyConn_UpgradeToTLS_CertProviderError(t *testing.T) { - serverTLSConfig, _ := staticServerTLSConfig(t) - proxyConn := &ProxyConn{ - TLSConfig: serverTLSConfig, + TLSConfig: &tls.Config{}, CertProvider: failingCertProvider{}, DownstreamAddress: "app.internal:443", Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp}}, Logger: zap.NewNop(), } - err := proxyConn.UpgradeToTLS() + _, err := upgradeToTLSHandshake(t, proxyConn, &tls.Config{ + ServerName: "app.internal", + MinVersion: tls.VersionTLS13, + }) require.ErrorIs(t, err, errCertProviderFailed) assert.ErrorContains(t, err, `failed to get certificate for "app.internal"`) diff --git a/internal/connect/listener.go b/internal/connect/listener.go index 302ba55a..c2da8139 100644 --- a/internal/connect/listener.go +++ b/internal/connect/listener.go @@ -107,9 +107,11 @@ func NewListener( } tlsConfig := &tls.Config{ - MinVersion: tls.VersionTLS13, - MaxVersion: tls.VersionTLS13, - GetCertificate: certProvider.GetCertificate, + MinVersion: tls.VersionTLS13, + MaxVersion: tls.VersionTLS13, + GetCertificate: func(hello *tls.ClientHelloInfo) (*tls.Certificate, error) { + return getCertificateForHello(certProvider, hello) + }, } connectValidator := &MessageValidator{ diff --git a/test/data/data.go b/test/data/data.go index ddd14421..544fab51 100644 --- a/test/data/data.go +++ b/test/data/data.go @@ -10,9 +10,6 @@ import ( //go:embed ca/tls.crt var CACert []byte -//go:embed ca/tls.key -var CAKey []byte - //go:embed client/key.pem var ClientKey []byte From 95736820f268c4b3a1cf8eb9410718dc7a9d6f26 Mon Sep 17 00:00:00 2001 From: Clement Tee Date: Tue, 11 Aug 2026 20:05:09 +0800 Subject: [PATCH 05/11] feat: cover resource aliases in minted downstream certificates The Twingate client can dial a web app by an alias while the tunnel is established against the resource address, so the minted certificate now covers the token aliases alongside the CONNECT host, cached per name set. Fold the test/data/ca fixture into the proxy certificate, which is already a CA, and assert leaf contents in the DynamicCert unit tests instead of the UpgradeToTLS handshake tests. Co-Authored-By: Claude Fable 5 --- internal/connect/cert.go | 57 +++++-- internal/connect/cert_provider.go | 5 +- internal/connect/cert_provider_test.go | 16 +- internal/connect/cert_reloader.go | 2 +- internal/connect/cert_test.go | 153 +++++++++--------- internal/connect/conn.go | 2 +- internal/connect/conn_test.go | 216 +++---------------------- test/data/ca/README.md | 8 - test/data/ca/tls.crt | 19 --- test/data/ca/tls.key | 28 ---- test/data/data.go | 3 - 11 files changed, 155 insertions(+), 354 deletions(-) delete mode 100644 test/data/ca/README.md delete mode 100644 test/data/ca/tls.crt delete mode 100644 test/data/ca/tls.key diff --git a/internal/connect/cert.go b/internal/connect/cert.go index 29244c7a..ad1d382f 100644 --- a/internal/connect/cert.go +++ b/internal/connect/cert.go @@ -17,6 +17,8 @@ import ( "fmt" "math/big" "net" + "slices" + "strings" "sync" "time" @@ -95,10 +97,14 @@ func NewDynamicCert(cfg config.TLSDynamicConfig, logger *zap.Logger) (*DynamicCe // Run implements CertProvider; dynamic mode has no background maintenance. func (c *DynamicCert) Run(_ context.Context) {} -// GetCertificateForHost returns a certificate for the requested host, minting -// a new one when none is cached or the cached one is inside the renewal window. -func (c *DynamicCert) GetCertificateForHost(host string) (*tls.Certificate, error) { - if cert, ok := c.cachedCert(host); ok { +// GetCertificateForHost returns a certificate covering host and aliases, +// minting a new one when none is cached or the cached one is inside the +// renewal window. +func (c *DynamicCert) GetCertificateForHost(host string, aliases ...string) (*tls.Certificate, error) { + names := certNames(host, aliases) + key := strings.Join(names, ",") + + if cert, ok := c.cachedCert(key); ok { return cert, nil } @@ -107,24 +113,39 @@ func (c *DynamicCert) GetCertificateForHost(host string) (*tls.Certificate, erro // Re-check under the lock: a caller ahead in the queue may have minted // this host already, which keeps concurrent cold misses to one mint. - if cert, ok := c.cachedCert(host); ok { + if cert, ok := c.cachedCert(key); ok { return cert, nil } - cert, err := c.mint(host) + cert, err := c.mint(names) if err != nil { return nil, err } - c.cache.Add(host, cert) + c.cache.Add(key, cert) return cert, nil } -// cachedCert returns the cached certificate for the given host +// certNames is host followed by its aliases, without duplicates. host stays +// first so it becomes the common name. +func certNames(host string, aliases []string) []string { + names := make([]string, 0, len(aliases)+1) + names = append(names, host) + + for _, alias := range aliases { + if alias != "" && !slices.Contains(names, alias) { + names = append(names, alias) + } + } + + return names +} + +// cachedCert returns the cached certificate for the given name set // while it is outside the renewal window. -func (c *DynamicCert) cachedCert(host string) (*tls.Certificate, bool) { - cert, ok := c.cache.Get(host) +func (c *DynamicCert) cachedCert(key string) (*tls.Certificate, bool) { + cert, ok := c.cache.Get(key) if !ok { return nil, false } @@ -132,7 +153,7 @@ func (c *DynamicCert) cachedCert(host string) (*tls.Certificate, bool) { return cert, time.Now().Before(cert.Leaf.NotAfter.Add(-c.cert.GetRenewBefore())) } -func (c *DynamicCert) mint(host string) (*tls.Certificate, error) { +func (c *DynamicCert) mint(names []string) (*tls.Certificate, error) { key, err := c.generateKey() if err != nil { return nil, fmt.Errorf("failed to generate leaf key: %w", err) @@ -146,17 +167,19 @@ func (c *DynamicCert) mint(host string) (*tls.Certificate, error) { now := time.Now() template := &x509.Certificate{ SerialNumber: serial, - Subject: pkix.Name{CommonName: host}, + Subject: pkix.Name{CommonName: names[0]}, NotBefore: now.Add(-clockSkewBuffer), NotAfter: now.Add(c.cert.GetDuration()), KeyUsage: x509.KeyUsageDigitalSignature, ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, } - if ip := net.ParseIP(host); ip != nil { - template.IPAddresses = []net.IP{ip} - } else { - template.DNSNames = []string{host} + for _, name := range names { + if ip := net.ParseIP(name); ip != nil { + template.IPAddresses = append(template.IPAddresses, ip) + } else { + template.DNSNames = append(template.DNSNames, name) + } } leafDER, err := x509.CreateCertificate(rand.Reader, template, c.caCert, key.Public(), c.caKey) @@ -170,7 +193,7 @@ func (c *DynamicCert) mint(host string) (*tls.Certificate, error) { } c.logger.Info("Minted downstream certificate", - zap.String("host", host), + zap.Strings("hosts", names), zap.Time("not_after", leaf.NotAfter), ) diff --git a/internal/connect/cert_provider.go b/internal/connect/cert_provider.go index 0603b844..b4f4bec2 100644 --- a/internal/connect/cert_provider.go +++ b/internal/connect/cert_provider.go @@ -22,10 +22,11 @@ type CertProvider interface { // GetCertificateForHost returns the certificate presented for the given // host: the SNI host on the outer TLS, the validated CONNECT host - // on the inner TLS. + // on the inner TLS. Aliases are covered as additional subject alternative + // names, since the downstream client may have dialled any of them. // // Note: SNI cannot carry an IP address (see RFC 6066 § 3) - GetCertificateForHost(host string) (*tls.Certificate, error) + GetCertificateForHost(host string, aliases ...string) (*tls.Certificate, error) } // getCertificateForHello serves the outer TLS handshake when the SNI diff --git a/internal/connect/cert_provider_test.go b/internal/connect/cert_provider_test.go index be494623..f0c15d46 100644 --- a/internal/connect/cert_provider_test.go +++ b/internal/connect/cert_provider_test.go @@ -25,13 +25,21 @@ type fakeAddrConn struct { func (c fakeAddrConn) LocalAddr() net.Addr { return c.addr } type recordingCertProvider struct { - host string + host string + aliases []string + shouldFail bool + err error } func (p *recordingCertProvider) Run(_ context.Context) {} -func (p *recordingCertProvider) GetCertificateForHost(host string) (*tls.Certificate, error) { +func (p *recordingCertProvider) GetCertificateForHost(host string, aliases ...string) (*tls.Certificate, error) { p.host = host + p.aliases = aliases + + if p.shouldFail { + return nil, p.err + } return &tls.Certificate{}, nil } @@ -88,14 +96,14 @@ func TestNewCertProviderFromConfig(t *testing.T) { }{ { name: "static", - tlsCfg: config.TLSConfig{Static: &config.TLSStaticConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}}, + tlsCfg: config.TLSConfig{Static: &config.TLSStaticConfig{CertificateFile: "../../test/data/proxy/tls.crt", PrivateKeyFile: "../../test/data/proxy/tls.key"}}, wantType: &CertReloader{}, }, { name: "dynamic", tlsCfg: config.TLSConfig{Dynamic: &config.TLSDynamicConfig{ CA: config.TLSDynamicCAConfig{ - SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/proxy/tls.crt", PrivateKeyFile: "../../test/data/proxy/tls.key"}, }, }}, wantType: &DynamicCert{}, diff --git a/internal/connect/cert_reloader.go b/internal/connect/cert_reloader.go index 6bbce400..1db99182 100644 --- a/internal/connect/cert_reloader.go +++ b/internal/connect/cert_reloader.go @@ -42,7 +42,7 @@ func (cr *CertReloader) Run(ctx context.Context) { // GetCertificateForHost implements CertProvider, the configured certificate is // served regardless of the requested host. -func (cr *CertReloader) GetCertificateForHost(_ string) (*tls.Certificate, error) { +func (cr *CertReloader) GetCertificateForHost(_ string, _ ...string) (*tls.Certificate, error) { cr.mu.RLock() defer cr.mu.RUnlock() diff --git a/internal/connect/cert_test.go b/internal/connect/cert_test.go index 05b77c57..1b4c41d7 100644 --- a/internal/connect/cert_test.go +++ b/internal/connect/cert_test.go @@ -6,7 +6,6 @@ package connect import ( "crypto/ecdsa" "crypto/elliptic" - "crypto/rsa" "crypto/x509" "net" "sync" @@ -41,7 +40,7 @@ func TestNewDynamicCert_Errors(t *testing.T) { }, { name: "mismatched certificate and key", - selfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/proxy/tls.key"}, + selfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/proxy/tls.crt", PrivateKeyFile: "../../test/data/api_server/tls.key"}, errContains: "failed to load CA key pair", }, { @@ -73,7 +72,7 @@ func TestNewDynamicCert_Errors(t *testing.T) { func TestDynamicCert_GetCertificateForHost_DNSHost(t *testing.T) { cert, err := NewDynamicCert(config.TLSDynamicConfig{ CA: config.TLSDynamicCAConfig{ - SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/proxy/tls.crt", PrivateKeyFile: "../../test/data/proxy/tls.key"}, }, }, zap.NewNop()) require.NoError(t, err) @@ -90,7 +89,7 @@ func TestDynamicCert_GetCertificateForHost_DNSHost(t *testing.T) { assert.Equal(t, elliptic.P256(), key.Curve) pool := x509.NewCertPool() - pool.AppendCertsFromPEM(data.CACert) + pool.AppendCertsFromPEM(data.ProxyCert) _, err = minted.Leaf.Verify(x509.VerifyOptions{ DNSName: "app.internal", @@ -103,7 +102,7 @@ func TestDynamicCert_GetCertificateForHost_DNSHost(t *testing.T) { func TestDynamicCert_GetCertificateForHost_IPHost(t *testing.T) { cert, err := NewDynamicCert(config.TLSDynamicConfig{ CA: config.TLSDynamicCAConfig{ - SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/proxy/tls.crt", PrivateKeyFile: "../../test/data/proxy/tls.key"}, }, }, zap.NewNop()) require.NoError(t, err) @@ -116,46 +115,95 @@ func TestDynamicCert_GetCertificateForHost_IPHost(t *testing.T) { assert.Empty(t, minted.Leaf.DNSNames) } -func TestDynamicCert_GetCertificateForHost_RSAKey(t *testing.T) { - cert, err := NewDynamicCert(config.TLSDynamicConfig{ - CA: config.TLSDynamicCAConfig{ - SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, +func TestDynamicCert_GetCertificateForHost_CoversAliases(t *testing.T) { + tests := []struct { + name string + host string + aliases []string + wantCN string + wantDNS []string + wantIPs []string + }{ + { + name: "ip host with dns aliases", + host: "10.0.0.5", + aliases: []string{"app.internal", "alt.internal"}, + wantCN: "10.0.0.5", + wantDNS: []string{"app.internal", "alt.internal"}, + wantIPs: []string{"10.0.0.5"}, }, - Cert: config.TLSDynamicCertConfig{KeyType: "rsa", KeyBits: 2048}, - }, zap.NewNop()) - require.NoError(t, err) + { + name: "dns host with dns aliases", + host: "app.internal", + aliases: []string{"alt.internal"}, + wantCN: "app.internal", + wantDNS: []string{"app.internal", "alt.internal"}, + }, + { + name: "alias repeating the host is not duplicated", + host: "app.internal", + aliases: []string{"app.internal", "alt.internal", ""}, + wantCN: "app.internal", + wantDNS: []string{"app.internal", "alt.internal"}, + }, + { + name: "no aliases", + host: "app.internal", + wantCN: "app.internal", + wantDNS: []string{"app.internal"}, + }, + } - minted, err := cert.GetCertificateForHost("app.internal") - require.NoError(t, err) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cert, err := NewDynamicCert(config.TLSDynamicConfig{ + CA: config.TLSDynamicCAConfig{ + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/proxy/tls.crt", PrivateKeyFile: "../../test/data/proxy/tls.key"}, + }, + }, zap.NewNop()) + require.NoError(t, err) + + minted, err := cert.GetCertificateForHost(tt.host, tt.aliases...) + require.NoError(t, err) + + assert.Equal(t, tt.wantCN, minted.Leaf.Subject.CommonName) + assert.Equal(t, tt.wantDNS, minted.Leaf.DNSNames) + + var gotIPs []string + for _, ip := range minted.Leaf.IPAddresses { + gotIPs = append(gotIPs, ip.String()) + } - _, ok := minted.PrivateKey.(*rsa.PrivateKey) - assert.True(t, ok, "expected an RSA leaf key") + assert.Equal(t, tt.wantIPs, gotIPs) + }) + } } -func TestDynamicCert_GetCertificateForHost_CachesPerHost(t *testing.T) { +func TestDynamicCert_GetCertificateForHost_CachesPerNameSet(t *testing.T) { cert, err := NewDynamicCert(config.TLSDynamicConfig{ CA: config.TLSDynamicCAConfig{ - SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/proxy/tls.crt", PrivateKeyFile: "../../test/data/proxy/tls.key"}, }, }, zap.NewNop()) require.NoError(t, err) - first, err := cert.GetCertificateForHost("app.internal") + first, err := cert.GetCertificateForHost("app.internal", "a.internal") require.NoError(t, err) - second, err := cert.GetCertificateForHost("app.internal") + // The same host with a different alias set needs its own certificate. + other, err := cert.GetCertificateForHost("app.internal", "b.internal") require.NoError(t, err) - assert.Same(t, first, second) + assert.NotEqual(t, first.Leaf.SerialNumber, other.Leaf.SerialNumber) - other, err := cert.GetCertificateForHost("other.internal") + again, err := cert.GetCertificateForHost("app.internal", "a.internal") require.NoError(t, err) - assert.NotSame(t, first, other) + assert.Same(t, first, again) } func TestDynamicCert_GetCertificateForHost_RenewsInsideWindow(t *testing.T) { cert, err := NewDynamicCert(config.TLSDynamicConfig{ CA: config.TLSDynamicCAConfig{ - SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/proxy/tls.crt", PrivateKeyFile: "../../test/data/proxy/tls.key"}, }, Cert: config.TLSDynamicCertConfig{ Duration: 2 * time.Hour, @@ -190,7 +238,7 @@ func TestDynamicCert_GetCertificateForHost_ConcurrentColdMissMintsOnce(t *testin cert, err := NewDynamicCert(config.TLSDynamicConfig{ CA: config.TLSDynamicCAConfig{ - SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/proxy/tls.crt", PrivateKeyFile: "../../test/data/proxy/tls.key"}, }, }, zap.NewNop()) require.NoError(t, err) @@ -230,58 +278,3 @@ func TestDynamicCert_GetCertificateForHost_ConcurrentColdMissMintsOnce(t *testin assert.Len(t, serials, 1, "concurrent cold misses should mint exactly once") assert.Equal(t, 1, cert.cache.Len()) } - -func TestDynamicCert_GetCertificateForHost_BoundsCacheSize(t *testing.T) { - cert, err := NewDynamicCert(config.TLSDynamicConfig{ - CA: config.TLSDynamicCAConfig{ - SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, - }, - }, zap.NewNop()) - require.NoError(t, err) - cert.cache.Resize(3) - - first, err := cert.GetCertificateForHost("first.internal") - require.NoError(t, err) - - for _, host := range []string{"second.internal", "third.internal", "fourth.internal"} { - _, err := cert.GetCertificateForHost(host) - require.NoError(t, err) - } - - assert.Equal(t, 3, cert.cache.Len(), "cache should stay at the cap") - assert.False(t, cert.cache.Contains("first.internal"), "the least recently used host should be evicted") - - // The evicted host is re-minted rather than served stale. - refreshed, err := cert.GetCertificateForHost("first.internal") - require.NoError(t, err) - assert.NotEqual(t, first.Leaf.SerialNumber, refreshed.Leaf.SerialNumber) -} - -func TestDynamicCert_GetCertificateForHost_EvictsByRecency(t *testing.T) { - cert, err := NewDynamicCert(config.TLSDynamicConfig{ - CA: config.TLSDynamicCAConfig{ - SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, - }, - }, zap.NewNop()) - require.NoError(t, err) - cert.cache.Resize(3) - - for _, host := range []string{"first.internal", "second.internal", "third.internal"} { - _, err := cert.GetCertificateForHost(host) - require.NoError(t, err) - } - - // Touching the oldest host makes the next-oldest the eviction candidate. - touched, err := cert.GetCertificateForHost("first.internal") - require.NoError(t, err) - - _, err = cert.GetCertificateForHost("fourth.internal") - require.NoError(t, err) - - assert.False(t, cert.cache.Contains("second.internal"), "the untouched host should be evicted") - assert.True(t, cert.cache.Contains("first.internal"), "the touched host should survive") - - cached, err := cert.GetCertificateForHost("first.internal") - require.NoError(t, err) - assert.Same(t, touched, cached, "the surviving host should still be served from cache") -} diff --git a/internal/connect/conn.go b/internal/connect/conn.go index ca6888c8..db1194df 100644 --- a/internal/connect/conn.go +++ b/internal/connect/conn.go @@ -257,7 +257,7 @@ func (p *ProxyConn) getTLSConfig() *tls.Config { return nil, fmt.Errorf("failed to parse downstream address %q: %w", p.DownstreamAddress, err) } - cert, err := p.CertProvider.GetCertificateForHost(host) + cert, err := p.CertProvider.GetCertificateForHost(host, p.Claims.Resource.Aliases...) if err != nil { return nil, fmt.Errorf("failed to get certificate for %q: %w", host, err) } diff --git a/internal/connect/conn_test.go b/internal/connect/conn_test.go index 7046402c..a4234ff5 100644 --- a/internal/connect/conn_test.go +++ b/internal/connect/conn_test.go @@ -5,7 +5,6 @@ package connect import ( "bufio" - "context" "crypto/tls" "crypto/x509" "errors" @@ -483,38 +482,26 @@ func TestProxyConn_Authenticate_FailedValidation(t *testing.T) { } // upgradeToTLSHandshake runs proxyConn.UpgradeToTLS against a TLS client -// connected over TCP and returns the leaf certificate the client saw. -func upgradeToTLSHandshake(t *testing.T, proxyConn *ProxyConn, clientTLSConfig *tls.Config) (*x509.Certificate, error) { +// connected over TCP. +func upgradeToTLSHandshake(t *testing.T, proxyConn *ProxyConn, clientTLSConfig *tls.Config) error { t.Helper() listener, addr := startMockListener(t) defer listener.Close() - type clientResult struct { - leaf *x509.Certificate - err error - } - - clientCh := make(chan clientResult, 1) + clientCh := make(chan error, 1) go func() { conn, err := net.Dial("tcp", addr) if err != nil { - clientCh <- clientResult{nil, err} + clientCh <- err return } defer conn.Close() - tlsConn := tls.Client(conn, clientTLSConfig) - if err := tlsConn.Handshake(); err != nil { - clientCh <- clientResult{nil, err} - - return - } - - clientCh <- clientResult{tlsConn.ConnectionState().PeerCertificates[0], nil} + clientCh <- tls.Client(conn, clientTLSConfig).Handshake() }() conn, err := listener.Accept() @@ -528,181 +515,36 @@ func upgradeToTLSHandshake(t *testing.T, proxyConn *ProxyConn, clientTLSConfig * <-clientCh - return nil, serverErr + return serverErr } - result := <-clientCh - require.NoError(t, result.err) - - return result.leaf, nil -} + require.NoError(t, <-clientCh) -func staticServerTLSConfig(t *testing.T) (*tls.Config, tls.Certificate) { - t.Helper() - - serverCert, err := tls.X509KeyPair(data.ProxyCert, data.ProxyKey) - require.NoError(t, err) - - return &tls.Config{ - Certificates: []tls.Certificate{serverCert}, - MinVersion: tls.VersionTLS13, - }, serverCert + return nil } -func TestProxyConn_UpgradeToTLS_WebAppResourceMintedCert(t *testing.T) { - cert, err := NewDynamicCert(config.TLSDynamicConfig{ - CA: config.TLSDynamicCAConfig{ - SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, - }, - Cert: config.TLSDynamicCertConfig{}, - }, zap.NewNop()) - require.NoError(t, err) - - caPool := x509.NewCertPool() - caPool.AppendCertsFromPEM(data.CACert) - +func TestProxyConn_getTLSConfig(t *testing.T) { + provider := &recordingCertProvider{} proxyConn := &ProxyConn{ TLSConfig: &tls.Config{}, - CertProvider: cert, + CertProvider: provider, DownstreamAddress: "grafana.internal:443", - Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp}}, + Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp, Aliases: []string{"echo.internal", "echo-alt.internal"}}}, Logger: zap.NewNop(), } - leaf, err := upgradeToTLSHandshake(t, proxyConn, &tls.Config{ - ServerName: "grafana.internal", - RootCAs: caPool, - MinVersion: tls.VersionTLS13, - }) + cert, err := proxyConn.getTLSConfig().GetCertificate(&tls.ClientHelloInfo{ServerName: "other.internal"}) require.NoError(t, err) - assert.Equal(t, []string{"grafana.internal"}, leaf.DNSNames) -} - -func TestProxyConn_UpgradeToTLS_IPResourceMintedCert(t *testing.T) { - cert, err := NewDynamicCert(config.TLSDynamicConfig{ - CA: config.TLSDynamicCAConfig{ - SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, - }, - Cert: config.TLSDynamicCertConfig{}, - }, zap.NewNop()) - require.NoError(t, err) - - caPool := x509.NewCertPool() - caPool.AppendCertsFromPEM(data.CACert) - - proxyConn := &ProxyConn{ - TLSConfig: &tls.Config{}, - CertProvider: cert, - DownstreamAddress: "10.0.0.5:443", - Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp}}, - Logger: zap.NewNop(), - } - - leaf, err := upgradeToTLSHandshake(t, proxyConn, &tls.Config{ - ServerName: "10.0.0.5", - RootCAs: caPool, - MinVersion: tls.VersionTLS13, - }) - require.NoError(t, err) - - require.Len(t, leaf.IPAddresses, 1) - assert.True(t, leaf.IPAddresses[0].Equal(net.ParseIP("10.0.0.5"))) -} - -func TestProxyConn_UpgradeToTLS_StaticPinsStaticCert(t *testing.T) { - serverTLSConfig, serverCert := staticServerTLSConfig(t) - - certReloader := NewCertReloader("../../test/data/proxy/tls.crt", "../../test/data/proxy/tls.key", zap.NewNop()) - certReloader.Run(t.Context()) - requireCertReloader(t, certReloader, serverCert) - - caCertPool := x509.NewCertPool() - caCertPool.AppendCertsFromPEM(data.ProxyCert) - - proxyConn := &ProxyConn{ - TLSConfig: serverTLSConfig, - CertProvider: certReloader, - DownstreamAddress: "app.internal:443", - Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp}}, - Logger: zap.NewNop(), - } - - leaf, err := upgradeToTLSHandshake(t, proxyConn, &tls.Config{ - ServerName: "127.0.0.1", - RootCAs: caCertPool, - MinVersion: tls.VersionTLS13, - }) - require.NoError(t, err) - - assert.Equal(t, serverCert.Certificate[0], leaf.Raw) -} - -func TestProxyConn_UpgradeToTLS_TerminatesTLSAtGateway(t *testing.T) { - cert, err := NewDynamicCert(config.TLSDynamicConfig{ - CA: config.TLSDynamicCAConfig{ - SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, - }, - Cert: config.TLSDynamicCertConfig{}, - }, zap.NewNop()) - require.NoError(t, err) - - caPool := x509.NewCertPool() - caPool.AppendCertsFromPEM(data.CACert) - - listener, addr := startMockListener(t) - defer listener.Close() - - const request = "GET / HTTP/1.1\r\n\r\n" - - done := make(chan struct{}) - - go func() { - defer close(done) - - conn, err := net.Dial("tcp", addr) - assert.NoError(t, err) - - defer conn.Close() - - tlsConn := tls.Client(conn, &tls.Config{ - ServerName: "app.internal", - RootCAs: caPool, - MinVersion: tls.VersionTLS13, - }) - assert.NoError(t, tlsConn.Handshake()) - - _, err = tlsConn.Write([]byte(request)) - assert.NoError(t, err) - }() - - conn, err := listener.Accept() - require.NoError(t, err) - - proxyConn := &ProxyConn{ - Conn: conn, - TLSConfig: &tls.Config{}, - CertProvider: cert, - DownstreamAddress: "app.internal:443", - Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp}}, - Logger: zap.NewNop(), - } - - require.NoError(t, proxyConn.UpgradeToTLS()) - - // The gateway reads the decrypted plaintext through the upgraded connection. - buf := make([]byte, len(request)) - _, err = io.ReadFull(proxyConn, buf) - require.NoError(t, err) - assert.Equal(t, request, string(buf)) - - <-done + assert.NotNil(t, cert) + assert.Equal(t, "grafana.internal", provider.host) + assert.Equal(t, []string{"echo.internal", "echo-alt.internal"}, provider.aliases) } func TestProxyConn_UpgradeToTLS_HandshakeError(t *testing.T) { cert, err := NewDynamicCert(config.TLSDynamicConfig{ CA: config.TLSDynamicCAConfig{ - SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/proxy/tls.crt", PrivateKeyFile: "../../test/data/proxy/tls.key"}, }, Cert: config.TLSDynamicCertConfig{}, }, zap.NewNop()) @@ -711,7 +553,7 @@ func TestProxyConn_UpgradeToTLS_HandshakeError(t *testing.T) { // The client trusts a different CA, so it rejects the minted certificate // and the server-side handshake fails. wrongPool := x509.NewCertPool() - wrongPool.AppendCertsFromPEM(data.ProxyCert) + wrongPool.AppendCertsFromPEM(data.ServerCert) proxyConn := &ProxyConn{ TLSConfig: &tls.Config{}, @@ -721,7 +563,7 @@ func TestProxyConn_UpgradeToTLS_HandshakeError(t *testing.T) { Logger: zap.NewNop(), } - _, err = upgradeToTLSHandshake(t, proxyConn, &tls.Config{ + err = upgradeToTLSHandshake(t, proxyConn, &tls.Config{ ServerName: "app.internal", RootCAs: wrongPool, MinVersion: tls.VersionTLS13, @@ -733,7 +575,7 @@ func TestProxyConn_UpgradeToTLS_HandshakeError(t *testing.T) { func TestProxyConn_UpgradeToTLS_MalformedDownstreamAddress(t *testing.T) { cert, err := NewDynamicCert(config.TLSDynamicConfig{ CA: config.TLSDynamicCAConfig{ - SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/ca/tls.crt", PrivateKeyFile: "../../test/data/ca/tls.key"}, + SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/proxy/tls.crt", PrivateKeyFile: "../../test/data/proxy/tls.key"}, }, Cert: config.TLSDynamicCertConfig{}, }, zap.NewNop()) @@ -747,7 +589,7 @@ func TestProxyConn_UpgradeToTLS_MalformedDownstreamAddress(t *testing.T) { Logger: zap.NewNop(), } - _, err = upgradeToTLSHandshake(t, proxyConn, &tls.Config{ + err = upgradeToTLSHandshake(t, proxyConn, &tls.Config{ ServerName: "garbage", MinVersion: tls.VersionTLS13, }) @@ -756,26 +598,18 @@ func TestProxyConn_UpgradeToTLS_MalformedDownstreamAddress(t *testing.T) { assert.ErrorContains(t, err, `failed to parse downstream address "garbage"`) } -var errCertProviderFailed = errors.New("cert provider failed") - -type failingCertProvider struct{} - -func (failingCertProvider) Run(_ context.Context) {} - -func (failingCertProvider) GetCertificateForHost(_ string) (*tls.Certificate, error) { - return nil, errCertProviderFailed -} - func TestProxyConn_UpgradeToTLS_CertProviderError(t *testing.T) { + var errCertProviderFailed = errors.New("cert provider failed") + proxyConn := &ProxyConn{ TLSConfig: &tls.Config{}, - CertProvider: failingCertProvider{}, + CertProvider: &recordingCertProvider{shouldFail: true, err: errCertProviderFailed}, DownstreamAddress: "app.internal:443", Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp}}, Logger: zap.NewNop(), } - _, err := upgradeToTLSHandshake(t, proxyConn, &tls.Config{ + err := upgradeToTLSHandshake(t, proxyConn, &tls.Config{ ServerName: "app.internal", MinVersion: tls.VersionTLS13, }) diff --git a/test/data/ca/README.md b/test/data/ca/README.md deleted file mode 100644 index 041383dd..00000000 --- a/test/data/ca/README.md +++ /dev/null @@ -1,8 +0,0 @@ -Self-signed CA certificate and key for signing dynamically minted downstream certificates - -```bash -openssl req -x509 -newkey rsa:2048 -keyout tls.key -out tls.crt -sha256 -days 18250 -nodes \ - -subj "/CN=test-ca" \ - -addext "basicConstraints=critical,CA:TRUE" \ - -addext "keyUsage=critical,keyCertSign,cRLSign" -``` diff --git a/test/data/ca/tls.crt b/test/data/ca/tls.crt deleted file mode 100644 index 22859998..00000000 --- a/test/data/ca/tls.crt +++ /dev/null @@ -1,19 +0,0 @@ ------BEGIN CERTIFICATE----- -MIIDFzCCAf+gAwIBAgIUf1FvlFCEMDMoIHx76ZX6ffy6TXowDQYJKoZIhvcNAQEL -BQAwEjEQMA4GA1UEAwwHdGVzdC1jYTAgFw0yNjA3MjkxNDQ4NTVaGA8yMDc2MDcx -NjE0NDg1NVowEjEQMA4GA1UEAwwHdGVzdC1jYTCCASIwDQYJKoZIhvcNAQEBBQAD -ggEPADCCAQoCggEBAJ65vCzrSnwqL9t7qGoprh4KEkZTQgZZ5nDpYVZ1Ivnh7s80 -TvGBpY59t70dEgtKU7TCP1PWJR3LilKn2K9XuxACXcsmFx7U6UUT7ZbbsL7zGsJB -F2wGwhwFy0ScfwPQgxSXCNg/4rhEhj9+GSNjtV9DQ3PIbfw/LeKOMAoGLOpXSjK/ -/OMK0SOlzaYKHIbBe7q81dBHnJbFUnwwNvBHIxHgkhoZ1CtkBwhvCS5zLahZybaR -gHq/daL8+DBApiKw9tmTBos53hQsPKzZOcQNgUOiHXKP8O0vbiepuzGWULNtj86X -KxDOFfi8+9850CApjLiPTG8TxOD1Wh2MghRT3iECAwEAAaNjMGEwHQYDVR0OBBYE -FFQTnqsrlcUztbh2xj+YT9o+HAkGMB8GA1UdIwQYMBaAFFQTnqsrlcUztbh2xj+Y -T9o+HAkGMA8GA1UdEwEB/wQFMAMBAf8wDgYDVR0PAQH/BAQDAgEGMA0GCSqGSIb3 -DQEBCwUAA4IBAQBwqMUGqljGTo5SrCe5+7yCmce7kaVtDTWxtUcNq/DUE9RwRLbf -51lePZSGzZR2FFD4+GGG2p9zlFZ3Cbs+pekA/0cTpg1rGv277lQvNzW+TUm5t1xx -vSC+Xb8xhiTXLAuhmvfm+O3svIeroBpwAXqGYNzVMeW5i+Q+m/Rhxn4pXWSY10Or -b3kv6zb2RzLy+pk3F+zgwvMqW27MabewDnk7jRffuC797f5L4/xhIj20DdKmECVv -LTuismQpC38lbtIAXpgbz0L4LE5QkIlLsR7mtXuMmtnRDEgsbsTgr6H3kLJ4j8ZV -jGtrUIEKCP1YiAGxPyTwf8i/i8qBHFqUTFZN ------END CERTIFICATE----- diff --git a/test/data/ca/tls.key b/test/data/ca/tls.key deleted file mode 100644 index 89d2b4f9..00000000 --- a/test/data/ca/tls.key +++ /dev/null @@ -1,28 +0,0 @@ ------BEGIN PRIVATE KEY----- -MIIEvwIBADANBgkqhkiG9w0BAQEFAASCBKkwggSlAgEAAoIBAQCeubws60p8Ki/b -e6hqKa4eChJGU0IGWeZw6WFWdSL54e7PNE7xgaWOfbe9HRILSlO0wj9T1iUdy4pS -p9ivV7sQAl3LJhce1OlFE+2W27C+8xrCQRdsBsIcBctEnH8D0IMUlwjYP+K4RIY/ -fhkjY7VfQ0NzyG38Py3ijjAKBizqV0oyv/zjCtEjpc2mChyGwXu6vNXQR5yWxVJ8 -MDbwRyMR4JIaGdQrZAcIbwkucy2oWcm2kYB6v3Wi/PgwQKYisPbZkwaLOd4ULDys -2TnEDYFDoh1yj/DtL24nqbsxllCzbY/OlysQzhX4vPvfOdAgKYy4j0xvE8Tg9Vod -jIIUU94hAgMBAAECggEAAIGH5vPBZcfDpcqNNPjDiunOI5vAgOEt/HZTVAbPu6YG -UHJH/c9Dk21Kqzy/EeVPgtVrWsLQFPomYNDoqQHEVqQkn5EURJ9PTA6oyfuSzWpc -ejniD3suXg+rACG96j7yqrDi5hTpYzF9ZRFA7Bg1W+OiFinRfJrzSZQu9S/jr5k3 -hKHUye8ht8e/CSm4qNLyCfrE1zshLUSFNgtjbpwK0Df9xYUJx2UFpthxQ4kE4dgz -rfs/s6Uy7sE+hF64jJlLzHMQciasycYivKPTTRrQL31XMS7yRPhBOEj6+p4V2vvi -c9SEKiFQ0PqcXXYYQrBidCTs/7hM1fQ55CqrU9l9DQKBgQDO5FJFdCyG9/rzNlx3 -rtHeR93OCNvaDjB63foN16IOEjWrcJtVGaFdDvvA3eqmp/Je3d4/DwAhnuOgqWmh -znowo+3i692KoHhlAB5JZIrV8pnEcU+CttlzLQ5+1g980epQnNX+BX/Xa1D9mOxK -PtRBSG+ffiE08s+QWK4orQzILQKBgQDEZpwsVb5Ykw0Bjhl9XzXvb6J+IR0KchQ0 -XDndiMwvIGEVnwFY2+JDNKnHk3rX1r2SckPuU99L50pUbOrtEEseVuXn036189jL -Y76VyAmDORseQol/+xR/RRvHubL5X7zqyguFf/GxRiIm0+HTwHqYf7DO+IvEaWzy -cIzeiYHSRQKBgQCInrNJU+73bZfUtWgYTGQgAZsRrD3p34baC+1M6MEFw7aXMHQS -nSs2dWd/s8t5I9xkEuTpBTHUAcVU2ap8hAmiRw4W1CRmn5MBNzPIyVD4+QvbcevM -aIxthypUTWzKwx+U/gw9g0opaZ+A51PZr2WgeoHjc3ngKhswua7AA442PQKBgQCY -CtOWIt7TlSt5a7dh7kZD0QlbWWkw//WSP19wmAlx5kAiS9DwKHE2E9vnq81qsExb -xee+5eE00p4hU5xe38E3gJBE8t7iHx9S/Sm6rHxowNm0iFAH6qkIiciiyqi6Exqe -LMHUnKLP2PpxUpA9rIF780Y+Q/13lSIJYYY8dPkyfQKBgQCGOdSzcFB3+7R2ShQe -Tl5sswacq7KD+V4xNo6xpfWYhQBcmeQPPOzWrLm7hkqofSz4ATwzJw7kA5SYiV4i -XyMTLBr7y1LOiLbT+FU03Wq0yrrWxyEjkQkcYAq2IjMsSnwKHYZgVWXdY2esDxtV -HBZmoRA4KE4OhhXyTjkmkhzwnw== ------END PRIVATE KEY----- diff --git a/test/data/data.go b/test/data/data.go index 544fab51..7e229194 100644 --- a/test/data/data.go +++ b/test/data/data.go @@ -7,9 +7,6 @@ import ( _ "embed" ) -//go:embed ca/tls.crt -var CACert []byte - //go:embed client/key.pem var ClientKey []byte From 09ce018a699bf4e8dae19a0d93d64f25203c8380 Mon Sep 17 00:00:00 2001 From: Clement Tee Date: Tue, 11 Aug 2026 20:05:09 +0800 Subject: [PATCH 06/11] fix: sort aliases so equal name sets share a minted certificate Tokens can carry the same aliases in different orders, which changed the cache key and minted a duplicate certificate per ordering. Co-Authored-By: Claude Fable 5 --- internal/connect/cert.go | 9 ++++++--- internal/connect/cert_test.go | 10 +++++++++- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/internal/connect/cert.go b/internal/connect/cert.go index ad1d382f..b1743a31 100644 --- a/internal/connect/cert.go +++ b/internal/connect/cert.go @@ -127,8 +127,9 @@ func (c *DynamicCert) GetCertificateForHost(host string, aliases ...string) (*tl return cert, nil } -// certNames is host followed by its aliases, without duplicates. host stays -// first so it becomes the common name. +// certNames is host followed by its aliases, sorted and without duplicates, +// so the same name set always yields the same cache key. host stays first so +// it becomes the common name. func certNames(host string, aliases []string) []string { names := make([]string, 0, len(aliases)+1) names = append(names, host) @@ -139,6 +140,8 @@ func certNames(host string, aliases []string) []string { } } + slices.Sort(names[1:]) + return names } @@ -192,7 +195,7 @@ func (c *DynamicCert) mint(names []string) (*tls.Certificate, error) { return nil, fmt.Errorf("failed to parse leaf certificate: %w", err) } - c.logger.Info("Minted downstream certificate", + c.logger.Debug("Minted downstream certificate", zap.Strings("hosts", names), zap.Time("not_after", leaf.NotAfter), ) diff --git a/internal/connect/cert_test.go b/internal/connect/cert_test.go index 1b4c41d7..61fd7be5 100644 --- a/internal/connect/cert_test.go +++ b/internal/connect/cert_test.go @@ -129,7 +129,7 @@ func TestDynamicCert_GetCertificateForHost_CoversAliases(t *testing.T) { host: "10.0.0.5", aliases: []string{"app.internal", "alt.internal"}, wantCN: "10.0.0.5", - wantDNS: []string{"app.internal", "alt.internal"}, + wantDNS: []string{"alt.internal", "app.internal"}, wantIPs: []string{"10.0.0.5"}, }, { @@ -198,6 +198,14 @@ func TestDynamicCert_GetCertificateForHost_CachesPerNameSet(t *testing.T) { again, err := cert.GetCertificateForHost("app.internal", "a.internal") require.NoError(t, err) assert.Same(t, first, again) + + // The same aliases in a different order are the same name set. + sorted, err := cert.GetCertificateForHost("app.internal", "a.internal", "b.internal") + require.NoError(t, err) + + reordered, err := cert.GetCertificateForHost("app.internal", "b.internal", "a.internal") + require.NoError(t, err) + assert.Same(t, sorted, reordered) } func TestDynamicCert_GetCertificateForHost_RenewsInsideWindow(t *testing.T) { From 8537f042256ceb13263c7cac60b1fe41c80cd5fe Mon Sep 17 00:00:00 2001 From: Clement Tee Date: Tue, 11 Aug 2026 20:05:09 +0800 Subject: [PATCH 07/11] refactor: issue dynamic certificates through a certIssuer interface Isolate the signing backend behind a certIssuer interface so alternative CA backends only add an issuer, and thread the TLS handshake context through CertProvider so issuers doing network I/O are canceled when the handshake is abandoned. Co-Authored-By: Claude Fable 5 --- .golangci.yml | 1 + internal/config/config.go | 8 +- internal/connect/cert.go | 131 +++++++++++++++---------- internal/connect/cert_provider.go | 6 +- internal/connect/cert_provider_test.go | 2 +- internal/connect/cert_reloader.go | 2 +- internal/connect/cert_reloader_test.go | 8 +- internal/connect/cert_test.go | 73 +++++++------- internal/connect/conn.go | 4 +- internal/connect/conn_test.go | 2 +- 10 files changed, 139 insertions(+), 98 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 5ad96838..31904035 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -49,6 +49,7 @@ linters: - ca - caProvider - CertProvider + - certIssuer # golang.org/x/crypto/ssh - Signer - PublicKey diff --git a/internal/config/config.go b/internal/config/config.go index 71223a4c..b2e206d5 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -99,7 +99,7 @@ type TLSStaticConfig struct { PrivateKeyFile string `yaml:"privateKeyFile"` } -// TLSDynamicConfig configures on-demand minting of downstream leaf certificates. +// TLSDynamicConfig configures on-demand issuing of downstream leaf certificates. type TLSDynamicConfig struct { CA TLSDynamicCAConfig `yaml:"ca"` Cert TLSDynamicCertConfig `yaml:"cert"` @@ -116,10 +116,10 @@ type TLSSelfSignCAConfig struct { PrivateKeyFile string `yaml:"privateKeyFile"` } -// TLSDynamicCertConfig controls the leaf certificates minted by the dynamic CA. +// TLSDynamicCertConfig controls the leaf certificates issued by the dynamic CA. type TLSDynamicCertConfig struct { Duration time.Duration `yaml:"duration"` // Leaf certificate lifetime. Defaults to 24h. - RenewBefore time.Duration `yaml:"renewBefore"` // Window before expiry in which a fresh leaf is minted. Defaults to 8h. + RenewBefore time.Duration `yaml:"renewBefore"` // Window before expiry in which a fresh leaf is issued. Defaults to 8h. KeyType string `yaml:"keyType"` // ecdsa or rsa. Defaults to ecdsa. KeyBits int `yaml:"keyBits"` // ECDSA: 256/384/521, RSA: 2048/3072/4096. Defaults to 256 for ECDSA, 2048 for RSA. } @@ -493,7 +493,7 @@ func (c *TLSDynamicCertConfig) GetDuration() time.Duration { return c.Duration } -// GetRenewBefore returns the re-mint window before expiry, defaulting to 8h. +// GetRenewBefore returns the re-issue window before expiry, defaulting to 8h. func (c *TLSDynamicCertConfig) GetRenewBefore() time.Duration { if c.RenewBefore == 0 { return defaultTLSCertRenewBefore diff --git a/internal/connect/cert.go b/internal/connect/cert.go index b1743a31..cf457328 100644 --- a/internal/connect/cert.go +++ b/internal/connect/cert.go @@ -43,12 +43,11 @@ var ( var serialNumberLimit = new(big.Int).Lsh(big.NewInt(1), 128) -// DynamicCert mints short-lived leaf certificates signed by the configured -// CA, caching one certificate per requested host and re-minting a fresh one -// once the cached certificate enters the renewal window. +// DynamicCert issues short-lived leaf certificates through the configured +// issuer, caching one certificate per requested name set and re-issuing a +// fresh one once the cached certificate enters the renewal window. type DynamicCert struct { - caCert *x509.Certificate - caKey crypto.Signer + issuer certIssuer cert config.TLSDynamicCertConfig logger *zap.Logger @@ -57,27 +56,9 @@ type DynamicCert struct { } func NewDynamicCert(cfg config.TLSDynamicConfig, logger *zap.Logger) (*DynamicCert, error) { - if cfg.CA.SelfSign == nil { - return nil, config.ErrMissingTLSCAConfig - } - - pair, err := tls.LoadX509KeyPair(cfg.CA.SelfSign.CertificateFile, cfg.CA.SelfSign.PrivateKeyFile) - if err != nil { - return nil, fmt.Errorf("failed to load CA key pair: %w", err) - } - - caCert, err := x509.ParseCertificate(pair.Certificate[0]) + issuer, err := newCertIssuer(cfg) if err != nil { - return nil, fmt.Errorf("failed to parse CA certificate: %w", err) - } - - if !caCert.IsCA { - return nil, errNotCACertificate - } - - caKey, ok := pair.PrivateKey.(crypto.Signer) - if !ok { - return nil, errCAKeyNotSigner + return nil, err } cache, err := lru.New[string, *tls.Certificate](maxCachedCerts) @@ -86,21 +67,38 @@ func NewDynamicCert(cfg config.TLSDynamicConfig, logger *zap.Logger) (*DynamicCe } return &DynamicCert{ - caCert: caCert, - caKey: caKey, + issuer: issuer, cert: cfg.Cert, logger: logger, cache: cache, }, nil } -// Run implements CertProvider; dynamic mode has no background maintenance. -func (c *DynamicCert) Run(_ context.Context) {} +// certIssuer issues a certificate covering a set of names and runs any +// background maintenance its backend needs. +type certIssuer interface { + run(ctx context.Context) + issue(ctx context.Context, names []string) (*tls.Certificate, error) +} + +func newCertIssuer(cfg config.TLSDynamicConfig) (certIssuer, error) { + switch { + case cfg.CA.SelfSign != nil: + return newSelfSignIssuer(cfg.CA.SelfSign, cfg.Cert) + default: + return nil, config.ErrMissingTLSCAConfig + } +} + +// Run implements CertProvider, delegating background maintenance to the issuer. +func (c *DynamicCert) Run(ctx context.Context) { + c.issuer.run(ctx) +} // GetCertificateForHost returns a certificate covering host and aliases, -// minting a new one when none is cached or the cached one is inside the +// issuing a new one when none is cached or the cached one is inside the // renewal window. -func (c *DynamicCert) GetCertificateForHost(host string, aliases ...string) (*tls.Certificate, error) { +func (c *DynamicCert) GetCertificateForHost(ctx context.Context, host string, aliases ...string) (*tls.Certificate, error) { names := certNames(host, aliases) key := strings.Join(names, ",") @@ -111,17 +109,22 @@ func (c *DynamicCert) GetCertificateForHost(host string, aliases ...string) (*tl c.mu.Lock() defer c.mu.Unlock() - // Re-check under the lock: a caller ahead in the queue may have minted - // this host already, which keeps concurrent cold misses to one mint. + // Re-check under the lock: a caller ahead in the queue may have issued + // this host already, which keeps concurrent cold misses to one issuance. if cert, ok := c.cachedCert(key); ok { return cert, nil } - cert, err := c.mint(names) + cert, err := c.issuer.issue(ctx, names) if err != nil { return nil, err } + c.logger.Debug("Issued downstream certificate", + zap.Strings("hosts", names), + zap.Time("not_after", cert.Leaf.NotAfter), + ) + c.cache.Add(key, cert) return cert, nil @@ -156,8 +159,41 @@ func (c *DynamicCert) cachedCert(key string) (*tls.Certificate, bool) { return cert, time.Now().Before(cert.Leaf.NotAfter.Add(-c.cert.GetRenewBefore())) } -func (c *DynamicCert) mint(names []string) (*tls.Certificate, error) { - key, err := c.generateKey() +// selfSignIssuer signs leaf certificates locally with a CA loaded from files. +type selfSignIssuer struct { + caCert *x509.Certificate + caKey crypto.Signer + cert config.TLSDynamicCertConfig +} + +func newSelfSignIssuer(cfg *config.TLSSelfSignCAConfig, certCfg config.TLSDynamicCertConfig) (*selfSignIssuer, error) { + pair, err := tls.LoadX509KeyPair(cfg.CertificateFile, cfg.PrivateKeyFile) + if err != nil { + return nil, fmt.Errorf("failed to load CA key pair: %w", err) + } + + caCert, err := x509.ParseCertificate(pair.Certificate[0]) + if err != nil { + return nil, fmt.Errorf("failed to parse CA certificate: %w", err) + } + + if !caCert.IsCA { + return nil, errNotCACertificate + } + + caKey, ok := pair.PrivateKey.(crypto.Signer) + if !ok { + return nil, errCAKeyNotSigner + } + + return &selfSignIssuer{caCert: caCert, caKey: caKey, cert: certCfg}, nil +} + +// run implements certIssuer; the self-sign backend has no background maintenance. +func (s *selfSignIssuer) run(_ context.Context) {} + +func (s *selfSignIssuer) issue(_ context.Context, names []string) (*tls.Certificate, error) { + key, err := s.generateKey() if err != nil { return nil, fmt.Errorf("failed to generate leaf key: %w", err) } @@ -172,7 +208,7 @@ func (c *DynamicCert) mint(names []string) (*tls.Certificate, error) { SerialNumber: serial, Subject: pkix.Name{CommonName: names[0]}, NotBefore: now.Add(-clockSkewBuffer), - NotAfter: now.Add(c.cert.GetDuration()), + NotAfter: now.Add(s.cert.GetDuration()), KeyUsage: x509.KeyUsageDigitalSignature, ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, } @@ -185,7 +221,7 @@ func (c *DynamicCert) mint(names []string) (*tls.Certificate, error) { } } - leafDER, err := x509.CreateCertificate(rand.Reader, template, c.caCert, key.Public(), c.caKey) + leafDER, err := x509.CreateCertificate(rand.Reader, template, s.caCert, key.Public(), s.caKey) if err != nil { return nil, fmt.Errorf("failed to sign leaf certificate: %w", err) } @@ -195,22 +231,17 @@ func (c *DynamicCert) mint(names []string) (*tls.Certificate, error) { return nil, fmt.Errorf("failed to parse leaf certificate: %w", err) } - c.logger.Debug("Minted downstream certificate", - zap.Strings("hosts", names), - zap.Time("not_after", leaf.NotAfter), - ) - return &tls.Certificate{ - Certificate: [][]byte{leafDER, c.caCert.Raw}, + Certificate: [][]byte{leafDER, s.caCert.Raw}, PrivateKey: key, Leaf: leaf, }, nil } -func (c *DynamicCert) generateKey() (crypto.Signer, error) { - switch c.cert.GetKeyType() { +func (s *selfSignIssuer) generateKey() (crypto.Signer, error) { + switch s.cert.GetKeyType() { case "ecdsa": - switch c.cert.GetKeyBits() { + switch s.cert.GetKeyBits() { case 256: return ecdsa.GenerateKey(elliptic.P256(), rand.Reader) case 384: @@ -218,11 +249,11 @@ func (c *DynamicCert) generateKey() (crypto.Signer, error) { case 521: return ecdsa.GenerateKey(elliptic.P521(), rand.Reader) default: - return nil, fmt.Errorf("%w: ECDSA %d", errUnsupportedKeyBits, c.cert.GetKeyBits()) + return nil, fmt.Errorf("%w: ECDSA %d", errUnsupportedKeyBits, s.cert.GetKeyBits()) } case "rsa": - return rsa.GenerateKey(rand.Reader, c.cert.GetKeyBits()) + return rsa.GenerateKey(rand.Reader, s.cert.GetKeyBits()) default: - return nil, fmt.Errorf("%w: %s", errUnsupportedKeyType, c.cert.GetKeyType()) + return nil, fmt.Errorf("%w: %s", errUnsupportedKeyType, s.cert.GetKeyType()) } } diff --git a/internal/connect/cert_provider.go b/internal/connect/cert_provider.go index b4f4bec2..cb0619a8 100644 --- a/internal/connect/cert_provider.go +++ b/internal/connect/cert_provider.go @@ -24,9 +24,11 @@ type CertProvider interface { // host: the SNI host on the outer TLS, the validated CONNECT host // on the inner TLS. Aliases are covered as additional subject alternative // names, since the downstream client may have dialled any of them. + // The context is the TLS handshake's, canceling any in-flight work when + // the handshake is abandoned. // // Note: SNI cannot carry an IP address (see RFC 6066 § 3) - GetCertificateForHost(host string, aliases ...string) (*tls.Certificate, error) + GetCertificateForHost(ctx context.Context, host string, aliases ...string) (*tls.Certificate, error) } // getCertificateForHello serves the outer TLS handshake when the SNI @@ -43,7 +45,7 @@ func getCertificateForHello(provider CertProvider, hello *tls.ClientHelloInfo) ( } } - return provider.GetCertificateForHost(host) + return provider.GetCertificateForHost(hello.Context(), host) } // newCertProviderFromConfig creates a CertProvider based on the provided diff --git a/internal/connect/cert_provider_test.go b/internal/connect/cert_provider_test.go index f0c15d46..4453685e 100644 --- a/internal/connect/cert_provider_test.go +++ b/internal/connect/cert_provider_test.go @@ -33,7 +33,7 @@ type recordingCertProvider struct { func (p *recordingCertProvider) Run(_ context.Context) {} -func (p *recordingCertProvider) GetCertificateForHost(host string, aliases ...string) (*tls.Certificate, error) { +func (p *recordingCertProvider) GetCertificateForHost(_ context.Context, host string, aliases ...string) (*tls.Certificate, error) { p.host = host p.aliases = aliases diff --git a/internal/connect/cert_reloader.go b/internal/connect/cert_reloader.go index 1db99182..57b6680d 100644 --- a/internal/connect/cert_reloader.go +++ b/internal/connect/cert_reloader.go @@ -42,7 +42,7 @@ func (cr *CertReloader) Run(ctx context.Context) { // GetCertificateForHost implements CertProvider, the configured certificate is // served regardless of the requested host. -func (cr *CertReloader) GetCertificateForHost(_ string, _ ...string) (*tls.Certificate, error) { +func (cr *CertReloader) GetCertificateForHost(_ context.Context, _ string, _ ...string) (*tls.Certificate, error) { cr.mu.RLock() defer cr.mu.RUnlock() diff --git a/internal/connect/cert_reloader_test.go b/internal/connect/cert_reloader_test.go index 590cdef3..36f29100 100644 --- a/internal/connect/cert_reloader_test.go +++ b/internal/connect/cert_reloader_test.go @@ -35,7 +35,7 @@ func TestReloadWhenFileChanged(t *testing.T) { replaceCertFiles(t, certFile, keyFile, newCert) assert.EventuallyWithT(t, func(c *assert.CollectT) { - cert, err := certReloader.GetCertificateForHost("") + cert, err := certReloader.GetCertificateForHost(t.Context(), "") assert.NoError(t, err) assert.Equal(c, newCert.Certificate, cert.Certificate) @@ -62,7 +62,7 @@ func TestDontReloadWhenMismatchedKeyAndCertificate(t *testing.T) { replaceCertFiles(t, certFile, keyFile, invalidCert) time.Sleep(5 * time.Millisecond) - cert, err := certReloader.GetCertificateForHost("") + cert, err := certReloader.GetCertificateForHost(t.Context(), "") require.NoError(t, err) // Ensure certificate is unchanged @@ -88,7 +88,7 @@ func TestDontReloadWhenContextIsCanceled(t *testing.T) { replaceCertFiles(t, certFile, keyFile, newCert) time.Sleep(5 * time.Millisecond) - cert, err := certReloader.GetCertificateForHost("") + cert, err := certReloader.GetCertificateForHost(t.Context(), "") require.NoError(t, err) assert.Equal(t, expectedCert.Certificate, cert.Certificate) @@ -144,7 +144,7 @@ func requireCertReloader(t *testing.T, certReloader *CertReloader, expectedCert t.Helper() require.EventuallyWithT(t, func(c *assert.CollectT) { - existingCert, err := certReloader.GetCertificateForHost("") + existingCert, err := certReloader.GetCertificateForHost(t.Context(), "") require.NoError(c, err) require.NotNil(c, existingCert) diff --git a/internal/connect/cert_test.go b/internal/connect/cert_test.go index 61fd7be5..5ff1a313 100644 --- a/internal/connect/cert_test.go +++ b/internal/connect/cert_test.go @@ -77,21 +77,21 @@ func TestDynamicCert_GetCertificateForHost_DNSHost(t *testing.T) { }, zap.NewNop()) require.NoError(t, err) - minted, err := cert.GetCertificateForHost("app.internal") + issued, err := cert.GetCertificateForHost(t.Context(), "app.internal") require.NoError(t, err) - assert.Equal(t, "app.internal", minted.Leaf.Subject.CommonName) - assert.Equal(t, []string{"app.internal"}, minted.Leaf.DNSNames) - assert.WithinDuration(t, time.Now().Add(24*time.Hour), minted.Leaf.NotAfter, time.Minute) + assert.Equal(t, "app.internal", issued.Leaf.Subject.CommonName) + assert.Equal(t, []string{"app.internal"}, issued.Leaf.DNSNames) + assert.WithinDuration(t, time.Now().Add(24*time.Hour), issued.Leaf.NotAfter, time.Minute) - key, ok := minted.PrivateKey.(*ecdsa.PrivateKey) + key, ok := issued.PrivateKey.(*ecdsa.PrivateKey) require.True(t, ok, "expected an ECDSA leaf key by default") assert.Equal(t, elliptic.P256(), key.Curve) pool := x509.NewCertPool() pool.AppendCertsFromPEM(data.ProxyCert) - _, err = minted.Leaf.Verify(x509.VerifyOptions{ + _, err = issued.Leaf.Verify(x509.VerifyOptions{ DNSName: "app.internal", Roots: pool, KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, @@ -107,12 +107,12 @@ func TestDynamicCert_GetCertificateForHost_IPHost(t *testing.T) { }, zap.NewNop()) require.NoError(t, err) - minted, err := cert.GetCertificateForHost("10.0.0.5") + issued, err := cert.GetCertificateForHost(t.Context(), "10.0.0.5") require.NoError(t, err) - require.Len(t, minted.Leaf.IPAddresses, 1) - assert.True(t, minted.Leaf.IPAddresses[0].Equal(net.ParseIP("10.0.0.5"))) - assert.Empty(t, minted.Leaf.DNSNames) + require.Len(t, issued.Leaf.IPAddresses, 1) + assert.True(t, issued.Leaf.IPAddresses[0].Equal(net.ParseIP("10.0.0.5"))) + assert.Empty(t, issued.Leaf.DNSNames) } func TestDynamicCert_GetCertificateForHost_CoversAliases(t *testing.T) { @@ -124,6 +124,13 @@ func TestDynamicCert_GetCertificateForHost_CoversAliases(t *testing.T) { wantDNS []string wantIPs []string }{ + { + name: "aliases are sorted after the host", + host: "app.internal", + aliases: []string{"b.internal", "a.internal"}, + wantCN: "app.internal", + wantDNS: []string{"app.internal", "a.internal", "b.internal"}, + }, { name: "ip host with dns aliases", host: "10.0.0.5", @@ -163,14 +170,14 @@ func TestDynamicCert_GetCertificateForHost_CoversAliases(t *testing.T) { }, zap.NewNop()) require.NoError(t, err) - minted, err := cert.GetCertificateForHost(tt.host, tt.aliases...) + issued, err := cert.GetCertificateForHost(t.Context(), tt.host, tt.aliases...) require.NoError(t, err) - assert.Equal(t, tt.wantCN, minted.Leaf.Subject.CommonName) - assert.Equal(t, tt.wantDNS, minted.Leaf.DNSNames) + assert.Equal(t, tt.wantCN, issued.Leaf.Subject.CommonName) + assert.Equal(t, tt.wantDNS, issued.Leaf.DNSNames) var gotIPs []string - for _, ip := range minted.Leaf.IPAddresses { + for _, ip := range issued.Leaf.IPAddresses { gotIPs = append(gotIPs, ip.String()) } @@ -187,23 +194,23 @@ func TestDynamicCert_GetCertificateForHost_CachesPerNameSet(t *testing.T) { }, zap.NewNop()) require.NoError(t, err) - first, err := cert.GetCertificateForHost("app.internal", "a.internal") + first, err := cert.GetCertificateForHost(t.Context(), "app.internal", "a.internal") require.NoError(t, err) // The same host with a different alias set needs its own certificate. - other, err := cert.GetCertificateForHost("app.internal", "b.internal") + other, err := cert.GetCertificateForHost(t.Context(), "app.internal", "b.internal") require.NoError(t, err) assert.NotEqual(t, first.Leaf.SerialNumber, other.Leaf.SerialNumber) - again, err := cert.GetCertificateForHost("app.internal", "a.internal") + again, err := cert.GetCertificateForHost(t.Context(), "app.internal", "a.internal") require.NoError(t, err) assert.Same(t, first, again) // The same aliases in a different order are the same name set. - sorted, err := cert.GetCertificateForHost("app.internal", "a.internal", "b.internal") + sorted, err := cert.GetCertificateForHost(t.Context(), "app.internal", "a.internal", "b.internal") require.NoError(t, err) - reordered, err := cert.GetCertificateForHost("app.internal", "b.internal", "a.internal") + reordered, err := cert.GetCertificateForHost(t.Context(), "app.internal", "b.internal", "a.internal") require.NoError(t, err) assert.Same(t, sorted, reordered) } @@ -220,7 +227,7 @@ func TestDynamicCert_GetCertificateForHost_RenewsInsideWindow(t *testing.T) { }, zap.NewNop()) require.NoError(t, err) - first, err := cert.GetCertificateForHost("app.internal") + first, err := cert.GetCertificateForHost(t.Context(), "app.internal") require.NoError(t, err) // Expire the cached certificate into its renewal window. @@ -229,19 +236,19 @@ func TestDynamicCert_GetCertificateForHost_RenewsInsideWindow(t *testing.T) { cached.Leaf.NotAfter = time.Now().Add(30 * time.Minute) - second, err := cert.GetCertificateForHost("app.internal") + second, err := cert.GetCertificateForHost(t.Context(), "app.internal") require.NoError(t, err) assert.NotEqual(t, first.Leaf.SerialNumber, second.Leaf.SerialNumber) - // Re-minting replaces the cached entry rather than adding another one. + // Re-issuing replaces the cached entry rather than adding another one. assert.Equal(t, 1, cert.cache.Len()) } -// Concurrent cold misses for one host must mint once. This is what the +// Concurrent cold misses for one host must issue once. This is what the // re-check inside the lock in GetCertificateForHost buys; without it every -// caller mints its own certificate. -func TestDynamicCert_GetCertificateForHost_ConcurrentColdMissMintsOnce(t *testing.T) { +// caller issues its own certificate. +func TestDynamicCert_GetCertificateForHost_ConcurrentColdMissIssuesOnce(t *testing.T) { const callers = 10 cert, err := NewDynamicCert(config.TLSDynamicConfig{ @@ -252,10 +259,10 @@ func TestDynamicCert_GetCertificateForHost_ConcurrentColdMissMintsOnce(t *testin require.NoError(t, err) var ( - mu sync.Mutex - wg sync.WaitGroup - mintErr error - serials = map[string]struct{}{} + mu sync.Mutex + wg sync.WaitGroup + issueErr error + serials = map[string]struct{}{} ) start := make(chan struct{}) @@ -264,13 +271,13 @@ func TestDynamicCert_GetCertificateForHost_ConcurrentColdMissMintsOnce(t *testin wg.Go(func() { <-start - got, err := cert.GetCertificateForHost("cold.internal") + got, err := cert.GetCertificateForHost(t.Context(), "cold.internal") mu.Lock() defer mu.Unlock() if err != nil { - mintErr = err + issueErr = err return } @@ -282,7 +289,7 @@ func TestDynamicCert_GetCertificateForHost_ConcurrentColdMissMintsOnce(t *testin close(start) wg.Wait() - require.NoError(t, mintErr) - assert.Len(t, serials, 1, "concurrent cold misses should mint exactly once") + require.NoError(t, issueErr) + assert.Len(t, serials, 1, "concurrent cold misses should issue exactly once") assert.Equal(t, 1, cert.cache.Len()) } diff --git a/internal/connect/conn.go b/internal/connect/conn.go index db1194df..30ebaf68 100644 --- a/internal/connect/conn.go +++ b/internal/connect/conn.go @@ -251,13 +251,13 @@ func (p *ProxyConn) UpgradeToTLS() error { // getTLSConfig pins the served certificate to the CONNECT host. func (p *ProxyConn) getTLSConfig() *tls.Config { tlsConfig := p.TLSConfig.Clone() - tlsConfig.GetCertificate = func(_ *tls.ClientHelloInfo) (*tls.Certificate, error) { + tlsConfig.GetCertificate = func(hello *tls.ClientHelloInfo) (*tls.Certificate, error) { host, _, err := net.SplitHostPort(p.DownstreamAddress) if err != nil { return nil, fmt.Errorf("failed to parse downstream address %q: %w", p.DownstreamAddress, err) } - cert, err := p.CertProvider.GetCertificateForHost(host, p.Claims.Resource.Aliases...) + cert, err := p.CertProvider.GetCertificateForHost(hello.Context(), host, p.Claims.Resource.Aliases...) if err != nil { return nil, fmt.Errorf("failed to get certificate for %q: %w", host, err) } diff --git a/internal/connect/conn_test.go b/internal/connect/conn_test.go index a4234ff5..e06bef52 100644 --- a/internal/connect/conn_test.go +++ b/internal/connect/conn_test.go @@ -550,7 +550,7 @@ func TestProxyConn_UpgradeToTLS_HandshakeError(t *testing.T) { }, zap.NewNop()) require.NoError(t, err) - // The client trusts a different CA, so it rejects the minted certificate + // The client trusts a different CA, so it rejects the issued certificate // and the server-side handshake fails. wrongPool := x509.NewCertPool() wrongPool.AppendCertsFromPEM(data.ServerCert) From b7273ef8ed2397b78a882bb954078ef8c225986a Mon Sep 17 00:00:00 2001 From: Clement Tee Date: Tue, 11 Aug 2026 20:24:39 +0800 Subject: [PATCH 08/11] Remove redundant comments --- internal/connect/cert_provider.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/internal/connect/cert_provider.go b/internal/connect/cert_provider.go index cb0619a8..00261c48 100644 --- a/internal/connect/cert_provider.go +++ b/internal/connect/cert_provider.go @@ -24,8 +24,6 @@ type CertProvider interface { // host: the SNI host on the outer TLS, the validated CONNECT host // on the inner TLS. Aliases are covered as additional subject alternative // names, since the downstream client may have dialled any of them. - // The context is the TLS handshake's, canceling any in-flight work when - // the handshake is abandoned. // // Note: SNI cannot carry an IP address (see RFC 6066 § 3) GetCertificateForHost(ctx context.Context, host string, aliases ...string) (*tls.Certificate, error) From 256a06c25b5f9bccb68f10d3c2894bd4ad1985f1 Mon Sep 17 00:00:00 2001 From: Clement Tee Date: Tue, 25 Aug 2026 13:32:40 +0800 Subject: [PATCH 09/11] Use existing `requestedHost` instead --- internal/connect/conn.go | 11 ++----- internal/connect/conn_test.go | 56 ++++++++++------------------------- internal/connect/connect.go | 22 +++++++------- 3 files changed, 27 insertions(+), 62 deletions(-) diff --git a/internal/connect/conn.go b/internal/connect/conn.go index 2e77535f..9f9da70c 100644 --- a/internal/connect/conn.go +++ b/internal/connect/conn.go @@ -59,7 +59,6 @@ type ProxyConn struct { ID string RequestedHost string - DownstreamAddress string UpstreamHost string Claims *token.GATClaims Token string @@ -261,14 +260,9 @@ func (p *ProxyConn) UpgradeToTLS() error { func (p *ProxyConn) getTLSConfig() *tls.Config { tlsConfig := p.TLSConfig.Clone() tlsConfig.GetCertificate = func(hello *tls.ClientHelloInfo) (*tls.Certificate, error) { - host, _, err := net.SplitHostPort(p.DownstreamAddress) + cert, err := p.CertProvider.GetCertificateForHost(hello.Context(), p.RequestedHost, p.Claims.Resource.Aliases...) if err != nil { - return nil, fmt.Errorf("failed to parse downstream address %q: %w", p.DownstreamAddress, err) - } - - cert, err := p.CertProvider.GetCertificateForHost(hello.Context(), host, p.Claims.Resource.Aliases...) - if err != nil { - return nil, fmt.Errorf("failed to get certificate for %q: %w", host, err) + return nil, fmt.Errorf("failed to get certificate for %q: %w", p.RequestedHost, err) } return cert, nil @@ -280,7 +274,6 @@ func (p *ProxyConn) getTLSConfig() *tls.Config { func (p *ProxyConn) setConnectInfo(connectInfo Info) { p.ID = connectInfo.ConnID p.RequestedHost = connectInfo.RequestedHost - p.DownstreamAddress = connectInfo.DownstreamAddress p.UpstreamHost = connectInfo.UpstreamHost p.Claims = connectInfo.Claims p.Token = connectInfo.Token diff --git a/internal/connect/conn_test.go b/internal/connect/conn_test.go index a67e78b4..fa418379 100644 --- a/internal/connect/conn_test.go +++ b/internal/connect/conn_test.go @@ -536,11 +536,11 @@ func upgradeToTLSHandshake(t *testing.T, proxyConn *ProxyConn, clientTLSConfig * func TestProxyConn_getTLSConfig(t *testing.T) { provider := &recordingCertProvider{} proxyConn := &ProxyConn{ - TLSConfig: &tls.Config{}, - CertProvider: provider, - DownstreamAddress: "grafana.internal:443", - Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp, Aliases: []string{"echo.internal", "echo-alt.internal"}}}, - Logger: zap.NewNop(), + TLSConfig: &tls.Config{}, + CertProvider: provider, + RequestedHost: "grafana.internal", + Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp, Aliases: []string{"echo.internal", "echo-alt.internal"}}}, + Logger: zap.NewNop(), } cert, err := proxyConn.getTLSConfig().GetCertificate(&tls.ClientHelloInfo{ServerName: "other.internal"}) @@ -566,11 +566,11 @@ func TestProxyConn_UpgradeToTLS_HandshakeError(t *testing.T) { wrongPool.AppendCertsFromPEM(data.ServerCert) proxyConn := &ProxyConn{ - TLSConfig: &tls.Config{}, - CertProvider: cert, - DownstreamAddress: "app.internal:443", - Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp}}, - Logger: zap.NewNop(), + TLSConfig: &tls.Config{}, + CertProvider: cert, + RequestedHost: "app.internal", + Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp}}, + Logger: zap.NewNop(), } err = upgradeToTLSHandshake(t, proxyConn, &tls.Config{ @@ -582,41 +582,15 @@ func TestProxyConn_UpgradeToTLS_HandshakeError(t *testing.T) { require.Error(t, err) } -func TestProxyConn_UpgradeToTLS_MalformedDownstreamAddress(t *testing.T) { - cert, err := NewDynamicCert(config.TLSDynamicConfig{ - CA: config.TLSDynamicCAConfig{ - SelfSign: &config.TLSSelfSignCAConfig{CertificateFile: "../../test/data/proxy/tls.crt", PrivateKeyFile: "../../test/data/proxy/tls.key"}, - }, - Cert: config.TLSDynamicCertConfig{}, - }, zap.NewNop()) - require.NoError(t, err) - - proxyConn := &ProxyConn{ - TLSConfig: &tls.Config{}, - CertProvider: cert, - DownstreamAddress: "garbage", - Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp}}, - Logger: zap.NewNop(), - } - - err = upgradeToTLSHandshake(t, proxyConn, &tls.Config{ - ServerName: "garbage", - MinVersion: tls.VersionTLS13, - }) - - require.Error(t, err) - assert.ErrorContains(t, err, `failed to parse downstream address "garbage"`) -} - func TestProxyConn_UpgradeToTLS_CertProviderError(t *testing.T) { var errCertProviderFailed = errors.New("cert provider failed") proxyConn := &ProxyConn{ - TLSConfig: &tls.Config{}, - CertProvider: &recordingCertProvider{shouldFail: true, err: errCertProviderFailed}, - DownstreamAddress: "app.internal:443", - Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp}}, - Logger: zap.NewNop(), + TLSConfig: &tls.Config{}, + CertProvider: &recordingCertProvider{shouldFail: true, err: errCertProviderFailed}, + RequestedHost: "app.internal", + Claims: &token.GATClaims{Resource: token.Resource{Type: token.ResourceTypeWebApp}}, + Logger: zap.NewNop(), } err := upgradeToTLSHandshake(t, proxyConn, &tls.Config{ diff --git a/internal/connect/connect.go b/internal/connect/connect.go index bd780ecf..cf546f3a 100644 --- a/internal/connect/connect.go +++ b/internal/connect/connect.go @@ -28,12 +28,11 @@ const AuthSignatureHeaderKey string = "X-Token-Signature" const ConnIDHeaderKey string = "X-Connection-Id" type Info struct { - RequestedHost string - DownstreamAddress string - UpstreamHost string - Claims *token.GATClaims - ConnID string - Token string + RequestedHost string + UpstreamHost string + Claims *token.GATClaims + ConnID string + Token string } type HTTPError struct { @@ -142,12 +141,11 @@ func (v *MessageValidator) ParseConnect(req *http.Request, ekm []byte) (connectI } return Info{ - RequestedHost: requestedHost, - DownstreamAddress: req.RequestURI, - UpstreamHost: upstreamHost, - Claims: gatClaims, - ConnID: connID, - Token: bearerToken, + RequestedHost: requestedHost, + UpstreamHost: upstreamHost, + Claims: gatClaims, + ConnID: connID, + Token: bearerToken, }, nil } From 9693e97d7738b89f8622fc78424944d3291dfd4b Mon Sep 17 00:00:00 2001 From: Clement Tee Date: Tue, 25 Aug 2026 20:54:20 +0800 Subject: [PATCH 10/11] Remove redundant change --- internal/connect/cert_reloader_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/connect/cert_reloader_test.go b/internal/connect/cert_reloader_test.go index 2c4f7dd3..6f8d192d 100644 --- a/internal/connect/cert_reloader_test.go +++ b/internal/connect/cert_reloader_test.go @@ -27,6 +27,7 @@ func TestCertReloader_Run(t *testing.T) { cr.Run(t.Context()) requireCertReloader(t, cr, cert) + newCert := generateCert(t) replaceCertFiles(t, certFile, keyFile, newCert) From 6e9d18bad9137aada5c72360f93d3066ddf53d39 Mon Sep 17 00:00:00 2001 From: Clement Tee <56408987+clement0010@users.noreply.github.com> Date: Tue, 25 Aug 2026 21:00:05 +0800 Subject: [PATCH 11/11] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Clement Tee <56408987+clement0010@users.noreply.github.com> --- internal/connect/cert_reloader_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/connect/cert_reloader_test.go b/internal/connect/cert_reloader_test.go index 6f8d192d..2c4f7dd3 100644 --- a/internal/connect/cert_reloader_test.go +++ b/internal/connect/cert_reloader_test.go @@ -27,7 +27,6 @@ func TestCertReloader_Run(t *testing.T) { cr.Run(t.Context()) requireCertReloader(t, cr, cert) - newCert := generateCert(t) replaceCertFiles(t, certFile, keyFile, newCert)