fix(legacy): quote the CA bundle path, and cover Windows certificate trust - #139
fix(legacy): quote the CA bundle path, and cover Windows certificate trust#139pjcdawkins wants to merge 6 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>
pjcdawkins
changed the title
|
📋 PR Summary This PR adds Windows-only test coverage characterizing which certificates the embedded PHP trusts. A new test ( Changes
|
![upsun-dispatch[bot]](https://avatars.githubusercontent.com/in/4153575?s=60&v=4){kind=small}
| // 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 + `"` |
There was a problem hiding this comment.
🔵 Info — The characterization test diverges from the real code path, so it misses the unquoted short-path defect.
iniSetting wraps the value in double quotes, producing -d openssl.cafile="<path>", but the production wrapper's settings() in php_manager_windows.go emits openssl.cafile= + path with no quotes. The test therefore exercises a form of the argument the CLI never produces; it faithfully reproduces the "store ignored" bug (row 2) but cannot catch a regression in the wrapper's own unquoted construction — including the Windows short-path syntax error, unexpected '~' case the PR description calls out.
| 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)) |
There was a problem hiding this comment.
🔵 Info — A bundle lacking a trailing newline would silently break the merged-bundle assertion.
os.WriteFile(mergedCAFile, append(bundled, ca.certPEM...), 0o600) concatenates the test CA's PEM directly onto the bundled cacert.pem bytes with no separator. If the bundled cacert.pem ever ships without a trailing newline, the boundary becomes -----END CERTIFICATE----------BEGIN CERTIFICATE----- on one line and Schannel will not parse the appended CA, making the third case fail with an error instead of OK:hello. The curl.se bundle currently ends with a newline, so this is fragile rather than broken today.
There was a problem hiding this comment.
Pull request overview
This PR adds Windows-only coverage to the Go wrapper’s legacy PHP integration, specifically validating how the embedded PHP/cURL on Windows chooses trust roots when openssl.cafile is (or isn’t) set—helping reproduce and guard the behavior reported in #110 and investigated in #138.
Changes:
- Add a Windows-only Go test that installs a temporary root CA into the Windows ROOT store, serves HTTPS with that CA, and validates PHP/cURL trust behavior across CA-file configurations.
- Add a
windows-latestCI job that downloads the embedded PHP artifacts needed to compile/run the Windows legacy package tests and executes the new test with an elevated-only opt-in env var.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/legacy/cert_store_windows_test.go | New Windows-only test that exercises PHP/cURL trust behavior vs. Windows cert store and CA bundle pinning. |
| .github/workflows/ci.yml | Adds a dedicated Windows job to run the new certificate-store trust test in CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func iniSetting(key, value string) string { | ||
| return key + `="` + value + `"` | ||
| } |
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.
pjcdawkins
changed the title
Related to #110 and to #138. Adds the only Windows coverage the repository has, and fixes one bug it found.
Why
For people whose organization inspects TLS traffic, an extra root certificate is installed in the operating system's trust store. On Windows, curl in the embedded PHP is built against Schannel, which can verify against that store — but the wrapper starts PHP with
-d openssl.cafile=<cache>/cacert.pem(internal/legacy/php_manager_windows.go), and PHP's curl extension turns that setting into its defaultCURLOPT_CAINFO.Reading curl's source suggests this replaces the store entirely rather than adding to it:
lib/vtls/schannel.c:if(conn_config->CAfile || conn_config->ca_info_blob)→use_manual_cred_validation = TRUElib/vtls/schannel_verify.c: that path builds a chain engine withengine_config.hExclusiveRoot = trust_store, where the store holds only the CA file's certificatesIf that is right, the CLI cannot see organization certificates on Windows however they are installed, unless the user knows to set
SSL_CERT_FILE. Nobody here runs Windows, so this measures it on a runner instead.What it covers
A Windows-only test (
internal/legacy/cert_store_windows_test.go) plus awindows-latestCI job. The test creates a throwaway CA, installs it in the machine's root store, serves HTTPS with a certificate signed by it, and makes three requests using the embedded PHP binary:windows-latestOK:hello— the Windows store is usedERR:60:schannel: the certificate or certificate chain is based on an untrusted rootOK:helloThe second row is the bug, and it is also what makes the test meaningful: the store holds a trusted CA and the request still fails. It is what users on such a machine get from every request the CLI makes, and it is the error they report. The third row is the mechanism a fix would rely on. The fourth uses
CertOpenSystemStore/CertEnumCertificatesInStorefromgolang.org/x/sys/windows, already a dependency, confirming such a bundle can be generated without anything new.The fix included here
PHP reads ini values as expressions, so
-d openssl.cafile=C:\Users\RUNNER~1\AppData\...producedsyntax error, unexpected '~'and a path truncated toC:\Users\RUNNER, givingcURL error 77. The wrapper passed this setting unquoted, so anyone whose cache directory is reached through a Windows short path hit that on every command. It is now quoted, with backslashes escaped, since a backslash escapes the next character inside quotes and would otherwise corrupt a network path.TestWindowsIniSettingchecks that PHP reads such paths back unchanged.The certificate store test now passes the wrapper's own
settings()rather than an argument the wrapper does not produce, so it exercises the real form.Notes
CLI_TEST_MODIFY_CERT_STORE=1; otherwise it skips. It deletes the certificate again 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 and the test binary a three minute one.CRYPT_E_NO_REVOCATION_CHECK, because the test CA publishes no revocation list. The test setsCURLSSLOPT_NO_REVOKE. Whether a real private CA hits the same thing is worth checking before relying on the store directly, and is one more reason to prefer a merged bundle.Written by Claude Code.