Sign internals of EXE package so that it works correctly when signed #15132

TravisEz13 · 2021-03-31T21:53:08Z

PR Summary

Sign internals of EXE package so that it works correctly when signed

Refactor exe package creation into separate function
Add functions to enable signing of engine of the exe package
Move signing of MSI to package creation Job

PR Context

The extracted EXE and MSI of the wrapper must be signed if the EXE is signed or install will fail.

PR Checklist

calculate product name suffix

fix engine signing

rjmholt

I'd like to get someone who understand this better to review it as well if I can. @adityapatwardhan, @anmenaga or possibly @andschwa.

rjmholt · 2021-04-05T18:45:06Z

tools/releaseBuild/azureDevOps/templates/windows-packaging.yml

+        New-ExePackage -ProductVersion '$(version)' -MsiLocationPath $msiPath -ProductTargetArchitecture ${{ parameters.architecture }}
+        $exePath = Get-ChildItem '.\PowerShell-*.exe' | Select-Object -First 1 -ExpandProperty fullname
+        $enginePath = Join-Path -Path '$(System.ArtifactsDirectory)\unsignedEngine' -ChildPath engine.exe
+        Dismount-ExePackageEngine -ExePath $exePath -EnginePath $enginePath


I'm mostly not clear on the Mount/Dismount-ExePackageEngine uses here:

Why is it Mount/Dismount? What does it leave on the filesystem?

Why do we dismount in one task and then mount later in another?

Are we depending on filesystem state between tasks?

It's more like an archive.... so perhaps... Expand/Compress

That makes some sense. What is exe package engine? What does each command do? Can we add comments that contain the answers to those questions and ideally the questions above?

The engine of the exe is the installer engine provided by WiX

Ok having read a bit about insignia, here are my recommendations:

Rename the functions to Disconnect-/Connect-ExePackageEngine

Add a comment in the function definitions like Needed to detach the WIX Burn engine from the exe for signing and Needed to reattach the WIX Burn engine to the exe after signing

Add a comment in the YAML-embedded script where we detach saying Detach the packaging engine so we can sign the exe

Add a similar comment when we re-attach saying Re-attach the packaging engine to the now-signed exe so we can create the MSI

It's not clear from this file why we need to re-attach Burn at the end though. We seem to do the following:

sign the exe

re-attach Burn to the exe

upload it (but the display name says unsigned??)

I don't follow this sequence.

tools/releaseBuild/azureDevOps/templates/windows-packaging.yml

rjmholt · 2021-04-06T21:33:02Z

tools/releaseBuild/azureDevOps/templates/windows-packaging.yml

+        New-ExePackage -ProductVersion '$(version)' -MsiLocationPath $msiPath -ProductTargetArchitecture ${{ parameters.architecture }}
+        $exePath = Get-ChildItem '.\PowerShell-*.exe' | Select-Object -First 1 -ExpandProperty fullname
+        $enginePath = Join-Path -Path '$(System.ArtifactsDirectory)\unsignedEngine' -ChildPath engine.exe
+        Dismount-ExePackageEngine -ExePath $exePath -EnginePath $enginePath


Ok having read a bit about insignia, here are my recommendations:

Rename the functions to Disconnect-/Connect-ExePackageEngine

Add a comment in the function definitions like Needed to detach the WIX Burn engine from the exe for signing and Needed to reattach the WIX Burn engine to the exe after signing

Add a comment in the YAML-embedded script where we detach saying Detach the packaging engine so we can sign the exe

Add a similar comment when we re-attach saying Re-attach the packaging engine to the now-signed exe so we can create the MSI

It's not clear from this file why we need to re-attach Burn at the end though. We seem to do the following:

sign the exe

re-attach Burn to the exe

upload it (but the display name says unsigned??)

I don't follow this sequence.

Co-authored-by: Robert Holt <rjmholt@gmail.com>

TravisEz13 · 2021-04-07T22:21:17Z

disconnect/connect are not data verbs

tools/releaseBuild/azureDevOps/templates/windows-packaging.yml

ghost · 2021-04-14T23:58:08Z

🎉v7.2.0-preview.5 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

@davidBar-On

[7.2.0-preview.5] - 2021-04-14 * Breaking Changes - Make PowerShell Linux deb and RPM packages universal (#15109) - Enforce AppLocker Deny configuration before Execution Policy Bypass configuration (#15035) - Disallow mixed dash and slash in command line parameter prefix (#15142) (Thanks @davidBar-On!) * Experimental Features - `PSNativeCommandArgumentPassing`: Use `ArgumentList` for native executable invocation (breaking change) (#14692) * Engine Updates and Fixes - Add `IArgumentCompleterFactory` for parameterized `ArgumentCompleters` (#12605) (Thanks @powercode!) * General Cmdlet Updates and Fixes - Fix SSH remoting connection never finishing with misconfigured endpoint (#15175) - Respect `TERM` and `NO_COLOR` environment variables for `$PSStyle` rendering (#14969) - Use `ProgressView.Classic` when Virtual Terminal is not supported (#15048) - Fix `Get-Counter` issue with `-Computer` parameter (#15166) (Thanks @krishnayalavarthi!) - Fix redundant iteration while splitting lines (#14851) (Thanks @hez2010!) - Enhance `Remove-Item -Recurse` to work with OneDrive (#14902) (Thanks @iSazonov!) - Change minimum depth to 0 for `ConvertTo-Json` (#14830) (Thanks @kvprasoon!) - Allow `Set-Clipboard` to accept empty string (#14579) - Turn on and off `DECCKM` to modify keyboard mode for Unix native commands to work correctly (#14943) - Fall back to `CopyAndDelete()` when `MoveTo()` fails due to an `IOException` (#15077) * Code Cleanup <details> <summary> We thank the following contributors! @xtqqczze, @iSazonov, @ZhiZe-ZG </summary> <ul> <li>Update .NET to <code>6.0.0-preview.3</code> (#15221)</li> <li>Add space before comma to hosting test to fix error reported by <code>SA1001</code> (#15224)</li> <li>Add <code>SecureStringHelper.FromPlainTextString</code> helper method for efficient secure string creation (#14124) (Thanks @xtqqczze!)</li> <li>Use static lambda keyword (#15154) (Thanks @iSazonov!)</li> <li>Remove unnecessary <code>Array</code> -> <code>List</code> -> <code>Array</code> conversion in <code>ProcessBaseCommand.AllProcesses</code> (#15052) (Thanks @xtqqczze!)</li> <li>Standardize grammar comments in Parser.cs (#15114) (Thanks @ZhiZe-ZG!)</li> <li>Enable <code>SA1001</code>: Commas should be spaced correctly (#14171) (Thanks @xtqqczze!)</li> <li>Refactor <code>MultipleServiceCommandBase.AllServices</code> (#15053) (Thanks @xtqqczze!)</li> </ul> </details> * Tools - Use Unix line endings for shell scripts (#15180) (Thanks @xtqqczze!) * Tests - Add the missing tag in Host Utilities tests (#14983) - Update `copy-props` version in `package.json` (#15124) * Build and Packaging Improvements <details> <summary> We thank the following contributors! @JustinGrote </summary> <ul> <li>Fix <code>yarn-lock</code> for <code>copy-props</code> (#15225)</li> <li>Make package validation regex accept universal Linux packages (#15226)</li> <li>Bump NJsonSchema from 10.4.0 to 10.4.1 (#15190)</li> <li>Make MSI and EXE signing always copy to fix daily build (#15191)</li> <li>Sign internals of EXE package so that it works correctly when signed (#15132)</li> <li>Bump Microsoft.NET.Test.Sdk from 16.9.1 to 16.9.4 (#15141)</li> <li>Update daily release tag format to work with new Microsoft Update work (#15164)</li> <li>Feature: Add Ubuntu 20.04 Support to install-powershell.sh (#15095) (Thanks @JustinGrote!)</li> <li>Treat rebuild branches like release branches (#15099)</li> <li>Update WiX to 3.11.2 (#15097)</li> <li>Bump NJsonSchema from 10.3.11 to 10.4.0 (#15092)</li> <li>Allow patching of preview releases (#15074)</li> <li>Bump Newtonsoft.Json from 12.0.3 to 13.0.1 (#15084, #15085)</li> <li>Update the <code>minSize</code> build package filter to be explicit (#15055)</li> <li>Bump NJsonSchema from 10.3.10 to 10.3.11 (#14965)</li> </ul> </details> * Documentation and Help Content - Merge `7.2.0-preview.4` changes to master (#15056) - Update `README` and `metadata.json` (#15046) - Fix broken links for `dotnet` CLI (#14937) [7.2.0-preview.5]: v7.2.0-preview.4...v7.2.0-preview.5

ghost assigned rjmholt Mar 31, 2021

TravisEz13 force-pushed the rebuild/v7.2.0-rebuild.4 branch from 8bcd0f7 to 40ab073 Compare April 1, 2021 18:12

TravisEz13 added 15 commits April 1, 2021 12:46

refactor exe wrapper generation into separate function

634ddd5

generate exe wrapper after msi is signed

53d3d0f

don't sign msi again

93e4684

don't upload msi agin

cbe5f00

fix version

3351bba

fix working directory for exe build

3d82cbf

add logging for getting windows names

9d2d984

calculate product name suffix

add signing task display name

a804c47

sign engine exe in exe wrapper

bbde77f

fix typo

a3bb634

fix engine signing

fix typo

4ff5f6e

go back to master for compliance repo

1b10180

test engine mounted in exe wrapper

4b8bc55

exclude packaging from future windows CI builds

94ac51f

fix package CI

f559332

TravisEz13 force-pushed the rebuild/v7.2.0-rebuild.4 branch from 40ab073 to f559332 Compare April 1, 2021 19:47

TravisEz13 marked this pull request as ready for review April 1, 2021 19:49

TravisEz13 requested review from adityapatwardhan, anmenaga and daxian-dbw as code owners April 1, 2021 19:49

rjmholt reviewed Apr 5, 2021

View reviewed changes

rjmholt reviewed Apr 6, 2021

View reviewed changes

TravisEz13 and others added 5 commits April 7, 2021 15:12

change function names

89cb3db

Update tools/releaseBuild/azureDevOps/templates/windows-packaging.yml

df63723

Co-authored-by: Robert Holt <rjmholt@gmail.com>

Update tools/releaseBuild/azureDevOps/templates/windows-packaging.yml

18b7f75

Co-authored-by: Robert Holt <rjmholt@gmail.com>

Update tools/releaseBuild/azureDevOps/templates/windows-packaging.yml

6f884c5

Co-authored-by: Robert Holt <rjmholt@gmail.com>

Update tools/releaseBuild/azureDevOps/templates/windows-packaging.yml

953bb7d

Co-authored-by: Robert Holt <rjmholt@gmail.com>

adityapatwardhan approved these changes Apr 8, 2021

View reviewed changes

tools/releaseBuild/azureDevOps/templates/windows-packaging.yml Outdated Show resolved Hide resolved

TravisEz13 commented Apr 8, 2021

View reviewed changes

tools/releaseBuild/azureDevOps/templates/windows-packaging.yml Outdated Show resolved Hide resolved

TravisEz13 and others added 3 commits April 8, 2021 15:42

Apply suggestions from code review

24da08b

add comments per review comments

a22dbdd

fix replace

042ae12

TravisEz13 merged commit 75dc913 into PowerShell:master Apr 9, 2021

TravisEz13 deleted the rebuild/v7.2.0-rebuild.4 branch April 9, 2021 17:26

TravisEz13 added the CL-BuildPackaging Indicates that a PR should be marked as a build or packaging change in the Change Log label Apr 9, 2021

TravisEz13 added this to the 7.2.0-preview.5 milestone Apr 9, 2021

Sign internals of EXE package so that it works correctly when signed #15132

Sign internals of EXE package so that it works correctly when signed #15132

Uh oh!

Conversation

TravisEz13 commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

rjmholt left a comment

Choose a reason for hiding this comment

Uh oh!

rjmholt Apr 5, 2021

Choose a reason for hiding this comment

Uh oh!

TravisEz13 Apr 5, 2021

Choose a reason for hiding this comment

Uh oh!

rjmholt Apr 5, 2021

Choose a reason for hiding this comment

Uh oh!

TravisEz13 Apr 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjmholt Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rjmholt Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

TravisEz13 commented Apr 7, 2021

Uh oh!

Uh oh!

Uh oh!

ghost commented Apr 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TravisEz13 commented Mar 31, 2021 •

edited

Loading

TravisEz13 Apr 6, 2021 •

edited

Loading