fix(legacy): trust the certificates in the Windows certificate store - #142
fix(legacy): trust the certificates in the Windows certificate store#142pjcdawkins wants to merge 9 commits into
Conversation
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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.
The embedded PHP has to be given a CA file, because its openssl extension cannot read the Windows certificate store, and curl built against Schannel then verifies against that file alone. 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 part of the CLI already trusted the store, so the two parts disagreed about the same connection. curl in the embedded PHP is now built against OpenSSL, which reads a CA file and the store together and does not limit the size of the file, so the bundle written to the cache directory holds the shipped certificates and the store's trusted roots. Pointing openssl.cafile at it covers the whole PHP layer, not just curl: Composer\CaBundle checks that setting, so Guzzle and the stream context follow the same file. The certificate store test changes with the behavior, as its comment said it would. Written by Claude Code.
This build has curl built against OpenSSL, which the CA bundle now depends on. It also brings security fixes since 8.4.20, including CVE-2026-12184, a segfault in file_get_contents with an https URL and a proxy set, which is a combination the CLI uses, and CVE-2026-14355 in the openssl extension. Written by Claude Code.
|
📋 PR Summary This incremental change makes the Windows CA bundle logic degrade gracefully instead of aborting the CLI. Changes
|
copy() runs in the errgroup which initializes the wrapper, so returning an error from reading the certificate store would stop every legacy command from running. That is worse than not seeing an organization's certificates: the shipped certificates keep everything else working, and are what the CLI trusted before it read the store. The reason is reported through the wrapper's debug output instead. Written by Claude Code.
|
Both review findings addressed. The store-read failure warning was right and is the more important of the two: On widening trust to user-installed roots: that is deliberate, and it is the point of the change. The ROOT store is what the operating system trusts, and it is what the Go part of this same CLI already trusted through the platform verifier — so before this change the two halves disagreed about the same connection. Filtering to Written by Claude Code. |
There was a problem hiding this comment.
📋 Upsun Dispatch Review: incremental · 5 files reviewed · no new issues · 1 still open
Outstanding from earlier reviews:
- #3679219852 — internal/legacy/ca_bundle_windows.go:56: Widens PHP trust to user-installed roots and unconstrained certs in the ROOT store.
Fixes #110, where
upsun loginfails withcURL error 60on a Windows machine whose TLS traffic is inspected.Needs cli-php-builds#1, which built the PHP this depends on. That PHP is already released as
php-8.4.23, so this works as it stands; #1 needs merging so the next build does not silently go back to Schannel.The problem
For someone whose organization inspects TLS traffic, an extra root certificate is installed in the Windows certificate store, and every other program on the machine then trusts it.
The embedded PHP has to be given a CA file: its openssl extension cannot read that store, and the CLI uses openssl streams for some requests. curl built against Schannel then verifies against the given file alone — so the CLI trusted only the certificates shipped with it, and every request on such a machine failed.
The Go part of the CLI verifies through the Windows platform verifier, so it already trusted that certificate. The two parts of the CLI disagreed about the same connection.
The change
curl in the embedded PHP is now built against OpenSSL, which loads a CA file and the Windows stores together, and does not limit the size of the file. So:
caBundle()reads the trusted roots from the store and appends those the shipped bundle does not already have, skipping expired certificates and any the Go parser cannot read.cacert.pemand pointsopenssl.cafileat it, as before. That setting covers the whole PHP layer, not just curl:Composer\CaBundlechecks it, so Guzzle'sverifyoption and the stream context inConfig::getStreamContextOptions()use the same file.openssl.cafileis now quoted. PHP parses ini values as expressions, so an unquoted path was truncated at a character such as~, and a cache directory reached through a Windows short path gavecURL error 77on every command. Backslashes are escaped too, since inside quotes a backslash escapes the next character.This widens trust to what the machine already trusts, which is the point, and to what every other program on it trusts, Go included. It is additive: nothing previously trusted stops working.
Tests
internal/legacy/cert_store_windows_test.goand awindows-latestCI job, which is the only Windows coverage in the repository. The test installs a throwaway CA in the machine's root store, serves HTTPS with a certificate signed by it, and makes requests with the embedded PHP binary. Measured on the runner:OK:hello— the store is usedOK:hello— this is the fix; it wascURL error 60ERR:60: OpenSSL verify result: self-signed certificate in certificate chainThe bundle came to 474 certificates in 768 KB.
TestWindowsIniSettingseparately checks that PHP reads a cache path, a Windows short path and a network path back unchanged.The test modifies the machine's certificate store, so it needs administrator rights and only runs when
CLI_TEST_MODIFY_CERT_STORE=1; otherwise it skips. It removes the certificate int.Cleanup. Adding to the user's store instead makes Windows ask for confirmation and the call never returns, so the add has a 30 second deadline.What was tried first
Merging the whole store while keeping Schannel. The runner rejected it: Schannel refuses a CA file over 1 MiB (
MAX_CAFILE_SIZEin curl'sschannel_verify.c), and a 564-root store gave a 758 KB bundle even after filtering — 76% of a hard limit, with a silent fallback to shipped-only above it. Switching the backend removed the limit, and with it the filtering heuristics and the fallback.Cost
Reading the store happens before every legacy command. Measured on the runner:
About 20 ms, nearly all of it reading and parsing certificates; the file comparison and write are free. Not cached deliberately: a cache would mean the CLI keeps rejecting a certificate the machine already trusts until it expired, which is this bug with a delay. The allocation count says the number belongs to the implementation rather than the store, so making it leaner is the first move if it ever matters.
PHP 8.4.23
Taken for the OpenSSL-backed curl this depends on. It also brings security fixes since 8.4.20, two of which apply here:
CVE-2026-12184, a segfault infile_get_contentswith an https URL and a proxy set, a combination the CLI uses; andCVE-2026-14355in the openssl extension.Written by Claude Code.