Fix signing for aspire-managed bundle payload#15990
Conversation
Ensure the managed bundle payload participates in the same signing flow as aspire.exe by forwarding signing properties through Bundle.proj, registering aspire-managed for signing, and adding a CI verification step for the Windows bundle payload.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15990Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15990" |
There was a problem hiding this comment.
Pull request overview
This PR closes a signing gap in the Aspire CLI bundle flow by ensuring the aspire-managed payload participates in the same signing pipeline as the native CLI, and by adding CI validation to prevent unsigned payloads from slipping into the bundle.
Changes:
- Register
aspire-managedbinaries for signing ineng/Signing.props. - Plumb CI/signing MSBuild properties through
eng/Bundle.proj(and local build scripts) so the managed publish step receives signing configuration. - Add a Windows CI check that validates
aspire-managed.exeis signed after publishing the bundle payload.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| eng/Signing.props | Adds aspire-managed to the list of files eligible for signing across OSes. |
| eng/Bundle.proj | Forwards CI/signing/version properties into dotnet publish invocations so managed payload publish can be signed. |
| eng/build.sh | Passes signing/MSBuild property args through to Bundle.proj when --bundle is used. |
| eng/build.ps1 | Passes signing/MSBuild property args through to Bundle.proj when -bundle is used. |
| eng/pipelines/templates/BuildAndTest.yml | Adds signing/property args to the bundle payload build and introduces a signature validation step on Windows. |
| eng/pipelines/templates/build_sign_native.yml | Adds extraBuildArgs passthrough to the bundle payload build step. |
|
|
||
| Write-Host "✅ aspire-managed.exe signature status: $($signature.Status)" | ||
| displayName: 🟣Verify managed bundle signature (${{ targetRid }}) | ||
| condition: and(succeeded(), eq(variables['Agent.OS'], 'Windows_NT'), in(variables['_SignType'], 'real', 'test'), startsWith('${{ targetRid }}', 'win-')) |
There was a problem hiding this comment.
The signature verification step is likely to run on PR builds where signing is disabled (e.g., _Sign is false and _SignArgs is empty). In that case aspire-managed.exe will legitimately be unsigned and this step will fail the pipeline. Update the condition to also require that signing is actually enabled (for example by checking variables['_Sign'] or that _SignArgs is non-empty).
| condition: and(succeeded(), eq(variables['Agent.OS'], 'Windows_NT'), in(variables['_SignType'], 'real', 'test'), startsWith('${{ targetRid }}', 'win-')) | |
| condition: and(succeeded(), eq(variables['Agent.OS'], 'Windows_NT'), eq(variables['_Sign'], 'true'), in(variables['_SignType'], 'real', 'test'), startsWith('${{ targetRid }}', 'win-')) |
| if ($signature.Status -eq [System.Management.Automation.SignatureStatus]::NotSigned) { | ||
| Write-Host "##[error]aspire-managed.exe is not signed: $managedPath" |
There was a problem hiding this comment.
Get-AuthenticodeSignature can return statuses other than NotSigned (e.g., HashMismatch, NotTrusted, UnknownError). The current check only fails on NotSigned, so an invalid or broken signature could still pass. Consider failing unless the status is Valid (or explicitly allow only the statuses you consider acceptable).
| if ($signature.Status -eq [System.Management.Automation.SignatureStatus]::NotSigned) { | |
| Write-Host "##[error]aspire-managed.exe is not signed: $managedPath" | |
| if ($signature.Status -ne [System.Management.Automation.SignatureStatus]::Valid) { | |
| Write-Host "##[error]aspire-managed.exe has an invalid signature status '$($signature.Status)': $managedPath" |
|
How can we add an automated test to verify they're signed? What about an E2E console test that runs tooling to check executables signing status? |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
E2E tests don’t run on the signed builds. Let’s talk about making that possible first (adding a pipeline that tests the fully signed internally built bits). Manual verification is fine for now. |
|
We only sign on the AzDO pipelines side of the fence so feeding signed bits from that side into the GitHub actions side where the E2E CLI tests are is going to be challenging. |
Gate the new bundle signature validation on signing actually being enabled and allow the expected test-signing status while still rejecting missing or broken signatures.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed the remaining actionable feedback in
|
|
🎬 CLI E2E Test Recordings — 56 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #24170081536 |
| /bl:${{ parameters.repoLogPath }}/BundlePayload-${{ targetRid }}.binlog | ||
| displayName: 🟣Build bundle payload (${{ targetRid }}) | ||
|
|
||
| - pwsh: | |
There was a problem hiding this comment.
Why are we trying to verify the signed status of the aspire-managed.exe, but not any other files? Seems odd that we'd do that here.
Fixes #15989
Description
Fix the CLI bundle signing gap where
aspire-managed.execould miss the same signing path asaspire.exe.Root cause: the managed bundle payload is published earlier via
eng/Bundle.proj, while the later native CLI build receives the usual signing properties. On top of that,eng/Signing.propsonly registeredaspire.exe/aspire, so the bundle could preserve an unsignedaspire-managed.exe.Changes
aspire-managed.exe/aspire-managedineng/Signing.propseng/Bundle.projso the managed payload publish participates in the same signing flowbuild.ps1/build.sh --bundlepathBuildAndTest.ymlthat fails if the produced bundle payload contains an unsignedaspire-managed.exeChecklist
<remarks />and<code />elements on your triple slash comments?aspire.devissue: