Skip to content

[build] Migrate stable .pkg signing to ESRP - #5700

Merged
jonpryor merged 17 commits into
mainfrom
esrp-legacy-pkg
Mar 17, 2021
Merged

[build] Migrate stable .pkg signing to ESRP#5700
jonpryor merged 17 commits into
mainfrom
esrp-legacy-pkg

Conversation

@pjcollins

@pjcollins pjcollins commented Mar 5, 2021

Copy link
Copy Markdown
Member

Context: #5529

Commit dc1f7ba enabled ESRP signing for all .NET 6 artifacts, and we've
been asked to apply these changes to our stable .pkg installer that is
still using a deprecated Apple Developer account and signing logic.

The mac build job has been updated to sign and harden all Mach-O files
included in both our stable and .NET 6 .pkg files. This was previously
happening in two different passes, the first for the stable .pkg content
during the build job and the second for the .NET 6 .pkg content during
the dotnet_create_pkg job. Signing of the stable .pkg has been migrated
to use the ESRP signing service via the tasks/targets imported by the new
MicroBuild package reference.

Our previous notarization preparation logic required us to unpack
bundletool.jar, and sign the version of aapt2 that is bundled inside.
This has been updated to remove all embedded versions of aapt2
instead, as we pass our own path to aapt2 to the tool as needed.

pjcollins and others added 2 commits March 4, 2021 16:01
Context: #5529

Commit dc1f7ba enabled ESRP signing for all .NET 6 artifacts, and we've
been asked to apply these changes to our stable .pkg installer that is
still using a deprecated Apple Developer account and signing logic.

The mac build job has been updated to sign and harden all Mach-O files
included in both our stable and .NET 6 .pkg files.  This was previously
happening in two different passes, the first for the stable .pkg content
during the build job and the second for the .NET 6 .pkg content during
the `dotnet_create_pkg` job.

Signing of the stable .pkg has been migrated to use the ESRP signing
service via the tasks/targets imported by the new MicroBuild package
reference.  Notarization of the stable .pkg has been moved to the mac
build job.  As a result, all legacy macOS automated tests will now run
against the final version of the package we ship.  This will allow us to
catch any potential issues with notarization earlier.
Base automatically changed from master to main March 5, 2021 23:08
@pjcollins
pjcollins marked this pull request as ready for review March 10, 2021 22:59
@pjcollins
pjcollins requested a review from jonpryor as a code owner March 10, 2021 22:59
@pjcollins

Copy link
Copy Markdown
Member Author

Latest "Real signed" test run looks good: https://dev.azure.com/devdiv/DevDiv/_build/results?buildId=4544409&view=results

@pjcollins

pjcollins commented Mar 11, 2021

Copy link
Copy Markdown
Member Author

Latest "Real signed" test run (plus smoke tests against notarized .pkg) - https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=4547971&view=results

@jonpryor

Copy link
Copy Markdown
Contributor

I have two concerns here:

  1. macOS Notarization is added to the Mac > Build stage. This has a benefit that it ensures all of our subsequent test stages are testing the (final!) notarized installer.

    The downside is that the Mac > Build stage is (1) significantly longer -- ~2h 30min in this PR vs. 1h 12min for afc03585 -- more than double.

    This in turn means that subsequent test stages are delayed, as well as .vsix signing.

  2. I haven't seen the Finalizer Installers > Queue Vsix Signing job complete successfully. It's always timing out (after ~90 minutes!).

Is there any way to keep our existing structure -- Mac > Build doesn't sign, test stages run against the unsigned installer, etc. -- while using the new ESRP signing service?

@pjcollins

Copy link
Copy Markdown
Member Author

The .vsix job timeout was caused by an unrelated issue and should now be resolved.

Unless we want to unpack, sign and repack the entire .pkg file in our "finalization" stage, we'll have to incur some additional cost of ESRP signing during the build. I think we can get the build time back down to close to what it is today by moving just the .pkg signing and the .pkg notarization calls to a separate job as suggested though, I'll try to rework this.

@pjcollins

pjcollins commented Mar 12, 2021

Copy link
Copy Markdown
Member Author

@jonpryor

Copy link
Copy Markdown
Contributor

The current 1h30min is much better than the previous 2h30min! :-)

Still ~20min longer than current, though.

Is this the best we can do? Should we just go with this?

Comment on lines +38 to +39
<!-- Unpack bundletool.jar and sign embedded aapt2 -->
<RemoveDir Directories="$(_BundleToolExtractLocation)" />

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just a thought, I didn't know you were having to deal with aapt2 inside bundletool.

We always pass a path to aapt2 to bundletool, so it uses our aapt2, I wonder if we could simply remove it from bundletool.jar? I guess that would invalidate bundletoo.jar's signature.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I like this idea, we can save some build time (and installation space) by stripping out the nested aapt2 rather than signing it. Let's see if this causes any test failures.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Looks like bundletool still expects aapt2 to exist even if it may not be using it in our case, I think I'll revert these latest changes:

[BT : 1.4.0] error : Unable to locate aapt2 inside jar. [/Users/runner/work/1/s/tests/Mono.Android-Tests/Runtime-AppBundle/Mono.Android-TestsAppBundle.csproj]
  com.android.tools.build.bundletool.model.exceptions.CommandExecutionException: Unable to locate aapt2 inside jar.
  	at com.android.tools.build.bundletool.model.exceptions.InternalExceptionBuilder.build(InternalExceptionBuilder.java:57)
  	at com.android.tools.build.bundletool.model.utils.SdkToolsLocator.extractAapt2(SdkToolsLocator.java:109)
  	at com.android.tools.build.bundletool.commands.CommandUtils.extractAapt2FromJar(CommandUtils.java:64)
  	at com.android.tools.build.bundletool.commands.BundletoolModule.lambda$provideAapt2Command$0(BundletoolModule.java:35)
  	at java.base/java.util.Optional.orElseGet(Optional.java:369)
  	at com.android.tools.build.bundletool.commands.BundletoolModule.provideAapt2Command(BundletoolModule.java:35)
  	at com.android.tools.build.bundletool.commands.BundletoolModule_ProvideAapt2CommandFactory.provideAapt2Command(BundletoolModule_ProvideAapt2CommandFactory.java:40)
  	at com.android.tools.build.bundletool.commands.BundletoolModule_ProvideAapt2CommandFactory.get(BundletoolModule_ProvideAapt2CommandFactory.java:31)
  	at com.android.tools.build.bundletool.commands.BundletoolModule_ProvideAapt2CommandFactory.get(BundletoolModule_ProvideAapt2CommandFactory.java:10)
  	at dagger.internal.DoubleCheck.get(DoubleCheck.java:47)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you know the command-line that hit this? Just want to check we don't have something wrong.

@pjcollins pjcollins Mar 15, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This happened in the apk tests job during DeployTestAabs: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=4555981&view=logs&j=cd6eb242-2e57-569b-620a-eaaded8e9a51&t=f5527f3e-27f9-5958-e19f-f922328304e1&l=715. My initial changes to strip aapt2 out only ran when doing real signing so the full set of tests didn't run against it. I can kick something off against that commit to see if anything else breaks. - https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=4558049&view=results

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah weird we pass --adb and they still extract it:

/Users/runner/Library/Android/jdk-11/bin/java -jar /Library/Frameworks/Xamarin.Android.framework/Libraries/xbuild/Xamarin/Android/bundletool.jar build-apks --connected-device --overwrite --mode default --bundle "../../../bin/TestRelease/Mono.Android_TestsAppBundle-Signed.aab" --output "../../../bin/TestRelease/Mono.Android_TestsAppBundle-Signed.aab.apks" --adb /Users/runner/Library/Android/sdk/platform-tools/adb --ks /Users/runner/work/1/s/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/test.keystore --ks-key-alias mykey --key-pass pass:android --ks-pass pass:android 
[BT : 1.4.0] error : Unable to locate aapt2 inside jar.

@jonathanpeppers jonathanpeppers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm good with the actual changes here, I don't know if we can do much more about the build times. We are probably now seeing the difference between the old signing method and new one.

@pjcollins

Copy link
Copy Markdown
Member Author

I think the ~20 minute build addition is probably a better price to pay than the added complexity of the alternative -- which would be unpacking and repacking our legacy .pkg installer in a post-build job. We can maybe reduce this added time a little bit more with the latest change that removes the version of aapt2 nested in bundletool.jar rather than signing it.

Latest real signed job: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=4555981&view=logs&s=1afc3bfe-122c-538b-e9ad-2a86c2efcfef&j=96fd57f5-f69e-53c7-3d47-f67e6cf9b93e

@pjcollins pjcollins added the do-not-merge PR should not be merged. label Mar 14, 2021
@pjcollins pjcollins removed the do-not-merge PR should not be merged. label Mar 14, 2021
@pjcollins
pjcollins marked this pull request as draft March 15, 2021 19:54
@pjcollins

Copy link
Copy Markdown
Member Author

Latest real signed run is looking good: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=4562177&view=logs&j=a62e275d-46d5-5c3b-5a47-c4e00ae7427d. This should be good to go now.

@pjcollins
pjcollins marked this pull request as ready for review March 16, 2021 17:42
@pjcollins pjcollins changed the title [build] Codesign all .pkg content during build job [build] Migrate stable .pkg signing to ESRP Mar 16, 2021
@jonpryor
jonpryor merged commit 82d1670 into main Mar 17, 2021
@jonpryor
jonpryor deleted the esrp-legacy-pkg branch March 17, 2021 00:35
@github-actions github-actions Bot locked and limited conversation to collaborators Jan 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants