diff --git a/internal/file/file.go b/internal/file/file.go index 595321fe..1fa725f7 100644 --- a/internal/file/file.go +++ b/internal/file/file.go @@ -18,6 +18,17 @@ 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" @@ -28,6 +39,27 @@ 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 4874ad01..cc6373ad 100644 --- a/internal/file/file_test.go +++ b/internal/file/file_test.go @@ -79,3 +79,52 @@ 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 index 41ec041a..215b7298 100644 --- a/internal/legacy/ca_bundle_windows.go +++ b/internal/legacy/ca_bundle_windows.go @@ -16,9 +16,10 @@ import ( // caBundle returns the certificates the legacy CLI should trust: those shipped // with it, plus the trusted roots from the Windows certificate store. // -// If the store cannot be read it returns the shipped certificates and the -// reason: they are what the CLI trusted before, so they are still usable, and -// the caller decides what to do about the rest. +// 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 @@ -30,18 +31,16 @@ import ( // larger than 1 MiB, which this bundle can exceed. func caBundle() ([]byte, error) { roots, err := systemRootsPEM() - if err != nil { - return caCert, err - } bundle := make([]byte, 0, len(caCert)+len(roots)+1) bundle = append(bundle, bytes.TrimRight(caCert, "\n")...) bundle = append(bundle, '\n') - return append(bundle, roots...), nil + return append(bundle, roots...), err } // systemRootsPEM returns the trusted roots from the Windows certificate store -// which the shipped certificates do not already cover. +// 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 @@ -53,25 +52,50 @@ func systemRootsPEM() ([]byte, error) { } var out bytes.Buffer - err = eachStoreCert("ROOT", func(der []byte) error { - if shipped[sha256.Sum256(der)] { + var skipped []error + err = eachStoreCert("ROOT", func(certContext *windows.CertContext, der []byte) error { + fingerprint := sha256.Sum256(der) + if shipped[fingerprint] { return nil } - cert, err := x509.ParseCertificate(der) + usable, err := usableTLSRoot(certContext, 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. + // 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 now := time.Now(); now.Before(cert.NotBefore) || now.After(cert.NotAfter) { + if !usable { return nil } return pem.Encode(&out, &pem.Block{Type: "CERTIFICATE", Bytes: der}) }) if err != nil { - return nil, err + skipped = append(skipped, err) } - return out.Bytes(), nil + return out.Bytes(), errors.Join(skipped...) +} + +// usableTLSRoot reports whether a certificate from the store belongs in the +// bundle: one 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) { + 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. @@ -93,9 +117,62 @@ func certFingerprints(bundle []byte) (map[[32]byte]bool, error) { return fingerprints, nil } -// eachStoreCert calls fn with the encoded bytes of every certificate in a -// Windows system store, as read by every program which verifies against it. -func eachStoreCert(name string, fn func(der []byte) error) error { +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) @@ -112,7 +189,7 @@ func eachStoreCert(name string, fn func(der []byte) error) error { return nil } // The context belongs to the store, so the bytes are only borrowed. - if err := fn(unsafe.Slice(certContext.EncodedCert, certContext.Length)); err != nil { + 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 index 33c94e10..4f2c1fd6 100644 --- a/internal/legacy/ca_bundle_windows_test.go +++ b/internal/legacy/ca_bundle_windows_test.go @@ -1,14 +1,24 @@ package legacy import ( + "crypto/rand" + "crypto/rsa" "crypto/x509" + "crypto/x509/pkix" + "encoding/asn1" "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) @@ -41,6 +51,156 @@ func TestWindowsCABundle(t *testing.T) { } } +// 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") + + // 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) { diff --git a/internal/legacy/cert_store_windows_test.go b/internal/legacy/cert_store_windows_test.go index a6feecd0..141c28dd 100644 --- a/internal/legacy/cert_store_windows_test.go +++ b/internal/legacy/cert_store_windows_test.go @@ -22,7 +22,6 @@ import ( "crypto/tls" "crypto/x509" "crypto/x509/pkix" - "encoding/pem" "errors" "math/big" "net" @@ -131,7 +130,6 @@ func TestWindowsCertStoreTrust(t *testing.T) { } type testCA struct { - certPEM []byte certDER []byte serverCert tls.Certificate } @@ -175,7 +173,6 @@ func generateTestCA(t *testing.T) testCA { require.NoError(t, err) return testCA{ - certPEM: pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: caDER}), certDER: caDER, serverCert: tls.Certificate{ Certificate: [][]byte{serverDER, caDER}, diff --git a/internal/legacy/php_manager_windows.go b/internal/legacy/php_manager_windows.go index f24e7a94..4108159f 100644 --- a/internal/legacy/php_manager_windows.go +++ b/internal/legacy/php_manager_windows.go @@ -30,12 +30,12 @@ func (m *phpManagerPerOS) binPath() string { func (m *phpManagerPerOS) writeCAFile() error { bundle, err := caBundle() if err != nil { - // The shipped certificates are still written, so everything except an - // organization's own certificates keeps working. That is what the CLI - // trusted before it read the store, and better than running nothing. + // 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.WriteIfNeeded(m.caFilePath(), bundle, 0o644) + return file.WriteIfChanged(m.caFilePath(), bundle, 0o644) } func (m *phpManagerPerOS) caFilePath() string {