fix(legacy): preserve Windows CA store semantics

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)

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")
+}

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
 		}