Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable Unpacking and Repacking of .pkg files and .app bundles #15206

Draft
wants to merge 52 commits into
base: main
Choose a base branch
from

Conversation

ellahathaway
Copy link
Member

@ellahathaway ellahathaway commented Oct 29, 2024

Related to #14438

This is the second part of #14438. This PR integrates the pkg tool, introduced in #15205, into SignTool. More specifically, this PR enables the unpacking and repacking of .pkgs and nested .app bundles within SignTool & adds tests & test data for this functionality into SignTool.Tests.

An important change here is that .pkg was removed as a signable extension. This change is needed avoid errors related to .pkgs being considered "not signed" now that they're included as a FileExtensionSignInfo item. Otherwise, this part of the code is hit, and the signing fails. .pkgs will be added back as a signable extension with #14435.

An alternative choice to above is to still allow the pkg and .apps to be signable files. This choice would invole adjusting this PR to include the signing validation of the pkg and .app as part of this PR.

Leaving as a draft until #15205 is merged.

@ellahathaway ellahathaway changed the title Integrate Pkg Tool into SignTool Enable Unpacking and Repacking of .pkg files and .app bundles Oct 29, 2024
ellahathaway added a commit to ellahathaway/arcade that referenced this pull request Oct 31, 2024
@mmitche mmitche marked this pull request as ready for review November 22, 2024 18:22
<FileExtensionSignInfo Condition="$([MSBuild]::IsOSPlatform('OSX'))" Include=".pkg" CertificateName="None" />
<!-- .app bundles are technically directories, but the Microsoft.DotNet.MacOsPkg
tool packs these bundles into zips when unpacking .pkgs -->
<FileExtensionSignInfo Condition="$([MSBuild]::IsOSPlatform('OSX'))" Include=".app" CertificateName="None" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave out the platform checks. This leaves the possibility someone assumes MacOS .pkgs will get signed properly on Windows, but instead they get ignored. Instead, the pack/unpack process should error out (which it will, since the tool won't work). Then we rely on teams not specifying that .pkgs should be signed on non-Mac platforms.

});

// OSX files need to be zipped first before being signed
// This is why the .pkgs and .apps are listed as .zip files below
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment currently out of date?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it, yes. This comment applies to the changes in ellahathaway@cd090b8

// unpacking, repacking, and notarization can only happen on a Mac.
internal static bool IsPkg(string path)
=> Path.GetExtension(path).Equals(".pkg", StringComparison.OrdinalIgnoreCase)
&& RuntimeInformation.IsOSPlatform(OSPlatform.OSX);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave out these platform checks, and instead let the pack/unpack fail on non-Mac

<FileExtensionSignInfo Condition="$([MSBuild]::IsOSPlatform('OSX'))" Include=".pkg" CertificateName="None" />
<!-- .app bundles are technically directories, but the Microsoft.DotNet.MacOsPkg
tool packs these bundles into zips when unpacking .pkgs -->
<FileExtensionSignInfo Condition="$([MSBuild]::IsOSPlatform('OSX'))" Include=".app" CertificateName="None" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't understand the .app file extension sign info. The .app doesn't get signed itself, so I don't think it should have an entry here.

mmitche and others added 28 commits December 6, 2024 11:08
…ation' into replace-sn-with-custom-implementation
@mmitche mmitche marked this pull request as draft December 18, 2024 19:22
@mmitche
Copy link
Member

mmitche commented Dec 18, 2024

Converting back to a draft and merging in the latest changes for OSX and strong name signing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants