From f18ba6ae236d27840649fac46afc53638f3f1ec2 Mon Sep 17 00:00:00 2001 From: Bojan Zivanovic Date: Mon, 10 Aug 2026 15:19:03 +0200 Subject: [PATCH 1/5] fix(legacy): preserve Windows certificate purposes --- internal/legacy/ca_bundle_windows.go | 70 +++++++++++++++++++++-- internal/legacy/ca_bundle_windows_test.go | 43 ++++++++++++++ 2 files changed, 108 insertions(+), 5 deletions(-) diff --git a/internal/legacy/ca_bundle_windows.go b/internal/legacy/ca_bundle_windows.go index 41ec041a..def1948b 100644 --- a/internal/legacy/ca_bundle_windows.go +++ b/internal/legacy/ca_bundle_windows.go @@ -53,10 +53,17 @@ func systemRootsPEM() ([]byte, error) { } var out bytes.Buffer - err = eachStoreCert("ROOT", func(der []byte) error { + err = eachStoreCert("ROOT", func(certContext *windows.CertContext, der []byte) error { if shipped[sha256.Sum256(der)] { return nil } + allowed, err := certAllowsServerAuth(certContext) + if err != nil { + return err + } + if !allowed { + return nil + } cert, err := x509.ParseCertificate(der) if err != nil { // A store can hold a certificate Go cannot read, and one which @@ -93,9 +100,62 @@ func certFingerprints(bundle []byte) (map[[32]byte]bool, error) { return fingerprints, nil } -// eachStoreCert calls fn with the encoded bytes of every certificate in a -// Windows system store, as read by every program which verifies against it. -func eachStoreCert(name string, fn func(der []byte) error) error { +var procCertGetEnhancedKeyUsage = windows.NewLazySystemDLL("crypt32.dll").NewProc("CertGetEnhancedKeyUsage") + +// certAllowsServerAuth reports whether a certificate's effective Windows EKUs +// allow it to authenticate a TLS server. Windows combines the EKU extension in +// the encoded certificate with an EKU property held only in the store. The +// property has to be checked before exporting the certificate to PEM, which +// cannot preserve it. +func certAllowsServerAuth(certContext *windows.CertContext) (bool, error) { + var size uint32 + result, _, callErr := procCertGetEnhancedKeyUsage.Call( + uintptr(unsafe.Pointer(certContext)), + 0, + 0, + uintptr(unsafe.Pointer(&size)), + ) + if result == 0 { + return false, fmt.Errorf("could not read certificate purposes: %w", callErr) + } + if size < uint32(unsafe.Sizeof(windows.CertEnhKeyUsage{})) { + return false, fmt.Errorf("could not read certificate purposes: unexpected data size %d", size) + } + + wordSize := uint32(unsafe.Sizeof(uintptr(0))) + buffer := make([]uintptr, (size+wordSize-1)/wordSize) + usage := (*windows.CertEnhKeyUsage)(unsafe.Pointer(&buffer[0])) + result, _, callErr = procCertGetEnhancedKeyUsage.Call( + uintptr(unsafe.Pointer(certContext)), + 0, + uintptr(unsafe.Pointer(usage)), + uintptr(unsafe.Pointer(&size)), + ) + if result == 0 { + return false, fmt.Errorf("could not read certificate purposes: %w", callErr) + } + if usage.Length == 0 { + // CRYPT_E_NOT_FOUND means there is no restriction, while a zero last + // error means the certificate has explicitly been given no valid uses. + return errors.Is(callErr, windows.Errno(windows.CRYPT_E_NOT_FOUND)), nil + } + + for _, identifier := range unsafe.Slice(usage.UsageIdentifiers, usage.Length) { + switch windows.BytePtrToString(identifier) { + case "1.3.6.1.5.5.7.3.1", // Server Authentication. + "1.3.6.1.4.1.311.10.3.3", // Microsoft Server Gated Crypto. + "2.16.840.1.113730.4.1", // Netscape Server Gated Crypto. + "2.5.29.37.0": // Any Extended Key Usage. + return true, nil + } + } + return false, nil +} + +// eachStoreCert calls fn with the context and encoded bytes of every +// certificate in a Windows system store, as read by every program which +// verifies against it. The context is needed for properties not held in DER. +func eachStoreCert(name string, fn func(certContext *windows.CertContext, der []byte) error) error { store, err := windows.CertOpenSystemStore(0, windows.StringToUTF16Ptr(name)) if err != nil { return fmt.Errorf("could not open the %s certificate store: %w", name, err) @@ -112,7 +172,7 @@ func eachStoreCert(name string, fn func(der []byte) error) error { return nil } // The context belongs to the store, so the bytes are only borrowed. - if err := fn(unsafe.Slice(certContext.EncodedCert, certContext.Length)); err != nil { + if err := fn(certContext, unsafe.Slice(certContext.EncodedCert, certContext.Length)); err != nil { windows.CertFreeCertificateContext(certContext) //nolint:errcheck return err } diff --git a/internal/legacy/ca_bundle_windows_test.go b/internal/legacy/ca_bundle_windows_test.go index 33c94e10..6e15247d 100644 --- a/internal/legacy/ca_bundle_windows_test.go +++ b/internal/legacy/ca_bundle_windows_test.go @@ -4,11 +4,15 @@ import ( "crypto/x509" "encoding/pem" "testing" + "unsafe" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/sys/windows" ) +var procCertAddEnhancedKeyUsageIdentifier = windows.NewLazySystemDLL("crypt32.dll").NewProc("CertAddEnhancedKeyUsageIdentifier") + func TestWindowsCABundle(t *testing.T) { bundle, err := caBundle() require.NoError(t, err) @@ -41,6 +45,45 @@ func TestWindowsCABundle(t *testing.T) { } } +func TestWindowsCertificatePurposes(t *testing.T) { + ca := generateTestCA(t) + certContext, err := windows.CertCreateCertificateContext( + windows.X509_ASN_ENCODING, + &ca.certDER[0], + uint32(len(ca.certDER)), + ) + require.NoError(t, err) + t.Cleanup(func() { + assert.NoError(t, windows.CertFreeCertificateContext(certContext)) + }) + + allowed, err := certAllowsServerAuth(certContext) + require.NoError(t, err) + assert.True(t, allowed, "a certificate without an EKU restriction is valid for every purpose") + + addWindowsCertificatePurpose(t, certContext, "1.3.6.1.5.5.7.3.3") // Code Signing. + allowed, err = certAllowsServerAuth(certContext) + require.NoError(t, err) + assert.False(t, allowed, "a certificate restricted to code signing must not become a TLS root") + + addWindowsCertificatePurpose(t, certContext, "1.3.6.1.5.5.7.3.1") // Server Authentication. + allowed, err = certAllowsServerAuth(certContext) + require.NoError(t, err) + assert.True(t, allowed, "a certificate which permits server authentication is a TLS root") +} + +func addWindowsCertificatePurpose(t *testing.T, certContext *windows.CertContext, identifier string) { + t.Helper() + + oid, err := windows.BytePtrFromString(identifier) + require.NoError(t, err) + result, _, callErr := procCertAddEnhancedKeyUsageIdentifier.Call( + uintptr(unsafe.Pointer(certContext)), + uintptr(unsafe.Pointer(oid)), + ) + require.NotZero(t, result, "adding certificate purpose failed: %s", callErr) +} + // BenchmarkWindowsCABundle measures reading the store and building the bundle, // which happens before every legacy command. func BenchmarkWindowsCABundle(b *testing.B) { From d1b2dc533446137301edd16c81eedb42d8d0011d Mon Sep 17 00:00:00 2001 From: Bojan Zivanovic Date: Mon, 10 Aug 2026 15:20:54 +0200 Subject: [PATCH 2/5] fix(legacy): fully compare cached CA bundles --- internal/file/file.go | 14 ++++++++++++++ internal/file/file_test.go | 22 ++++++++++++++++++++++ internal/legacy/php_manager_windows.go | 2 +- 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/internal/file/file.go b/internal/file/file.go index 595321fe..6d7565bb 100644 --- a/internal/file/file.go +++ b/internal/file/file.go @@ -18,6 +18,20 @@ func WriteIfNeeded(destFilename string, source []byte, perm os.FileMode) error { return Write(destFilename, source, perm) } +// WriteIfChanged writes data to a destination file unless its complete +// contents already match. Unlike WriteIfNeeded, it is suitable for dynamic +// data which can change without changing its size or final bytes. +func WriteIfChanged(destFilename string, source []byte, perm os.FileMode) error { + existing, err := os.ReadFile(destFilename) + if err == nil && bytes.Equal(existing, source) { + return nil + } + if err != nil && !errors.Is(err, fs.ErrNotExist) { + return err + } + return Write(destFilename, source, perm) +} + // Write creates or overwrites a file, somewhat atomically, using a temporary file next to it. func Write(path string, content []byte, fileMode fs.FileMode) error { tmpFile := path + ".tmp" diff --git a/internal/file/file_test.go b/internal/file/file_test.go index 4874ad01..1e7c21cd 100644 --- a/internal/file/file_test.go +++ b/internal/file/file_test.go @@ -79,3 +79,25 @@ func TestWriteIfNeeded(t *testing.T) { }) } } + +func TestWriteIfChangedChecksTheWholeFile(t *testing.T) { + destFile := filepath.Join(t.TempDir(), "dynamic-file") + original := bytes.Repeat([]byte{'a'}, 64*1024) + updated := bytes.Clone(original) + updated[0] = 'b' + + require.NoError(t, os.WriteFile(destFile, original, 0o600)) + require.NoError(t, WriteIfChanged(destFile, updated, 0o600)) + actual, err := os.ReadFile(destFile) + require.NoError(t, err) + assert.Equal(t, updated, actual, + "equal size and matching final 32 KB must not hide an earlier change") + + before, err := os.Stat(destFile) + require.NoError(t, err) + time.Sleep(5 * time.Millisecond) + require.NoError(t, WriteIfChanged(destFile, updated, 0o600)) + after, err := os.Stat(destFile) + require.NoError(t, err) + assert.Equal(t, before.ModTime(), after.ModTime(), "matching contents should not be rewritten") +} diff --git a/internal/legacy/php_manager_windows.go b/internal/legacy/php_manager_windows.go index f24e7a94..89e85a62 100644 --- a/internal/legacy/php_manager_windows.go +++ b/internal/legacy/php_manager_windows.go @@ -35,7 +35,7 @@ func (m *phpManagerPerOS) writeCAFile() error { // trusted before it read the store, and better than running nothing. m.copyWarnings = append(m.copyWarnings, err.Error()) } - return file.WriteIfNeeded(m.caFilePath(), bundle, 0o644) + return file.WriteIfChanged(m.caFilePath(), bundle, 0o644) } func (m *phpManagerPerOS) caFilePath() string { From ad6e6adbe9d96b40d87a9d54f5b605d86c7f6b29 Mon Sep 17 00:00:00 2001 From: Bojan Zivanovic Date: Mon, 10 Aug 2026 15:22:22 +0200 Subject: [PATCH 3/5] test(legacy): drop unused CA PEM data --- internal/legacy/cert_store_windows_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/internal/legacy/cert_store_windows_test.go b/internal/legacy/cert_store_windows_test.go index a6feecd0..141c28dd 100644 --- a/internal/legacy/cert_store_windows_test.go +++ b/internal/legacy/cert_store_windows_test.go @@ -22,7 +22,6 @@ import ( "crypto/tls" "crypto/x509" "crypto/x509/pkix" - "encoding/pem" "errors" "math/big" "net" @@ -131,7 +130,6 @@ func TestWindowsCertStoreTrust(t *testing.T) { } type testCA struct { - certPEM []byte certDER []byte serverCert tls.Certificate } @@ -175,7 +173,6 @@ func generateTestCA(t *testing.T) testCA { require.NoError(t, err) return testCA{ - certPEM: pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: caDER}), certDER: caDER, serverCert: tls.Certificate{ Certificate: [][]byte{serverDER, caDER}, From 1d6aa0ad7f231f9578f1aaf85c00a6e0f512b149 Mon Sep 17 00:00:00 2001 From: Bojan Zivanovic Date: Mon, 10 Aug 2026 15:45:17 +0200 Subject: [PATCH 4/5] fix(file): read the cached file only when it can match WriteIfChanged read the whole destination file to compare it, even when its size already showed the contents had changed. It now compares the size first, so only a file which could still match is read. The test for the purposes Windows reports for a certificate now covers the EKU extension as well as the store property, and the two together. Windows reports the purposes on both lists, so a property can withdraw the server authentication an extension grants, which is what decides whether a root belongs in the bundle. Setting the property outright rather than adding to it keeps the combined cases unambiguous. The test for leaving an unchanged file alone compared modification times a few milliseconds apart, which the clock can be too coarse to distinguish, so a rewrite could have gone unnoticed. It now backdates the file and checks that the older time survives. Co-Authored-By: Claude Opus 5 (1M context) --- internal/file/file.go | 28 ++++- internal/file/file_test.go | 37 +++++- internal/legacy/ca_bundle_windows_test.go | 146 ++++++++++++++++++---- 3 files changed, 177 insertions(+), 34 deletions(-) diff --git a/internal/file/file.go b/internal/file/file.go index 6d7565bb..1fa725f7 100644 --- a/internal/file/file.go +++ b/internal/file/file.go @@ -22,11 +22,8 @@ func WriteIfNeeded(destFilename string, source []byte, perm os.FileMode) error { // contents already match. Unlike WriteIfNeeded, it is suitable for dynamic // data which can change without changing its size or final bytes. func WriteIfChanged(destFilename string, source []byte, perm os.FileMode) error { - existing, err := os.ReadFile(destFilename) - if err == nil && bytes.Equal(existing, source) { - return nil - } - if err != nil && !errors.Is(err, fs.ErrNotExist) { + matches, err := fullyMatches(destFilename, source) + if err != nil || matches { return err } return Write(destFilename, source, perm) @@ -42,6 +39,27 @@ func Write(path string, content []byte, fileMode fs.FileMode) error { return os.Rename(tmpFile, path) } +// fullyMatches checks if a file exists and its whole contents equal data. The +// size is compared first, so a file which cannot match is not read. +func fullyMatches(filename string, data []byte) (bool, error) { + fi, err := os.Stat(filename) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return false, nil + } + return false, err + } + if fi.Size() != int64(len(data)) { + return false, nil + } + + existing, err := os.ReadFile(filename) + if err != nil { + return false, err + } + return bytes.Equal(existing, data), nil +} + // probablyMatches checks if a file exists and matches the end of source data (up to checkSize bytes). func probablyMatches(filename string, data []byte, checkSize int) (bool, error) { f, err := os.Open(filename) diff --git a/internal/file/file_test.go b/internal/file/file_test.go index 1e7c21cd..cc6373ad 100644 --- a/internal/file/file_test.go +++ b/internal/file/file_test.go @@ -80,6 +80,32 @@ func TestWriteIfNeeded(t *testing.T) { } } +func TestWriteIfChanged(t *testing.T) { + source := []byte("new contents") + + cases := []struct { + name string + initialData []byte + }{ + {"a missing file", nil}, + {"a shorter file", []byte("old")}, + {"a longer file", []byte("old contents, but longer than the new ones")}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + destFile := filepath.Join(t.TempDir(), "dynamic-file") + if c.initialData != nil { + require.NoError(t, os.WriteFile(destFile, c.initialData, 0o600)) + } + + require.NoError(t, WriteIfChanged(destFile, source, 0o600)) + actual, err := os.ReadFile(destFile) + require.NoError(t, err) + assert.Equal(t, source, actual) + }) + } +} + func TestWriteIfChangedChecksTheWholeFile(t *testing.T) { destFile := filepath.Join(t.TempDir(), "dynamic-file") original := bytes.Repeat([]byte{'a'}, 64*1024) @@ -93,11 +119,12 @@ func TestWriteIfChangedChecksTheWholeFile(t *testing.T) { assert.Equal(t, updated, actual, "equal size and matching final 32 KB must not hide an earlier change") - before, err := os.Stat(destFile) - require.NoError(t, err) - time.Sleep(5 * time.Millisecond) + // A rewrite is detected by an older modification time being lost, because + // the clock can tick too slowly to tell two writes apart. + past := time.Unix(1, 0) + require.NoError(t, os.Chtimes(destFile, past, past)) require.NoError(t, WriteIfChanged(destFile, updated, 0o600)) - after, err := os.Stat(destFile) + info, err := os.Stat(destFile) require.NoError(t, err) - assert.Equal(t, before.ModTime(), after.ModTime(), "matching contents should not be rewritten") + assert.Equal(t, past.UTC(), info.ModTime().UTC(), "matching contents should not be rewritten") } diff --git a/internal/legacy/ca_bundle_windows_test.go b/internal/legacy/ca_bundle_windows_test.go index 6e15247d..f2541749 100644 --- a/internal/legacy/ca_bundle_windows_test.go +++ b/internal/legacy/ca_bundle_windows_test.go @@ -1,9 +1,14 @@ package legacy import ( + "crypto/rand" + "crypto/rsa" "crypto/x509" + "crypto/x509/pkix" "encoding/pem" + "math/big" "testing" + "time" "unsafe" "github.com/stretchr/testify/assert" @@ -11,7 +16,7 @@ import ( "golang.org/x/sys/windows" ) -var procCertAddEnhancedKeyUsageIdentifier = windows.NewLazySystemDLL("crypt32.dll").NewProc("CertAddEnhancedKeyUsageIdentifier") +var procCertSetEnhancedKeyUsage = windows.NewLazySystemDLL("crypt32.dll").NewProc("CertSetEnhancedKeyUsage") func TestWindowsCABundle(t *testing.T) { bundle, err := caBundle() @@ -45,43 +50,136 @@ func TestWindowsCABundle(t *testing.T) { } } +// TestWindowsCertificatePurposes checks which certificates count as TLS roots. +// A certificate is valid only for the purposes named by both its EKU extension +// and its store property, and one which names neither is valid for all of them. func TestWindowsCertificatePurposes(t *testing.T) { - ca := generateTestCA(t) + const ( + serverAuth = "1.3.6.1.5.5.7.3.1" + codeSigning = "1.3.6.1.5.5.7.3.3" + gatedCrypto = "1.3.6.1.4.1.311.10.3.3" + ) + + cases := []struct { + name string + extension []x509.ExtKeyUsage + property []string + allowed bool + }{ + { + name: "no purposes at all", + allowed: true, + }, + { + name: "an extension for code signing", + extension: []x509.ExtKeyUsage{x509.ExtKeyUsageCodeSigning}, + allowed: false, + }, + { + name: "an extension for code signing and server authentication", + extension: []x509.ExtKeyUsage{x509.ExtKeyUsageCodeSigning, x509.ExtKeyUsageServerAuth}, + allowed: true, + }, + { + name: "an extension for any purpose", + extension: []x509.ExtKeyUsage{x509.ExtKeyUsageAny}, + allowed: true, + }, + { + name: "a property for code signing", + property: []string{codeSigning}, + allowed: false, + }, + { + name: "a property for code signing and server authentication", + property: []string{codeSigning, serverAuth}, + allowed: true, + }, + { + name: "a property for server gated crypto", + property: []string{gatedCrypto}, + allowed: true, + }, + { + name: "a property which withdraws the extension's server authentication", + extension: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + property: []string{codeSigning}, + allowed: false, + }, + { + name: "a property which keeps the extension's server authentication", + extension: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageCodeSigning}, + property: []string{serverAuth}, + allowed: true, + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + certContext := createTestCertContext(t, c.extension) + if len(c.property) > 0 { + setWindowsCertificatePurposes(t, certContext, c.property) + } + + allowed, err := certAllowsServerAuth(certContext) + require.NoError(t, err) + assert.Equal(t, c.allowed, allowed) + }) + } +} + +// createTestCertContext creates a context for a self-signed CA, which is given +// an EKU extension only when usages are passed. +func createTestCertContext(t *testing.T, usages []x509.ExtKeyUsage) *windows.CertContext { + t.Helper() + + key, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + template := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "Upsun CLI test CA (safe to delete)"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(24 * time.Hour), + IsCA: true, + KeyUsage: x509.KeyUsageCertSign, + ExtKeyUsage: usages, + BasicConstraintsValid: true, + } + der, err := x509.CreateCertificate(rand.Reader, template, template, &key.PublicKey, key) + require.NoError(t, err) + certContext, err := windows.CertCreateCertificateContext( windows.X509_ASN_ENCODING, - &ca.certDER[0], - uint32(len(ca.certDER)), + &der[0], + uint32(len(der)), ) require.NoError(t, err) t.Cleanup(func() { assert.NoError(t, windows.CertFreeCertificateContext(certContext)) }) - - allowed, err := certAllowsServerAuth(certContext) - require.NoError(t, err) - assert.True(t, allowed, "a certificate without an EKU restriction is valid for every purpose") - - addWindowsCertificatePurpose(t, certContext, "1.3.6.1.5.5.7.3.3") // Code Signing. - allowed, err = certAllowsServerAuth(certContext) - require.NoError(t, err) - assert.False(t, allowed, "a certificate restricted to code signing must not become a TLS root") - - addWindowsCertificatePurpose(t, certContext, "1.3.6.1.5.5.7.3.1") // Server Authentication. - allowed, err = certAllowsServerAuth(certContext) - require.NoError(t, err) - assert.True(t, allowed, "a certificate which permits server authentication is a TLS root") + return certContext } -func addWindowsCertificatePurpose(t *testing.T, certContext *windows.CertContext, identifier string) { +// setWindowsCertificatePurposes replaces a certificate's EKU property, which is +// what restricting a root's purposes in the certificate manager writes, and +// which exporting the certificate cannot preserve. +func setWindowsCertificatePurposes(t *testing.T, certContext *windows.CertContext, identifiers []string) { t.Helper() - oid, err := windows.BytePtrFromString(identifier) - require.NoError(t, err) - result, _, callErr := procCertAddEnhancedKeyUsageIdentifier.Call( + oids := make([]*byte, len(identifiers)) + for i, identifier := range identifiers { + oid, err := windows.BytePtrFromString(identifier) + require.NoError(t, err) + oids[i] = oid + } + usage := windows.CertEnhKeyUsage{ + Length: uint32(len(oids)), + UsageIdentifiers: &oids[0], + } + result, _, callErr := procCertSetEnhancedKeyUsage.Call( uintptr(unsafe.Pointer(certContext)), - uintptr(unsafe.Pointer(oid)), + uintptr(unsafe.Pointer(&usage)), ) - require.NotZero(t, result, "adding certificate purpose failed: %s", callErr) + require.NotZero(t, result, "setting certificate purposes failed: %s", callErr) } // BenchmarkWindowsCABundle measures reading the store and building the bundle, From c2c18b50f74c0403336d54b634357e8ee4b538fa Mon Sep 17 00:00:00 2001 From: Bojan Zivanovic Date: Mon, 10 Aug 2026 15:51:35 +0200 Subject: [PATCH 5/5] fix(legacy): skip only the root whose purposes cannot be read Reading the purposes Windows reports can fail for one certificate in the store, and that error stopped the whole enumeration. The bundle then held nothing but the shipped certificates, so a single unreadable root cost the machine every root its organization had added, leaving the CLI unable to reach a server behind TLS inspection. Such a certificate is now left out on its own, and its fingerprint and the reason are reported, so the bundle keeps every other root in the store. The bundle is now assembled whether or not all of the store could be read, which is what lets a partial failure keep the roots it did read. Co-Authored-By: Claude Opus 5 (1M context) --- internal/legacy/ca_bundle_windows.go | 61 +++++++++++++++-------- internal/legacy/ca_bundle_windows_test.go | 27 ++++++++-- internal/legacy/php_manager_windows.go | 6 +-- 3 files changed, 65 insertions(+), 29 deletions(-) diff --git a/internal/legacy/ca_bundle_windows.go b/internal/legacy/ca_bundle_windows.go index def1948b..215b7298 100644 --- a/internal/legacy/ca_bundle_windows.go +++ b/internal/legacy/ca_bundle_windows.go @@ -16,9 +16,10 @@ import ( // caBundle returns the certificates the legacy CLI should trust: those shipped // with it, plus the trusted roots from the Windows certificate store. // -// If the store cannot be read it returns the shipped certificates and the -// reason: they are what the CLI trusted before, so they are still usable, and -// the caller decides what to do about the rest. +// It always returns a usable bundle. If any of the store could not be read it +// also returns the reason, having left out only what it could not read: the +// shipped certificates are what the CLI trusted before, so they are still +// usable, and the caller decides what to do about the rest. // // An organization which inspects TLS traffic installs its own root certificate // in that store, and every other program on the machine then trusts it. The @@ -30,18 +31,16 @@ import ( // larger than 1 MiB, which this bundle can exceed. func caBundle() ([]byte, error) { roots, err := systemRootsPEM() - if err != nil { - return caCert, err - } bundle := make([]byte, 0, len(caCert)+len(roots)+1) bundle = append(bundle, bytes.TrimRight(caCert, "\n")...) bundle = append(bundle, '\n') - return append(bundle, roots...), nil + return append(bundle, roots...), err } // systemRootsPEM returns the trusted roots from the Windows certificate store -// which the shipped certificates do not already cover. +// which the shipped certificates do not already cover, together with the +// reasons for anything it could not read. // // The ROOT store merges the machine's roots with the current user's, which is // what the operating system, and so every other program on it, trusts. That @@ -53,32 +52,50 @@ func systemRootsPEM() ([]byte, error) { } var out bytes.Buffer + var skipped []error err = eachStoreCert("ROOT", func(certContext *windows.CertContext, der []byte) error { - if shipped[sha256.Sum256(der)] { - return nil - } - allowed, err := certAllowsServerAuth(certContext) - if err != nil { - return err - } - if !allowed { + fingerprint := sha256.Sum256(der) + if shipped[fingerprint] { return nil } - cert, err := x509.ParseCertificate(der) + usable, err := usableTLSRoot(certContext, der) if err != nil { - // A store can hold a certificate Go cannot read, and one which - // cannot be parsed would make the whole file unusable. + // One certificate Windows will not report on must not cost the + // trust in every other root, so only it is left out. + skipped = append(skipped, fmt.Errorf("skipped the certificate %x: %w", fingerprint[:8], err)) return nil } - if now := time.Now(); now.Before(cert.NotBefore) || now.After(cert.NotAfter) { + if !usable { return nil } return pem.Encode(&out, &pem.Block{Type: "CERTIFICATE", Bytes: der}) }) if err != nil { - return nil, err + skipped = append(skipped, err) + } + return out.Bytes(), errors.Join(skipped...) +} + +// usableTLSRoot reports whether a certificate from the store belongs in the +// bundle: one Windows allows to authenticate a TLS server, which is currently +// valid, and which Go can read. It returns an error for a certificate it cannot +// judge, which the caller has to leave out rather than trust. +func usableTLSRoot(certContext *windows.CertContext, der []byte) (bool, error) { + allowed, err := certAllowsServerAuth(certContext) + if err != nil { + return false, err + } + if !allowed { + return false, nil + } + cert, err := x509.ParseCertificate(der) + if err != nil { + // A store can hold a certificate Go cannot read, and one which cannot + // be parsed would make the whole file unusable. + return false, nil } - return out.Bytes(), nil + now := time.Now() + return !now.Before(cert.NotBefore) && !now.After(cert.NotAfter), nil } // certFingerprints reads a PEM bundle and returns a fingerprint per certificate. diff --git a/internal/legacy/ca_bundle_windows_test.go b/internal/legacy/ca_bundle_windows_test.go index f2541749..4f2c1fd6 100644 --- a/internal/legacy/ca_bundle_windows_test.go +++ b/internal/legacy/ca_bundle_windows_test.go @@ -5,6 +5,7 @@ import ( "crypto/rsa" "crypto/x509" "crypto/x509/pkix" + "encoding/asn1" "encoding/pem" "math/big" "testing" @@ -115,7 +116,7 @@ func TestWindowsCertificatePurposes(t *testing.T) { } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - certContext := createTestCertContext(t, c.extension) + _, certContext := createTestCertContext(t, c.extension) if len(c.property) > 0 { setWindowsCertificatePurposes(t, certContext, c.property) } @@ -127,9 +128,26 @@ func TestWindowsCertificatePurposes(t *testing.T) { } } +func TestWindowsUsableTLSRoot(t *testing.T) { + der, certContext := createTestCertContext(t, nil) + usable, err := usableTLSRoot(certContext, der) + require.NoError(t, err) + assert.True(t, usable, "expected a valid certificate with no restriction to be a usable root") + + // An EKU extension which cannot be decoded. Windows tolerates it, reporting + // the purposes it could read rather than an error, so what keeps such a + // certificate out of the bundle is Go refusing to parse it. The reason is + // not asserted, only that it is never trusted. + undecodable := pkix.Extension{Id: asn1.ObjectIdentifier{2, 5, 29, 37}, Value: []byte{0xff, 0xff}} + der, certContext = createTestCertContext(t, nil, undecodable) + usable, err = usableTLSRoot(certContext, der) + t.Logf("an undecodable EKU extension: usable=%t, err=%v", usable, err) + assert.False(t, usable, "expected a certificate with an undecodable EKU extension not to be trusted") +} + // createTestCertContext creates a context for a self-signed CA, which is given -// an EKU extension only when usages are passed. -func createTestCertContext(t *testing.T, usages []x509.ExtKeyUsage) *windows.CertContext { +// an EKU extension only when usages are passed, and returns its encoding too. +func createTestCertContext(t *testing.T, usages []x509.ExtKeyUsage, extra ...pkix.Extension) ([]byte, *windows.CertContext) { t.Helper() key, err := rsa.GenerateKey(rand.Reader, 2048) @@ -142,6 +160,7 @@ func createTestCertContext(t *testing.T, usages []x509.ExtKeyUsage) *windows.Cer IsCA: true, KeyUsage: x509.KeyUsageCertSign, ExtKeyUsage: usages, + ExtraExtensions: extra, BasicConstraintsValid: true, } der, err := x509.CreateCertificate(rand.Reader, template, template, &key.PublicKey, key) @@ -156,7 +175,7 @@ func createTestCertContext(t *testing.T, usages []x509.ExtKeyUsage) *windows.Cer t.Cleanup(func() { assert.NoError(t, windows.CertFreeCertificateContext(certContext)) }) - return certContext + return der, certContext } // setWindowsCertificatePurposes replaces a certificate's EKU property, which is diff --git a/internal/legacy/php_manager_windows.go b/internal/legacy/php_manager_windows.go index 89e85a62..4108159f 100644 --- a/internal/legacy/php_manager_windows.go +++ b/internal/legacy/php_manager_windows.go @@ -30,9 +30,9 @@ func (m *phpManagerPerOS) binPath() string { func (m *phpManagerPerOS) writeCAFile() error { bundle, err := caBundle() if err != nil { - // The shipped certificates are still written, so everything except an - // organization's own certificates keeps working. That is what the CLI - // trusted before it read the store, and better than running nothing. + // Whatever could be read is still written, and the shipped certificates + // always are, so the CLI keeps at least the trust it had before it read + // the store. That is better than running nothing. m.copyWarnings = append(m.copyWarnings, err.Error()) } return file.WriteIfChanged(m.caFilePath(), bundle, 0o644)