installer icon indicating copy to clipboard operation
installer copied to clipboard

Evaluate usefulness and correctness of ValidateBundledManifestSigning target

Open mmitche opened this issue 2 years ago • 3 comments

This target is supposed to validate the signing of bundled manifests. But, it is disabled for workloads that are post-build signed. I think this is probably largely a holdover from 6.0. Now nothing is post-build signed, but we don't sign some workloads out of the main branch, so removing the exclusions for emscripten in the target would simply cause the build to fail.

https://github.com/dotnet/installer/blob/0b82a5aa64ad639184b1da058a7f11323d89a031/src/redist/targets/BundledManifests.targets#L54-L89

On the other hand if the exclusion is always there, and signing is done in release branches, then we're never checking emscripten's signing. We do end up checking Maui (they always sign), but Aspire does not.

Is this target still valuable? Should it be changed somehow?

/cc @joeloff

mmitche avatar Jan 10 '24 21:01 mmitche

@joperezr

mmitche avatar Jan 10 '24 21:01 mmitche

I believe that the exclusion list is leftover from the post-build signing days but as you noted, still applies to main since we don't sign main builds of runtime/emsdk.

I guess the question is, should we have any signing validation here? I believe Jacques added it after we had issues in VS with signed workloads in the early days that weren't caught with the existing arcade sign checking. We haven't had those issues recently.

Is the check bothersome or slow such that we'd want to get rid of it? I can update the comment if we want to keep it.

marcpopMSFT avatar Jan 24 '24 21:01 marcpopMSFT

Correct. We had manifests from runtime flow through in 6.0 to installer, then when we staged and inserted into Visual Studio we'd trip PR checks because the runtime manifest wasn't signed, which then requires resetting the whole build of 6.0.

joeloff avatar Feb 20 '24 21:02 joeloff