Skip to content

Commit b4454a6

Browse files
committed
tlsconfig: make root pool tests deterministic across platforms
The existing tests relied on x509.SystemCertPool behaving as a regular in-memory cert pool. This assumption only holds on Linux; on macOS and Windows the pool delegates to platform APIs, leading to non-deterministic behavior and test failures. Refactor tests to: - inject a fake "system" cert pool backed by generated test roots - verify leaf certificates instead of root certificates - avoid reliance on host trust stores This makes the tests portable and deterministic while still validating the intended semantics of ExclusiveRootPools. Note: real system pool behavior remains platform-dependent and would ideally be covered by integration tests. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
1 parent 0819711 commit b4454a6

File tree

3 files changed

+209
-87
lines changed

3 files changed

+209
-87
lines changed

tlsconfig/config.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,9 @@ type Options struct {
3434
// the system pool will be used.
3535
ExclusiveRootPools bool
3636
MinVersion uint16
37+
38+
// systemCertPool allows mocking the system cert-pool for testing.
39+
systemCertPool func() (*x509.CertPool, error)
3740
}
3841

3942
// DefaultServerAcceptedCiphers should be uses by code which already has a crypto/tls
@@ -86,7 +89,11 @@ func certPool(opts Options) (*x509.CertPool, error) {
8689
if opts.ExclusiveRootPools {
8790
pool = x509.NewCertPool()
8891
} else {
89-
pool, err = x509.SystemCertPool()
92+
if opts.systemCertPool != nil {
93+
pool, err = opts.systemCertPool()
94+
} else {
95+
pool, err = x509.SystemCertPool()
96+
}
9097
if err != nil {
9198
return nil, fmt.Errorf("failed to read system certificates: %v", err)
9299
}

tlsconfig/config_test.go

Lines changed: 55 additions & 86 deletions
Original file line numberDiff line numberDiff line change
@@ -4,41 +4,14 @@ import (
44
"bytes"
55
"crypto/tls"
66
"crypto/x509"
7-
"encoding/pem"
87
"errors"
98
"os"
9+
"path/filepath"
1010
"reflect"
11-
"runtime"
1211
"testing"
1312
)
1413

15-
// This is the currently active Amazon Root CA 1 (CN=Amazon Root CA 1,O=Amazon,C=US),
16-
// downloaded from: https://www.amazontrust.com/repository/AmazonRootCA1.pem
17-
// It's valid since May 26 00:00:00 2015 GMT and expires on Jan 17 00:00:00 2038 GMT.
18-
// Download updated versions from https://www.amazontrust.com/repository/
1914
const (
20-
systemRootTrustedCert = `
21-
-----BEGIN CERTIFICATE-----
22-
MIIDQTCCAimgAwIBAgITBmyfz5m/jAo54vB4ikPmljZbyjANBgkqhkiG9w0BAQsF
23-
ADA5MQswCQYDVQQGEwJVUzEPMA0GA1UEChMGQW1hem9uMRkwFwYDVQQDExBBbWF6
24-
b24gUm9vdCBDQSAxMB4XDTE1MDUyNjAwMDAwMFoXDTM4MDExNzAwMDAwMFowOTEL
25-
MAkGA1UEBhMCVVMxDzANBgNVBAoTBkFtYXpvbjEZMBcGA1UEAxMQQW1hem9uIFJv
26-
b3QgQ0EgMTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBALJ4gHHKeNXj
27-
ca9HgFB0fW7Y14h29Jlo91ghYPl0hAEvrAIthtOgQ3pOsqTQNroBvo3bSMgHFzZM
28-
9O6II8c+6zf1tRn4SWiw3te5djgdYZ6k/oI2peVKVuRF4fn9tBb6dNqcmzU5L/qw
29-
IFAGbHrQgLKm+a/sRxmPUDgH3KKHOVj4utWp+UhnMJbulHheb4mjUcAwhmahRWa6
30-
VOujw5H5SNz/0egwLX0tdHA114gk957EWW67c4cX8jJGKLhD+rcdqsq08p8kDi1L
31-
93FcXmn/6pUCyziKrlA4b9v7LWIbxcceVOF34GfID5yHI9Y/QCB/IIDEgEw+OyQm
32-
jgSubJrIqg0CAwEAAaNCMEAwDwYDVR0TAQH/BAUwAwEB/zAOBgNVHQ8BAf8EBAMC
33-
AYYwHQYDVR0OBBYEFIQYzIU07LwMlJQuCFmcx7IQTgoIMA0GCSqGSIb3DQEBCwUA
34-
A4IBAQCY8jdaQZChGsV2USggNiMOruYou6r4lK5IpDB/G/wkjUu0yKGX9rbxenDI
35-
U5PMCCjjmCXPI6T53iHTfIUJrU6adTrCC2qJeHZERxhlbI1Bjjt/msv0tadQ1wUs
36-
N+gDS63pYaACbvXy8MWy7Vu33PqUXHeeE6V/Uq2V8viTO96LXFvKWlJbYK8U90vv
37-
o/ufQJVtMVT8QtPHRh8jrdkPSHCa2XV4cdFyQzR1bldZwgJcJmApzyMZFo6IQ6XU
38-
5MsI+yMRQ+hDKXJioaldXgjUkK642M4UwtBV8ob2xJNDd2ZhwLnoQdeXeGADbkpy
39-
rqXRfboQnoZsG4q5WTP468SQvvG5
40-
-----END CERTIFICATE-----
41-
`
4215
rsaPrivateKeyFile = "fixtures/key.pem"
4316
certificateFile = "fixtures/cert.pem"
4417
multiCertificateFile = "fixtures/multi.pem"
@@ -195,39 +168,39 @@ func TestConfigServerTLSClientCASet(t *testing.T) {
195168

196169
// Exclusive root pools determines whether the CA pool will be a union of the system
197170
// certificate pool and custom certs, or an exclusive or of the custom certs and system pool
171+
//
172+
// NOTE: In reality this behavior depends on the platform trust store and would
173+
// be best validated via integration tests. Here we inject a fake system pool so
174+
// the unit test can deterministically verify the intended semantics.
198175
func TestConfigServerExclusiveRootPools(t *testing.T) {
199-
if runtime.GOOS == "windows" || runtime.GOOS == "darwin" {
200-
// FIXME: see https://github.com/docker/go-connections/issues/105.
201-
t.Skip("FIXME: failing on Windows and darwin")
202-
}
203176
key, cert := getCertAndKey()
204-
ca := getMultiCert()
205177

206-
caBytes, err := os.ReadFile(ca)
178+
customRoot, err := newTestTrustRoot()
207179
if err != nil {
208-
t.Fatal("Unable to read CA certs", err)
180+
t.Fatal("Unable to generate fake custom CA", err)
209181
}
210182

211-
var testCerts []*x509.Certificate
212-
for _, pemBytes := range [][]byte{caBytes, []byte(systemRootTrustedCert)} {
213-
pemBlock, _ := pem.Decode(pemBytes)
214-
if pemBlock == nil {
215-
t.Fatal("Malformed certificate")
216-
}
217-
crt, err := x509.ParseCertificate(pemBlock.Bytes)
218-
if err != nil {
219-
t.Fatal("Unable to parse certificate")
220-
}
221-
testCerts = append(testCerts, crt)
183+
ca := filepath.Join(t.TempDir(), "custom-ca.pem")
184+
if err := os.WriteFile(ca, customRoot.RootPEM, 0o644); err != nil {
185+
t.Fatal("Unable to write custom CA file", err)
222186
}
223187

188+
systemLeaf, err := systemRootTrustedX509()
189+
if err != nil {
190+
t.Fatal("Unable to load fake system certificate", err)
191+
}
192+
193+
testCerts := []*x509.Certificate{customRoot.LeafCert, systemLeaf}
194+
224195
// ExclusiveRootPools not set, so should be able to verify both system-signed certs
225196
// and custom CA-signed certs
226197
tlsConfig, err := Server(Options{
227198
CertFile: cert,
228199
KeyFile: key,
229200
ClientAuth: tls.VerifyClientCertIfGiven,
230201
CAFile: ca,
202+
203+
systemCertPool: fakeSystemCertPool(),
231204
})
232205

233206
if err != nil || tlsConfig == nil {
@@ -248,6 +221,7 @@ func TestConfigServerExclusiveRootPools(t *testing.T) {
248221
ClientAuth: tls.VerifyClientCertIfGiven,
249222
CAFile: ca,
250223
ExclusiveRootPools: true,
224+
systemCertPool: fakeSystemCertPool(),
251225
})
252226

253227
if err != nil || tlsConfig == nil {
@@ -264,24 +238,19 @@ func TestConfigServerExclusiveRootPools(t *testing.T) {
264238
}
265239
}
266240

267-
// No CA file provided, system cert should be verifiable only
241+
// No CA file provided means the server does not configure ClientCAs.
268242
tlsConfig, err = Server(Options{
269243
CertFile: cert,
270244
KeyFile: key,
245+
246+
systemCertPool: fakeSystemCertPool(),
271247
})
272248

273249
if err != nil || tlsConfig == nil {
274250
t.Fatal("Unable to configure server TLS", err)
275251
}
276-
277-
for i, crt := range testCerts {
278-
_, err := crt.Verify(x509.VerifyOptions{Roots: tlsConfig.ClientCAs})
279-
switch {
280-
case i == 1 && err != nil:
281-
t.Fatal("Unable to verify system root-signed certificate, even though the root pool should be the system pool only", err)
282-
case i == 0 && err == nil:
283-
t.Fatal("Successfully verified custom certificate though the root pool should be the system pool only", err)
284-
}
252+
if tlsConfig.ClientCAs != nil {
253+
t.Fatal("Expected ClientCAs to be nil when no CA file is provided")
285254
}
286255
}
287256

@@ -538,34 +507,36 @@ func TestConfigClientTLSEncryptedKey(t *testing.T) {
538507

539508
// Exclusive root pools determines whether the CA pool will be a union of the system
540509
// certificate pool and custom certs, or an exclusive or of the custom certs and system pool
510+
//
511+
// NOTE: In reality this behavior depends on the platform trust store and would
512+
// be best validated via integration tests. Here we inject a fake system pool so
513+
// the unit test can deterministically verify the intended semantics.
541514
func TestConfigClientExclusiveRootPools(t *testing.T) {
542-
if runtime.GOOS == "windows" || runtime.GOOS == "darwin" {
543-
// FIXME: see https://github.com/docker/go-connections/issues/105.
544-
t.Skip("FIXME: failing on Windows and darwin")
545-
}
546-
ca := getMultiCert()
515+
key, cert := getCertAndKey()
547516

548-
caBytes, err := os.ReadFile(ca)
517+
customRoot, err := newTestTrustRoot()
549518
if err != nil {
550-
t.Fatal("Unable to read CA certs", err)
519+
t.Fatal("Unable to generate fake custom CA", err)
551520
}
552521

553-
var testCerts []*x509.Certificate
554-
for _, pemBytes := range [][]byte{caBytes, []byte(systemRootTrustedCert)} {
555-
pemBlock, _ := pem.Decode(pemBytes)
556-
if pemBlock == nil {
557-
t.Fatal("Malformed certificate")
558-
}
559-
crt, err := x509.ParseCertificate(pemBlock.Bytes)
560-
if err != nil {
561-
t.Fatal("Unable to parse certificate")
562-
}
563-
testCerts = append(testCerts, crt)
522+
ca := filepath.Join(t.TempDir(), "custom-ca.pem")
523+
if err := os.WriteFile(ca, customRoot.RootPEM, 0o644); err != nil {
524+
t.Fatal("Unable to write custom CA file", err)
525+
}
526+
527+
systemLeaf, err := systemRootTrustedX509()
528+
if err != nil {
529+
t.Fatal("Unable to load fake system certificate", err)
564530
}
565531

532+
testCerts := []*x509.Certificate{customRoot.LeafCert, systemLeaf}
533+
566534
// ExclusiveRootPools not set, so should be able to verify both system-signed certs
567535
// and custom CA-signed certs
568-
tlsConfig, err := Client(Options{CAFile: ca})
536+
tlsConfig, err := Client(Options{
537+
CAFile: ca,
538+
systemCertPool: fakeSystemCertPool(),
539+
})
569540

570541
if err != nil || tlsConfig == nil {
571542
t.Fatal("Unable to configure client TLS", err)
@@ -582,6 +553,7 @@ func TestConfigClientExclusiveRootPools(t *testing.T) {
582553
tlsConfig, err = Client(Options{
583554
CAFile: ca,
584555
ExclusiveRootPools: true,
556+
systemCertPool: fakeSystemCertPool(),
585557
})
586558

587559
if err != nil || tlsConfig == nil {
@@ -598,21 +570,18 @@ func TestConfigClientExclusiveRootPools(t *testing.T) {
598570
}
599571
}
600572

601-
// No CA file provided, system cert should be verifiable only
602-
tlsConfig, err = Client(Options{})
573+
// No CA file provided means the client leaves RootCAs unset.
574+
tlsConfig, err = Client(Options{
575+
CertFile: cert,
576+
KeyFile: key,
577+
systemCertPool: fakeSystemCertPool(),
578+
})
603579

604580
if err != nil || tlsConfig == nil {
605581
t.Fatal("Unable to configure client TLS", err)
606582
}
607-
608-
for i, crt := range testCerts {
609-
_, err := crt.Verify(x509.VerifyOptions{Roots: tlsConfig.RootCAs})
610-
switch {
611-
case i == 1 && err != nil:
612-
t.Fatal("Unable to verify system root-signed certificate, even though the root pool should be the system pool only", err)
613-
case i == 0 && err == nil:
614-
t.Fatal("Successfully verified custom certificate though the root pool should be the system pool only", err)
615-
}
583+
if tlsConfig.RootCAs != nil {
584+
t.Fatal("Expected RootCAs to be nil when no CA file is provided")
616585
}
617586
}
618587

0 commit comments

Comments
 (0)