feat(legacy): add the Windows certificate store to the CA bundle - #140
Closed
pjcdawkins wants to merge 2 commits into
Closed
feat(legacy): add the Windows certificate store to the CA bundle#140pjcdawkins wants to merge 2 commits into
pjcdawkins wants to merge 2 commits into
Conversation
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.
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.
Contributor
Author
|
Superseded by #142, which fixes this properly rather than working around it. Merging the whole store under Schannel does not work: it refuses a CA file over 1 MiB, and the runner measured a 758 KB bundle from a 564-root store, so this approach lived just under a hard limit and fell back to the shipped certificates alone when it exceeded it — silently reintroducing the bug. The upsun/cli-php-builds#1 builds curl against OpenSSL instead, which reads a CA file and the store together with no size limit. #142 is the result: no heuristic, no cap, no fallback. Written by Claude Code. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #110. Stacked on #139, so review that first — the base will change to
mainonce it merges.Draft while one decision is open: whether reading the certificate store needs caching. The CI job now measures it, so the numbers are in the run below rather than guessed at.
The problem
The embedded PHP has to be given a CA file. Its openssl extension cannot read the Windows certificate store at all, and the CLI uses openssl streams for some requests, so leaving the file unset is not an option. But setting one also stops curl reading the store, which #139 measured. The result is that the CLI trusted only the certificates shipped with it, and failed wherever an organization installs its own root certificate —
cURL error 60, as reported.The Go half of the CLI verifies through the Windows platform verifier, so it already trusted such a certificate. The two halves disagreed about the same connection.
The change
caBundle()reads the trusted roots from the Windows ROOT store and appends them to the shipped certificates. The wrapper writes that tocacert.pemin the cache directory and pointsopenssl.cafileat it, as before.Pointing the ini setting at the merged file is enough for the whole PHP layer, not just curl:
Composer\CaBundlechecksopenssl.cafilewhen it resolves a bundle, so Guzzle'sverifyoption and the stream context inConfig::getStreamContextOptions()follow the same file.This widens trust to what the machine already trusts, which is the point — and it is what every other program on it does, including Go. It is additive, so nothing previously trusted stops working.
Open question: caching
writeCAFileruns on every legacy command, inside the init lock. It reads the store, builds the bundle, andWriteIfNeededcompares it against the cached file, writing only when the store has changed. Caching the enumeration would save that work at the cost of not noticing a newly installed certificate until the cache expired.BenchmarkWindowsSystemRootsPEMandBenchmarkWindowsCAFilemeasure both halves on the runner. See the "Measure building the CA bundle" step.Notes
SSL_CERT_FILEkeeps overriding everything, becauseComposer\CaBundlechecks it beforeopenssl.cafile.SSL_CERT_FILEon Windows and macOS, since Go only reads it on Unix. That is a separate change.Written by Claude Code.