Use CertHelper on Mac instead of hardcoded PFX paths#5149
Use CertHelper on Mac instead of hardcoded PFX paths#5149DrewScoggins wants to merge 1 commit intodotnet:mainfrom
Conversation
On macOS, CertHelper now writes PFX files to ~/certs/ on disk instead of using the X509Store (Keychain), avoiding Keychain access prompts that would require code-signing the binary. Certs are pulled fresh from Key Vault via the client.pem bootstrap flow on each run. On Windows/Linux, behavior is unchanged (X509Store). Remove the Mac-specific branch in get_certificates() that read PFX files from /Users/helix-runner/certs/. CertHelper now handles all platforms uniformly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates CertHelper to avoid macOS Keychain prompts by switching macOS certificate handling to disk-based PFX files under ~/certs, while keeping Windows/Linux behavior based on X509Store. This also removes the Helix/macOS hardcoded PFX path logic from the Python helper and adds tests around loading Key Vault certs as raw bytes.
Changes:
- Add raw-bytes mode to
KeyVaultCert.LoadKeyVaultCertsAsyncand exposeKeyVaultCertificateBytes. - On macOS, write/read PFX files to/from
~/certsinstead of usingX509Store; keep blob logging + store rotation on non-macOS. - Remove macOS hardcoded
/Users/helix-runner/certs/*.pfxhandling fromscripts/performance/common.pyand add new unit tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/tools/CertHelperTests/KeyVaultCertTests.cs |
Adds coverage for raw-bytes-only loading and bootstrap-required behavior. |
src/tools/CertHelper/Program.cs |
Implements macOS disk-based PFX persistence and conditionalizes store + blob logging to non-macOS. |
src/tools/CertHelper/LocalCert.cs |
Skips Keychain/X509 store enumeration on macOS by forcing bootstrap mode. |
src/tools/CertHelper/KeyVaultCert.cs |
Adds PFX-bytes collection + OS/default-controlled X509 loading behavior. |
scripts/performance/common.py |
Simplifies cert retrieval to always execute CertHelper (removes macOS hardcoded path reads). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) | ||
| { | ||
| // Skip Keychain access on macOS to avoid password prompts. | ||
| // Certs are managed as files on disk instead. | ||
| RequiresBootstrap = true; | ||
| } | ||
| else | ||
| { | ||
| LocalMachineCerts = store ?? new TestableX509Store(); |
There was a problem hiding this comment.
LocalCert ignores the injected IX509Store on macOS because the OS check short-circuits before assigning LocalMachineCerts or calling GetLocalCerts(). This makes unit tests that pass a mocked store fail when executed on macOS, and reduces testability of the macOS code path. Consider honoring the injected store regardless of OS (or gating the macOS skip only when store is null), so tests and custom callers can still provide a store implementation.
| if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) | |
| { | |
| // Skip Keychain access on macOS to avoid password prompts. | |
| // Certs are managed as files on disk instead. | |
| RequiresBootstrap = true; | |
| } | |
| else | |
| { | |
| LocalMachineCerts = store ?? new TestableX509Store(); | |
| if (store != null) | |
| { | |
| // Honor the injected store on all platforms, including macOS. | |
| LocalMachineCerts = store; | |
| GetLocalCerts(); | |
| } | |
| else if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) | |
| { | |
| // Skip default Keychain access on macOS to avoid password prompts. | |
| // Certs are managed as files on disk instead. | |
| RequiresBootstrap = true; | |
| } | |
| else | |
| { | |
| LocalMachineCerts = new TestableX509Store(); |
| public async Task LoadKeyVaultCertsAsync(bool? rawBytesOnly = null) | ||
| { | ||
| KeyVaultCertificates.Add(await FindCertificateInKeyVaultAsync(Constants.Cert1Name)); | ||
| KeyVaultCertificates.Add(await FindCertificateInKeyVaultAsync(Constants.Cert2Name)); | ||
| bool skipX509Load = rawBytesOnly ?? RuntimeInformation.IsOSPlatform(OSPlatform.OSX); | ||
|
|
||
| if (KeyVaultCertificates.Where(c => c == null).Count() > 0) | ||
| var (cert1, bytes1) = await FindCertificateInKeyVaultAsync(Constants.Cert1Name, skipX509Load); | ||
| var (cert2, bytes2) = await FindCertificateInKeyVaultAsync(Constants.Cert2Name, skipX509Load); | ||
|
|
||
| KeyVaultCertificateBytes.Add(bytes1); | ||
| KeyVaultCertificateBytes.Add(bytes2); | ||
|
|
||
| if (!skipX509Load) | ||
| { | ||
| throw new Exception("One or more certificates not found"); | ||
| KeyVaultCertificates.Add(cert1!); | ||
| KeyVaultCertificates.Add(cert2!); | ||
|
|
||
| if (KeyVaultCertificates.Where(c => c == null).Count() > 0) | ||
| { | ||
| throw new Exception("One or more certificates not found"); | ||
| } | ||
| } |
There was a problem hiding this comment.
LoadKeyVaultCertsAsync appends into KeyVaultCertificateBytes/KeyVaultCertificates without clearing them first. If the method is called more than once on the same KeyVaultCert instance, the collections will accumulate duplicates and ShouldRotateCerts() comparisons (counts/thumbprints) can become incorrect. Clear the collections (or create fresh ones) at the start of LoadKeyVaultCertsAsync.
|
|
||
| static void WriteCertsToDisk(List<byte[]> certBytes) | ||
| { | ||
| var certDir = GetMacCertDirectory(); | ||
| Directory.CreateDirectory(certDir); | ||
|
|
||
| var certNames = new[] { Constants.Cert1Name, Constants.Cert2Name }; | ||
| for (int i = 0; i < certBytes.Count && i < certNames.Length; i++) | ||
| { | ||
| var pfxPath = Path.Combine(certDir, $"{certNames[i]}.pfx"); | ||
| File.WriteAllBytes(pfxPath, certBytes[i]); |
There was a problem hiding this comment.
WriteCertsToDisk writes PFX files (including private keys) to disk but doesn’t set restrictive permissions on the directory/files. On macOS this can leave private key material readable by other users depending on umask/defaults. Consider setting the directory to user-only and setting each .pfx file mode to 0600 (or equivalent) after writing.
| static void WriteCertsToDisk(List<byte[]> certBytes) | |
| { | |
| var certDir = GetMacCertDirectory(); | |
| Directory.CreateDirectory(certDir); | |
| var certNames = new[] { Constants.Cert1Name, Constants.Cert2Name }; | |
| for (int i = 0; i < certBytes.Count && i < certNames.Length; i++) | |
| { | |
| var pfxPath = Path.Combine(certDir, $"{certNames[i]}.pfx"); | |
| File.WriteAllBytes(pfxPath, certBytes[i]); | |
| static void SetMacPermissions(string path, string mode) | |
| { | |
| if (!RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) | |
| { | |
| return; | |
| } | |
| try | |
| { | |
| var psi = new System.Diagnostics.ProcessStartInfo | |
| { | |
| FileName = "/bin/chmod", | |
| RedirectStandardOutput = false, | |
| RedirectStandardError = false, | |
| UseShellExecute = false, | |
| CreateNoWindow = true, | |
| }; | |
| psi.ArgumentList.Add(mode); | |
| psi.ArgumentList.Add(path); | |
| using var process = System.Diagnostics.Process.Start(psi); | |
| process?.WaitForExit(); | |
| } | |
| catch | |
| { | |
| // Best-effort: ignore failures to adjust permissions. | |
| } | |
| } | |
| static void WriteCertsToDisk(List<byte[]> certBytes) | |
| { | |
| var certDir = GetMacCertDirectory(); | |
| Directory.CreateDirectory(certDir); | |
| // Ensure the certificate directory is only accessible by the current user on macOS. | |
| SetMacPermissions(certDir, "700"); | |
| var certNames = new[] { Constants.Cert1Name, Constants.Cert2Name }; | |
| for (int i = 0; i < certBytes.Count && i < certNames.Length; i++) | |
| { | |
| var pfxPath = Path.Combine(certDir, $"{certNames[i]}.pfx"); | |
| File.WriteAllBytes(pfxPath, certBytes[i]); | |
| // Restrict certificate file permissions to user read/write on macOS. | |
| SetMacPermissions(pfxPath, "600"); |
| var certNames = new[] { Constants.Cert1Name, Constants.Cert2Name }; | ||
| for (int i = 0; i < certBytes.Count && i < certNames.Length; i++) | ||
| { | ||
| var pfxPath = Path.Combine(certDir, $"{certNames[i]}.pfx"); | ||
| File.WriteAllBytes(pfxPath, certBytes[i]); | ||
| Console.Error.WriteLine($"Wrote certificate to {pfxPath}"); | ||
| } |
There was a problem hiding this comment.
WriteCertsToDisk silently truncates to the smaller of certBytes.Count and the fixed certNames.Length (2). If KeyVaultCertificateBytes is unexpectedly missing a cert (or contains extra entries), the tool will succeed but output an incomplete/mismatched set of files. Consider validating that the expected number of certs is present and failing fast if it isn’t.
| foreach (var certName in new[] { Constants.Cert1Name, Constants.Cert2Name }) | ||
| { | ||
| var pfxPath = Path.Combine(certDir, $"{certName}.pfx"); | ||
| if (File.Exists(pfxPath)) | ||
| { | ||
| Console.WriteLine(Convert.ToBase64String(File.ReadAllBytes(pfxPath))); | ||
| } | ||
| else | ||
| { | ||
| Console.Error.WriteLine($"Certificate file not found: {pfxPath}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
ReadCertsFromDisk only logs to stderr when a certificate file is missing and the process still exits 0, which causes callers (scripts/performance/common.py) to treat the run as successful but receive an incomplete cert list. Consider failing with a non-zero exit code (or otherwise signaling failure) when any expected PFX file is missing.
| var currentKeyValutCertThumbprints = ""; | ||
| foreach (var cert in kvc.KeyVaultCertificates) | ||
| { | ||
| currentKeyValutCertThumbprints += $"[{DateTimeOffset.UtcNow}] {cert.Thumbprint}{Environment.NewLine}"; | ||
| } | ||
| var blob = bcc.GetBlobClient(System.Environment.MachineName); | ||
| if (blob.Exists()) | ||
| { | ||
| var result = blob.DownloadContent(); | ||
| var currentBlob = result.Value.Content.ToString(); | ||
| currentBlob = currentBlob + currentKeyValutCertThumbprints; | ||
| blob.Upload(new MemoryStream(Encoding.UTF8.GetBytes(currentBlob)), overwrite: true); | ||
| } | ||
| else | ||
| { | ||
| blob.Upload(new MemoryStream(Encoding.UTF8.GetBytes(currentKeyValutCertThumbprints)), overwrite: false); |
There was a problem hiding this comment.
Typo in variable name: currentKeyValutCertThumbprints should be currentKeyVaultCertThumbprints for clarity and to match the KeyVault naming used elsewhere.
| var currentKeyValutCertThumbprints = ""; | |
| foreach (var cert in kvc.KeyVaultCertificates) | |
| { | |
| currentKeyValutCertThumbprints += $"[{DateTimeOffset.UtcNow}] {cert.Thumbprint}{Environment.NewLine}"; | |
| } | |
| var blob = bcc.GetBlobClient(System.Environment.MachineName); | |
| if (blob.Exists()) | |
| { | |
| var result = blob.DownloadContent(); | |
| var currentBlob = result.Value.Content.ToString(); | |
| currentBlob = currentBlob + currentKeyValutCertThumbprints; | |
| blob.Upload(new MemoryStream(Encoding.UTF8.GetBytes(currentBlob)), overwrite: true); | |
| } | |
| else | |
| { | |
| blob.Upload(new MemoryStream(Encoding.UTF8.GetBytes(currentKeyValutCertThumbprints)), overwrite: false); | |
| var currentKeyVaultCertThumbprints = ""; | |
| foreach (var cert in kvc.KeyVaultCertificates) | |
| { | |
| currentKeyVaultCertThumbprints += $"[{DateTimeOffset.UtcNow}] {cert.Thumbprint}{Environment.NewLine}"; | |
| } | |
| var blob = bcc.GetBlobClient(System.Environment.MachineName); | |
| if (blob.Exists()) | |
| { | |
| var result = blob.DownloadContent(); | |
| var currentBlob = result.Value.Content.ToString(); | |
| currentBlob = currentBlob + currentKeyVaultCertThumbprints; | |
| blob.Upload(new MemoryStream(Encoding.UTF8.GetBytes(currentBlob)), overwrite: true); | |
| } | |
| else | |
| { | |
| blob.Upload(new MemoryStream(Encoding.UTF8.GetBytes(currentKeyVaultCertThumbprints)), overwrite: false); |
| { | ||
| KeyVaultCertificates.Add(await FindCertificateInKeyVaultAsync(Constants.Cert1Name)); | ||
| KeyVaultCertificates.Add(await FindCertificateInKeyVaultAsync(Constants.Cert2Name)); | ||
| bool skipX509Load = rawBytesOnly ?? RuntimeInformation.IsOSPlatform(OSPlatform.OSX); |
There was a problem hiding this comment.
LoadKeyVaultCertsAsync’s default behavior now depends on the current OS (macOS defaults to skipping X509 loading). This makes the method’s semantics non-deterministic across platforms and can cause unit tests/callers that expect KeyVaultCertificates to be populated (when calling with no arguments) to behave differently on macOS. Consider making the default consistent (e.g., always load X509) and having the caller explicitly request raw-bytes-only mode where needed.
| bool skipX509Load = rawBytesOnly ?? RuntimeInformation.IsOSPlatform(OSPlatform.OSX); | |
| bool skipX509Load = rawBytesOnly ?? false; |
On macOS, CertHelper now writes PFX files to ~/certs/ on disk instead of using the X509Store (Keychain), avoiding Keychain access prompts that would require code-signing the binary. Certs are pulled fresh from Key Vault via the client.pem bootstrap flow on each run.
On Windows/Linux, behavior is unchanged (X509Store).
Remove the Mac-specific branch in get_certificates() that read PFX files from /Users/helix-runner/certs/. CertHelper now handles all platforms uniformly.