fix(proxy): remove obsolete Phase AI CA roots on Windows with certutil

src/pkg/proxy/trust.go

@@ -18,7 +18,11 @@ import (
 
 const phaseAITrustNickname = "Phase AI Proxy CA"
 
-const windowsPhaseCARotationScript = `$ErrorActionPreference='Stop'; $desired='__PHASE_SHA256__'; $phaseRoots=Get-ChildItem -Path Cert:\CurrentUser\Root | Where-Object { $_.Subject -eq $_.Issuer -and $_.Subject -match '(^|,\s*)CN=Phase AI Proxy CA(,|$)' -and $_.Subject -match '(^|,\s*)O=Phase(,|$)' -and ($_.Extensions | Where-Object { $_.Oid.Value -eq '2.5.29.19' -and $_.CertificateAuthority }) }; if ($phaseRoots | Where-Object { $_.GetCertHashString('SHA256').ToUpperInvariant() -eq $desired }) { Write-Output 'PHASE_CA_CURRENT'; exit 0 }; $phaseRoots | ForEach-Object { Remove-Item -LiteralPath $_.PSPath -Force }`
+// windowsPhaseRootInspectionScript only reads the current-user Root store. It
+// prints PHASE_CA_CURRENT when the CA being trusted is already there, otherwise
+// one PHASE_CA_OBSOLETE line per older Phase-owned root. Deleting through the
+// Cert: drive fails under -NonInteractive, so trustWindows removes them with certutil.
+const windowsPhaseRootInspectionScript = `$ErrorActionPreference='Stop'; $desired='__PHASE_SHA256__'; $phaseRoots=Get-ChildItem -Path Cert:\CurrentUser\Root | Where-Object { $_.Subject -eq $_.Issuer -and $_.Subject -match '(^|,\s*)CN=Phase AI Proxy CA(,|$)' -and $_.Subject -match '(^|,\s*)O=Phase(,|$)' -and ($_.Extensions | Where-Object { $_.Oid.Value -eq '2.5.29.19' -and $_.CertificateAuthority }) }; if ($phaseRoots | Where-Object { $_.GetCertHashString('SHA256').ToUpperInvariant() -eq $desired }) { Write-Output 'PHASE_CA_CURRENT'; exit 0 }; $phaseRoots | ForEach-Object { Write-Output ('PHASE_CA_OBSOLETE ' + $_.Thumbprint) }`
 
 // ErrUserCATrustUnsupported means Phase cannot safely modify a trust store for
 // only the current user on this operating system. Callers can continue with
@@ -128,7 +132,7 @@ func UserCATrustNotice(goos string) string {
 	case "darwin":
 		return "macOS asks for your login password or Touch ID once: the public Phase AI CA is added to your login keychain's trust settings. No sudo, nothing system-wide."
 	case "windows":
-		return "Windows shows one security warning to confirm adding the public Phase AI CA to your user Root store. Nothing system-wide."
+		return "Windows shows a security warning to confirm adding the public Phase AI CA to your user Root store, plus one for each older Phase AI CA it removes. Nothing system-wide."
 	default:
 		return ""
 	}
@@ -153,7 +157,35 @@ func (osTrustCommandRunner) LookPath(file string) (string, error) {
 }
 
 func (osTrustCommandRunner) Run(name string, args ...string) ([]byte, error) {
-	return exec.Command(name, args...).CombinedOutput()
+	command := exec.Command(name, args...)
+	if strings.EqualFold(filepath.Base(name), "powershell.exe") {
+		command.Env = windowsPowerShellEnvironment(os.Environ())
+	}
+	return command.CombinedOutput()
+}
+
+// windowsPowerShellEnvironment gives Windows PowerShell its own module path.
+// Launched from PowerShell 7 it inherits that shell's PSModulePath, cannot
+// auto-load the certificate provider, and reports that the Cert: drive does
+// not exist.
+func windowsPowerShellEnvironment(base []string) []string {
+	systemRoot := os.Getenv("SystemRoot")
+	if systemRoot == "" {
+		systemRoot = `C:\Windows`
+	}
+	programFiles := os.Getenv("ProgramFiles")
+	if programFiles == "" {
+		programFiles = `C:\Program Files`
+	}
+	modulePath := "PSModulePath=" + programFiles + `\WindowsPowerShell\Modules;` + systemRoot + `\System32\WindowsPowerShell\v1.0\Modules`
+	environment := make([]string, 0, len(base)+1)
+	for _, entry := range base {
+		if key, _, _ := strings.Cut(entry, "="); strings.EqualFold(key, "PSModulePath") {
+			continue
+		}
+		environment = append(environment, entry)
+	}
+	return append(environment, modulePath)
 }
 
 type userCATrustInstaller struct {
@@ -296,17 +328,29 @@ func (i userCATrustInstaller) trustWindows(certificatePath string, certificate *
 	}
 	fingerprint := sha256.Sum256(certificate.Raw)
 	fingerprintHex := strings.ToUpper(hex.EncodeToString(fingerprint[:]))
-	rotationScript := strings.Replace(windowsPhaseCARotationScript, "__PHASE_SHA256__", fingerprintHex, 1)
-	rotationOutput, rotationErr := i.Runner.Run(
+	inspectionScript := strings.Replace(windowsPhaseRootInspectionScript, "__PHASE_SHA256__", fingerprintHex, 1)
+	inspectionOutput, inspectionErr := i.Runner.Run(
 		powershell, "-NoLogo", "-NoProfile", "-NonInteractive",
-		"-Command", rotationScript,
+		"-Command", inspectionScript,
 	)
-	if rotationErr != nil {
-		return UserCATrustResult{}, trustCommandError("inspect and rotate Phase AI CA in Windows current-user Root store", rotationErr, rotationOutput)
+	if inspectionErr != nil {
+		return UserCATrustResult{}, trustCommandError("inspect Phase AI CA in Windows current-user Root store", inspectionErr, inspectionOutput)
 	}
-	if strings.Contains(string(rotationOutput), "PHASE_CA_CURRENT") {
+	if strings.Contains(string(inspectionOutput), "PHASE_CA_CURRENT") {
 		return UserCATrustResult{Store: "Windows current-user Root store", AlreadyTrusted: true}, nil
 	}
+	obsoleteThumbprints, err := parseWindowsObsoletePhaseRoots(inspectionOutput)
+	if err != nil {
+		return UserCATrustResult{}, fmt.Errorf("inspect Phase AI CA in Windows current-user Root store: %w", err)
+	}
+	// Remove obsolete roots first so a rotation never leaves an old Phase root
+	// trusted. Windows confirms each removal, as it does the add below.
+	for _, thumbprint := range obsoleteThumbprints {
+		deleteOutput, deleteErr := i.Runner.Run(certutil, "-user", "-delstore", "Root", thumbprint)
+		if deleteErr != nil {
+			return UserCATrustResult{}, trustCommandError("remove obsolete Phase AI CA from Windows current-user Root store", deleteErr, deleteOutput)
+		}
+	}
 	// -user is the security boundary: without it certutil targets the local
 	// machine store and may elevate. -f makes re-enabling idempotent.
 	output, err := i.Runner.Run(certutil, "-user", "-addstore", "-f", "Root", certificatePath)
@@ -316,6 +360,24 @@ func (i userCATrustInstaller) trustWindows(certificatePath string, certificate *
 	return UserCATrustResult{Store: "Windows current-user Root store"}, nil
 }
 
+// parseWindowsObsoletePhaseRoots returns the SHA-1 thumbprints the inspection
+// script printed. certutil -delstore also matches store indexes and common
+// names, so anything but a 40-hex thumbprint is rejected.
+func parseWindowsObsoletePhaseRoots(output []byte) ([]string, error) {
+	thumbprints := make([]string, 0)
+	for _, line := range strings.Split(string(output), "\n") {
+		thumbprint, found := strings.CutPrefix(strings.TrimSpace(line), "PHASE_CA_OBSOLETE ")
+		if !found {
+			continue
+		}
+		if raw, err := hex.DecodeString(thumbprint); err != nil || len(raw) != 20 {
+			return nil, fmt.Errorf("unexpected certificate thumbprint %q", thumbprint)
+		}
+		thumbprints = append(thumbprints, thumbprint)
+	}
+	return thumbprints, nil
+}
+
 func parsePEMCertificates(payload []byte) []*x509.Certificate {
 	result := make([]*x509.Certificate, 0)
 	rest := payload

src/pkg/proxy/trust_test.go

@@ -189,10 +189,80 @@ func TestUserCATrustWindowsUsesUserStore(t *testing.T) {
 	if !reflect.DeepEqual(runner.commands[1].Args, want) {
 		t.Fatalf("certutil args = %v, want %v", runner.commands[1].Args, want)
 	}
-	rotationArgs := strings.Join(runner.commands[0].Args, " ")
-	for _, required := range []string{"Cert:\\CurrentUser\\Root", "CN=Phase AI Proxy CA", "O=Phase", "Subject -eq $_.Issuer", "2.5.29.19", "Remove-Item -LiteralPath"} {
-		if !strings.Contains(rotationArgs, required) {
-			t.Fatalf("Windows rotation is not scoped to Phase-owned current-user roots; missing %q in %s", required, rotationArgs)
+	inspectionArgs := strings.Join(runner.commands[0].Args, " ")
+	for _, required := range []string{"Cert:\\CurrentUser\\Root", "CN=Phase AI Proxy CA", "O=Phase", "Subject -eq $_.Issuer", "2.5.29.19", "PHASE_CA_OBSOLETE"} {
+		if !strings.Contains(inspectionArgs, required) {
+			t.Fatalf("Windows inspection is not scoped to Phase-owned current-user roots; missing %q in %s", required, inspectionArgs)
+		}
+	}
+	// The Cert: drive refuses Root store removals under -NonInteractive.
+	if strings.Contains(inspectionArgs, "Remove-Item") {
+		t.Fatalf("Windows inspection must not delete through the Cert: drive: %s", inspectionArgs)
+	}
+}
+
+func TestUserCATrustWindowsRemovesObsoletePhaseRootsWithCertutilBeforeTrusting(t *testing.T) {
+	certificatePath, _, _ := writeTrustTestCA(t)
+	obsolete := []string{strings.Repeat("A1", 20), strings.Repeat("0F", 20)}
+	runner := &recordingTrustRunner{run: func(_ string, args ...string) ([]byte, error) {
+		if len(args) > 0 && args[0] == "-NoLogo" {
+			return []byte("PHASE_CA_OBSOLETE " + obsolete[0] + "\r\nPHASE_CA_OBSOLETE " + obsolete[1] + "\r\n"), nil
+		}
+		return nil, nil
+	}}
+	result, err := (userCATrustInstaller{GOOS: "windows", Home: `C:\\Users\\phase`, Runner: runner}).Trust(certificatePath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if result.AlreadyTrusted || len(runner.commands) != 4 {
+		t.Fatalf("unexpected result=%+v commands=%v", result, runner.commands)
+	}
+	for index, thumbprint := range obsolete {
+		command := runner.commands[index+1]
+		want := []string{"-user", "-delstore", "Root", thumbprint}
+		if command.Name != "/usr/bin/certutil" || !reflect.DeepEqual(command.Args, want) {
+			t.Fatalf("removal %d = %s %v, want certutil %v", index, command.Name, command.Args, want)
+		}
+	}
+	want := []string{"-user", "-addstore", "-f", "Root", certificatePath}
+	if !reflect.DeepEqual(runner.commands[3].Args, want) {
+		t.Fatalf("certutil args = %v, want %v", runner.commands[3].Args, want)
+	}
+}
+
+func TestUserCATrustWindowsRejectsMalformedObsoleteThumbprint(t *testing.T) {
+	certificatePath, _, _ := writeTrustTestCA(t)
+	// certutil -delstore also accepts store indexes: "0" is the first root.
+	runner := &recordingTrustRunner{run: func(_ string, args ...string) ([]byte, error) {
+		if len(args) > 0 && args[0] == "-NoLogo" {
+			return []byte("PHASE_CA_OBSOLETE 0\r\n"), nil
+		}
+		return nil, nil
+	}}
+	_, err := (userCATrustInstaller{GOOS: "windows", Home: `C:\\Users\\phase`, Runner: runner}).Trust(certificatePath)
+	if err == nil || len(runner.commands) != 1 {
+		t.Fatalf("malformed thumbprint err=%v commands=%v", err, runner.commands)
+	}
+}
+
+func TestUserCATrustWindowsStopsWhenObsoleteRootRemovalFails(t *testing.T) {
+	certificatePath, _, _ := writeTrustTestCA(t)
+	runner := &recordingTrustRunner{run: func(_ string, args ...string) ([]byte, error) {
+		switch {
+		case len(args) > 0 && args[0] == "-NoLogo":
+			return []byte("PHASE_CA_OBSOLETE " + strings.Repeat("AB", 20) + "\r\n"), nil
+		case len(args) > 1 && args[1] == "-delstore":
+			return []byte("CertUtil: -delstore command FAILED: 0x800704c7 (WIN32: 1223 ERROR_CANCELLED)"), errors.New("exit status 2")
+		}
+		return nil, nil
+	}}
+	_, err := (userCATrustInstaller{GOOS: "windows", Home: `C:\\Users\\phase`, Runner: runner}).Trust(certificatePath)
+	if err == nil || !strings.Contains(err.Error(), "ERROR_CANCELLED") {
+		t.Fatalf("removal failure err = %v", err)
+	}
+	for _, command := range runner.commands {
+		if len(command.Args) > 1 && command.Args[1] == "-addstore" {
+			t.Fatalf("new CA was added after an obsolete root could not be removed: %v", runner.commands)
 		}
 	}
 }
@@ -353,3 +423,19 @@ func TestUserCATrustLinuxMissingCertutilIsAMissingToolError(t *testing.T) {
 		t.Fatalf("commands ran without certutil: %v", runner.commands)
 	}
 }
+
+func TestWindowsPowerShellEnvironmentReplacesTheInheritedModulePath(t *testing.T) {
+	environment := windowsPowerShellEnvironment([]string{`PATH=C:\bin`, `PSModulePath=C:\Program Files\PowerShell\7\Modules`, `HOME=C:\Users\phase`})
+	var modulePaths []string
+	for _, entry := range environment {
+		if strings.HasPrefix(strings.ToLower(entry), "psmodulepath=") {
+			modulePaths = append(modulePaths, entry)
+		}
+	}
+	if len(modulePaths) != 1 || !strings.Contains(modulePaths[0], `\WindowsPowerShell\v1.0\Modules`) || strings.Contains(modulePaths[0], `PowerShell\7`) {
+		t.Fatalf("module path = %v", modulePaths)
+	}
+	if len(environment) != 3 {
+		t.Fatalf("environment = %v", environment)
+	}
+}