From 602354b9382760ad9d3ddfd65334a3a8e4ab68fe Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Wed, 29 Jul 2026 22:36:47 +0100 Subject: [PATCH 1/6] test(legacy): spike how Windows certificate trust works for embedded PHP Adds a Windows-only test and a CI job to answer, on a real Windows machine, what curl in the embedded PHP does with the Windows certificate store. That decides how the CLI should configure its CA bundle for people whose organization inspects TLS traffic and installs an extra root certificate. The test installs a throwaway CA in the current user's root store, then serves HTTPS with a certificate signed by it and makes three requests from the embedded PHP: - with no CA file, expecting the store to be used - with the bundled CA file the wrapper pins today, expecting the store to be ignored - with a CA file that also holds the store's certificate, expecting trust to be restored It also reads the store through the syscall package, to check that generating such a bundle is possible without a new dependency. Because it modifies the user's certificate store, it only runs when CLI_TEST_MODIFY_CERT_STORE is set, and it removes the certificate again afterwards. No behavior is changed. The purpose is evidence: the Schannel behavior described above is currently only inferred from curl's source. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 30 +++ internal/legacy/cert_store_windows_test.go | 252 +++++++++++++++++++++ 2 files changed, 282 insertions(+) create mode 100644 internal/legacy/cert_store_windows_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ca013aaf..100414bb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -139,3 +139,33 @@ jobs: - name: Run integration tests run: make integration-test + + # Spike: check how the embedded PHP treats the Windows certificate store. + windows-cert-store: + runs-on: windows-latest + + steps: + - name: Check out repository code + uses: actions/checkout@v7 + + - name: Setup Go + uses: actions/setup-go@v7 + with: + go-version-file: ./go.mod + + - name: Download embedded PHP files + shell: bash + run: | + php_version=$(grep -m1 '^PHP_VERSION' Makefile | cut -d' ' -f3) + mkdir -p internal/legacy/archives + curl -fSL "https://github.com/upsun/cli-php-builds/releases/download/php-$php_version/php-$php_version-windows-amd64.exe" \ + -o internal/legacy/archives/php_windows.exe + curl -fSL https://curl.se/ca/cacert.pem -o internal/legacy/archives/cacert.pem + # Only needed so that the package builds. + touch internal/legacy/archives/platform.phar + + - name: Run certificate store tests + shell: bash + env: + CLI_TEST_MODIFY_CERT_STORE: '1' + run: GOEXPERIMENT=jsonv2 go test -v -count=1 -run TestWindowsCertStoreTrust ./internal/legacy/ diff --git a/internal/legacy/cert_store_windows_test.go b/internal/legacy/cert_store_windows_test.go new file mode 100644 index 00000000..f6bade8e --- /dev/null +++ b/internal/legacy/cert_store_windows_test.go @@ -0,0 +1,252 @@ +package legacy + +// This file answers two questions about certificate trust on Windows, which +// decide how the CLI should configure its CA bundle there: +// +// 1. Does a CA bundle stop the embedded PHP from using the Windows +// certificate store? The store is where an organization installs the extra +// root certificate needed when it inspects TLS traffic. curl in the +// embedded PHP is built against Schannel, and its source suggests that +// passing any CA file makes it verify against that file alone. +// +// 2. Can a CA bundle which includes the store's certificates restore trust, +// and can the store be read from Go in the first place? +// +// The test installs a throwaway CA in the current user's root store, so it +// only runs when CLI_TEST_MODIFY_CERT_STORE is set. It cleans up afterwards. + +import ( + "bytes" + "context" + "crypto/rand" + "crypto/rsa" + "crypto/sha1" //nolint:gosec // Windows identifies certificates by SHA-1 thumbprint. + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "encoding/hex" + "encoding/pem" + "math/big" + "net" + "net/http" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "strings" + "syscall" + "testing" + "time" + "unsafe" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// phpRequestScript makes one HTTPS request and reports the curl result. +const phpRequestScript = `$ch = curl_init(getenv('TEST_URL')); +curl_setopt($ch, CURLOPT_RETURNTRANSFER, true); +$body = curl_exec($ch); +echo curl_errno($ch) === 0 ? 'OK:' . $body : 'ERR:' . curl_errno($ch) . ':' . curl_error($ch);` + +// curlPeerFailedVerification is CURLE_PEER_FAILED_VERIFICATION, reported as +// "cURL error 60" by the CLI. +const curlPeerFailedVerification = "ERR:60:" + +func TestWindowsCertStoreTrust(t *testing.T) { + if os.Getenv("CLI_TEST_MODIFY_CERT_STORE") != "1" { + t.Skip("set CLI_TEST_MODIFY_CERT_STORE=1 to let this test add a certificate to the user's root store") + } + + ca := generateTestCA(t) + + server := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("hello")) + })) + server.TLS = &tls.Config{Certificates: []tls.Certificate{ca.serverCert}} //nolint:gosec // test server; Go picks the minimum version. + server.StartTLS() + defer server.Close() + + installCAInUserRootStore(t, ca.certDER) + + // Lay out the PHP binary and its bundled CA file the way the CLI does. + 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) + require.NoError(t, os.WriteFile(mergedCAFile, append(bundled, ca.certPEM...), 0o600)) + + cases := []struct { + name string + args []string + wantTrust bool + }{ + { + name: "no CA file, so the Windows certificate store is used", + args: nil, + wantTrust: true, + }, + { + name: "the bundled CA file replaces the store, as the wrapper configures today", + args: []string{"-d", "openssl.cafile=" + bundledCAFile}, + wantTrust: false, + }, + { + name: "a CA file including the store's certificate restores trust", + args: []string{"-d", "openssl.cafile=" + mergedCAFile}, + wantTrust: true, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + output := runPHPRequest(t, phpBin, server.URL, c.args...) + t.Logf("php %v -> %s", c.args, output) + if c.wantTrust { + assert.Equal(t, "OK:hello", output) + } else { + assert.Contains(t, output, curlPeerFailedVerification) + } + }) + } + + t.Run("the root store can be read from Go", func(t *testing.T) { + assert.True(t, userRootStoreContains(t, ca.certDER), + "expected to find the installed certificate by enumerating the ROOT store") + }) +} + +type testCA struct { + certPEM []byte + certDER []byte + serverCert tls.Certificate +} + +// generateTestCA creates a CA and a certificate for 127.0.0.1 signed by it. +func generateTestCA(t *testing.T) testCA { + t.Helper() + + caKey, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + + notBefore := time.Now().Add(-time.Hour) + notAfter := time.Now().Add(24 * time.Hour) + + caTemplate := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "Upsun CLI test CA (safe to delete)"}, + NotBefore: notBefore, + NotAfter: notAfter, + IsCA: true, + KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageDigitalSignature, + BasicConstraintsValid: true, + } + caDER, err := x509.CreateCertificate(rand.Reader, caTemplate, caTemplate, &caKey.PublicKey, caKey) + require.NoError(t, err) + caCert, err := x509.ParseCertificate(caDER) + require.NoError(t, err) + + serverKey, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + serverTemplate := &x509.Certificate{ + SerialNumber: big.NewInt(2), + Subject: pkix.Name{CommonName: "127.0.0.1"}, + NotBefore: notBefore, + NotAfter: notAfter, + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, + } + serverDER, err := x509.CreateCertificate(rand.Reader, serverTemplate, caCert, &serverKey.PublicKey, caKey) + require.NoError(t, err) + + return testCA{ + certPEM: pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: caDER}), + certDER: caDER, + serverCert: tls.Certificate{ + Certificate: [][]byte{serverDER, caDER}, + PrivateKey: serverKey, + }, + } +} + +// installCAInUserRootStore trusts a certificate for the current user only, +// which needs no elevation, and removes it when the test ends. +func installCAInUserRootStore(t *testing.T, certDER []byte) { + t.Helper() + + certFile := filepath.Join(t.TempDir(), "test-ca.cer") + require.NoError(t, os.WriteFile(certFile, certDER, 0o600)) + runCertutil(t, "-user", "-f", "-addstore", "Root", certFile) + + sum := sha1.Sum(certDER) //nolint:gosec // Windows identifies certificates by SHA-1 thumbprint. + thumbprint := strings.ToUpper(hex.EncodeToString(sum[:])) + t.Cleanup(func() { + runCertutil(t, "-user", "-f", "-delstore", "Root", thumbprint) + }) +} + +func runCertutil(t *testing.T, args ...string) { + t.Helper() + + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + output, err := exec.CommandContext(ctx, "certutil", args...).CombinedOutput() + require.NoError(t, err, "certutil %v failed: %s", args, output) + t.Logf("certutil %v -> %s", args, strings.TrimSpace(string(output))) +} + +func runPHPRequest(t *testing.T, phpBin, url string, extraArgs ...string) string { + t.Helper() + + args := append([]string{"-n"}, extraArgs...) + args = append(args, "-r", phpRequestScript) + + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + cmd := exec.CommandContext(ctx, phpBin, args...) + cmd.Env = append(os.Environ(), "TEST_URL="+url) + output, err := cmd.CombinedOutput() + require.NoError(t, err, "PHP exited with an error: %s", output) + + return strings.TrimSpace(string(output)) +} + +// userRootStoreContains reports whether the ROOT store holds a certificate, +// using the same API a merged CA bundle would need to read it. +func userRootStoreContains(t *testing.T, certDER []byte) bool { + t.Helper() + + name, err := syscall.UTF16PtrFromString("ROOT") + require.NoError(t, err) + store, err := syscall.CertOpenSystemStore(0, name) + require.NoError(t, err) + defer func() { + assert.NoError(t, syscall.CertCloseStore(store, 0)) + }() + + var count int + var context *syscall.CertContext + for { + context, err = syscall.CertEnumCertificatesInStore(store, context) + if err != nil || context == nil { + t.Logf("read %d certificates from the ROOT store", count) + return false + } + count++ + encoded := unsafe.Slice(context.EncodedCert, context.Length) + if bytes.Equal(encoded, certDER) { + t.Logf("found the installed certificate after reading %d from the ROOT store", count) + return true + } + } +} From c56976674a5674399e351a114a8e8876e682b27d Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Thu, 30 Jul 2026 00:00:01 +0100 Subject: [PATCH 2/6] test(legacy): add the test CA through the store API, not certutil The first CI run showed that "certutil -user -addstore Root" asks the user to confirm before trusting a certificate, so it timed out instead of running unattended. Use CertCreateCertificateContext and CertAddCertificateContextToStore from golang.org/x/sys/windows instead, which is silent, and delete the certificate the same way afterwards. This also exercises the calls a merged CA bundle would need in order to read the store. Co-Authored-By: Claude Opus 5 (1M context) --- internal/legacy/cert_store_windows_test.go | 111 ++++++++++++++------- 1 file changed, 76 insertions(+), 35 deletions(-) diff --git a/internal/legacy/cert_store_windows_test.go b/internal/legacy/cert_store_windows_test.go index f6bade8e..acfbba16 100644 --- a/internal/legacy/cert_store_windows_test.go +++ b/internal/legacy/cert_store_windows_test.go @@ -20,11 +20,9 @@ import ( "context" "crypto/rand" "crypto/rsa" - "crypto/sha1" //nolint:gosec // Windows identifies certificates by SHA-1 thumbprint. "crypto/tls" "crypto/x509" "crypto/x509/pkix" - "encoding/hex" "encoding/pem" "math/big" "net" @@ -34,13 +32,13 @@ import ( "os/exec" "path/filepath" "strings" - "syscall" "testing" "time" "unsafe" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/sys/windows" ) // phpRequestScript makes one HTTPS request and reports the curl result. @@ -181,28 +179,86 @@ func generateTestCA(t *testing.T) testCA { // installCAInUserRootStore trusts a certificate for the current user only, // which needs no elevation, and removes it when the test ends. +// +// This uses the store API rather than "certutil -addstore", which asks the +// user to confirm and so cannot run unattended. func installCAInUserRootStore(t *testing.T, certDER []byte) { t.Helper() - certFile := filepath.Join(t.TempDir(), "test-ca.cer") - require.NoError(t, os.WriteFile(certFile, certDER, 0o600)) - runCertutil(t, "-user", "-f", "-addstore", "Root", certFile) + store := openUserRootStore(t) + defer func() { + assert.NoError(t, windows.CertCloseStore(store, 0)) + }() + + certContext, err := windows.CertCreateCertificateContext(windows.X509_ASN_ENCODING, &certDER[0], uint32(len(certDER))) + require.NoError(t, err) + defer func() { + assert.NoError(t, windows.CertFreeCertificateContext(certContext)) + }() + + require.NoError(t, windows.CertAddCertificateContextToStore(store, certContext, windows.CERT_STORE_ADD_REPLACE_EXISTING, nil)) + t.Log("installed the test CA in the user's root store") - sum := sha1.Sum(certDER) //nolint:gosec // Windows identifies certificates by SHA-1 thumbprint. - thumbprint := strings.ToUpper(hex.EncodeToString(sum[:])) t.Cleanup(func() { - runCertutil(t, "-user", "-f", "-delstore", "Root", thumbprint) + removeCertFromUserRootStore(t, certDER) }) } -func runCertutil(t *testing.T, args ...string) { +// removeCertFromUserRootStore deletes a certificate installed by the test. +func removeCertFromUserRootStore(t *testing.T, certDER []byte) { t.Helper() - ctx, cancel := context.WithTimeout(context.Background(), time.Minute) - defer cancel() - output, err := exec.CommandContext(ctx, "certutil", args...).CombinedOutput() - require.NoError(t, err, "certutil %v failed: %s", args, output) - t.Logf("certutil %v -> %s", args, strings.TrimSpace(string(output))) + store := openUserRootStore(t) + defer func() { + assert.NoError(t, windows.CertCloseStore(store, 0)) + }() + + found := eachCertInStore(t, store, func(context *windows.CertContext, encoded []byte) bool { + if !bytes.Equal(encoded, certDER) { + return false + } + // CertDeleteCertificateFromStore frees the context, so the + // enumeration must stop here. + assert.NoError(t, windows.CertDeleteCertificateFromStore(context)) + return true + }) + assert.True(t, found, "expected to find the test CA in order to remove it") +} + +func openUserRootStore(t *testing.T) windows.Handle { + t.Helper() + + name, err := windows.UTF16PtrFromString("ROOT") + require.NoError(t, err) + store, err := windows.CertOpenSystemStore(0, name) + require.NoError(t, err) + + return store +} + +// eachCertInStore calls fn for each certificate until it returns true. +func eachCertInStore(t *testing.T, store windows.Handle, fn func(context *windows.CertContext, encoded []byte) bool) bool { + t.Helper() + + var count int + var context *windows.CertContext + for { + var err error + context, err = windows.CertEnumCertificatesInStore(store, context) + if context == nil { + if err != nil && err != windows.Errno(windows.CRYPT_E_NOT_FOUND) { + t.Logf("stopped reading the store: %s", err) + } + t.Logf("read %d certificates from the ROOT store", count) + return false + } + count++ + encoded := unsafe.Slice(context.EncodedCert, context.Length) + if fn(context, encoded) { + t.Logf("matched a certificate after reading %d from the ROOT store", count) + return true + } + } } func runPHPRequest(t *testing.T, phpBin, url string, extraArgs ...string) string { @@ -226,27 +282,12 @@ func runPHPRequest(t *testing.T, phpBin, url string, extraArgs ...string) string func userRootStoreContains(t *testing.T, certDER []byte) bool { t.Helper() - name, err := syscall.UTF16PtrFromString("ROOT") - require.NoError(t, err) - store, err := syscall.CertOpenSystemStore(0, name) - require.NoError(t, err) + store := openUserRootStore(t) defer func() { - assert.NoError(t, syscall.CertCloseStore(store, 0)) + assert.NoError(t, windows.CertCloseStore(store, 0)) }() - var count int - var context *syscall.CertContext - for { - context, err = syscall.CertEnumCertificatesInStore(store, context) - if err != nil || context == nil { - t.Logf("read %d certificates from the ROOT store", count) - return false - } - count++ - encoded := unsafe.Slice(context.EncodedCert, context.Length) - if bytes.Equal(encoded, certDER) { - t.Logf("found the installed certificate after reading %d from the ROOT store", count) - return true - } - } + return eachCertInStore(t, store, func(_ *windows.CertContext, encoded []byte) bool { + return bytes.Equal(encoded, certDER) + }) } From f4cc592304fbbee8c6a7d2f24deed15300750fc0 Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Thu, 30 Jul 2026 00:20:36 +0100 Subject: [PATCH 3/6] test(legacy): install the test CA in the machine store Writing to the current user's root store makes Windows ask the user to confirm, and the store API call simply never returns when it does: the second CI run spent its whole ten minute budget inside CertAddCertificateContextToStore. Use the machine store, which needs administrator rights (CI has them) but no confirmation. Also guard the call with a 30 second deadline and cap the test binary at three minutes, so a prompt reports what happened instead of stalling the job. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 2 +- internal/legacy/cert_store_windows_test.go | 86 ++++++++++++++++------ 2 files changed, 65 insertions(+), 23 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 100414bb..03467c0b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -168,4 +168,4 @@ jobs: shell: bash env: CLI_TEST_MODIFY_CERT_STORE: '1' - run: GOEXPERIMENT=jsonv2 go test -v -count=1 -run TestWindowsCertStoreTrust ./internal/legacy/ + run: GOEXPERIMENT=jsonv2 go test -v -count=1 -timeout 3m -run TestWindowsCertStoreTrust ./internal/legacy/ diff --git a/internal/legacy/cert_store_windows_test.go b/internal/legacy/cert_store_windows_test.go index acfbba16..42e40cb1 100644 --- a/internal/legacy/cert_store_windows_test.go +++ b/internal/legacy/cert_store_windows_test.go @@ -12,8 +12,9 @@ package legacy // 2. Can a CA bundle which includes the store's certificates restore trust, // and can the store be read from Go in the first place? // -// The test installs a throwaway CA in the current user's root store, so it -// only runs when CLI_TEST_MODIFY_CERT_STORE is set. It cleans up afterwards. +// The test installs a throwaway CA in the machine's root store, so it needs +// administrator rights and only runs when CLI_TEST_MODIFY_CERT_STORE is set. +// It removes the certificate again afterwards. import ( "bytes" @@ -53,7 +54,7 @@ const curlPeerFailedVerification = "ERR:60:" func TestWindowsCertStoreTrust(t *testing.T) { if os.Getenv("CLI_TEST_MODIFY_CERT_STORE") != "1" { - t.Skip("set CLI_TEST_MODIFY_CERT_STORE=1 to let this test add a certificate to the user's root store") + t.Skip("set CLI_TEST_MODIFY_CERT_STORE=1, and run as administrator, to let this test add a certificate to the root store") } ca := generateTestCA(t) @@ -65,7 +66,7 @@ func TestWindowsCertStoreTrust(t *testing.T) { server.StartTLS() defer server.Close() - installCAInUserRootStore(t, ca.certDER) + installCAInRootStore(t, ca.certDER) // Lay out the PHP binary and its bundled CA file the way the CLI does. cacheDir := t.TempDir() @@ -118,7 +119,7 @@ func TestWindowsCertStoreTrust(t *testing.T) { } t.Run("the root store can be read from Go", func(t *testing.T) { - assert.True(t, userRootStoreContains(t, ca.certDER), + assert.True(t, rootStoreContains(t, ca.certDER), "expected to find the installed certificate by enumerating the ROOT store") }) } @@ -177,15 +178,16 @@ func generateTestCA(t *testing.T) testCA { } } -// installCAInUserRootStore trusts a certificate for the current user only, -// which needs no elevation, and removes it when the test ends. +// installCAInRootStore trusts a certificate and removes it when the test ends. // -// This uses the store API rather than "certutil -addstore", which asks the -// user to confirm and so cannot run unattended. -func installCAInUserRootStore(t *testing.T, certDER []byte) { +// It writes to the machine store, which needs administrator rights but no +// confirmation. Adding to the current user's store instead makes Windows ask +// the user to confirm, whether that is done through certutil or through the +// store API, so it cannot run unattended. +func installCAInRootStore(t *testing.T, certDER []byte) { t.Helper() - store := openUserRootStore(t) + store := openMachineRootStore(t) defer func() { assert.NoError(t, windows.CertCloseStore(store, 0)) }() @@ -196,19 +198,23 @@ func installCAInUserRootStore(t *testing.T, certDER []byte) { assert.NoError(t, windows.CertFreeCertificateContext(certContext)) }() - require.NoError(t, windows.CertAddCertificateContextToStore(store, certContext, windows.CERT_STORE_ADD_REPLACE_EXISTING, nil)) - t.Log("installed the test CA in the user's root store") + // Guard against the confirmation prompt: if one appears the call never + // returns, and without this the whole test binary would time out. + withoutHanging(t, "adding the certificate to the machine root store", func() error { + return windows.CertAddCertificateContextToStore(store, certContext, windows.CERT_STORE_ADD_REPLACE_EXISTING, nil) + }) + t.Log("installed the test CA in the machine root store") t.Cleanup(func() { - removeCertFromUserRootStore(t, certDER) + removeCertFromRootStore(t, certDER) }) } -// removeCertFromUserRootStore deletes a certificate installed by the test. -func removeCertFromUserRootStore(t *testing.T, certDER []byte) { +// removeCertFromRootStore deletes a certificate installed by the test. +func removeCertFromRootStore(t *testing.T, certDER []byte) { t.Helper() - store := openUserRootStore(t) + store := openMachineRootStore(t) defer func() { assert.NoError(t, windows.CertCloseStore(store, 0)) }() @@ -225,7 +231,27 @@ func removeCertFromUserRootStore(t *testing.T, certDER []byte) { assert.True(t, found, "expected to find the test CA in order to remove it") } -func openUserRootStore(t *testing.T) windows.Handle { +// openMachineRootStore opens the machine's trusted root store for writing. +func openMachineRootStore(t *testing.T) windows.Handle { + t.Helper() + + name, err := windows.UTF16PtrFromString("ROOT") + require.NoError(t, err) + store, err := windows.CertOpenStore( + windows.CERT_STORE_PROV_SYSTEM, + 0, + 0, + windows.CERT_SYSTEM_STORE_LOCAL_MACHINE, + uintptr(unsafe.Pointer(name)), + ) + require.NoError(t, err, "opening the machine root store needs administrator rights") + + return store +} + +// openRootStore opens the trusted roots as a program verifying a certificate +// would see them, which includes both the machine and user stores. +func openRootStore(t *testing.T) windows.Handle { t.Helper() name, err := windows.UTF16PtrFromString("ROOT") @@ -236,6 +262,22 @@ func openUserRootStore(t *testing.T) windows.Handle { return store } +// withoutHanging fails the test if fn does not return in time. +func withoutHanging(t *testing.T, description string, fn func() error) { + t.Helper() + + done := make(chan error, 1) + go func() { + done <- fn() + }() + select { + case err := <-done: + require.NoError(t, err, description) + case <-time.After(30 * time.Second): + require.FailNow(t, "timed out "+description, "Windows may be waiting for confirmation, which cannot be given here") + } +} + // eachCertInStore calls fn for each certificate until it returns true. func eachCertInStore(t *testing.T, store windows.Handle, fn func(context *windows.CertContext, encoded []byte) bool) bool { t.Helper() @@ -277,12 +319,12 @@ func runPHPRequest(t *testing.T, phpBin, url string, extraArgs ...string) string return strings.TrimSpace(string(output)) } -// userRootStoreContains reports whether the ROOT store holds a certificate, -// using the same API a merged CA bundle would need to read it. -func userRootStoreContains(t *testing.T, certDER []byte) bool { +// 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 := openUserRootStore(t) + store := openRootStore(t) defer func() { assert.NoError(t, windows.CertCloseStore(store, 0)) }() From c991c6832c18439667101b10530ee57a73ce9bcc Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Thu, 30 Jul 2026 00:23:42 +0100 Subject: [PATCH 4/6] test(legacy): quote the ini value and skip revocation checks Two problems the third CI run exposed, both in the test rather than in the CLI: Passing the CA file as "-d openssl.cafile=C:\Users\RUNNER~1\..." made PHP report "syntax error, unexpected '~'" and use a truncated path. PHP reads ini values as expressions, in which "~" is an operator, and the runner's temporary directory is a Windows short path. Quoting the value avoids it. Note that the wrapper passes this setting unquoted too, so it would behave the same way for a user whose cache directory resolves to a short path. Without a CA file, Schannel found the test CA in the store and then failed with CRYPT_E_NO_REVOCATION_CHECK, because a throwaway CA publishes no revocation list. Set CURLSSLOPT_NO_REVOKE, since the question here is which certificates are trusted. Co-Authored-By: Claude Opus 5 (1M context) --- internal/legacy/cert_store_windows_test.go | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/internal/legacy/cert_store_windows_test.go b/internal/legacy/cert_store_windows_test.go index 42e40cb1..0deaf68b 100644 --- a/internal/legacy/cert_store_windows_test.go +++ b/internal/legacy/cert_store_windows_test.go @@ -43,8 +43,13 @@ import ( ) // phpRequestScript makes one HTTPS request and reports the curl result. +// +// Revocation checks are turned off because the test CA publishes no +// revocation list, which Schannel otherwise treats as a failure. This test is +// about which certificates are trusted, not about revocation. const phpRequestScript = `$ch = curl_init(getenv('TEST_URL')); curl_setopt($ch, CURLOPT_RETURNTRANSFER, true); +curl_setopt($ch, CURLOPT_SSL_OPTIONS, CURLSSLOPT_NO_REVOKE); $body = curl_exec($ch); echo curl_errno($ch) === 0 ? 'OK:' . $body : 'ERR:' . curl_errno($ch) . ':' . curl_error($ch);` @@ -96,12 +101,12 @@ func TestWindowsCertStoreTrust(t *testing.T) { }, { name: "the bundled CA file replaces the store, as the wrapper configures today", - args: []string{"-d", "openssl.cafile=" + bundledCAFile}, + args: []string{"-d", iniSetting("openssl.cafile", bundledCAFile)}, wantTrust: false, }, { name: "a CA file including the store's certificate restores trust", - args: []string{"-d", "openssl.cafile=" + mergedCAFile}, + args: []string{"-d", iniSetting("openssl.cafile", mergedCAFile)}, wantTrust: true, }, } @@ -303,6 +308,13 @@ func eachCertInStore(t *testing.T, store windows.Handle, fn func(context *window } } +// iniSetting quotes the value of a PHP setting. PHP reads ini values as +// expressions in which characters such as "~" are operators, and Windows short +// paths contain them, for example C:\Users\RUNNER~1\AppData. +func iniSetting(key, value string) string { + return key + `="` + value + `"` +} + func runPHPRequest(t *testing.T, phpBin, url string, extraArgs ...string) string { t.Helper() From 9eb27334c4218367e85e85860007122bdc0d1452 Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Thu, 30 Jul 2026 00:31:42 +0100 Subject: [PATCH 5/6] test(legacy): tidy the certificate store test for keeping Reframe the file comment around what the test covers rather than the questions it was written to answer, name the cases for the configuration they use rather than for the current wrapper settings, and note which expectation should change when that configuration does. Also stop shadowing the context package, use errors.Is for the end-of-enumeration check, and describe the store without assuming which one was opened. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 2 +- internal/legacy/cert_store_windows_test.go | 49 +++++++++++----------- 2 files changed, 26 insertions(+), 25 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 03467c0b..9c337f00 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -140,7 +140,7 @@ jobs: - name: Run integration tests run: make integration-test - # Spike: check how the embedded PHP treats the Windows certificate store. + # The only Windows coverage: which certificates the embedded PHP trusts. windows-cert-store: runs-on: windows-latest diff --git a/internal/legacy/cert_store_windows_test.go b/internal/legacy/cert_store_windows_test.go index 0deaf68b..af20f1cb 100644 --- a/internal/legacy/cert_store_windows_test.go +++ b/internal/legacy/cert_store_windows_test.go @@ -1,16 +1,13 @@ package legacy -// This file answers two questions about certificate trust on Windows, which -// decide how the CLI should configure its CA bundle there: +// This file covers which certificates the embedded PHP trusts on Windows. // -// 1. Does a CA bundle stop the embedded PHP from using the Windows -// certificate store? The store is where an organization installs the extra -// root certificate needed when it inspects TLS traffic. curl in the -// embedded PHP is built against Schannel, and its source suggests that -// passing any CA file makes it verify against that file alone. -// -// 2. Can a CA bundle which includes the store's certificates restore trust, -// and can the store be read from Go in the first place? +// It matters because an organization that inspects TLS traffic installs an +// extra root certificate in the Windows certificate store, and curl in the +// embedded PHP is built against Schannel, which can read that store. Passing +// curl a CA file makes it verify against that file alone, so the settings the +// wrapper chooses decide whether such certificates are seen at all. See +// https://github.com/upsun/cli/issues/110. // // The test installs a throwaway CA in the machine's root store, so it needs // administrator rights and only runs when CLI_TEST_MODIFY_CERT_STORE is set. @@ -25,6 +22,7 @@ import ( "crypto/x509" "crypto/x509/pkix" "encoding/pem" + "errors" "math/big" "net" "net/http" @@ -95,17 +93,20 @@ func TestWindowsCertStoreTrust(t *testing.T) { wantTrust bool }{ { - name: "no CA file, so the Windows certificate store is used", + name: "no CA file, so the store is used", args: nil, wantTrust: true, }, { - name: "the bundled CA file replaces the store, as the wrapper configures today", + // 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", args: []string{"-d", iniSetting("openssl.cafile", bundledCAFile)}, wantTrust: false, }, { - name: "a CA file including the store's certificate restores trust", + name: "a CA file which includes the store's certificate", args: []string{"-d", iniSetting("openssl.cafile", mergedCAFile)}, wantTrust: true, }, @@ -224,13 +225,13 @@ func removeCertFromRootStore(t *testing.T, certDER []byte) { assert.NoError(t, windows.CertCloseStore(store, 0)) }() - found := eachCertInStore(t, store, func(context *windows.CertContext, encoded []byte) bool { + found := eachCertInStore(t, store, func(certContext *windows.CertContext, encoded []byte) bool { if !bytes.Equal(encoded, certDER) { return false } // CertDeleteCertificateFromStore frees the context, so the // enumeration must stop here. - assert.NoError(t, windows.CertDeleteCertificateFromStore(context)) + assert.NoError(t, windows.CertDeleteCertificateFromStore(certContext)) return true }) assert.True(t, found, "expected to find the test CA in order to remove it") @@ -284,25 +285,25 @@ func withoutHanging(t *testing.T, description string, fn func() error) { } // eachCertInStore calls fn for each certificate until it returns true. -func eachCertInStore(t *testing.T, store windows.Handle, fn func(context *windows.CertContext, encoded []byte) bool) bool { +func eachCertInStore(t *testing.T, store windows.Handle, fn func(certContext *windows.CertContext, encoded []byte) bool) bool { t.Helper() var count int - var context *windows.CertContext + var certContext *windows.CertContext for { var err error - context, err = windows.CertEnumCertificatesInStore(store, context) - if context == nil { - if err != nil && err != windows.Errno(windows.CRYPT_E_NOT_FOUND) { + certContext, err = windows.CertEnumCertificatesInStore(store, certContext) + if certContext == nil { + if err != nil && !errors.Is(err, windows.Errno(windows.CRYPT_E_NOT_FOUND)) { t.Logf("stopped reading the store: %s", err) } - t.Logf("read %d certificates from the ROOT store", count) + t.Logf("read %d certificates from the store", count) return false } count++ - encoded := unsafe.Slice(context.EncodedCert, context.Length) - if fn(context, encoded) { - t.Logf("matched a certificate after reading %d from the ROOT store", count) + encoded := unsafe.Slice(certContext.EncodedCert, certContext.Length) + if fn(certContext, encoded) { + t.Logf("matched a certificate after reading %d from the store", count) return true } } From e9de5c6f15fde279771d633968338e3f37bb87a3 Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Thu, 30 Jul 2026 00:47:03 +0100 Subject: [PATCH 6/6] fix(legacy): quote the CA bundle path passed to PHP PHP parses ini values as expressions, so an unquoted path is truncated at a character such as "~". A cache directory reached through a Windows short path, for example C:\Users\RUNNER~1\AppData\Local, therefore made every request fail with cURL error 77 instead of finding the bundle. Inside quotes a backslash escapes the next character, so the path is escaped as well, which matters for a network path. The certificate store test now passes the wrapper's own settings, rather than building an argument the wrapper does not produce, and a new test checks that PHP reads such paths back unchanged. Written by Claude Code. --- .github/workflows/ci.yml | 4 +- internal/legacy/cert_store_windows_test.go | 18 +++++--- internal/legacy/php_manager_windows.go | 14 +++++- internal/legacy/php_manager_windows_test.go | 50 +++++++++++++++++++++ 4 files changed, 76 insertions(+), 10 deletions(-) create mode 100644 internal/legacy/php_manager_windows_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9c337f00..1146170d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -164,8 +164,8 @@ jobs: # Only needed so that the package builds. touch internal/legacy/archives/platform.phar - - name: Run certificate store tests + - name: Run Windows tests shell: bash env: CLI_TEST_MODIFY_CERT_STORE: '1' - run: GOEXPERIMENT=jsonv2 go test -v -count=1 -timeout 3m -run TestWindowsCertStoreTrust ./internal/legacy/ + run: GOEXPERIMENT=jsonv2 go test -v -count=1 -timeout 3m -run TestWindows ./internal/legacy/ diff --git a/internal/legacy/cert_store_windows_test.go b/internal/legacy/cert_store_windows_test.go index af20f1cb..499b9660 100644 --- a/internal/legacy/cert_store_windows_test.go +++ b/internal/legacy/cert_store_windows_test.go @@ -85,7 +85,9 @@ func TestWindowsCertStoreTrust(t *testing.T) { mergedCAFile := filepath.Join(cacheDir, "merged.pem") bundled, err := os.ReadFile(bundledCAFile) require.NoError(t, err) - require.NoError(t, os.WriteFile(mergedCAFile, append(bundled, ca.certPEM...), 0o600)) + merged := append(bytes.TrimRight(bundled, "\n"), '\n') + merged = append(merged, ca.certPEM...) + require.NoError(t, os.WriteFile(mergedCAFile, merged, 0o600)) cases := []struct { name string @@ -102,7 +104,7 @@ func TestWindowsCertStoreTrust(t *testing.T) { // organization's certificate is not seen. Expect this // expectation to change along with that. name: "a CA file is used instead of the store", - args: []string{"-d", iniSetting("openssl.cafile", bundledCAFile)}, + args: settingArgs(manager.settings()), wantTrust: false, }, { @@ -309,11 +311,13 @@ func eachCertInStore(t *testing.T, store windows.Handle, fn func(certContext *wi } } -// iniSetting quotes the value of a PHP setting. PHP reads ini values as -// expressions in which characters such as "~" are operators, and Windows short -// paths contain them, for example C:\Users\RUNNER~1\AppData. -func iniSetting(key, value string) string { - return key + `="` + value + `"` +// settingArgs turns PHP settings into options, as makeCmd does. +func settingArgs(settings []string) []string { + args := make([]string, 0, len(settings)*2) + for _, s := range settings { + args = append(args, "-d", s) + } + return args } func runPHPRequest(t *testing.T, phpBin, url string, extraArgs ...string) string { diff --git a/internal/legacy/php_manager_windows.go b/internal/legacy/php_manager_windows.go index 1e5f5610..e1afb8df 100644 --- a/internal/legacy/php_manager_windows.go +++ b/internal/legacy/php_manager_windows.go @@ -3,6 +3,7 @@ package legacy import ( _ "embed" "path/filepath" + "strings" "github.com/upsun/cli/internal/file" ) @@ -27,6 +28,17 @@ func (m *phpManagerPerOS) binPath() string { func (m *phpManagerPerOS) settings() []string { return []string{ - "openssl.cafile=" + filepath.Join(m.cacheDir, "cacert.pem"), + iniSetting("openssl.cafile", filepath.Join(m.cacheDir, "cacert.pem")), } } + +// iniSetting formats a PHP setting for the -d option. +// +// PHP reads ini values as expressions, in which characters such as "~" are +// operators, so a value has to be quoted. Windows short paths contain them, +// for example C:\Users\RUNNER~1\AppData. Inside quotes a backslash escapes the +// next character, so the backslashes have to be doubled. +func iniSetting(key, value string) string { + escaped := strings.NewReplacer(`\`, `\\`, `"`, `\"`).Replace(value) + return key + `="` + escaped + `"` +} diff --git a/internal/legacy/php_manager_windows_test.go b/internal/legacy/php_manager_windows_test.go new file mode 100644 index 00000000..e7f1aa00 --- /dev/null +++ b/internal/legacy/php_manager_windows_test.go @@ -0,0 +1,50 @@ +package legacy + +import ( + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestWindowsIniSetting checks that PHP reads a path back unchanged. +// +// PHP parses ini values as expressions, so an unquoted path is truncated at a +// character such as "~", which Windows short paths contain, and inside quotes a +// backslash escapes the next character. +func TestWindowsIniSetting(t *testing.T) { + cacheDir := t.TempDir() + manager := newPHPManager(cacheDir) + require.NoError(t, manager.copy()) + + readSetting := func(t *testing.T, args ...string) string { + t.Helper() + args = append([]string{"-n"}, args...) + args = append(args, "-r", `echo ini_get("openssl.cafile");`) + output, err := exec.Command(manager.binPath(), args...).CombinedOutput() //nolint:gosec + require.NoError(t, err, "PHP exited with an error: %s", output) + return strings.TrimSpace(string(output)) + } + + cases := []struct { + name string + path string + }{ + {"a path in the cache directory", filepath.Join(cacheDir, "cacert.pem")}, + {"a short path", `C:\Users\RUNNER~1\AppData\Local\cacert.pem`}, + {"a network path", `\\server\share\cacert.pem`}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + assert.Equal(t, c.path, readSetting(t, "-d", iniSetting("openssl.cafile", c.path))) + }) + } + + t.Run("the settings the wrapper passes", func(t *testing.T) { + assert.Equal(t, filepath.Join(cacheDir, "cacert.pem"), + readSetting(t, settingArgs(manager.settings())...)) + }) +}