diff --git a/README.md b/README.md index 3510fa86..5e893ba3 100644 --- a/README.md +++ b/README.md @@ -712,7 +712,9 @@ transport.tls.keyFile = "certificate.key" transport.tls.trustedCaFile = "ca.crt" ``` -By default, when neither `transport.tls.trustedCaFile` nor `transport.tls.useSystemRoots` is set, `frpc` does not verify the server certificate. Set `transport.tls.useSystemRoots = true` to verify it against the host's root store instead. With `wss`, verification applies to the TLS endpoint, which may be a reverse proxy. `useSystemRoots` is ignored when `transport.tls.trustedCaFile` is set. +By default, when neither `transport.tls.trustedCaFile` nor `transport.tls.useSystemCa` is set, `frpc` does not verify the server certificate. Set `transport.tls.useSystemCa = true` to verify it using the host's system CAs. If `trustedCaFile` is also set, its CA certificates are added to the system CA pool; setting only `trustedCaFile` trusts only the CAs in that file. CA loading or certificate verification failures cause the connection to fail without falling back to another trust source or skipping verification. + +`useSystemCa` is supported only in `frpc`'s `transport.tls`, and requires `transport.tls.enable = true` unless `transport.protocol = "wss"`. With `wss`, the outer TLS connection is always enabled and verification applies to the TLS endpoint, which may be a reverse proxy. Setting `useSystemCa = true` in `frps`'s `transport.tls` or either process's `webServer.tls` is a configuration error. **`frps` TLS settings:** diff --git a/Release.md b/Release.md index 304a5d83..6e94d4a8 100644 --- a/Release.md +++ b/Release.md @@ -1 +1,3 @@ ## Features + +* frpc can verify server certificates using system CAs, optionally combined with CAs from `trustedCaFile`. diff --git a/client/connector.go b/client/connector.go index 7ffc6860..95138dda 100644 --- a/client/connector.go +++ b/client/connector.go @@ -109,12 +109,12 @@ func (c *defaultConnectorImpl) Open() error { sn = c.cfg.ServerAddr } if lo.FromPtr(c.cfg.Transport.TLS.Enable) { - tlsConfig, err = transport.NewClientTLSConfigWithSystemRoots( + tlsConfig, err = transport.NewClientTLSConfigWithSystemCA( c.cfg.Transport.TLS.CertFile, c.cfg.Transport.TLS.KeyFile, c.cfg.Transport.TLS.TrustedCaFile, sn, - c.cfg.Transport.TLS.UseSystemRoots) + c.cfg.Transport.TLS.UseSystemCa) } else { tlsConfig, err = transport.NewClientTLSConfig("", "", "", sn) } @@ -195,12 +195,12 @@ func (c *defaultConnectorImpl) realConnect() (net.Conn, error) { sn = c.cfg.ServerAddr } - tlsConfig, err = transport.NewClientTLSConfigWithSystemRoots( + tlsConfig, err = transport.NewClientTLSConfigWithSystemCA( c.cfg.Transport.TLS.CertFile, c.cfg.Transport.TLS.KeyFile, c.cfg.Transport.TLS.TrustedCaFile, sn, - c.cfg.Transport.TLS.UseSystemRoots) + c.cfg.Transport.TLS.UseSystemCa) if err != nil { xl.Warnf("fail to build tls configuration, err: %v", err) return nil, err diff --git a/conf/frpc_full_example.toml b/conf/frpc_full_example.toml index f779cc9a..7c553615 100644 --- a/conf/frpc_full_example.toml +++ b/conf/frpc_full_example.toml @@ -131,9 +131,11 @@ transport.tls.enable = true # transport.tls.trustedCaFile = "ca.crt" # transport.tls.serverName = "example.com" -# If useSystemRoots is true and trustedCaFile is not set, frpc verifies the server certificate -# against the host's root store. -# transport.tls.useSystemRoots = false +# If useSystemCa is true, frpc verifies the server certificate using system CAs. +# If trustedCaFile is also set, its CA certificates are added to the system CA pool. +# Only supported in frpc transport.tls; requires tls.enable = true unless protocol is wss. +# CA loading or certificate verification failures cause the connection to fail. +# transport.tls.useSystemCa = false # If the disableCustomTLSFirstByte is set to false, frpc will establish a connection with frps using the # first custom byte when tls is enabled. diff --git a/pkg/config/v1/client.go b/pkg/config/v1/client.go index e30a65c6..6736030a 100644 --- a/pkg/config/v1/client.go +++ b/pkg/config/v1/client.go @@ -175,9 +175,6 @@ type TLSClientConfig struct { // first custom byte when tls is enabled. // Since v0.50.0, the default value has been changed to true, and the first custom byte is disabled by default. DisableCustomTLSFirstByte *bool `json:"disableCustomTLSFirstByte,omitempty"` - // UseSystemRoots enables server certificate verification using system roots. - // TrustedCaFile takes precedence. If neither is configured, verification is disabled. - UseSystemRoots bool `json:"useSystemRoots,omitempty"` TLSConfig } diff --git a/pkg/config/v1/common.go b/pkg/config/v1/common.go index 41bd9a4c..f24f55bd 100644 --- a/pkg/config/v1/common.go +++ b/pkg/config/v1/common.go @@ -79,6 +79,10 @@ type TLSConfig struct { KeyFile string `json:"keyFile,omitempty"` // TrustedCaFile specifies the path of the trusted ca file that will load. TrustedCaFile string `json:"trustedCaFile,omitempty"` + // UseSystemCa enables server certificate verification using system CAs. + // If TrustedCaFile is also set, its certificates are added to the system CA pool. + // Only supported in frpc's transport.tls. With neither trust source, frpc skips verification. + UseSystemCa bool `json:"useSystemCa,omitempty"` // ServerName specifies the custom server name of tls certificate. By // default, server name if same to ServerAddr. ServerName string `json:"serverName,omitempty"` diff --git a/pkg/config/v1/validation/client.go b/pkg/config/v1/validation/client.go index d51e4384..e02dcf12 100644 --- a/pkg/config/v1/validation/client.go +++ b/pkg/config/v1/validation/client.go @@ -164,8 +164,8 @@ func validateTransportConfig(c *v1.ClientTransportConfig) (Warning, error) { warnings = AppendError(warnings, checkTLSConfig("transport.tls.keyFile", c.TLS.KeyFile)) warnings = AppendError(warnings, checkTLSConfig("transport.tls.trustedCaFile", c.TLS.TrustedCaFile)) // wss keeps TLS on regardless of transport.tls.enable. - if c.TLS.UseSystemRoots && c.Protocol != "wss" { - warnings = AppendError(warnings, fmt.Errorf("transport.tls.useSystemRoots is ignored when transport.tls.enable is false")) + if c.TLS.UseSystemCa && c.Protocol != "wss" { + errs = AppendError(errs, fmt.Errorf("transport.tls.useSystemCa requires transport.tls.enable to be true unless transport.protocol is wss")) } } diff --git a/pkg/config/v1/validation/common.go b/pkg/config/v1/validation/common.go index c7b93a32..d791c0d4 100644 --- a/pkg/config/v1/validation/common.go +++ b/pkg/config/v1/validation/common.go @@ -23,6 +23,9 @@ import ( func validateWebServerConfig(c *v1.WebServerConfig) error { if c.TLS != nil { + if c.TLS.UseSystemCa { + return fmt.Errorf("webServer.tls.useSystemCa is only supported in frpc transport.tls") + } if c.TLS.CertFile == "" { return fmt.Errorf("tls.certFile must be specified when tls is enabled") } diff --git a/pkg/config/v1/validation/server.go b/pkg/config/v1/validation/server.go index 54bd8348..a7615aa9 100644 --- a/pkg/config/v1/validation/server.go +++ b/pkg/config/v1/validation/server.go @@ -44,6 +44,9 @@ func (v *ConfigValidator) ValidateServerConfig(c *v1.ServerConfig) (Warning, err if err := validateWebServerConfig(&c.WebServer); err != nil { errs = AppendError(errs, err) } + if c.Transport.TLS.UseSystemCa { + errs = AppendError(errs, fmt.Errorf("transport.tls.useSystemCa is only supported in frpc transport.tls")) + } errs = AppendError(errs, ValidatePort(c.BindPort, "bindPort")) errs = AppendError(errs, ValidatePort(c.KCPBindPort, "kcpBindPort")) diff --git a/pkg/config/v1/validation/tls_test.go b/pkg/config/v1/validation/tls_test.go new file mode 100644 index 00000000..5c1cd36e --- /dev/null +++ b/pkg/config/v1/validation/tls_test.go @@ -0,0 +1,146 @@ +// Copyright 2026 The frp Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package validation + +import ( + "encoding/json" + "fmt" + "testing" + + "github.com/stretchr/testify/require" + + v1 "github.com/fatedier/frp/pkg/config/v1" +) + +func TestClientTLSCATrustSources(t *testing.T) { + for _, test := range []struct { + name string + tlsJSON string + useSystemCa bool + caFile string + }{ + {name: "default", tlsJSON: `{}`}, + {name: "file only", tlsJSON: `{"trustedCaFile":"ca.pem"}`, caFile: "ca.pem"}, + {name: "system only", tlsJSON: `{"useSystemCa":true}`, useSystemCa: true}, + {name: "combined", tlsJSON: `{"useSystemCa":true,"trustedCaFile":"ca.pem"}`, useSystemCa: true, caFile: "ca.pem"}, + } { + t.Run(test.name, func(t *testing.T) { + cfg := &v1.ClientCommonConfig{} + require.NoError(t, json.Unmarshal([]byte(`{"transport":{"tls":`+test.tlsJSON+`}}`), cfg)) + require.NoError(t, cfg.Complete()) + require.Equal(t, test.useSystemCa, cfg.Transport.TLS.UseSystemCa) + require.Equal(t, test.caFile, cfg.Transport.TLS.TrustedCaFile) + warnings, err := NewConfigValidator(nil).ValidateClientCommonConfig(cfg) + require.NoError(t, warnings) + require.NoError(t, err) + }) + } +} + +func TestClientSystemCARequiresTLS(t *testing.T) { + for _, protocol := range []string{"tcp", "kcp", "quic", "websocket", "wss"} { + for _, state := range []struct { + name string + enable *bool + }{ + {name: "default"}, + {name: "enabled", enable: new(true)}, + {name: "disabled", enable: new(false)}, + } { + t.Run(protocol+"/"+state.name, func(t *testing.T) { + cfg := &v1.ClientCommonConfig{} + cfg.Transport.Protocol = protocol + cfg.Transport.TLS.Enable = state.enable + cfg.Transport.TLS.UseSystemCa = true + require.NoError(t, cfg.Complete()) + warnings, err := NewConfigValidator(nil).ValidateClientCommonConfig(cfg) + require.NoError(t, warnings, "useSystemCa must produce an error, not a warning") + if state.name == "disabled" && protocol != "wss" { + require.ErrorContains(t, err, "transport.tls.useSystemCa") + require.ErrorContains(t, err, "transport.tls.enable") + } else { + require.NoError(t, err) + } + }) + } + } +} + +func TestDisabledTLSFileWarnings(t *testing.T) { + for _, protocol := range []string{"tcp", "kcp", "quic", "websocket", "wss"} { + t.Run(protocol, func(t *testing.T) { + cfg := &v1.ClientCommonConfig{} + cfg.Transport.Protocol = protocol + cfg.Transport.TLS.Enable = new(false) + cfg.Transport.TLS.CertFile = "client.pem" + cfg.Transport.TLS.KeyFile = "client.key" + cfg.Transport.TLS.TrustedCaFile = "ca.pem" + require.NoError(t, cfg.Complete()) + warnings, err := NewConfigValidator(nil).ValidateClientCommonConfig(cfg) + require.NoError(t, err) + for _, field := range []string{"certFile", "keyFile", "trustedCaFile"} { + require.ErrorContains(t, warnings, "transport.tls."+field+" is invalid when transport.tls.enable is false") + } + }) + } +} + +func TestSystemCAUnsupportedLocations(t *testing.T) { + for _, test := range []struct { + name string + server bool + configJSON string + fieldPath string + }{ + { + name: "frps transport", server: true, + configJSON: `{"transport":{"tls":{"useSystemCa":%t}}}`, fieldPath: "transport.tls.useSystemCa", + }, + { + name: "frpc webServer", configJSON: `{"webServer":{"tls":{"certFile":"server.pem","keyFile":"server.key","useSystemCa":%t}}}`, + fieldPath: "webServer.tls.useSystemCa", + }, + { + name: "frps webServer", server: true, + configJSON: `{"webServer":{"tls":{"certFile":"server.pem","keyFile":"server.key","useSystemCa":%t}}}`, + fieldPath: "webServer.tls.useSystemCa", + }, + } { + for _, enabled := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/%t", test.name, enabled), func(t *testing.T) { + data := fmt.Appendf(nil, test.configJSON, enabled) + var warnings Warning + var err error + if test.server { + cfg := &v1.ServerConfig{} + require.NoError(t, json.Unmarshal(data, cfg)) + require.NoError(t, cfg.Complete()) + warnings, err = NewConfigValidator(nil).ValidateServerConfig(cfg) + } else { + cfg := &v1.ClientCommonConfig{} + require.NoError(t, json.Unmarshal(data, cfg)) + require.NoError(t, cfg.Complete()) + warnings, err = NewConfigValidator(nil).ValidateClientCommonConfig(cfg) + } + require.NoError(t, warnings) + if enabled { + require.EqualError(t, err, test.fieldPath+" is only supported in frpc transport.tls") + } else { + require.NoError(t, err) + } + }) + } + } +} diff --git a/pkg/transport/tls.go b/pkg/transport/tls.go index 4ffbde93..2bfe4842 100644 --- a/pkg/transport/tls.go +++ b/pkg/transport/tls.go @@ -80,17 +80,23 @@ func newRandomTLSKeyPair() (*tls.Certificate, error) { // Only support one ca file to add func newCertPool(caPath string) (*x509.CertPool, error) { pool := x509.NewCertPool() + if err := appendCertsFromFile(pool, caPath); err != nil { + return nil, err + } + return pool, nil +} +func appendCertsFromFile(pool *x509.CertPool, caPath string) error { caCrt, err := os.ReadFile(caPath) if err != nil { - return nil, err + return err } if !pool.AppendCertsFromPEM(caCrt) { - return nil, fmt.Errorf("failed to parse CA certificate from file %q: no valid PEM certificates found", caPath) + return fmt.Errorf("failed to parse CA certificate from file %q: no valid PEM certificates found", caPath) } - return pool, nil + return nil } func NewServerTLSConfig(certPath, keyPath, caPath string) (*tls.Config, error) { @@ -126,13 +132,21 @@ func NewServerTLSConfig(certPath, keyPath, caPath string) (*tls.Config, error) { } func NewClientTLSConfig(certPath, keyPath, caPath, serverName string) (*tls.Config, error) { - return NewClientTLSConfigWithSystemRoots(certPath, keyPath, caPath, serverName, false) + return NewClientTLSConfigWithSystemCA(certPath, keyPath, caPath, serverName, false) } -// NewClientTLSConfigWithSystemRoots behaves like NewClientTLSConfig, except that when -// caPath is empty and useSystemRoots is true, the server certificate is verified against -// the host's root store. -func NewClientTLSConfigWithSystemRoots(certPath, keyPath, caPath, serverName string, useSystemRoots bool) (*tls.Config, error) { +// NewClientTLSConfigWithSystemCA enables verification against system CAs when +// useSystemCa is true, adding certificates from caPath if it is also set. +// Without either trust source, server certificate verification remains disabled. +func NewClientTLSConfigWithSystemCA(certPath, keyPath, caPath, serverName string, useSystemCa bool) (*tls.Config, error) { + return newClientTLSConfig(certPath, keyPath, caPath, serverName, useSystemCa, x509.SystemCertPool) +} + +func newClientTLSConfig( + certPath, keyPath, caPath, serverName string, + useSystemCa bool, + systemCertPool func() (*x509.CertPool, error), +) (*tls.Config, error) { base := &tls.Config{} if certPath != "" && keyPath != "" { @@ -147,6 +161,24 @@ func NewClientTLSConfigWithSystemRoots(certPath, keyPath, caPath, serverName str base.ServerName = serverName switch { + case caPath != "" && useSystemCa: + // SystemCertPool returns a copy, so appending does not change the system pool. + // Design basis: Go 1.26.7 crypto/x509/verify.go:559-573 first uses the + // Windows/macOS verifier, then falls back to Go verification using only the + // added CAs if platform verification fails. This platform behavior has not been + // tested here; preserve the system pool instead of copying its subjects. + pool, err := systemCertPool() + if err != nil { + return nil, fmt.Errorf("failed to load system CA pool: %w", err) + } + if pool == nil { + return nil, fmt.Errorf("failed to load system CA pool: no pool returned") + } + if err := appendCertsFromFile(pool, caPath); err != nil { + return nil, err + } + base.RootCAs = pool + base.InsecureSkipVerify = false case caPath != "": pool, err := newCertPool(caPath) if err != nil { @@ -155,7 +187,7 @@ func NewClientTLSConfigWithSystemRoots(certPath, keyPath, caPath, serverName str base.RootCAs = pool base.InsecureSkipVerify = false - case useSystemRoots: + case useSystemCa: // A nil RootCAs makes crypto/tls use the host root store, through the // platform verifier on macOS and Windows. base.InsecureSkipVerify = false diff --git a/pkg/transport/tls_test.go b/pkg/transport/tls_test.go index d1a32486..894ec37e 100644 --- a/pkg/transport/tls_test.go +++ b/pkg/transport/tls_test.go @@ -23,6 +23,7 @@ import ( "encoding/pem" "errors" "math/big" + "net" "os" "path/filepath" "testing" @@ -31,18 +32,21 @@ import ( "github.com/stretchr/testify/require" ) -func newSelfSignedCert(t *testing.T) (tls.Certificate, []byte) { +func newSelfSignedCert(t *testing.T, name string) (tls.Certificate, []byte) { t.Helper() key, err := rsa.GenerateKey(rand.Reader, 2048) require.NoError(t, err) template := x509.Certificate{ - SerialNumber: big.NewInt(1), - Subject: pkix.Name{CommonName: "localhost"}, - DNSNames: []string{"localhost"}, - NotBefore: time.Now().Add(-time.Hour), - NotAfter: time.Now().Add(time.Hour), + SerialNumber: big.NewInt(1), + IsCA: true, + BasicConstraintsValid: true, + KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageDigitalSignature, + Subject: pkix.Name{CommonName: name}, + DNSNames: []string{name}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), } certDER, err := x509.CreateCertificate(rand.Reader, &template, &template, &key.PublicKey, key) require.NoError(t, err) @@ -56,84 +60,191 @@ func newSelfSignedCert(t *testing.T) (tls.Certificate, []byte) { } func TestNewClientTLSConfigVerification(t *testing.T) { - _, caPEM := newSelfSignedCert(t) + _, systemPEM := newSelfSignedCert(t, "system.example.com") + _, filePEM := newSelfSignedCert(t, "file.example.com") caFile := filepath.Join(t.TempDir(), "ca.crt") - require.NoError(t, os.WriteFile(caFile, caPEM, 0o600)) + require.NoError(t, os.WriteFile(caFile, filePEM, 0o600)) - tests := []struct { + systemPool := x509.NewCertPool() + require.True(t, systemPool.AppendCertsFromPEM(systemPEM)) + filePool := x509.NewCertPool() + require.True(t, filePool.AppendCertsFromPEM(filePEM)) + combinedPool := systemPool.Clone() + require.True(t, combinedPool.AppendCertsFromPEM(filePEM)) + + for _, test := range []struct { name string caPath string - useSystemRoots bool + useSystemCa bool insecureSkipVerify bool - rootCAsSet bool + wantPool *x509.CertPool }{ - { - name: "no trusted ca file, no system roots", - insecureSkipVerify: true, - }, - { - name: "system roots", - useSystemRoots: true, - }, - { - name: "trusted ca file", - caPath: caFile, - rootCAsSet: true, - }, - { - name: "trusted ca file takes precedence over system roots", - caPath: caFile, - useSystemRoots: true, - rootCAsSet: true, - }, + {name: "no trust source", insecureSkipVerify: true}, + {name: "file CA only", caPath: caFile, wantPool: filePool}, + {name: "system CA only", useSystemCa: true}, + {name: "system and file CAs", caPath: caFile, useSystemCa: true, wantPool: combinedPool}, + } { + t.Run(test.name, func(t *testing.T) { + var loadedPool *x509.CertPool + loads := 0 + loadSystemPool := func() (*x509.CertPool, error) { + loads++ + loadedPool = systemPool.Clone() + return loadedPool, nil + } + cfg, err := newClientTLSConfig("", "", test.caPath, "example.com", test.useSystemCa, loadSystemPool) + require.NoError(t, err) + require.Equal(t, "example.com", cfg.ServerName) + require.Equal(t, test.insecureSkipVerify, cfg.InsecureSkipVerify) + if test.wantPool == nil { + require.Nil(t, cfg.RootCAs) + } else { + require.NotNil(t, cfg.RootCAs) + require.True(t, cfg.RootCAs.Equal(test.wantPool)) + } + if test.caPath != "" && test.useSystemCa { + require.Equal(t, 1, loads) + require.Same(t, loadedPool, cfg.RootCAs, "append to the returned system pool") + } else { + require.Zero(t, loads, "only combined trust needs an explicit system pool") + } + }) } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - require := require.New(t) + // The four-argument API keeps the original skip/file-only semantics for + // xtcp visitors and plugins, regardless of the new transport option. + cfg, err := NewClientTLSConfig("", "", "", "example.com") + require.NoError(t, err) + require.True(t, cfg.InsecureSkipVerify) + require.Nil(t, cfg.RootCAs) + cfg, err = NewClientTLSConfig("", "", caFile, "example.com") + require.NoError(t, err) + require.False(t, cfg.InsecureSkipVerify) + require.True(t, cfg.RootCAs.Equal(filePool)) +} - cfg, err := NewClientTLSConfigWithSystemRoots("", "", test.caPath, "example.com", test.useSystemRoots) - require.NoError(err) - require.Equal("example.com", cfg.ServerName) - require.Equal(test.insecureSkipVerify, cfg.InsecureSkipVerify) - require.Equal(test.rootCAsSet, cfg.RootCAs != nil) +func TestNewClientTLSConfigCAErrors(t *testing.T) { + _, caPEM := newSelfSignedCert(t, "localhost") + dir := t.TempDir() + caFile := filepath.Join(dir, "ca.crt") + badFile := filepath.Join(dir, "bad.crt") + require.NoError(t, os.WriteFile(caFile, caPEM, 0o600)) + require.NoError(t, os.WriteFile(badFile, []byte("not a PEM certificate"), 0o600)) + systemErr := errors.New("system trust store unavailable") + + for _, test := range []struct { + name string + caPath string + useSystemCa bool + pool *x509.CertPool + poolErr error + wantErr string + }{ + {name: "file missing", caPath: filepath.Join(dir, "missing.crt"), wantErr: "missing.crt"}, + {name: "file invalid", caPath: badFile, wantErr: "no valid PEM certificates found"}, + { + name: "combined file missing", caPath: filepath.Join(dir, "missing.crt"), useSystemCa: true, + pool: x509.NewCertPool(), wantErr: "missing.crt", + }, + { + name: "combined file invalid", caPath: badFile, useSystemCa: true, + pool: x509.NewCertPool(), wantErr: "no valid PEM certificates found", + }, + { + name: "system pool error", caPath: caFile, useSystemCa: true, + pool: x509.NewCertPool(), poolErr: systemErr, wantErr: "failed to load system CA pool", + }, + {name: "nil system pool", caPath: caFile, useSystemCa: true, wantErr: "no pool returned"}, + } { + t.Run(test.name, func(t *testing.T) { + cfg, err := newClientTLSConfig("", "", test.caPath, "localhost", test.useSystemCa, + func() (*x509.CertPool, error) { return test.pool, test.poolErr }) + require.ErrorContains(t, err, test.wantErr) + require.Nil(t, cfg, "do not return a partially configured TLS client or fall back") + if test.poolErr != nil { + require.ErrorIs(t, err, test.poolErr) + } }) } } -func TestClientTLSConfigHandshakeWithUntrustedServer(t *testing.T) { - require := require.New(t) +func TestClientTLSConfigHandshake(t *testing.T) { + systemCert, systemPEM := newSelfSignedCert(t, "localhost") + fileCert, filePEM := newSelfSignedCert(t, "localhost") + untrustedCert, _ := newSelfSignedCert(t, "localhost") + caFile := filepath.Join(t.TempDir(), "ca.crt") + require.NoError(t, os.WriteFile(caFile, filePEM, 0o600)) + systemPool := x509.NewCertPool() + require.True(t, systemPool.AppendCertsFromPEM(systemPEM)) - cert, _ := newSelfSignedCert(t) - ln, err := tls.Listen("tcp", "127.0.0.1:0", &tls.Config{Certificates: []tls.Certificate{cert}}) - require.NoError(err) - t.Cleanup(func() { _ = ln.Close() }) + for _, server := range []struct { + name string + cert tls.Certificate + }{ + {name: "system CA", cert: systemCert}, + {name: "file CA", cert: fileCert}, + {name: "untrusted CA", cert: untrustedCert}, + } { + t.Run(server.name, func(t *testing.T) { + ln, err := tls.Listen("tcp", "127.0.0.1:0", &tls.Config{Certificates: []tls.Certificate{server.cert}}) + require.NoError(t, err) + t.Cleanup(func() { _ = ln.Close() }) + go func() { + for { + conn, err := ln.Accept() + if err != nil { + return + } + _ = conn.SetDeadline(time.Now().Add(5 * time.Second)) + _ = conn.(*tls.Conn).Handshake() + _ = conn.Close() + } + }() - go func() { - for { - conn, err := ln.Accept() - if err != nil { - return + for _, test := range []struct { + name string + caPath string + useSystemCa bool + serverName string + wantErr bool + }{ + {name: "no trust source"}, + {name: "file CA only", caPath: caFile, wantErr: server.name != "file CA"}, + // All certificates were just generated and are absent from the real + // host store, even on hosts without any installed system roots. + {name: "system CA only rejects unknown certificate", useSystemCa: true, wantErr: true}, + {name: "combined CAs", caPath: caFile, useSystemCa: true, wantErr: server.name == "untrusted CA"}, + { + name: "combined CAs reject wrong name", caPath: caFile, useSystemCa: true, + serverName: "other.example.com", wantErr: true, + }, + } { + t.Run(test.name, func(t *testing.T) { + serverName := test.serverName + if serverName == "" { + serverName = "localhost" + } + var cfg *tls.Config + var err error + if test.caPath != "" && test.useSystemCa { + cfg, err = newClientTLSConfig("", "", test.caPath, serverName, true, + func() (*x509.CertPool, error) { return systemPool.Clone(), nil }) + } else { + cfg, err = NewClientTLSConfigWithSystemCA("", "", test.caPath, serverName, test.useSystemCa) + } + require.NoError(t, err) + conn, err := tls.DialWithDialer(&net.Dialer{Timeout: 5 * time.Second}, "tcp", ln.Addr().String(), cfg) + if conn != nil { + _ = conn.Close() + } + if test.wantErr { + var verificationErr *tls.CertificateVerificationError + require.ErrorAs(t, err, &verificationErr) + } else { + require.NoError(t, err) + } + }) } - _ = conn.(*tls.Conn).Handshake() - _ = conn.Close() - } - }() - - dial := func(useSystemRoots bool) error { - cfg, err := NewClientTLSConfigWithSystemRoots("", "", "", "localhost", useSystemRoots) - require.NoError(err) - conn, err := tls.Dial("tcp", ln.Addr().String(), cfg) - if err != nil { - return err - } - return conn.Close() + }) } - - require.NoError(dial(false)) - - err = dial(true) - require.Error(err) - var verificationErr *tls.CertificateVerificationError - require.True(errors.As(err, &verificationErr), "expected a certificate verification error, got %v", err) } diff --git a/test/e2e/v1/basic/client_server.go b/test/e2e/v1/basic/client_server.go index e8256cdb..85c0d647 100644 --- a/test/e2e/v1/basic/client_server.go +++ b/test/e2e/v1/basic/client_server.go @@ -177,8 +177,8 @@ var _ = ginkgo.Describe("[Feature: Client-Server]", func() { client: "transport.tls.enable = false", expectError: true, }) - defineClientServerTest("useSystemRoots rejects the default self-signed server certificate", f, &generalTestConfigures{ - client: "transport.tls.useSystemRoots = true", + defineClientServerTest("useSystemCa rejects the default self-signed server certificate", f, &generalTestConfigures{ + client: "transport.tls.useSystemCa = true", expectError: true, }) })