Skip to content

Commit 68e1953

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 a0082dd commit 68e1953

File tree

3 files changed

+229
-88
lines changed

3 files changed

+229
-88
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
@@ -84,7 +87,11 @@ func certPool(opts Options) (*x509.CertPool, error) {
8487
if opts.ExclusiveRootPools {
8588
pool = x509.NewCertPool()
8689
} else {
87-
pool, err = x509.SystemCertPool()
90+
if opts.systemCertPool != nil {
91+
pool, err = opts.systemCertPool()
92+
} else {
93+
pool, err = x509.SystemCertPool()
94+
}
8895
if err != nil {
8996
return nil, fmt.Errorf("failed to read system certificates: %v", err)
9097
}

tlsconfig/config_test.go

Lines changed: 75 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -7,38 +7,12 @@ import (
77
"encoding/pem"
88
"errors"
99
"os"
10+
"path/filepath"
1011
"reflect"
11-
"runtime"
1212
"testing"
1313
)
1414

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/
1915
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-
`
4216
rsaPrivateKeyFile = "fixtures/key.pem"
4317
certificateFile = "fixtures/cert.pem"
4418
multiCertificateFile = "fixtures/multi.pem"
@@ -63,6 +37,26 @@ func getCertAndEncryptedKey() (string, string) {
6337
return rsaEncryptedPrivateKeyFile, certificateOfEncryptedKeyFile
6438
}
6539

40+
func mustReadCertificate(t *testing.T, path string) *x509.Certificate {
41+
t.Helper()
42+
43+
pemBytes, err := os.ReadFile(path)
44+
if err != nil {
45+
t.Fatal("Unable to read certificate", err)
46+
}
47+
48+
pemBlock, _ := pem.Decode(pemBytes)
49+
if pemBlock == nil {
50+
t.Fatal("Malformed certificate")
51+
}
52+
53+
crt, err := x509.ParseCertificate(pemBlock.Bytes)
54+
if err != nil {
55+
t.Fatal("Unable to parse certificate")
56+
}
57+
return crt
58+
}
59+
6660
// If the cert files and directory are provided but are invalid, an error is
6761
// returned.
6862
func TestConfigServerTLSFailsIfUnableToLoadCerts(t *testing.T) {
@@ -195,39 +189,39 @@ func TestConfigServerTLSClientCASet(t *testing.T) {
195189

196190
// Exclusive root pools determines whether the CA pool will be a union of the system
197191
// certificate pool and custom certs, or an exclusive or of the custom certs and system pool
192+
//
193+
// NOTE: In reality this behavior depends on the platform trust store and would
194+
// be best validated via integration tests. Here we inject a fake system pool so
195+
// the unit test can deterministically verify the intended semantics.
198196
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-
}
203197
key, cert := getCertAndKey()
204-
ca := getMultiCert()
205198

206-
caBytes, err := os.ReadFile(ca)
199+
customRoot, err := newTestTrustRoot()
207200
if err != nil {
208-
t.Fatal("Unable to read CA certs", err)
201+
t.Fatal("Unable to generate fake custom CA", err)
209202
}
210203

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)
204+
ca := filepath.Join(t.TempDir(), "custom-ca.pem")
205+
if err := os.WriteFile(ca, customRoot.RootPEM, 0o644); err != nil {
206+
t.Fatal("Unable to write custom CA file", err)
207+
}
208+
209+
systemLeaf, err := systemRootTrustedX509()
210+
if err != nil {
211+
t.Fatal("Unable to load fake system certificate", err)
222212
}
223213

214+
testCerts := []*x509.Certificate{customRoot.LeafCert, systemLeaf}
215+
224216
// ExclusiveRootPools not set, so should be able to verify both system-signed certs
225217
// and custom CA-signed certs
226218
tlsConfig, err := Server(Options{
227219
CertFile: cert,
228220
KeyFile: key,
229221
ClientAuth: tls.VerifyClientCertIfGiven,
230222
CAFile: ca,
223+
224+
systemCertPool: fakeSystemCertPool(),
231225
})
232226

233227
if err != nil || tlsConfig == nil {
@@ -248,6 +242,7 @@ func TestConfigServerExclusiveRootPools(t *testing.T) {
248242
ClientAuth: tls.VerifyClientCertIfGiven,
249243
CAFile: ca,
250244
ExclusiveRootPools: true,
245+
systemCertPool: fakeSystemCertPool(),
251246
})
252247

253248
if err != nil || tlsConfig == nil {
@@ -264,24 +259,19 @@ func TestConfigServerExclusiveRootPools(t *testing.T) {
264259
}
265260
}
266261

267-
// No CA file provided, system cert should be verifiable only
262+
// No CA file provided means the server does not configure ClientCAs.
268263
tlsConfig, err = Server(Options{
269264
CertFile: cert,
270265
KeyFile: key,
266+
267+
systemCertPool: fakeSystemCertPool(),
271268
})
272269

273270
if err != nil || tlsConfig == nil {
274271
t.Fatal("Unable to configure server TLS", err)
275272
}
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-
}
273+
if tlsConfig.ClientCAs != nil {
274+
t.Fatal("Expected ClientCAs to be nil when no CA file is provided")
285275
}
286276
}
287277

@@ -538,35 +528,36 @@ func TestConfigClientTLSEncryptedKey(t *testing.T) {
538528

539529
// Exclusive root pools determines whether the CA pool will be a union of the system
540530
// certificate pool and custom certs, or an exclusive or of the custom certs and system pool
531+
//
532+
// NOTE: In reality this behavior depends on the platform trust store and would
533+
// be best validated via integration tests. Here we inject a fake system pool so
534+
// the unit test can deterministically verify the intended semantics.
541535
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()
536+
key, cert := getCertAndKey()
547537

548-
caBytes, err := os.ReadFile(ca)
538+
customRoot, err := newTestTrustRoot()
549539
if err != nil {
550-
t.Fatal("Unable to read CA certs", err)
540+
t.Fatal("Unable to generate fake custom CA", err)
551541
}
552542

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)
543+
ca := filepath.Join(t.TempDir(), "custom-ca.pem")
544+
if err := os.WriteFile(ca, customRoot.RootPEM, 0o644); err != nil {
545+
t.Fatal("Unable to write custom CA file", err)
564546
}
565547

548+
systemLeaf, err := systemRootTrustedX509()
549+
if err != nil {
550+
t.Fatal("Unable to load fake system certificate", err)
551+
}
552+
553+
testCerts := []*x509.Certificate{customRoot.LeafCert, systemLeaf}
554+
566555
// ExclusiveRootPools not set, so should be able to verify both system-signed certs
567556
// and custom CA-signed certs
568-
tlsConfig, err := Client(Options{CAFile: ca})
569-
557+
tlsConfig, err := Client(Options{
558+
CAFile: ca,
559+
systemCertPool: fakeSystemCertPool(),
560+
})
570561
if err != nil || tlsConfig == nil {
571562
t.Fatal("Unable to configure client TLS", err)
572563
}
@@ -582,8 +573,8 @@ func TestConfigClientExclusiveRootPools(t *testing.T) {
582573
tlsConfig, err = Client(Options{
583574
CAFile: ca,
584575
ExclusiveRootPools: true,
576+
systemCertPool: fakeSystemCertPool(),
585577
})
586-
587578
if err != nil || tlsConfig == nil {
588579
t.Fatal("Unable to configure client TLS", err)
589580
}
@@ -598,21 +589,18 @@ func TestConfigClientExclusiveRootPools(t *testing.T) {
598589
}
599590
}
600591

601-
// No CA file provided, system cert should be verifiable only
602-
tlsConfig, err = Client(Options{})
592+
// No CA file provided means the client leaves RootCAs unset.
593+
tlsConfig, err = Client(Options{
594+
CertFile: cert,
595+
KeyFile: key,
596+
systemCertPool: fakeSystemCertPool(),
597+
})
603598

604599
if err != nil || tlsConfig == nil {
605600
t.Fatal("Unable to configure client TLS", err)
606601
}
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-
}
602+
if tlsConfig.RootCAs != nil {
603+
t.Fatal("Expected RootCAs to be nil when no CA file is provided")
616604
}
617605
}
618606

0 commit comments

Comments
 (0)