[msbuild] Rework code signing. by rolfbjarne · Pull Request #14387 · dotnet/macios

rolfbjarne · 2022-03-11T17:15:35Z

The main theme here is that code signing will be done in the outermost
executable project, not in any app extension projects or watch projects, nor
during the RID-specific build of a .NET universal app. This makes codesigning
easier to reason about and other affected logic (such as strip/dsymutil)
easier to handle, in particular for .NET universal apps. Another benefit is
that the differences between the iOS and macOS code bases have been
eliminated.

The first step is to collect all the information we need from the targets
files. Every app bundle (be it app extension, watch app or main app) will add
its own output app bundle (.app/.appex) to the _CodesignBundle item group.
Then every app bundle will load this informarion from referenced app bundles,
and finally store this information on disk (in the 'codesign-bundle.items'
file). This means that in the end the main app bundle will have a list of all
contained app bundles in the app (recursively), in the _CodesignBundle item
group.

Separately we keep a list of other items that need signing, in the
_CodesignItems item group, and we do the same store/load logic for every
contained/contained app bundle (in the 'codesign.items' file, so a the end the
main app bundle will have a list of all the _CodesignItems for all contained
app bundles (recursively).

The previous steps occur in the _CollectCodesigningData and
_StoreCodesigningData targets.

The next step is to use the new ComputeCodesignItems task to compute
everything we need to know for code signing. This task takes over the
responsibility for listing all the *.dylib and *.metallib files, and the
*.framework directories in the app bundles, that need signing (which was
previously done in the targets file). This logic is significantly easier to
write, debug and test in C# than MSBuild.

In addition the ComputeCodesignItems also figures out a stamp file path we use
to determine if something needs (re-)signing. Previously .framework
directories did not have a stamp location, so they'd always end up resigned in
a rebuild, while now we'll automatically skip signing *.framework directories
unless something changed in them.

I've also tried to comment everything thorougly, for the next poor soul having
to deal with any bugs.

Behavioral differences:

We were always signing *.dylib files for macOS. We're now doing the same
thing for all platforms.
We're now always signing *.framework directories for all platforms (like we
do for *.dylib files), since frameworks are pretty much like dylibs anyways.

I've verified that this works both by running the submission tests and running
and launching a sample project on device from Windows.

This PR might be easier to review commit-by-commit, because the diff turns out weird otherwise.

So that it's available for code in Xamarin.MacDev.Tasks.Core.

I'm removing the old logic first, because the new logic is so different that a diff just complicates understanding what's happening (there's not much value in comparing textually what's changed when it's pretty much a complete rewrite).

The main theme here is that code signing will be done in the outermost executable project, not in any app extension projects or watch projects, nor during the RID-specific build of a .NET universal app. This makes codesigning easier to reason about and other affected logic (such as strip/dsymutil) easier to handle, in particular for .NET universal apps. Another benefit is that the differences between the iOS and macOS code bases have been eliminated. The first step is to collect all the information we need from the targets files. Every app bundle (be it app extension, watch app or main app) will add its own output app bundle (.app/.appex) to the _CodesignBundle item group. Then every app bundle will load this informarion from referenced app bundles, and finally store this information on disk (in the 'codesign-bundle.items' file). This means that in the end the main app bundle will have a list of all contained app bundles in the app (recursively), in the _CodesignBundle item group. Separately we keep a list of other items that need signing, in the _CodesignItems item group, and we do the same store/load logic for every contained/contained app bundle (in the 'codesign.items' file, so a the end the main app bundle will have a list of all the _CodesignItems for all contained app bundles (recursively). The previous steps occur in the _CollectCodesigningData and _StoreCodesigningData targets. The next step is to use the new ComputeCodesignItems task to compute everything we need to know for code signing. This task takes over the responsibility for listing all the *.dylib and *.metallib files, and the *.framework directories in the app bundles, that need signing (which was previously done in the targets file). This logic is significantly easier to write, debug and test in C# than MSBuild. In addition the ComputeCodesignItems also figures out a stamp file path we use to determine if something needs (re-)signing. Previously .framework directories did not have a stamp location, so they'd always end up resigned in a rebuild, while now we'll automatically skip signing *.framework directories unless something changed in them. I've also tried to comment everything thorougly, for the next poor soul having to deal with any bugs, as well has adding a comprehensive test for the new task. Behavioral differences: * We were always signing *.dylib files for macOS. We're now doing the same thing for all platforms. * We're now always signing *.framework directories for all platforms (like we do for *.dylib files), since frameworks are pretty much like dylibs anyways.

…properties. The CodesignEntitlements and CodesignResourceRules properties can be relative paths, and they might be coming from a referenced project. This means that if they're relative paths, we must resolve them to a full path using the project that defined them (which is specified using the 'SourceProjectPath' metadata).

…ditions as for other platforms. The CompileEntitlements task already handles each platform as it should. This way we have fewer differences in the code between platforms.

This way the property works when exposed to containing projects as well.

emaf · 2022-03-16T21:40:53Z

@rolfbjarne this looks good to me, but the only thing that worries me from Windows is copying empty files to the Mac. I was looking into the Codesign task and that one is not copying any files to the Mac. I think this is because at the point this is executed, all the needed files exist in the Mac and no extra files are needed (https://github.com/xamarin/xamarin-macios/blob/main/msbuild/Xamarin.MacDev.Tasks/Tasks/Codesign.cs#L20).

vs-mobiletools-engineering-service2 · 2022-03-16T21:59:25Z

❌ Tests failed on macOS M1 - Mac Big Sur (11.5) ❌

Tests failed on M1 - Mac Big Sur (11.5).

Failed tests are:

dontlink
introspection
xammac_tests
monotouch-test

Pipeline on Agent
Merge 71482a9 into dfa9ce9

vs-mobiletools-engineering-service2 · 2022-03-16T22:06:49Z

❌ Tests failed on macOS Mac Catalina (10.15) ❌

Tests failed on Mac Catalina (10.15).

Failed tests are:

xammac_tests
monotouch-test

Pipeline on Agent
Merge 71482a9 into dfa9ce9

dalexsoto

wow 👍

vs-mobiletools-engineering-service2 · 2022-03-17T03:22:33Z

❌ [CI Build] Tests failed on VSTS: simulator tests iOS ❌

Tests failed on VSTS: simulator tests iOS.

Test results

2 tests failed, 144 tests passed.

Failed tests

link all/Mac [dotnet]/Debug [dotnet]: Failed (Test run failed.
Tests run: 87 Passed: 75 Inconclusive: 0 Failed: 1 Ignored: 11)
Xtro/.NET: BuildFailure

Pipeline on Agent XAMBOT-1109.Monterey'
Merge 71482a9 into dfa9ce9

rolfbjarne · 2022-03-17T07:01:41Z

@emaf

@rolfbjarne this looks good to me, but the only thing that worries me from Windows is copying empty files to the Mac. I was looking into the Codesign task and that one is not copying any files to the Mac. I think this is because at the point this is executed, all the needed files exist in the Mac and no extra files are needed (https://github.com/xamarin/xamarin-macios/blob/main/msbuild/Xamarin.MacDev.Tasks/Tasks/Codesign.cs#L20).

That's correct, everything needed for the Codesign task should already be on the mac.

Are you saying that the new ComputeCodesignItems should do the same thing?

emaf · 2022-03-17T14:21:02Z

@rolfbjarne Yeah, so we make sure we don't copy any empty files from Windows.

If you implement ITaskCallback I'd also return false for ShouldCreateOutputFile so we don't create output files in Windows, since this new task is not really changing any of the files, but computing what needs to be signed, right?

rolfbjarne · 2022-03-17T14:59:19Z

If you implement ITaskCallback I'd also return false for ShouldCreateOutputFile so we don't create output files in Windows, since this new task is not really changing any of the files, but computing what needs to be signed, right?

Correct, this new task is not creating or changing any files, it's just computing stuff.

…n macOS and Windows. The ComputeCodesignItems does not touch any files, and all the input files should already exist on the mac, so there's no need to copy files back and forth.

rolfbjarne · 2022-03-17T16:15:35Z

@emaf

@rolfbjarne Yeah, so we make sure we don't copy any empty files from Windows.

If you implement ITaskCallback I'd also return false for ShouldCreateOutputFile so we don't create output files in Windows, since this new task is not really changing any of the files, but computing what needs to be signed, right?

I've changed the task to specifically say files shouldn't be copied back and forth between windows and mac (using ITaskCallback.ShouldCopyToBuildServer and ShouldCreateOutputFile).

vs-mobiletools-engineering-service2 · 2022-03-17T19:04:37Z

❌ Tests failed on macOS Mac Catalina (10.15) ❌

Tests failed on Mac Catalina (10.15).

Failed tests are:

xammac_tests
monotouch-test

Pipeline on Agent
Merge 5d36a7e into 31e8838

vs-mobiletools-engineering-service2 · 2022-03-18T01:56:03Z

❌ [CI Build] Tests failed on VSTS: simulator tests iOS ❌

Tests failed on VSTS: simulator tests iOS.

Test results

2 tests failed, 144 tests passed.

Failed tests

link all/Mac [dotnet]/Debug [dotnet]: Failed (Test run failed.
Tests run: 87 Passed: 75 Inconclusive: 0 Failed: 1 Ignored: 11)
Xtro/.NET: BuildFailure

Pipeline on Agent XAMBOT-1107.Monterey'
Merge 5d36a7e into 31e8838

rolfbjarne · 2022-03-22T11:53:17Z

Test failures are unrelated:

link all/Mac [dotnet]/Debug [dotnet]: https://github.com/xamarin/maccore/issues/2557
Xtro/.NET: https://github.com/xamarin/maccore/issues/2552

suihanhbr · 2022-07-30T09:34:04Z

I got this error when I build my project today.

/Library/Frameworks/Mono.framework/External/xbuild/Xamarin/iOS/Xamarin.Shared.targets(3,3): Error MSB4018: The "ComputeCodesignItems" task failed unexpectedly.
System.ArgumentOutOfRangeException: startIndex cannot be larger than length of string.
Parameter name: startIndex
  at System.String.Substring (System.Int32 startIndex, System.Int32 length) [0x00073] in /Users/builder/jenkins/workspace/build-package-osx-mono/2020-02/external/bockbuild/builds/mono-x64/external/corefx/src/Common/src/CoreLib/System/String.Manipulation.cs:1626 
  at System.String.Substring (System.Int32 startIndex) [0x00000] in /Users/builder/jenkins/workspace/build-package-osx-mono/2020-02/external/bockbuild/builds/mono-x64/external/corefx/src/Common/src/CoreLib/System/String.Manipulation.cs:1592 
  at Xamarin.MacDev.Tasks.ComputeCodesignItemsTaskBase.Execute () [0x00249] in /Users/builder/azdo/_work/1/s/xamarin-macios/msbuild/Xamarin.MacDev.Tasks.Core/Tasks/ComputeCodesignItemsTaskBase.cs:107 
  at Xamarin.MacDev.Tasks.ComputeCodesignItems.Execute () [0x0002a] in /Users/builder/azdo/_work/1/s/xamarin-macios/msbuild/Xamarin.MacDev.Tasks/Tasks/ComputeCodesignItems.cs:15 
  at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute () [0x00023] in /Users/builder/jenkins/workspace/build-package-osx-mono-pullrequest/pr/external/bockbuild/builds/msbuild-15/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs:578 
  at Microsoft.Build.BackEnd.TaskBuilder.ExecuteInstantiatedTask (Microsoft.Build.BackEnd.ITaskExecutionHost taskExecutionHost, Microsoft.Build.BackEnd.Logging.TaskLoggingContext taskLoggingContext, Microsoft.Build.BackEnd.TaskHost taskHost, Microsoft.Build.BackEnd.ItemBucket bucket, Microsoft.Build.BackEnd.TaskExecutionMode howToExecuteTask) [0x002b9] in /Users/builder/jenkins/workspace/build-package-osx-mono-pullrequest/pr/external/bockbuild/builds/msbuild-15/src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs:807  (MSB4018) (SuiHanIME.iOS)

chamons · 2022-08-01T22:06:11Z

@suihanhbr - Please file and issue with steps to reproduce/sample so we can look into this.

A stack trace in a PR comment thread is insufficient for us to track down any potential bug.

rolfbjarne added not-notes-worthy Ignore for release notes run-dotnet-tests Run all the .NET tests labels Mar 11, 2022

This comment was marked as outdated.

Sign in to view

rolfbjarne force-pushed the msbuild-sign-at-the-end branch from dc7d4c4 to 8c377f3 Compare March 14, 2022 13:03

This comment was marked as outdated.

Sign in to view

rolfbjarne added 6 commits March 16, 2022 21:00

[msbuild] Move ITaskItemExtensions to Xamarin.MacDev.Task.Core.

1b18074

So that it's available for code in Xamarin.MacDev.Tasks.Core.

[msbuild] Begone old code signing logic.

5934522

I'm removing the old logic first, because the new logic is so different that a diff just complicates understanding what's happening (there's not much value in comparing textually what's changed when it's pretty much a complete rewrite).

[msbuild] Execute _CompileEntitlements target on macOS under same con…

5ec43ff

…ditions as for other platforms. The CompileEntitlements task already handles each platform as it should. This way we have fewer differences in the code between platforms.

[msbuild] Resolve the path to the entitlements to a full path.

71482a9

This way the property works when exposed to containing projects as well.

rolfbjarne force-pushed the msbuild-sign-at-the-end branch from f8c39b6 to 71482a9 Compare March 16, 2022 20:23

rolfbjarne marked this pull request as ready for review March 16, 2022 20:24

rolfbjarne requested review from chamons, emaf and mauroa as code owners March 16, 2022 20:24

emaf approved these changes Mar 16, 2022

View reviewed changes

dalexsoto approved these changes Mar 16, 2022

View reviewed changes

[msbuild] Remove code that got duplicated in a rebase.

2057fa3

mauroa approved these changes Mar 17, 2022

View reviewed changes

[msbuild] Make the ComputeCodesignItems task not copy anything betwee…

5d36a7e

…n macOS and Windows. The ComputeCodesignItems does not touch any files, and all the input files should already exist on the mac, so there's no need to copy files back and forth.

rolfbjarne merged commit 7d500da into dotnet:main Mar 22, 2022

rolfbjarne deleted the msbuild-sign-at-the-end branch March 22, 2022 11:53

Uh oh!

Conversation

rolfbjarne commented Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

emaf commented Mar 16, 2022

Uh oh!

vs-mobiletools-engineering-service2 commented Mar 16, 2022

❌ Tests failed on macOS M1 - Mac Big Sur (11.5) ❌

Uh oh!

vs-mobiletools-engineering-service2 commented Mar 16, 2022

❌ Tests failed on macOS Mac Catalina (10.15) ❌

Uh oh!

dalexsoto left a comment

Choose a reason for hiding this comment

Uh oh!

vs-mobiletools-engineering-service2 commented Mar 17, 2022

❌ [CI Build] Tests failed on VSTS: simulator tests iOS ❌

Test results

Failed tests

Uh oh!

rolfbjarne commented Mar 17, 2022

Uh oh!

emaf commented Mar 17, 2022

Uh oh!

rolfbjarne commented Mar 17, 2022

Uh oh!

rolfbjarne commented Mar 17, 2022

Uh oh!

vs-mobiletools-engineering-service2 commented Mar 17, 2022

❌ Tests failed on macOS Mac Catalina (10.15) ❌

Uh oh!

vs-mobiletools-engineering-service2 commented Mar 18, 2022

❌ [CI Build] Tests failed on VSTS: simulator tests iOS ❌

Test results

Failed tests

Uh oh!

rolfbjarne commented Mar 22, 2022

Uh oh!

suihanhbr commented Jul 30, 2022

Uh oh!

chamons commented Aug 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rolfbjarne commented Mar 11, 2022 •

edited

Loading