diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 24ba78f42..a0b190935 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -174,7 +174,7 @@ jobs: with: go-version-file: ./go.mod - - name: Download embedded PHP files + - name: Download the embedded PHP shell: bash run: | # Line endings may be converted on checkout. @@ -185,9 +185,7 @@ jobs: mkdir -p internal/legacy/archives curl -fSL "https://github.com/upsun/cli-php-builds/releases/download/php-$php_version-$(var PHP_BUILD)/$asset" \ -o internal/legacy/archives/php_windows.exe - curl -fSL "https://curl.se/ca/cacert-$(var CACERT_DATE).pem" -o internal/legacy/archives/cacert.pem echo "$php_sha256 internal/legacy/archives/php_windows.exe" | sha256sum -c - - echo "$(var CACERT_SHA256) internal/legacy/archives/cacert.pem" | sha256sum -c - # Only needed so that the package builds. touch internal/legacy/archives/platform.phar touch internal/legacy/archives/commands-upsun.json internal/legacy/archives/commands-platformsh.json @@ -197,9 +195,3 @@ jobs: env: CLI_TEST_MODIFY_CERT_STORE: '1' run: 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 - if: always() - shell: bash - run: go test -run '^$' -bench BenchmarkWindows -benchmem ./internal/legacy/ diff --git a/.github/workflows/update-php.yml b/.github/workflows/update-php.yml index dc9002bd4..857f0a48d 100644 --- a/.github/workflows/update-php.yml +++ b/.github/workflows/update-php.yml @@ -1,5 +1,5 @@ # Opens or updates a pull request when there is a new build of the embedded PHP -# (from upsun/cli-php-builds) or a new CA bundle. See scripts/update-php.sh. +# (from upsun/cli-php-builds). See scripts/update-php.sh. name: Update PHP on: diff --git a/CLAUDE.md b/CLAUDE.md index 965cd4877..3bf49f415 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -135,7 +135,7 @@ The CLI operates as a wrapper around a legacy PHP CLI: - All platforms use static binaries built with [static-php-cli](https://github.com/crazywhalecc/static-php-cli) - Supported platforms: linux/amd64, linux/arm64, darwin/amd64, darwin/arm64, windows/amd64 - Extensions included: curl, filter, openssl, pcntl (Unix), phar, posix (Unix), zlib -- Windows requires `cacert.pem` for OpenSSL (embedded separately), pinned by date and checksum (`CACERT_DATE`, `CACERT_SHA256`) +- On Windows, curl is built with Schannel, which verifies certificates with Windows (its certificate store, distrust list and revocation). No CA file is set, as Schannel would use it instead of the store, so the legacy CLI must make HTTPS requests with curl (Guzzle), not PHP streams (OpenSSL), and uses `CURLSSLOPT_REVOKE_BEST_EFFORT`, as roots installed for TLS inspection often have no revocation information **Downloading PHP Binaries**: ```bash @@ -147,7 +147,7 @@ make download-php ``` **Upgrading PHP Version**: -- The `update-php` workflow runs daily: it finds the latest cli-php-builds release for the current PHP minor version, and the latest CA bundle, and opens or updates a pull request that updates `PHP_VERSION`, `PHP_BUILD`, the checksums and the CA bundle (with `scripts/update-php.sh`). The pull request lists the changed source archives (from the releases' `sources.json`) +- The `update-php` workflow runs daily: it finds the latest cli-php-builds release for the current PHP minor version, and opens or updates a pull request that updates `PHP_VERSION`, `PHP_BUILD` and the checksums (with `scripts/update-php.sh`). The pull request lists the changed source archives (from the releases' `sources.json`) - For a new PHP minor version: trigger the build workflow at [upsun/cli-php-builds](https://github.com/upsun/cli-php-builds/actions) with the new version, then set `PHP_VERSION` to it and `PHP_BUILD` to `0` (builds are numbered from 1), run `scripts/update-php.sh `, and `make download-php` to check the downloads ## Development Notes diff --git a/Makefile b/Makefile index 66fd4437b..0c9338f84 100644 --- a/Makefile +++ b/Makefile @@ -1,11 +1,7 @@ # The PHP version, and the cli-php-builds build number (rebuilt when a library is updated). # Checksums of the binaries are pinned in PHP_SHA256SUMS. Updated by the update-php workflow. PHP_VERSION = 8.5.12 -PHP_BUILD = 1 - -# The CA bundle embedded for PHP on Windows, from https://curl.se/docs/caextract.html. -CACERT_DATE = 2026-10-20 -CACERT_SHA256 = 796f1389bfea13c0ed78f663b765baaff54a508e5483786bd6ece73c5e3e3cbe +PHP_BUILD = 2 .DEFAULT_GOAL := help @@ -76,7 +72,7 @@ single: goreleaser $(ARCHIVES)/platform.phar $(LEGACY_INDEXES) php ## Build a si PHP_VERSION=$(PHP_VERSION) goreleaser build --single-target --id=$(GORELEASER_ID) --snapshot --clean .PHONY: snapshot -snapshot: goreleaser $(ARCHIVES)/platform.phar $(LEGACY_INDEXES) php $(ARCHIVES)/cacert.pem ## Build all release artifacts in dist/ without publishing (needs signing keys) +snapshot: goreleaser $(ARCHIVES)/platform.phar $(LEGACY_INDEXES) php ## Build all release artifacts in dist/ without publishing (needs signing keys) ifndef RSA_SIGNING_KEY_FILE $(error RSA_SIGNING_KEY_FILE is not set. Set it to the path of your RSA private key for APK signing, or use 'make snapshot-no-nfpm' to skip packaging.) endif @@ -101,7 +97,7 @@ php: $(ARCHIVES)/php_$(GOOS)_$(GOARCH) ## Download the PHP binary for the curren # Download PHP binaries for all platforms. .PHONY: download-php -download-php: $(PHP_UNIX_BINARIES) $(ARCHIVES)/php_windows.exe $(ARCHIVES)/cacert.pem +download-php: $(PHP_UNIX_BINARIES) $(ARCHIVES)/php_windows.exe # Build the legacy CLI phar from the legacy/ subdirectory. # The self:build command handles composer --no-dev rebuild, Box installation, and cache warming. @@ -147,13 +143,7 @@ $(ARCHIVES)/php_windows.exe: $(ARCHIVES)/php-version $(call verify-sha256,$@,$(call php-sha256,php-$(PHP_VERSION)-windows-amd64.exe)) .PHONY: $(ARCHIVES)/php_windows_amd64 -$(ARCHIVES)/php_windows_amd64: $(ARCHIVES)/php_windows.exe $(ARCHIVES)/cacert.pem - -.PHONY: $(ARCHIVES)/cacert.pem -$(ARCHIVES)/cacert.pem: - mkdir -p $(ARCHIVES) - curl -fSL https://curl.se/ca/cacert-$(CACERT_DATE).pem -o $@ - $(call verify-sha256,$@,$(CACERT_SHA256)) +$(ARCHIVES)/php_windows_amd64: $(ARCHIVES)/php_windows.exe .PHONY: FORCE FORCE: @@ -162,7 +152,7 @@ FORCE: # Create and publish a release. .PHONY: release -release: goreleaser clean $(ARCHIVES)/platform.phar $(LEGACY_INDEXES) php $(ARCHIVES)/cacert.pem +release: goreleaser clean $(ARCHIVES)/platform.phar $(LEGACY_INDEXES) php ifndef RSA_SIGNING_KEY_FILE $(error RSA_SIGNING_KEY_FILE is not set. Set it to the path of your RSA private key for APK signing.) endif diff --git a/internal/file/file.go b/internal/file/file.go index 1fa725f72..595321fe9 100644 --- a/internal/file/file.go +++ b/internal/file/file.go @@ -18,17 +18,6 @@ func WriteIfNeeded(destFilename string, source []byte, perm os.FileMode) error { return Write(destFilename, source, perm) } -// WriteIfChanged writes data to a destination file unless its complete -// contents already match. Unlike WriteIfNeeded, it is suitable for dynamic -// data which can change without changing its size or final bytes. -func WriteIfChanged(destFilename string, source []byte, perm os.FileMode) error { - matches, err := fullyMatches(destFilename, source) - if err != nil || matches { - return err - } - return Write(destFilename, source, perm) -} - // Write creates or overwrites a file, somewhat atomically, using a temporary file next to it. func Write(path string, content []byte, fileMode fs.FileMode) error { tmpFile := path + ".tmp" @@ -39,27 +28,6 @@ func Write(path string, content []byte, fileMode fs.FileMode) error { return os.Rename(tmpFile, path) } -// fullyMatches checks if a file exists and its whole contents equal data. The -// size is compared first, so a file which cannot match is not read. -func fullyMatches(filename string, data []byte) (bool, error) { - fi, err := os.Stat(filename) - if err != nil { - if errors.Is(err, fs.ErrNotExist) { - return false, nil - } - return false, err - } - if fi.Size() != int64(len(data)) { - return false, nil - } - - existing, err := os.ReadFile(filename) - if err != nil { - return false, err - } - return bytes.Equal(existing, data), nil -} - // probablyMatches checks if a file exists and matches the end of source data (up to checkSize bytes). func probablyMatches(filename string, data []byte, checkSize int) (bool, error) { f, err := os.Open(filename) diff --git a/internal/file/file_test.go b/internal/file/file_test.go index cc6373ad3..4874ad01e 100644 --- a/internal/file/file_test.go +++ b/internal/file/file_test.go @@ -79,52 +79,3 @@ func TestWriteIfNeeded(t *testing.T) { }) } } - -func TestWriteIfChanged(t *testing.T) { - source := []byte("new contents") - - cases := []struct { - name string - initialData []byte - }{ - {"a missing file", nil}, - {"a shorter file", []byte("old")}, - {"a longer file", []byte("old contents, but longer than the new ones")}, - } - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - destFile := filepath.Join(t.TempDir(), "dynamic-file") - if c.initialData != nil { - require.NoError(t, os.WriteFile(destFile, c.initialData, 0o600)) - } - - require.NoError(t, WriteIfChanged(destFile, source, 0o600)) - actual, err := os.ReadFile(destFile) - require.NoError(t, err) - assert.Equal(t, source, actual) - }) - } -} - -func TestWriteIfChangedChecksTheWholeFile(t *testing.T) { - destFile := filepath.Join(t.TempDir(), "dynamic-file") - original := bytes.Repeat([]byte{'a'}, 64*1024) - updated := bytes.Clone(original) - updated[0] = 'b' - - require.NoError(t, os.WriteFile(destFile, original, 0o600)) - require.NoError(t, WriteIfChanged(destFile, updated, 0o600)) - actual, err := os.ReadFile(destFile) - require.NoError(t, err) - assert.Equal(t, updated, actual, - "equal size and matching final 32 KB must not hide an earlier change") - - // A rewrite is detected by an older modification time being lost, because - // the clock can tick too slowly to tell two writes apart. - past := time.Unix(1, 0) - require.NoError(t, os.Chtimes(destFile, past, past)) - require.NoError(t, WriteIfChanged(destFile, updated, 0o600)) - info, err := os.Stat(destFile) - require.NoError(t, err) - assert.Equal(t, past.UTC(), info.ModTime().UTC(), "matching contents should not be rewritten") -} diff --git a/internal/legacy/ca_bundle_windows.go b/internal/legacy/ca_bundle_windows.go deleted file mode 100644 index 51b732c5d..000000000 --- a/internal/legacy/ca_bundle_windows.go +++ /dev/null @@ -1,218 +0,0 @@ -package legacy - -import ( - "bytes" - "crypto/sha256" - "crypto/x509" - "encoding/hex" - "encoding/pem" - "errors" - "fmt" - "time" - "unsafe" - - "golang.org/x/sys/windows" -) - -// caBundle returns the certificates the legacy CLI should trust: those shipped -// with it, plus the trusted roots from the Windows certificate store. -// -// It always returns a usable bundle. If any of the store could not be read it -// also returns the reason, having left out only what it could not read: the -// shipped certificates are what the CLI trusted before, so they are still -// usable, and the caller decides what to do about the rest. -// -// An organization which inspects TLS traffic installs its own root certificate -// in that store, and every other program on the machine then trusts it. The -// embedded PHP has to be given a CA file, because its openssl extension cannot -// read the store, so the store's certificates are added to the file instead. -// -// This needs curl in the embedded PHP to be built against OpenSSL. Built -// against Schannel, as static-php-cli does by default, it refuses a CA file -// larger than 1 MiB, which this bundle can exceed. -func caBundle() ([]byte, error) { - roots, err := systemRootsPEM() - - bundle := make([]byte, 0, len(caCert)+len(roots)+1) - bundle = append(bundle, bytes.TrimRight(caCert, "\n")...) - bundle = append(bundle, '\n') - return append(bundle, roots...), err -} - -// distrustedRoots are SHA-256 fingerprints of roots left out of the bundle -// although Windows trusts them. -// -// Windows distrusts some intermediates without distrusting their roots, and -// OpenSSL cannot see that, so it would still trust the intermediates through -// the roots. The shipped bundle must not have them either, which a test checks. -var distrustedRoots = map[string]bool{ - // The roots of the compromised eMudhra intermediates. emSign ECC Root CA - G3 - // cross-signs emSign Root TLS CA - G3. - // See: https://emudhra.we-have-a-problem.com/ - "d7a7a0fb5d7e2731d771e9484ebcdef71d5f0c3e0a2948782bc83ee0ea699ef4": true, // AAA Certificate Services. - "7dd78d5f4f13459a83dff9abbba62edbaf6f2d102bf257fd712f4d9f2746ed8d": true, // emSign Root TLS CA - G3. - "86a1ecba089c4a8d3bbe2734c612ba341d813e043cf9e8a862cd5c57a36bbe6b": true, // emSign ECC Root CA - G3. -} - -// systemRootsPEM returns the trusted roots from the Windows certificate store -// which the shipped certificates do not already cover, together with the -// reasons for anything it could not read. -// -// The ROOT store merges the machine's roots with the current user's, which is -// what the operating system, and so every other program on it, trusts. That -// includes anything the user has added themselves. -func systemRootsPEM() ([]byte, error) { - shipped, err := certFingerprints(caCert) - if err != nil { - return nil, err - } - - var out bytes.Buffer - var skipped []error - err = eachStoreCert("ROOT", func(certContext *windows.CertContext, der []byte) error { - fingerprint := sha256.Sum256(der) - if shipped[fingerprint] { - return nil - } - usable, err := usableTLSRoot(certContext, der) - if err != nil { - // One certificate Windows will not report on must not cost the - // trust in every other root, so only it is left out. - skipped = append(skipped, fmt.Errorf("skipped the certificate %x: %w", fingerprint[:8], err)) - return nil - } - if !usable { - return nil - } - return pem.Encode(&out, &pem.Block{Type: "CERTIFICATE", Bytes: der}) - }) - if err != nil { - skipped = append(skipped, err) - } - return out.Bytes(), errors.Join(skipped...) -} - -// usableTLSRoot reports whether a certificate from the store belongs in the -// bundle: one which is not distrusted, which Windows allows to authenticate a -// TLS server, which is currently valid, and which Go can read. It returns an -// error for a certificate it cannot judge, which the caller has to leave out -// rather than trust. -func usableTLSRoot(certContext *windows.CertContext, der []byte) (bool, error) { - fingerprint := sha256.Sum256(der) - if distrustedRoots[hex.EncodeToString(fingerprint[:])] { - return false, nil - } - allowed, err := certAllowsServerAuth(certContext) - if err != nil { - return false, err - } - if !allowed { - return false, nil - } - cert, err := x509.ParseCertificate(der) - if err != nil { - // A store can hold a certificate Go cannot read, and one which cannot - // be parsed would make the whole file unusable. - return false, nil - } - now := time.Now() - return !now.Before(cert.NotBefore) && !now.After(cert.NotAfter), 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 -} - -var procCertGetEnhancedKeyUsage = windows.NewLazySystemDLL("crypt32.dll").NewProc("CertGetEnhancedKeyUsage") - -// certAllowsServerAuth reports whether a certificate's effective Windows EKUs -// allow it to authenticate a TLS server. Windows combines the EKU extension in -// the encoded certificate with an EKU property held only in the store. The -// property has to be checked before exporting the certificate to PEM, which -// cannot preserve it. -func certAllowsServerAuth(certContext *windows.CertContext) (bool, error) { - var size uint32 - result, _, callErr := procCertGetEnhancedKeyUsage.Call( - uintptr(unsafe.Pointer(certContext)), - 0, - 0, - uintptr(unsafe.Pointer(&size)), - ) - if result == 0 { - return false, fmt.Errorf("could not read certificate purposes: %w", callErr) - } - if size < uint32(unsafe.Sizeof(windows.CertEnhKeyUsage{})) { - return false, fmt.Errorf("could not read certificate purposes: unexpected data size %d", size) - } - - wordSize := uint32(unsafe.Sizeof(uintptr(0))) - buffer := make([]uintptr, (size+wordSize-1)/wordSize) - usage := (*windows.CertEnhKeyUsage)(unsafe.Pointer(&buffer[0])) - result, _, callErr = procCertGetEnhancedKeyUsage.Call( - uintptr(unsafe.Pointer(certContext)), - 0, - uintptr(unsafe.Pointer(usage)), - uintptr(unsafe.Pointer(&size)), - ) - if result == 0 { - return false, fmt.Errorf("could not read certificate purposes: %w", callErr) - } - if usage.Length == 0 { - // CRYPT_E_NOT_FOUND means there is no restriction, while a zero last - // error means the certificate has explicitly been given no valid uses. - return errors.Is(callErr, windows.Errno(windows.CRYPT_E_NOT_FOUND)), nil - } - - for _, identifier := range unsafe.Slice(usage.UsageIdentifiers, usage.Length) { - switch windows.BytePtrToString(identifier) { - case "1.3.6.1.5.5.7.3.1", // Server Authentication. - "1.3.6.1.4.1.311.10.3.3", // Microsoft Server Gated Crypto. - "2.16.840.1.113730.4.1", // Netscape Server Gated Crypto. - "2.5.29.37.0": // Any Extended Key Usage. - return true, nil - } - } - return false, nil -} - -// eachStoreCert calls fn with the context and encoded bytes of every -// certificate in a Windows system store, as read by every program which -// verifies against it. The context is needed for properties not held in DER. -func eachStoreCert(name string, fn func(certContext *windows.CertContext, 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 fmt.Errorf("could not read the %s certificate store: %w", name, err) - } - return nil - } - // The context belongs to the store, so the bytes are only borrowed. - if err := fn(certContext, 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 deleted file mode 100644 index 85673cdd8..000000000 --- a/internal/legacy/ca_bundle_windows_test.go +++ /dev/null @@ -1,268 +0,0 @@ -package legacy - -import ( - "crypto/rand" - "crypto/rsa" - "crypto/sha256" - "crypto/x509" - "crypto/x509/pkix" - "encoding/asn1" - "encoding/hex" - "encoding/pem" - "math/big" - "testing" - "time" - "unsafe" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "golang.org/x/sys/windows" -) - -var procCertSetEnhancedKeyUsage = windows.NewLazySystemDLL("crypt32.dll").NewProc("CertSetEnhancedKeyUsage") - -func TestWindowsCABundle(t *testing.T) { - bundle, err := caBundle() - require.NoError(t, err) - - shipped, err := certFingerprints(caCert) - require.NoError(t, err) - held, err := certFingerprints(bundle) - require.NoError(t, err) - - // The size varies by machine, so it is reported rather than asserted. It - // only has to be a size curl will read, which OpenSSL does not limit. - t.Logf("bundle: %d certificates in %d KB (%d shipped, %d from the store)", - len(held), len(bundle)/1024, len(shipped), len(held)-len(shipped)) - - assert.Greater(t, len(held), len(shipped), "expected the store to add certificates") - 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") - 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 - } - _, err := x509.ParseCertificate(block.Bytes) - require.NoError(t, err, "expected every certificate in the bundle to be readable") - } -} - -// TestWindowsCABundleDistrustedRoots checks that the roots of the compromised -// eMudhra intermediates are in neither the shipped certificates nor the bundle. -// Windows installs them on demand, so the store may not hold them yet: then -// only the shipped certificates are really checked. -func TestWindowsCABundleDistrustedRoots(t *testing.T) { - roots := map[string]string{ - "AAA Certificate Services": "d7a7a0fb5d7e2731d771e9484ebcdef71d5f0c3e0a2948782bc83ee0ea699ef4", - "emSign Root TLS CA - G3": "7dd78d5f4f13459a83dff9abbba62edbaf6f2d102bf257fd712f4d9f2746ed8d", - "emSign ECC Root CA - G3": "86a1ecba089c4a8d3bbe2734c612ba341d813e043cf9e8a862cd5c57a36bbe6b", - } - - inStore := make(map[[32]byte]bool) - require.NoError(t, eachStoreCert("ROOT", func(_ *windows.CertContext, der []byte) error { - inStore[sha256.Sum256(der)] = true - return nil - })) - shipped, err := certFingerprints(caCert) - require.NoError(t, err) - bundle, err := caBundle() - require.NoError(t, err) - held, err := certFingerprints(bundle) - require.NoError(t, err) - - for name, fingerprintHex := range roots { - require.True(t, distrustedRoots[fingerprintHex], "expected %s to be distrusted", name) - var fingerprint [32]byte - _, err := hex.Decode(fingerprint[:], []byte(fingerprintHex)) - require.NoError(t, err) - t.Logf("%s: in the store: %t", name, inStore[fingerprint]) - assert.False(t, shipped[fingerprint], "expected %s not to be shipped", name) - assert.False(t, held[fingerprint], "expected %s not to be in the bundle", name) - } -} - -// TestWindowsCertificatePurposes checks which certificates count as TLS roots. -// A certificate is valid only for the purposes named by both its EKU extension -// and its store property, and one which names neither is valid for all of them. -func TestWindowsCertificatePurposes(t *testing.T) { - const ( - serverAuth = "1.3.6.1.5.5.7.3.1" - codeSigning = "1.3.6.1.5.5.7.3.3" - gatedCrypto = "1.3.6.1.4.1.311.10.3.3" - ) - - cases := []struct { - name string - extension []x509.ExtKeyUsage - property []string - allowed bool - }{ - { - name: "no purposes at all", - allowed: true, - }, - { - name: "an extension for code signing", - extension: []x509.ExtKeyUsage{x509.ExtKeyUsageCodeSigning}, - allowed: false, - }, - { - name: "an extension for code signing and server authentication", - extension: []x509.ExtKeyUsage{x509.ExtKeyUsageCodeSigning, x509.ExtKeyUsageServerAuth}, - allowed: true, - }, - { - name: "an extension for any purpose", - extension: []x509.ExtKeyUsage{x509.ExtKeyUsageAny}, - allowed: true, - }, - { - name: "a property for code signing", - property: []string{codeSigning}, - allowed: false, - }, - { - name: "a property for code signing and server authentication", - property: []string{codeSigning, serverAuth}, - allowed: true, - }, - { - name: "a property for server gated crypto", - property: []string{gatedCrypto}, - allowed: true, - }, - { - name: "a property which withdraws the extension's server authentication", - extension: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, - property: []string{codeSigning}, - allowed: false, - }, - { - name: "a property which keeps the extension's server authentication", - extension: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageCodeSigning}, - property: []string{serverAuth}, - allowed: true, - }, - } - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - _, certContext := createTestCertContext(t, c.extension) - if len(c.property) > 0 { - setWindowsCertificatePurposes(t, certContext, c.property) - } - - allowed, err := certAllowsServerAuth(certContext) - require.NoError(t, err) - assert.Equal(t, c.allowed, allowed) - }) - } -} - -func TestWindowsUsableTLSRoot(t *testing.T) { - der, certContext := createTestCertContext(t, nil) - usable, err := usableTLSRoot(certContext, der) - require.NoError(t, err) - assert.True(t, usable, "expected a valid certificate with no restriction to be a usable root") - - fingerprint := sha256.Sum256(der) - distrustedRoots[hex.EncodeToString(fingerprint[:])] = true - t.Cleanup(func() { delete(distrustedRoots, hex.EncodeToString(fingerprint[:])) }) - usable, err = usableTLSRoot(certContext, der) - require.NoError(t, err) - assert.False(t, usable, "expected a distrusted root not to be usable") - - // An EKU extension which cannot be decoded. Windows tolerates it, reporting - // the purposes it could read rather than an error, so what keeps such a - // certificate out of the bundle is Go refusing to parse it. The reason is - // not asserted, only that it is never trusted. - undecodable := pkix.Extension{Id: asn1.ObjectIdentifier{2, 5, 29, 37}, Value: []byte{0xff, 0xff}} - der, certContext = createTestCertContext(t, nil, undecodable) - usable, err = usableTLSRoot(certContext, der) - t.Logf("an undecodable EKU extension: usable=%t, err=%v", usable, err) - assert.False(t, usable, "expected a certificate with an undecodable EKU extension not to be trusted") -} - -// createTestCertContext creates a context for a self-signed CA, which is given -// an EKU extension only when usages are passed, and returns its encoding too. -func createTestCertContext(t *testing.T, usages []x509.ExtKeyUsage, extra ...pkix.Extension) ([]byte, *windows.CertContext) { - t.Helper() - - key, err := rsa.GenerateKey(rand.Reader, 2048) - require.NoError(t, err) - template := &x509.Certificate{ - SerialNumber: big.NewInt(1), - Subject: pkix.Name{CommonName: "Upsun CLI test CA (safe to delete)"}, - NotBefore: time.Now().Add(-time.Hour), - NotAfter: time.Now().Add(24 * time.Hour), - IsCA: true, - KeyUsage: x509.KeyUsageCertSign, - ExtKeyUsage: usages, - ExtraExtensions: extra, - BasicConstraintsValid: true, - } - der, err := x509.CreateCertificate(rand.Reader, template, template, &key.PublicKey, key) - require.NoError(t, err) - - certContext, err := windows.CertCreateCertificateContext( - windows.X509_ASN_ENCODING, - &der[0], - uint32(len(der)), - ) - require.NoError(t, err) - t.Cleanup(func() { - assert.NoError(t, windows.CertFreeCertificateContext(certContext)) - }) - return der, certContext -} - -// setWindowsCertificatePurposes replaces a certificate's EKU property, which is -// what restricting a root's purposes in the certificate manager writes, and -// which exporting the certificate cannot preserve. -func setWindowsCertificatePurposes(t *testing.T, certContext *windows.CertContext, identifiers []string) { - t.Helper() - - oids := make([]*byte, len(identifiers)) - for i, identifier := range identifiers { - oid, err := windows.BytePtrFromString(identifier) - require.NoError(t, err) - oids[i] = oid - } - usage := windows.CertEnhKeyUsage{ - Length: uint32(len(oids)), - UsageIdentifiers: &oids[0], - } - result, _, callErr := procCertSetEnhancedKeyUsage.Call( - uintptr(unsafe.Pointer(certContext)), - uintptr(unsafe.Pointer(&usage)), - ) - require.NotZero(t, result, "setting certificate purposes failed: %s", callErr) -} - -// 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 := caBundle(); err != nil { - b.Fatal(err) - } - } -} - -// 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{cacheDir: 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 7b6f80373..cb001e5f7 100644 --- a/internal/legacy/cert_store_windows_test.go +++ b/internal/legacy/cert_store_windows_test.go @@ -2,12 +2,10 @@ package legacy // This file covers which certificates the embedded PHP trusts on Windows. // -// It matters because an organization that inspects TLS traffic installs an -// extra root certificate in the Windows certificate store. The embedded PHP -// layer uses curl and OpenSSL streams, and the wrapper's CA-file settings -// determine whether the PHP layer trusts only the shipped bundle or also the -// certificates trusted by Windows (e.g. corporate TLS inspection roots). See -// https://github.com/upsun/cli/issues/110. +// Its curl is built with Schannel, which verifies certificates with Windows: +// the certificate store, including a root installed by an organization which +// inspects TLS traffic (https://github.com/upsun/cli/issues/110), and the +// distrust and revocation lists. // // 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. @@ -18,10 +16,10 @@ import ( "context" "crypto/rand" "crypto/rsa" - "crypto/sha256" "crypto/tls" "crypto/x509" "crypto/x509/pkix" + "encoding/pem" "errors" "math/big" "net" @@ -30,6 +28,7 @@ import ( "os" "os/exec" "path/filepath" + "strconv" "strings" "testing" "time" @@ -41,19 +40,21 @@ 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); +curl_setopt($ch, CURLOPT_SSL_OPTIONS, (int) getenv('TEST_SSL_OPTIONS')); +if (getenv('TEST_CAINFO') !== false) { + curl_setopt($ch, CURLOPT_CAINFO, getenv('TEST_CAINFO')); +} $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:" +// curl error codes, reported as "cURL error N" by the CLI. +const ( + curlSSLConnectError = "ERR:35:" + curlPeerFailedVerification = "ERR:60:" + curlSSLOptRevokeBestEffort = 8 // CURLSSLOPT_REVOKE_BEST_EFFORT +) func TestWindowsCertStoreTrust(t *testing.T) { if os.Getenv("CLI_TEST_MODIFY_CERT_STORE") != "1" { @@ -71,62 +72,62 @@ func TestWindowsCertStoreTrust(t *testing.T) { installCAInRootStore(t, ca.certDER) - // 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() - // 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)) + t.Run("curl is built with Schannel", func(t *testing.T) { + assert.Equal(t, "Schannel", runPHP(t, phpBin, nil, `echo curl_version()['ssl_version'];`)) + }) + + // A CA file which does not hold the installed root. + otherCAFile := filepath.Join(cacheDir, "other.pem") + require.NoError(t, os.WriteFile(otherCAFile, pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", Bytes: generateTestCA(t).certDER, + }), 0o600)) cases := []struct { - name string - args []string - wantTrust bool + name string + sslOptions int + caFile string + want string }{ { - name: "no CA file, so the store is used", - args: nil, - wantTrust: true, + name: "the store is used, with revocation checked where possible, as the legacy CLI does", + sslOptions: curlSSLOptRevokeBestEffort, + want: "OK:hello", }, { - name: "the settings the wrapper passes", - args: settingArgs(manager.settings()), - wantTrust: true, + // The test CA publishes no revocation list or OCSP responder, like + // many roots installed for TLS inspection. + name: "strict revocation checking rejects a root without revocation information", + want: curlSSLConnectError, }, { - // 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, + // So the CLI must not set openssl.cafile, which PHP passes to curl. + name: "a CA file replaces the store", + sslOptions: curlSSLOptRevokeBestEffort, + caFile: otherCAFile, + want: curlPeerFailedVerification, }, } 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) + env := []string{"TEST_URL=" + server.URL, "TEST_SSL_OPTIONS=" + strconv.Itoa(c.sslOptions)} + if c.caFile != "" { + env = append(env, "TEST_CAINFO="+c.caFile) + } + output := runPHP(t, phpBin, env, phpRequestScript) + t.Logf("%v -> %s", env, output) + if strings.HasPrefix(c.want, "ERR:") { + assert.True(t, strings.HasPrefix(output, c.want), "expected %q to start with %q", output, c.want) } else { - assert.Contains(t, output, curlPeerFailedVerification) + assert.Equal(t, c.want, output) } }) } - - t.Run("the installed certificate is in the bundle", func(t *testing.T) { - bundle, err := caBundle() - require.NoError(t, err) - fingerprints, err := certFingerprints(bundle) - 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 certificates in the bundle, %d bytes", len(fingerprints), len(bundle)) - }) } type testCA struct { @@ -293,25 +294,14 @@ func eachCertInStore(t *testing.T, store windows.Handle, fn func(certContext *wi } } -// 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 { +// runPHP runs PHP code with the embedded binary, as the wrapper does. +func runPHP(t *testing.T, phpBin string, env []string, code 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) + cmd := exec.CommandContext(ctx, phpBin, "-n", "-r", code) + cmd.Env = append(os.Environ(), env...) output, err := cmd.CombinedOutput() require.NoError(t, err, "PHP exited with an error: %s", output) diff --git a/internal/legacy/legacy.go b/internal/legacy/legacy.go index 354eda692..0fb89c554 100644 --- a/internal/legacy/legacy.go +++ b/internal/legacy/legacy.go @@ -114,10 +114,6 @@ func (c *CLIWrapper) init() error { return err } - for _, warning := range phpMgr.warnings() { - c.debug("%s", warning) - } - c.debug("Initialized PHP CLI (%s)", time.Since(preInit)) return nil @@ -172,16 +168,12 @@ func (c *CLIWrapper) Exec(ctx context.Context, args ...string) error { // makeCmd makes a legacy CLI command with the given context and arguments. func (c *CLIWrapper) makeCmd(ctx context.Context, args []string, cacheDir string) *exec.Cmd { phpMgr := newPHPManager(cacheDir) - settings := phpMgr.settings() // "-n" tells PHP to ignore any php.ini and scan directory on the host. // The embedded PHP binary is statically built with all extensions it // needs compiled in, and cannot load shared extensions referenced by a // system php.ini (e.g. inside Lando containers). - var cmdArgs = make([]string, 0, len(args)+3+len(settings)*2) + var cmdArgs = make([]string, 0, len(args)+2) cmdArgs = append(cmdArgs, "-n") - for _, s := range settings { - cmdArgs = append(cmdArgs, "-d", s) - } cmdArgs = append(cmdArgs, c.pharPath(cacheDir)) cmdArgs = append(cmdArgs, args...) return exec.CommandContext(ctx, phpMgr.binPath(), cmdArgs...) //nolint:gosec diff --git a/internal/legacy/php_manager.go b/internal/legacy/php_manager.go index c8273fd82..a234f1f7e 100644 --- a/internal/legacy/php_manager.go +++ b/internal/legacy/php_manager.go @@ -6,26 +6,12 @@ type phpManager interface { // binPath returns the path to the temporary PHP binary. binPath() string - - // settings returns PHP INI entries (key=value format). - settings() []string - - // warnings returns anything from copy which is worth reporting, but is not - // a reason to stop the CLI from running. - warnings() []string } type phpManagerPerOS struct { cacheDir string - - // copyWarnings is set by copy, on the platforms which have any. - copyWarnings []string } func newPHPManager(cacheDir string) phpManager { - return &phpManagerPerOS{cacheDir: cacheDir} -} - -func (m *phpManagerPerOS) warnings() []string { - return m.copyWarnings + return &phpManagerPerOS{cacheDir} } diff --git a/internal/legacy/php_manager_unix.go b/internal/legacy/php_manager_unix.go index 73c29358e..a75043f14 100644 --- a/internal/legacy/php_manager_unix.go +++ b/internal/legacy/php_manager_unix.go @@ -15,7 +15,3 @@ func (m *phpManagerPerOS) copy() error { func (m *phpManagerPerOS) binPath() string { return filepath.Join(m.cacheDir, "php") } - -func (m *phpManagerPerOS) settings() []string { - return nil -} diff --git a/internal/legacy/php_manager_windows.go b/internal/legacy/php_manager_windows.go index 4108159ff..148346379 100644 --- a/internal/legacy/php_manager_windows.go +++ b/internal/legacy/php_manager_windows.go @@ -3,7 +3,6 @@ package legacy import ( _ "embed" "path/filepath" - "strings" "github.com/upsun/cli/internal/file" ) @@ -11,50 +10,13 @@ import ( //go:embed archives/php_windows.exe var phpCLI []byte -//go:embed archives/cacert.pem -var caCert []byte - +// copy writes the PHP binary. Its curl is built with Schannel, which verifies +// certificates with Windows (its certificate store, and its distrust and +// revocation lists) unless given a CA file, so none is written. func (m *phpManagerPerOS) copy() error { - if err := file.WriteIfNeeded(m.binPath(), phpCLI, 0o755); err != nil { - return err - } - return m.writeCAFile() + return file.WriteIfNeeded(m.binPath(), phpCLI, 0o755) } 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 { - // Whatever could be read is still written, and the shipped certificates - // always are, so the CLI keeps at least the trust it had before it read - // the store. That is better than running nothing. - m.copyWarnings = append(m.copyWarnings, err.Error()) - } - return file.WriteIfChanged(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", m.caFilePath()), - } -} - -// 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 deleted file mode 100644 index e7f1aa000..000000000 --- a/internal/legacy/php_manager_windows_test.go +++ /dev/null @@ -1,50 +0,0 @@ -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())...)) - }) -} diff --git a/internal/legacy/php_sha256sums.txt b/internal/legacy/php_sha256sums.txt index 88948b362..3366c0184 100644 --- a/internal/legacy/php_sha256sums.txt +++ b/internal/legacy/php_sha256sums.txt @@ -1,6 +1,6 @@ -51a811393751f40fb5f9b1ae08df42c69e696dfd54a141f9c5e65af5af0939d1 php-8.5.12-darwin-amd64 -4dd04d8dc838833516ba87e0194619ad0997d1f4d3be59e7a63f985c00a01cc5 php-8.5.12-darwin-arm64 -08942c10c9e9f9a797c8fcfc5d94823c2430c3d6c39494a2e71ec13e2c4b80c6 php-8.5.12-linux-amd64 -06d2e3bd8dfd2bd5eab03c54ebcfa0a95681db8dbce225c09d497b8c59bfb1f3 php-8.5.12-linux-arm64 -9fc842adc796ea2312ee92de34ac34c4bf91ec631de366066474567a89d15508 php-8.5.12-windows-amd64.exe +f4a5eeded2980d8c748d8614ad638b55704c77e7be91646d5b08ad5db6cc5caa php-8.5.12-darwin-amd64 +26d411b11e2e73e3d29cd8da515cd3eb8cabb31eaae11e78ee9d795ad4b6cb96 php-8.5.12-darwin-arm64 +de99cc83a4d3e6345bc2110401a7111dd2e752fa7659182d38a2bcd27d337f8c php-8.5.12-linux-amd64 +e743085928bb2c6e9e772cda2943d9670711df890f4b09e57ba9aa8c22e96065 php-8.5.12-linux-arm64 +375d5655b96f49519f2f4f451f5e59eb1cfc498732b4e7e484d5aa5b63943c38 php-8.5.12-windows-amd64.exe 1ed4e5fd58d550190b4ce21e0159d20420294c93b264a9a5615d9c19fecc21a5 sources.json diff --git a/legacy/phpstan-baseline.neon b/legacy/phpstan-baseline.neon index ef11d45da..8661fc841 100644 --- a/legacy/phpstan-baseline.neon +++ b/legacy/phpstan-baseline.neon @@ -5664,12 +5664,6 @@ parameters: count: 1 path: src/Service/Api.php - - - message: '#^Method Platformsh\\Cli\\Service\\Api\:\:caBundlePath\(\) should return string but returns mixed\.$#' - identifier: return.type - count: 1 - path: src/Service/Api.php - - message: '#^Method Platformsh\\Cli\\Service\\Api\:\:checkCanCreate\(\) should return array\{can_create\: bool, message\: string, required_action\: array\{action\: string, type\: string\}\|null\} but returns array\.$#' identifier: return.type @@ -5970,18 +5964,6 @@ parameters: count: 1 path: src/Service/Config.php - - - message: '#^Cannot access offset ''cafile'' on mixed\.$#' - identifier: offsetAccess.nonOffsetAccessible - count: 1 - path: src/Service/Config.php - - - - message: '#^Cannot access offset ''capath'' on mixed\.$#' - identifier: offsetAccess.nonOffsetAccessible - count: 1 - path: src/Service/Config.php - - message: '#^Cannot access offset ''certifier_url''\|''oauth2_auth_url''\|''oauth2_revoke_url''\|''oauth2_token_url'' on mixed\.$#' identifier: offsetAccess.nonOffsetAccessible @@ -6096,12 +6078,6 @@ parameters: count: 2 path: src/Service/Config.php - - - message: '#^Cannot access offset ''verify_peer'' on mixed\.$#' - identifier: offsetAccess.nonOffsetAccessible - count: 1 - path: src/Service/Config.php - - message: '#^Cannot access offset ''wrapped_disabled_commands'' on mixed\.$#' identifier: offsetAccess.nonOffsetAccessible diff --git a/legacy/src/Connector.php b/legacy/src/Connector.php new file mode 100644 index 000000000..107ec1020 --- /dev/null +++ b/legacy/src/Connector.php @@ -0,0 +1,58 @@ + */ + private readonly array $curlOptions; + + /** + * @param array $config + * The library's options, plus "curl" (array): curl options. + */ + public function __construct(array $config = [], ?SessionInterface $session = null) + { + $curlOptions = \is_array($config['curl'] ?? null) ? $config['curl'] : []; + unset($config['curl']); + $this->curlOptions = $curlOptions; + if ($curlOptions) { + $middlewares = \is_array($config['middlewares'] ?? null) ? $config['middlewares'] : []; + $middlewares[] = fn(callable $handler): \Closure => function (RequestInterface $request, array $options) use ($handler, $curlOptions) { + $options['curl'] = (\is_array($options['curl'] ?? null) ? $options['curl'] : []) + $curlOptions; + return $handler($request, $options); + }; + $config['middlewares'] = $middlewares; + } + parent::__construct($config, $session); + } + + public function getOAuth2Provider(): Platformsh + { + $provider = parent::getOAuth2Provider(); + if ($this->curlOptions) { + // The same options as the library's provider passes to its HTTP client. + $provider->setHttpClient(new Client([ + 'debug' => $this->config['debug'], + 'verify' => $this->config['verify'], + 'proxy' => $this->config['proxy'], + 'curl' => $this->curlOptions, + ])); + } + return $provider; + } +} diff --git a/legacy/src/CredentialHelper/Manager.php b/legacy/src/CredentialHelper/Manager.php index 98a4a7fa5..909263ca7 100644 --- a/legacy/src/CredentialHelper/Manager.php +++ b/legacy/src/CredentialHelper/Manager.php @@ -4,6 +4,8 @@ namespace Platformsh\Cli\CredentialHelper; +use GuzzleHttp\Client; +use GuzzleHttp\Exception\GuzzleException; use Platformsh\Cli\Exception\ProcessFailedException; use Platformsh\Cli\Service\Config; use Platformsh\Cli\Service\Shell; @@ -164,10 +166,12 @@ private function download(array $helper, string $destination): void } // Download from the URL. - $contextOptions = $this->config->getStreamContextOptions(60); - $contextOptions['http']['follow_location'] = 1; - $contents = file_get_contents($helper['url'], false, \stream_context_create($contextOptions)); - if (!$contents) { + try { + $contents = (string) (new Client())->request('GET', $helper['url'], ['timeout' => 60] + $this->config->getGuzzleOptions())->getBody(); + } catch (GuzzleException $e) { + throw new \RuntimeException('Failed to download credentials helper from URL: ' . $helper['url'], 0, $e); + } + if ($contents === '') { throw new \RuntimeException('Failed to download credentials helper from URL: ' . $helper['url']); } diff --git a/legacy/src/Service/Api.php b/legacy/src/Service/Api.php index cad767ac0..6395e619e 100644 --- a/legacy/src/Service/Api.php +++ b/legacy/src/Service/Api.php @@ -12,7 +12,6 @@ use Symfony\Component\Filesystem\Filesystem; use GuzzleHttp\Exception\RequestException; use Platformsh\Client\Model\Integration; -use Composer\CaBundle\CaBundle; use Doctrine\Common\Cache\CacheProvider; use Doctrine\Common\Cache\VoidCache; use GuzzleHttp\Client; @@ -37,7 +36,7 @@ use Platformsh\Cli\Util\NamespacedCache; use Platformsh\Cli\Util\NestedArrayUtil; use Platformsh\Cli\Util\Sort; -use Platformsh\Client\Connection\Connector; +use Platformsh\Cli\Connector; use Platformsh\Client\Exception\ApiResponseException; use Platformsh\Client\Exception\EnvironmentStateException; use Platformsh\Client\Model\AutoscalingSettings; @@ -324,14 +323,16 @@ private function getConnectorOptions(): array $connectorOptions['accounts'] = $this->config->get('api.accounts_api_url'); } $connectorOptions['certifier_url'] = $this->config->get('api.certifier_url'); - $connectorOptions['verify'] = $this->config->getBool('api.skip_ssl') ? false : $this->caBundlePath(); + $guzzleOptions = $this->config->getGuzzleOptions(); + $connectorOptions['verify'] = $guzzleOptions['verify']; + $connectorOptions['curl'] = $guzzleOptions['curl'] ?? []; $connectorOptions['debug'] = false; $connectorOptions['client_id'] = $this->config->get('api.oauth2_client_id'); $connectorOptions['user_agent'] = $this->config->getUserAgent(); $connectorOptions['timeout'] = $this->config->getInt('api.default_timeout'); - $connectorOptions['proxy'] = $this->guzzleProxyConfig(); + $connectorOptions['proxy'] = $guzzleOptions['proxy']; $connectorOptions['token_url'] = $this->config->get('api.oauth2_token_url'); $connectorOptions['revoke_url'] = $this->config->get('api.oauth2_revoke_url'); @@ -412,25 +413,6 @@ private function addLegacyAuthOptions(array &$connectorOptions): void $connectorOptions['on_refresh_error'] = fn(IdentityProviderException $e): ?AccessToken => $this->onRefreshError($e); } - /** - * Returns the path to the CA bundle or file detected by Composer. - * - * If a system CA bundle cannot be detected, Composer will use its bundled - * file. It will copy the file to a temporary directory if necessary (when - * running inside a Phar). - * - * @return string - */ - private function caBundlePath(): string - { - static $path; - if ($path === null) { - $path = CaBundle::getSystemCaRootBundlePath(); - $this->io->debug('Determined CA bundle path: ' . $path); - } - return $path; - } - private function onStepUpAuthResponse(ResponseInterface $response): ?AccessToken { if ($this->inLoginCheck) { @@ -626,31 +608,6 @@ private function tokenFromSession(SessionInterface $session): ?AccessToken return new AccessToken($tokenData); } - /** - * Returns proxy config in the format expected by Guzzle. - * - * @return array - */ - private function guzzleProxyConfig(): array - { - $config = array_map(function ($proxyUrl) { - // If Guzzle is going to use PHP's built-in HTTP streams, - // rather than curl, then transform the proxy scheme. - if (!\extension_loaded('curl') && \ini_get('allow_url_fopen')) { - return \str_replace(['http://', 'https://'], ['tcp://', 'tcp://'], $proxyUrl); - } - return $proxyUrl; - }, $this->config->getProxies()); - - // Guzzle only reads the no_proxy environment variable itself when no - // proxy has been configured explicitly, so pass the hosts on. - if ($noProxy = $this->config->getNoProxy()) { - $config['no'] = $noProxy; - } - - return $config; - } - /** * Returns the API client object. * @@ -803,13 +760,14 @@ public function getCurlOptions(string $url, int $connectTimeout): array CURLOPT_PROTOCOLS => CURLPROTO_HTTPS | CURLPROTO_HTTP, ]; - if ($this->config->getBool('api.skip_ssl')) { + $guzzleOptions = $this->config->getGuzzleOptions(); + if ($guzzleOptions['verify'] === false) { $options[CURLOPT_SSL_VERIFYPEER] = false; $options[CURLOPT_SSL_VERIFYHOST] = 0; - } else { - $caBundlePath = $this->caBundlePath(); - $options[\is_dir($caBundlePath) ? CURLOPT_CAPATH : CURLOPT_CAINFO] = $caBundlePath; + } elseif (\is_string($guzzleOptions['verify'])) { + $options[\is_dir($guzzleOptions['verify']) ? CURLOPT_CAPATH : CURLOPT_CAINFO] = $guzzleOptions['verify']; } + $options += $guzzleOptions['curl'] ?? []; $proxies = $this->config->getProxies(); $scheme = \parse_url($url, PHP_URL_SCHEME); @@ -1687,11 +1645,8 @@ public function getHttpClient(): ClientInterface */ public function getExternalHttpClient(): ClientInterface { - $options = [ - 'headers' => ['User-Agent' => $this->config->getUserAgent()], + $options = $this->config->getGuzzleOptions() + [ 'debug' => false, - 'verify' => $this->config->getBool('api.skip_ssl') ? false : $this->caBundlePath(), - 'proxy' => $this->guzzleProxyConfig(), 'timeout' => $this->config->getInt('api.default_timeout'), ]; diff --git a/legacy/src/Service/Config.php b/legacy/src/Service/Config.php index ff5a3eff9..0a6f80b49 100644 --- a/legacy/src/Service/Config.php +++ b/legacy/src/Service/Config.php @@ -20,6 +20,8 @@ class Config /** @var array */ private array $env; + private static ?string $caBundlePath = null; + private ?Filesystem $fs = null; private ?string $version = null; private ?string $homeDir = null; @@ -621,42 +623,78 @@ public function getNoProxy(): array } /** - * Returns an array of context options for HTTP/HTTPS streams. + * Returns Guzzle request options for HTTP requests: TLS verification, proxies and the user agent. * - * @return array{http: array, ssl?: array} + * @return array{headers: array, proxy: array, verify: bool|string, curl?: array} */ - public function getStreamContextOptions(int|float|null $timeout = null): array + public function getGuzzleOptions(): array { - $opts = [ - // See https://www.php.net/manual/en/context.http.php - 'http' => [ - 'method' => 'GET', - 'timeout' => $timeout !== null ? $timeout : $this->getInt('api.default_timeout'), - 'user_agent' => $this->getUserAgent(), - ], + $options = [ + 'headers' => ['User-Agent' => $this->getUserAgent()], + 'proxy' => $this->getGuzzleProxies(), ]; - - // The PHP stream context only accepts a single proxy option, under the schemes 'tcp' or 'ssl'. - $proxies = $this->getProxies(); - foreach ($proxies as $proxyUrl) { - $opts['http']['proxy'] = \str_replace(['http://', 'https://'], ['tcp://', 'ssl://'], $proxyUrl); - break; - } - - // Set up SSL options. if ($this->getBool('api.skip_ssl')) { - $opts['ssl']['verify_peer'] = false; - $opts['ssl']['verify_peer_name'] = false; + $options['verify'] = false; + } elseif (self::curlUsesSchannel()) { + // Schannel (on Windows) verifies certificates with Windows: its certificate store, distrust list and + // revocation. Given a CA file it would use that instead of the store, so none is set. Revocation is + // checked where possible, as roots installed to inspect TLS traffic often have no revocation information: + // for the server, and for an HTTPS proxy. + $options['verify'] = true; + $options['curl'] = [ + \CURLOPT_SSL_OPTIONS => \CURLSSLOPT_REVOKE_BEST_EFFORT, + \CURLOPT_PROXY_SSL_OPTIONS => \CURLSSLOPT_REVOKE_BEST_EFFORT, + ]; } else { - $caBundlePath = CaBundle::getSystemCaRootBundlePath(); - if (\is_dir($caBundlePath)) { - $opts['ssl']['capath'] = $caBundlePath; - } else { - $opts['ssl']['cafile'] = $caBundlePath; + $options['verify'] = $this->caBundlePath(); + } + return $options; + } + + /** + * Checks whether curl is built with Schannel, the Windows TLS library. + */ + public static function curlUsesSchannel(): bool + { + $version = \extension_loaded('curl') ? \curl_version() : false; + return \is_array($version) && \is_string($version['ssl_version'] ?? null) && \str_starts_with($version['ssl_version'], 'Schannel'); + } + + /** + * Returns the path to the CA bundle or file detected by Composer. + * + * If a system CA bundle cannot be detected, Composer will use its bundled + * file. It will copy the file to a temporary directory if necessary (when + * running inside a Phar). + */ + private function caBundlePath(): string + { + return self::$caBundlePath ??= CaBundle::getSystemCaRootBundlePath(); + } + + /** + * Returns proxy settings for Guzzle. + * + * @return array + */ + private function getGuzzleProxies(): array + { + $config = array_map(function ($proxyUrl) { + // If Guzzle is going to use PHP's built-in HTTP streams, + // rather than curl, then transform the proxy scheme. + if (!\extension_loaded('curl') && \ini_get('allow_url_fopen')) { + return \str_replace(['http://', 'https://'], ['tcp://', 'tcp://'], $proxyUrl); } + return $proxyUrl; + }, $this->getProxies()); + + // Guzzle only reads the no_proxy environment variable itself when no + // proxy has been configured explicitly, so pass the hosts on. + if ($noProxy = $this->getNoProxy()) { + $config['no'] = $noProxy; } - return $opts; + return $config; } /** diff --git a/legacy/tests/ConfigTest.php b/legacy/tests/ConfigTest.php index 5720cf441..90a185f9b 100644 --- a/legacy/tests/ConfigTest.php +++ b/legacy/tests/ConfigTest.php @@ -150,4 +150,27 @@ public function testGetNoProxy(): void } } } + + public function testGetGuzzleOptions(): void + { + $config = new Config([], $this->configFile); + $options = $config->getGuzzleOptions(); + $this->assertEquals($config->getUserAgent(), $options['headers']['User-Agent']); + if (Config::curlUsesSchannel()) { + // Schannel verifies with Windows when it is not given a CA file. + $this->assertTrue($options['verify']); + $this->assertEquals([ + CURLOPT_SSL_OPTIONS => CURLSSLOPT_REVOKE_BEST_EFFORT, + CURLOPT_PROXY_SSL_OPTIONS => CURLSSLOPT_REVOKE_BEST_EFFORT, + ], $options['curl'] ?? null); + } else { + $this->assertIsString($options['verify']); + $this->assertFileExists($options['verify']); + $this->assertArrayNotHasKey('curl', $options); + } + + $options = (new Config(['MOCK_CLI_SKIP_SSL' => '1'], $this->configFile))->getGuzzleOptions(); + $this->assertFalse($options['verify']); + $this->assertArrayNotHasKey('curl', $options); + } } diff --git a/legacy/tests/ConnectorTest.php b/legacy/tests/ConnectorTest.php new file mode 100644 index 000000000..8cee7bcfd --- /dev/null +++ b/legacy/tests/ConnectorTest.php @@ -0,0 +1,33 @@ + self::$httpServerUrl, + 'api_token' => 'test', + 'api_token_type' => 'access', + 'curl' => [CURLOPT_COOKIE => 'test=curl-option'], + ], new Session()); + + $response = $connector->getClient()->request('GET', self::$httpServerUrl . '/cookie', ['auth' => false]); + $this->assertEquals('test=curl-option', (string) $response->getBody()); + + $response = $connector->getOAuth2Provider()->getHttpClient()->request('GET', self::$httpServerUrl . '/cookie'); + $this->assertEquals('test=curl-option', (string) $response->getBody()); + } +} diff --git a/legacy/tests/HttpServerTrait.php b/legacy/tests/HttpServerTrait.php new file mode 100644 index 000000000..4a9b4efa8 --- /dev/null +++ b/legacy/tests/HttpServerTrait.php @@ -0,0 +1,42 @@ +start(); + self::$httpServerUrl = 'http://127.0.0.1:' . $port; + $deadline = microtime(true) + 5; + while (!PortUtil::isPortInUse($port, '127.0.0.1')) { + if (microtime(true) > $deadline || !self::$httpServer->isRunning()) { + throw new \RuntimeException('The test HTTP server did not start: ' . self::$httpServer->getErrorOutput()); + } + usleep(50000); + } + } + + #[AfterClass] + public static function stopHttpServer(): void + { + self::$httpServer?->stop(0); + self::$httpServer = null; + } +} diff --git a/legacy/tests/data/http-server.php b/legacy/tests/data/http-server.php new file mode 100644 index 000000000..ca2cf9138 --- /dev/null +++ b/legacy/tests/data/http-server.php @@ -0,0 +1,12 @@ +&2 +if [[ "$new_tag" == "$old_tag" ]]; then + echo "Up to date: $old_tag" + exit 0 +fi +if [[ "$(sort -V <<< "$old_tag"$'\n'"$new_tag" | tail -n1)" != "$new_tag" ]]; then + echo "The latest release $new_tag is older than $old_tag" >&2 exit 1 fi +new_version=$(sed -E 's/^php-(.*)-[0-9]+$/\1/' <<< "$new_tag") +new_build=${new_tag##*-} -titles=() -body="" - -if [[ "$new_tag" != "$old_tag" ]]; then - if [[ "$(sort -V <<< "$old_tag"$'\n'"$new_tag" | tail -n1)" != "$new_tag" ]]; then - echo "The latest release $new_tag is older than $old_tag" >&2 - exit 1 - fi - new_version=$(sed -E 's/^php-(.*)-[0-9]+$/\1/' <<< "$new_tag") - new_build=${new_tag##*-} - - gh release download "$new_tag" --repo "$repo" --dir "$tmp/new" --pattern SHA256SUMS.txt --pattern sources.json - if ! gh release download "$old_tag" --repo "$repo" --dir "$tmp/old" --pattern sources.json; then - mkdir -p "$tmp/old" && echo '{}' > "$tmp/old/sources.json" - fi - - cp "$tmp/new/SHA256SUMS.txt" "$sums_file" - set_makefile_var PHP_VERSION "$new_version" - set_makefile_var PHP_BUILD "$new_build" - - if [[ "$new_version" == "$php_version" ]]; then - titles+=("rebuild PHP $new_version (build $new_build)") - else - titles+=("upgrade PHP to $new_version (build $new_build)") - fi - - # Lists changed source archives, one row per change, with the platforms it applies to. - changes=$(jq -rn --slurpfile old "$tmp/old/sources.json" --slurpfile new "$tmp/new/sources.json" ' - $old[0] as $o | $new[0] as $n - | [($o + $n | keys[]) as $p | (($o[$p] // {}) + ($n[$p] // {}) | keys[]) as $lib - | {p: $p, lib: $lib, old: ($o[$p][$lib] // "-"), new: ($n[$p][$lib] // "-")} - | select(.old != .new)] - | group_by([.lib, .old, .new]) - | map("| \(.[0].lib) | \(.[0].old) | \(.[0].new) | \(map(.p) | join(", ")) |") - | .[]') - body+="Updates the embedded PHP binaries from [$old_tag](https://github.com/$repo/releases/tag/$old_tag)" - body+=" to [$new_tag](https://github.com/$repo/releases/tag/$new_tag)."$'\n\n' - if [[ -n "$changes" ]]; then - body+="| Source | Old | New | Platforms |"$'\n'"|---|---|---|---|"$'\n'"$changes"$'\n\n' - else - body+="No source archives changed."$'\n\n' - fi +gh release download "$new_tag" --repo "$repo" --dir "$tmp/new" --pattern SHA256SUMS.txt --pattern sources.json +if ! gh release download "$old_tag" --repo "$repo" --dir "$tmp/old" --pattern sources.json; then + mkdir -p "$tmp/old" && echo '{}' > "$tmp/old/sources.json" fi -if [[ "$new_cacert_date" > "$cacert_date" ]]; then - file="cacert-$new_cacert_date.pem" - curl -fsSL "https://curl.se/ca/$file" -o "$tmp/$file" - curl -fsSL "https://curl.se/ca/$file.sha256" -o "$tmp/$file.sha256" - (cd "$tmp" && sha256sum -c "$file.sha256") - set_makefile_var CACERT_DATE "$new_cacert_date" - set_makefile_var CACERT_SHA256 "$(cut -d' ' -f1 "$tmp/$file.sha256")" - titles+=("update the CA bundle to $new_cacert_date") - body+="Updates the CA bundle (embedded for PHP on Windows) from $cacert_date to $new_cacert_date." - body+=" See: https://curl.se/docs/caextract.html"$'\n\n' -fi +cp "$tmp/new/SHA256SUMS.txt" "$sums_file" +set_makefile_var PHP_VERSION "$new_version" +set_makefile_var PHP_BUILD "$new_build" -if [[ ${#titles[@]} -eq 0 ]]; then - echo "Up to date: $old_tag, CA bundle $cacert_date" - exit 0 +if [[ "$new_version" == "$php_version" ]]; then + title="build: rebuild PHP $new_version (build $new_build)" +else + title="build: upgrade PHP to $new_version (build $new_build)" fi -title="build: ${titles[0]}" -if [[ ${#titles[@]} -gt 1 ]]; then - title+=" and ${titles[1]}" +# Lists changed source archives, one row per change, with the platforms it applies to. +changes=$(jq -rn --slurpfile old "$tmp/old/sources.json" --slurpfile new "$tmp/new/sources.json" ' + $old[0] as $o | $new[0] as $n + | [($o + $n | keys[]) as $p | (($o[$p] // {}) + ($n[$p] // {}) | keys[]) as $lib + | {p: $p, lib: $lib, old: ($o[$p][$lib] // "-"), new: ($n[$p][$lib] // "-")} + | select(.old != .new)] + | group_by([.lib, .old, .new]) + | map("| \(.[0].lib) | \(.[0].old) | \(.[0].new) | \(map(.p) | join(", ")) |") + | .[]') +body="Updates the embedded PHP binaries from [$old_tag](https://github.com/$repo/releases/tag/$old_tag)" +body+=" to [$new_tag](https://github.com/$repo/releases/tag/$new_tag)."$'\n\n' +if [[ -n "$changes" ]]; then + body+="| Source | Old | New | Platforms |"$'\n'"|---|---|---|---|"$'\n'"$changes"$'\n\n' +else + body+="No source archives changed."$'\n\n' fi + echo "$title" > "$title_file" printf '%s' "$body" > "$body_file" echo "$title"