fix(windows): import WireServer certs into trust stores - #9353
Merged
Sylvain Boily (djsly) merged 3 commits intoSep 1, 2026
Merged
Conversation
Install certificates downloaded by Get-CACertificates into the LocalMachine certificate stores instead of leaving them only under C:\ca. Legacy endpoint certificates and rcv1p root certificates are added to the Root store, while rcv1p intermediate certificates are added to the CA store to preserve the correct trust-chain semantics. Use ErrorAction Stop so import failures follow the existing Get-CACertificates error handling and fail provisioning when FailOnError is requested. Extend the focused Pester coverage to verify the file paths, destination stores, and terminating error behavior passed to Import-Certificate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Ramkumar Chinchani (rchincha)
September 1, 2026 00:24
View session
Contributor
Windows Unit Test Results 3 files 13 suites 51s ⏱️ Results for commit 4092b42. ♻️ This comment has been updated with latest results. |
Contributor
There was a problem hiding this comment.
Pull request overview
Imports WireServer certificates into Windows LocalMachine trust stores.
Changes:
- Imports legacy/root certificates into
Root. - Imports intermediate certificates into
CA. - Adds Pester assertions for paths, stores, and error behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
staging/cse/windows/kubernetesfunc.ps1 |
Adds certificate-store imports. |
staging/cse/windows/kubernetesfunc.tests.ps1 |
Tests import parameters. |
Suppressed comments (1)
staging/cse/windows/kubernetesfunc.ps1:450
- 🔴 High Risk — 🔧 Script Logic: This rcv1p path has the same Windows PowerShell 5.1 encoding problem:
>serializes the response string as UTF-16LE, then the newly added import attempts to parse it as a certificate. Use an explicit ASCII/UTF-8 write so root and intermediate imports receive valid PEM/base64 bytes.
Import-Certificate -FilePath $certFilePath -CertStoreLocation $certStoreLocation -ErrorAction Stop | Out-Null
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Write legacy and rcv1p WireServer certificate bodies with explicit ASCII encoding before passing them to Import-Certificate. This prevents Windows PowerShell 5.1 redirection from producing UTF-16LE files that the certificate parser cannot import. Strengthen unit coverage with byte-level encoding assertions and update the Windows rcv1p e2e validator to parse each downloaded certificate and verify its thumbprint exists in the LocalMachine Root or CA store. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Ramkumar Chinchani (rchincha)
requested review from
Devinwong,
Sri Harsha (SriHarsha001),
Abdul Asfari (aboodasfari),
Calvin S. (calvin197),
fcher,
Ganeshkumar Ashokavardhanan (ganeshkumarashok),
Karen Chen (karenychen),
lilypan26,
Mark Ibrahim (mxj220),
Peter Damianov (pdamianov-dev),
Patrick W. Healy (phealy),
Runzhen (runzhen),
Shun Lyu (sinmentis),
sulixu,
Thibault Cohen (titilambert),
Xu Xue (xuexu6666) and
Zachary (zachary-bailey)
as code owners
September 1, 2026 00:52
Ramkumar Chinchani (rchincha)
requested review from
Abigail Liang (abigailliang-aks-sig-node) and
janenotjung-hue
as code owners
September 1, 2026 00:52
Copilot started reviewing on behalf of
Ramkumar Chinchani (rchincha)
September 1, 2026 00:53
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
staging/cse/windows/kubernetesfunc.ps1:451
- 🟡 Medium Risk — Any rcv1p import failure is handled by the function-level catch as
Failed to retrieve CA certificates (HTTP N/A), even though retrieval succeeded. This makes certificate parsing and LocalMachine store failures appear to be network failures in CSE diagnostics. Preserve import-stage context or make the shared error message cover both retrieval and installation.
Import-Certificate -FilePath $certFilePath -CertStoreLocation $certStoreLocation -ErrorAction Stop | Out-Null
Wrap each WireServer certificate import with certificate and destination-store context so malformed input and certificate-store permission failures are actionable during provisioning. Rename the shared failure message from retrieval to processing because the handler covers download, parsing, file writing, and trust-store import. Add focused Pester coverage for both retrieval and import diagnostics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Ramkumar Chinchani (rchincha)
September 1, 2026 00:58
View session
| $downloadedAny = $false | ||
|
|
||
| foreach ($requestType in $operationRequestTypes) { | ||
| # Keep intermediates out of the trusted root store while making both chains system-wide. |
Contributor
There was a problem hiding this comment.
How does this keep intermediates out of the trusted root store?
Contributor
Author
There was a problem hiding this comment.
Wireserver returns both root certs and intermediate certs
In Windows, the following is true (iinm):
Root store: Cert:\LocalMachine\Root (Trusted Root Certification Authorities)
Intermediate store: Cert:\LocalMachine\CA (Intermediate Certification Authorities)
So intermediate certs are kept of out Cert:\LocalMachine\Root
That's all the comment is indicating.
Tim Wright (timmy-wright)
approved these changes
Sep 1, 2026
Contributor
Author
|
There appears to be a legitimate e2e failure. |
Ramkumar Chinchani (rchincha)
marked this pull request as draft
September 1, 2026 14:22
Ramkumar Chinchani (rchincha)
marked this pull request as ready for review
September 1, 2026 18:49
Contributor
Author
After turning on KEEP_VMSS=true, logs show the cert import is fine and the kubelet error is due to the issue the following PR is trying to fix |
Sylvain Boily (djsly)
enabled auto-merge (squash)
September 1, 2026 19:25
Sylvain Boily (djsly)
approved these changes
Sep 1, 2026
Sylvain Boily (djsly)
deleted the
rchinchani/import-wireserver-certs-to-system-trust
branch
September 1, 2026 22:48
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Install certificates downloaded by Get-CACertificates into the LocalMachine certificate stores instead of leaving them only under C:\ca. Legacy endpoint certificates and rcv1p root certificates are added to the Root store, while rcv1p intermediate certificates are added to the CA store to preserve the correct trust-chain semantics.
Use ErrorAction Stop so import failures follow the existing Get-CACertificates error handling and fail provisioning when FailOnError is requested. Extend the focused Pester coverage to verify the file paths, destination stores, and terminating error behavior passed to Import-Certificate.
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #