From 32fbb7b53167eb05064cee273797a4d1dacfc989 Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Thu, 30 Jul 2026 00:59:26 +0100 Subject: [PATCH 1/2] feat(legacy): add the Windows certificate store to the CA bundle The embedded PHP has to be given a CA file, because its openssl extension cannot read the Windows certificate store, and setting one stops curl reading the store as well. So the CLI trusted only the certificates shipped with it, and failed wherever an organization installs its own root certificate, which is what issue #110 reported. The Go layer already trusts the store, so the two halves of the CLI disagreed. The bundle written to the cache directory now holds the shipped certificates and the store's trusted roots. Written by Claude Code. --- .github/workflows/ci.yml | 5 ++ internal/legacy/ca_bundle_windows.go | 58 +++++++++++++++++++ internal/legacy/ca_bundle_windows_test.go | 67 ++++++++++++++++++++++ internal/legacy/cert_store_windows_test.go | 34 +++++------ internal/legacy/php_manager_windows.go | 19 +++++- 5 files changed, 159 insertions(+), 24 deletions(-) create mode 100644 internal/legacy/ca_bundle_windows.go create mode 100644 internal/legacy/ca_bundle_windows_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1146170d..193c89bf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -169,3 +169,8 @@ jobs: env: CLI_TEST_MODIFY_CERT_STORE: '1' run: GOEXPERIMENT=jsonv2 go test -v -count=1 -timeout 3m -run TestWindows ./internal/legacy/ + + # Reading the certificate store happens before every legacy command. + - name: Measure building the CA bundle + shell: bash + run: GOEXPERIMENT=jsonv2 go test -run '^$' -bench BenchmarkWindows -benchmem ./internal/legacy/ diff --git a/internal/legacy/ca_bundle_windows.go b/internal/legacy/ca_bundle_windows.go new file mode 100644 index 00000000..b3bb6edf --- /dev/null +++ b/internal/legacy/ca_bundle_windows.go @@ -0,0 +1,58 @@ +package legacy + +import ( + "bytes" + "encoding/pem" + "errors" + "fmt" + "unsafe" + + "golang.org/x/sys/windows" +) + +// caBundle returns the certificates the legacy CLI should trust. +// +// The shipped certificates alone are not enough: an organization which inspects +// TLS traffic installs its own root certificate in the Windows store, and every +// other program on the machine then trusts it. curl in the embedded PHP could +// read that store, but only while no CA file is set, and a CA file has to be +// set because the openssl extension cannot read the store at all. So the roots +// from the store are added to the shipped ones. +func caBundle() ([]byte, error) { + roots, err := systemRootsPEM() + if err != nil { + return nil, 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 +} + +// systemRootsPEM returns the trusted root certificates from the Windows store, +// as read by every program which verifies against it, including Go itself. +func systemRootsPEM() ([]byte, error) { + store, err := windows.CertOpenSystemStore(0, windows.StringToUTF16Ptr("ROOT")) + if err != nil { + return nil, fmt.Errorf("could not open the certificate store: %w", err) + } + defer windows.CertCloseStore(store, 0) //nolint:errcheck + + var out bytes.Buffer + var certContext *windows.CertContext + for { + certContext, err = windows.CertEnumCertificatesInStore(store, certContext) + if certContext == nil { + if err != nil && !errors.Is(err, windows.Errno(windows.CRYPT_E_NOT_FOUND)) { + return nil, fmt.Errorf("could not read the certificate store: %w", err) + } + return out.Bytes(), nil + } + // The context belongs to the store, so the bytes are copied out. + encoded := unsafe.Slice(certContext.EncodedCert, certContext.Length) + if err := pem.Encode(&out, &pem.Block{Type: "CERTIFICATE", Bytes: encoded}); err != nil { + return nil, err + } + } +} diff --git a/internal/legacy/ca_bundle_windows_test.go b/internal/legacy/ca_bundle_windows_test.go new file mode 100644 index 00000000..6d1563a6 --- /dev/null +++ b/internal/legacy/ca_bundle_windows_test.go @@ -0,0 +1,67 @@ +package legacy + +import ( + "crypto/x509" + "encoding/pem" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestWindowsCABundle(t *testing.T) { + bundle, err := caBundle() + require.NoError(t, err) + + shipped := countCertificates(t, caCert) + total := countCertificates(t, bundle) + t.Logf("%d certificates in %d KB, of which %d are shipped", total, len(bundle)/1024, shipped) + + // Every shipped certificate is still there, and the store adds more. + assert.Greater(t, total, shipped, "expected the store to add certificates") + require.True(t, x509.NewCertPool().AppendCertsFromPEM(bundle), + "expected the bundle to be readable as a pool of trusted roots") +} + +// countCertificates parses a bundle and returns how many certificates it holds. +func countCertificates(t *testing.T, bundle []byte) int { + t.Helper() + + var count int + for rest := bundle; len(rest) > 0; { + var block *pem.Block + block, rest = pem.Decode(rest) + if block == nil { + require.Empty(t, rest, "expected the whole bundle to be readable") + break + } + require.Equal(t, "CERTIFICATE", block.Type) + _, err := x509.ParseCertificate(block.Bytes) + require.NoError(t, err) + count++ + } + return count +} + +// BenchmarkWindowsSystemRootsPEM measures reading the store, which happens +// before every legacy command, to decide whether the result needs caching. +func BenchmarkWindowsSystemRootsPEM(b *testing.B) { + for b.Loop() { + if _, err := systemRootsPEM(); err != nil { + b.Fatal(err) + } + } +} + +// BenchmarkWindowsCAFile measures the whole of what a command pays: reading the +// store, building the bundle, and finding the cached file already up to date. +func BenchmarkWindowsCAFile(b *testing.B) { + manager := &phpManagerPerOS{b.TempDir()} + require.NoError(b, manager.writeCAFile()) + + for b.Loop() { + if err := manager.writeCAFile(); err != nil { + b.Fatal(err) + } + } +} diff --git a/internal/legacy/cert_store_windows_test.go b/internal/legacy/cert_store_windows_test.go index 499b9660..e6e9c981 100644 --- a/internal/legacy/cert_store_windows_test.go +++ b/internal/legacy/cert_store_windows_test.go @@ -71,23 +71,16 @@ func TestWindowsCertStoreTrust(t *testing.T) { installCAInRootStore(t, ca.certDER) - // Lay out the PHP binary and its bundled CA file the way the CLI does. + // Lay out the PHP binary and its CA file the way the CLI does. The CA file + // is written after the certificate is installed, so it includes it. cacheDir := t.TempDir() manager := newPHPManager(cacheDir) require.NoError(t, manager.copy()) phpBin := manager.binPath() - bundledCAFile := filepath.Join(cacheDir, "cacert.pem") - require.FileExists(t, bundledCAFile) - - // A bundle holding both the shipped certificates and the one from the - // store: what the wrapper would have to generate to keep organization - // certificates working. - mergedCAFile := filepath.Join(cacheDir, "merged.pem") - bundled, err := os.ReadFile(bundledCAFile) - require.NoError(t, err) - merged := append(bytes.TrimRight(bundled, "\n"), '\n') - merged = append(merged, ca.certPEM...) - require.NoError(t, os.WriteFile(mergedCAFile, merged, 0o600)) + + // The certificates shipped with the CLI, without those from the store. + shippedCAFile := filepath.Join(cacheDir, "shipped.pem") + require.NoError(t, os.WriteFile(shippedCAFile, caCert, 0o600)) cases := []struct { name string @@ -100,17 +93,16 @@ func TestWindowsCertStoreTrust(t *testing.T) { wantTrust: true, }, { - // The wrapper passes this setting today, which is why an - // organization's certificate is not seen. Expect this - // expectation to change along with that. - name: "a CA file is used instead of the store", + name: "the settings the wrapper passes", args: settingArgs(manager.settings()), - wantTrust: false, + wantTrust: true, }, { - name: "a CA file which includes the store's certificate", - args: []string{"-d", iniSetting("openssl.cafile", mergedCAFile)}, - wantTrust: true, + // Setting a CA file stops curl reading the store, so this is what + // the wrapper used to do, and what issue #110 reported. + name: "only the shipped certificates", + args: []string{"-d", iniSetting("openssl.cafile", shippedCAFile)}, + wantTrust: false, }, } diff --git a/internal/legacy/php_manager_windows.go b/internal/legacy/php_manager_windows.go index e1afb8df..4ade4d72 100644 --- a/internal/legacy/php_manager_windows.go +++ b/internal/legacy/php_manager_windows.go @@ -18,17 +18,30 @@ func (m *phpManagerPerOS) copy() error { if err := file.WriteIfNeeded(m.binPath(), phpCLI, 0o755); err != nil { return err } - // Write cacert.pem for OpenSSL CA bundle (Windows needs this explicitly). - return file.WriteIfNeeded(filepath.Join(m.cacheDir, "cacert.pem"), caCert, 0o644) + return m.writeCAFile() } func (m *phpManagerPerOS) binPath() string { return filepath.Join(m.cacheDir, "php.exe") } +// writeCAFile writes the CA bundle, which Windows needs to be given +// explicitly, and which depends on the machine's certificate store. +func (m *phpManagerPerOS) writeCAFile() error { + bundle, err := caBundle() + if err != nil { + return err + } + return file.WriteIfNeeded(m.caFilePath(), bundle, 0o644) +} + +func (m *phpManagerPerOS) caFilePath() string { + return filepath.Join(m.cacheDir, "cacert.pem") +} + func (m *phpManagerPerOS) settings() []string { return []string{ - iniSetting("openssl.cafile", filepath.Join(m.cacheDir, "cacert.pem")), + iniSetting("openssl.cafile", m.caFilePath()), } } From 9cb3459f5d98776f976d4f344e6a3adcd43bfeae Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Thu, 30 Jul 2026 01:11:17 +0100 Subject: [PATCH 2/2] feat(legacy): add only the locally installed roots, not the whole store A Windows runner has 382 certificates in its root store, so the whole store plus the shipped certificates came to more than the 1 MiB curl will read from a CA file (MAX_CAFILE_SIZE in its schannel_verify.c), and every request failed with cURL error 77 instead. One certificate in the store could not be parsed by Go either. Windows keeps the certificates from Microsoft's root program in the AuthRoot store as well as in the root store, so leaving those out leaves the roots the machine was told to trust locally, which is what an organization installs and what the shipped certificates cannot cover. That cannot lose any trust the CLI had before, because before it trusted only the certificates shipped with it. Expired and unreadable certificates are skipped, and the shipped certificates alone are used if the result would still be too large. Written by Claude Code. --- .github/workflows/ci.yml | 1 + internal/legacy/ca_bundle_windows.go | 128 +++++++++++++++++---- internal/legacy/ca_bundle_windows_test.go | 55 +++++---- internal/legacy/cert_store_windows_test.go | 28 ++--- 4 files changed, 150 insertions(+), 62 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 193c89bf..e1d4daf5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -172,5 +172,6 @@ jobs: # Reading the certificate store happens before every legacy command. - name: Measure building the CA bundle + if: always() shell: bash run: GOEXPERIMENT=jsonv2 go test -run '^$' -bench BenchmarkWindows -benchmem ./internal/legacy/ diff --git a/internal/legacy/ca_bundle_windows.go b/internal/legacy/ca_bundle_windows.go index b3bb6edf..8043a547 100644 --- a/internal/legacy/ca_bundle_windows.go +++ b/internal/legacy/ca_bundle_windows.go @@ -2,57 +2,141 @@ package legacy import ( "bytes" + "crypto/sha256" + "crypto/x509" "encoding/pem" "errors" "fmt" + "time" "unsafe" "golang.org/x/sys/windows" ) -// caBundle returns the certificates the legacy CLI should trust. +// maxCAFileSize is the largest CA file curl will read on Windows, from +// MAX_CAFILE_SIZE in its schannel_verify.c. A larger file fails every request. +const maxCAFileSize = 1024 * 1024 + +// caBundle returns the certificates the legacy CLI should trust: those shipped +// with it, plus the roots installed locally on this machine. +// +// An organization which inspects TLS traffic installs its own root certificate +// in the Windows store, and every other program on the machine then trusts it. +// curl in the embedded PHP could read that store, but only while no CA file is +// set, and one has to be set because the openssl extension cannot read the +// store at all. // -// The shipped certificates alone are not enough: an organization which inspects -// TLS traffic installs its own root certificate in the Windows store, and every -// other program on the machine then trusts it. curl in the embedded PHP could -// read that store, but only while no CA file is set, and a CA file has to be -// set because the openssl extension cannot read the store at all. So the roots -// from the store are added to the shipped ones. +// Only the locally installed roots are added, not the whole store: Windows also +// keeps the certificates it gets from Microsoft's root program there, which are +// numerous enough to exceed the size curl will read. Leaving those out cannot +// lose any trust the CLI had before, because before it trusted only the +// certificates shipped with it. func caBundle() ([]byte, error) { - roots, err := systemRootsPEM() + extra, err := localRootsPEM() if err != nil { return nil, err } - bundle := make([]byte, 0, len(caCert)+len(roots)+1) + bundle := make([]byte, 0, len(caCert)+len(extra)+1) bundle = append(bundle, bytes.TrimRight(caCert, "\n")...) bundle = append(bundle, '\n') - return append(bundle, roots...), nil + bundle = append(bundle, extra...) + + if len(bundle) > maxCAFileSize { + // curl would refuse the file, so no request would work at all. The + // shipped certificates alone are what the CLI trusted before. + return caCert, nil + } + return bundle, nil } -// systemRootsPEM returns the trusted root certificates from the Windows store, -// as read by every program which verifies against it, including Go itself. -func systemRootsPEM() ([]byte, error) { - store, err := windows.CertOpenSystemStore(0, windows.StringToUTF16Ptr("ROOT")) +// localRootsPEM returns the roots this machine was told to trust, meaning those +// in the store which are neither shipped with the CLI nor part of Microsoft's +// root program. +func localRootsPEM() ([]byte, error) { + shipped, err := certFingerprints(caCert) if err != nil { - return nil, fmt.Errorf("could not open the certificate store: %w", err) + return nil, err + } + // AuthRoot holds Microsoft's root program, which the shipped certificates + // cover for the purposes of the CLI. + publicRoots, err := storeFingerprints("AuthRoot") + if err != nil { + return nil, err } - defer windows.CertCloseStore(store, 0) //nolint:errcheck var out bytes.Buffer + err = eachStoreCert("ROOT", func(der []byte) error { + fingerprint := sha256.Sum256(der) + if shipped[fingerprint] || publicRoots[fingerprint] { + return nil + } + cert, err := x509.ParseCertificate(der) + if err != nil { + return nil // Not a certificate this build of Go can read. + } + if now := time.Now(); now.Before(cert.NotBefore) || now.After(cert.NotAfter) { + return nil + } + return pem.Encode(&out, &pem.Block{Type: "CERTIFICATE", Bytes: der}) + }) + if err != nil { + return nil, err + } + return out.Bytes(), nil +} + +// certFingerprints reads a PEM bundle and returns a fingerprint per certificate. +func certFingerprints(bundle []byte) (map[[32]byte]bool, error) { + fingerprints := make(map[[32]byte]bool) + for rest := bundle; len(rest) > 0; { + var block *pem.Block + block, rest = pem.Decode(rest) + if block == nil { + break + } + if block.Type == "CERTIFICATE" { + fingerprints[sha256.Sum256(block.Bytes)] = true + } + } + if len(fingerprints) == 0 { + return nil, errors.New("no certificates found in the shipped CA bundle") + } + return fingerprints, nil +} + +// storeFingerprints returns a fingerprint per certificate in a Windows store. +func storeFingerprints(name string) (map[[32]byte]bool, error) { + fingerprints := make(map[[32]byte]bool) + err := eachStoreCert(name, func(der []byte) error { + fingerprints[sha256.Sum256(der)] = true + return nil + }) + return fingerprints, err +} + +// 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 { + store, err := windows.CertOpenSystemStore(0, windows.StringToUTF16Ptr(name)) + if err != nil { + return fmt.Errorf("could not open the %s certificate store: %w", name, err) + } + defer windows.CertCloseStore(store, 0) //nolint:errcheck + var certContext *windows.CertContext for { certContext, err = windows.CertEnumCertificatesInStore(store, certContext) if certContext == nil { if err != nil && !errors.Is(err, windows.Errno(windows.CRYPT_E_NOT_FOUND)) { - return nil, fmt.Errorf("could not read the certificate store: %w", err) + return fmt.Errorf("could not read the %s certificate store: %w", name, err) } - return out.Bytes(), nil + return nil } - // The context belongs to the store, so the bytes are copied out. - encoded := unsafe.Slice(certContext.EncodedCert, certContext.Length) - if err := pem.Encode(&out, &pem.Block{Type: "CERTIFICATE", Bytes: encoded}); err != nil { - return nil, err + // The context belongs to the store, so the bytes are only borrowed. + if err := fn(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 6d1563a6..b07cdd41 100644 --- a/internal/legacy/ca_bundle_windows_test.go +++ b/internal/legacy/ca_bundle_windows_test.go @@ -13,21 +13,35 @@ func TestWindowsCABundle(t *testing.T) { bundle, err := caBundle() require.NoError(t, err) - shipped := countCertificates(t, caCert) - total := countCertificates(t, bundle) - t.Logf("%d certificates in %d KB, of which %d are shipped", total, len(bundle)/1024, shipped) + shipped, err := certFingerprints(caCert) + require.NoError(t, err) + held, err := certFingerprints(bundle) + require.NoError(t, err) - // Every shipped certificate is still there, and the store adds more. - assert.Greater(t, total, shipped, "expected the store to add certificates") + // What the machine holds, to show how much of the store is left out, and + // why the whole store would not fit. This varies by machine, so it is + // reported rather than asserted. + root, err := storeFingerprints("ROOT") + require.NoError(t, err) + authRoot, err := storeFingerprints("AuthRoot") + require.NoError(t, err) + var inRootProgram int + for fingerprint := range root { + if authRoot[fingerprint] { + inRootProgram++ + } + } + t.Logf("bundle: %d certificates in %d KB (%d shipped, %d added)", + len(held), len(bundle)/1024, len(shipped), len(held)-len(shipped)) + t.Logf("store: %d roots, of which %d are in Microsoft's root program (AuthRoot holds %d)", + len(root), inRootProgram, len(authRoot)) + + assert.Less(t, len(bundle), maxCAFileSize, "expected curl to be willing to read the bundle") + for fingerprint := range shipped { + require.True(t, held[fingerprint], "expected every shipped certificate to still be trusted") + } require.True(t, x509.NewCertPool().AppendCertsFromPEM(bundle), "expected the bundle to be readable as a pool of trusted roots") -} - -// countCertificates parses a bundle and returns how many certificates it holds. -func countCertificates(t *testing.T, bundle []byte) int { - t.Helper() - - var count int for rest := bundle; len(rest) > 0; { var block *pem.Block block, rest = pem.Decode(rest) @@ -35,26 +49,23 @@ func countCertificates(t *testing.T, bundle []byte) int { require.Empty(t, rest, "expected the whole bundle to be readable") break } - require.Equal(t, "CERTIFICATE", block.Type) _, err := x509.ParseCertificate(block.Bytes) - require.NoError(t, err) - count++ + require.NoError(t, err, "expected every certificate in the bundle to be readable") } - return count } -// BenchmarkWindowsSystemRootsPEM measures reading the store, which happens -// before every legacy command, to decide whether the result needs caching. -func BenchmarkWindowsSystemRootsPEM(b *testing.B) { +// BenchmarkWindowsCABundle measures reading the store and building the bundle, +// which happens before every legacy command. +func BenchmarkWindowsCABundle(b *testing.B) { for b.Loop() { - if _, err := systemRootsPEM(); err != nil { + if _, err := caBundle(); err != nil { b.Fatal(err) } } } -// BenchmarkWindowsCAFile measures the whole of what a command pays: reading the -// store, building the bundle, and finding the cached file already up to date. +// BenchmarkWindowsCAFile measures the whole of what a command pays: building +// the bundle, and finding the cached file already up to date. func BenchmarkWindowsCAFile(b *testing.B) { manager := &phpManagerPerOS{b.TempDir()} require.NoError(b, manager.writeCAFile()) diff --git a/internal/legacy/cert_store_windows_test.go b/internal/legacy/cert_store_windows_test.go index e6e9c981..548083c0 100644 --- a/internal/legacy/cert_store_windows_test.go +++ b/internal/legacy/cert_store_windows_test.go @@ -18,6 +18,7 @@ import ( "context" "crypto/rand" "crypto/rsa" + "crypto/sha256" "crypto/tls" "crypto/x509" "crypto/x509/pkix" @@ -118,9 +119,15 @@ func TestWindowsCertStoreTrust(t *testing.T) { }) } - t.Run("the root store can be read from Go", func(t *testing.T) { - assert.True(t, rootStoreContains(t, ca.certDER), - "expected to find the installed certificate by enumerating the ROOT store") + t.Run("the installed certificate is one of the locally trusted roots", func(t *testing.T) { + // It is not in Microsoft's root program, so it must be picked up. + local, err := localRootsPEM() + require.NoError(t, err) + fingerprints, err := certFingerprints(local) + require.NoError(t, err) + assert.True(t, fingerprints[sha256.Sum256(ca.certDER)], + "expected the installed certificate to be added to the bundle") + t.Logf("%d locally trusted roots in %d bytes", len(fingerprints), len(local)) }) } @@ -327,18 +334,3 @@ func runPHPRequest(t *testing.T, phpBin, url string, extraArgs ...string) string return strings.TrimSpace(string(output)) } - -// rootStoreContains reports whether the trusted roots hold a certificate, -// using the same API a merged CA bundle would need to read them. -func rootStoreContains(t *testing.T, certDER []byte) bool { - t.Helper() - - store := openRootStore(t) - defer func() { - assert.NoError(t, windows.CertCloseStore(store, 0)) - }() - - return eachCertInStore(t, store, func(_ *windows.CertContext, encoded []byte) bool { - return bytes.Equal(encoded, certDER) - }) -}