Add signing information to asset manifest (port #41889 and #42688) by mmitche · Pull Request #43896 · dotnet/runtime

mmitche · 2020-10-27T15:58:15Z

Addresses the other part of dotnet/core-eng#10459 (first addressed in WindowsDesktop here dotnet/windowsdesktop#1039).

Validated that this works and is properly producing the manifest in this build. The manifest now contains signing information which will be consumed by the release pipeline.

Also change over to using 2019 Pool for the leg.

hoyosjs · 2020-10-29T00:42:08Z

@mmitche do you have the sample runs?

mmitche · 2020-10-29T01:32:21Z

@mmitche do you have the sample runs?

Not yet. THat comment was from the previous PR. I'll do one tomorrow.

jkoritzinsky · 2020-11-06T20:49:28Z

Can we start some sample runs for this PR? I'd like to get this in ASAP if possible before I merge in my shared framework pr.

mmitche · 2020-11-06T20:58:32Z

Working on it...FYI if we want to maintain full signed builds then the entire stack needs to have PB signing turned on at once, which is currently blocked on net6.0 TFM changes in sdk right now.

jkoritzinsky · 2020-11-06T21:07:08Z

If you’re ok with not having full signed builds I’m ok with that. I just want all the work in dotnet/runtime done first so we don’t need to do additional work in our repo once the issues in the SDK are resolved.

Basically I want the manifest work done before I merge my work in.

mmitche · 2020-11-06T21:08:23Z

https://dev.azure.com/dnceng/internal/_build/results?buildId=878258&view=results has the last PBSign=true (off this branch), should run pbsigning.

jkoritzinsky · 2020-11-06T22:16:29Z

@hoyosjs once we have the sample run completed, can you validate? I'd like to get this merged in as soon as possible once we've validated that the manifest/signing is good.

hoyosjs · 2020-11-06T22:20:58Z

@hoyosjs Juan Sebastian Hoyos Ayala FTE once we have the sample run completed, can you validate? I'd like to get this merged in as soon as possible once we've validated that the manifest/signing is good.

yes, sounds good

mmitche · 2020-11-09T15:31:02Z

@jkoritzinsky @hoyosjs Everything looks good here except for one thing. In the baseline build some of the mscordaccore.dll files (linux-*) are getting signed with Microsoft400 instead of MicrosoftSHA2. Most have MicrosoftSHA2. I think this is not intentional, but appears to be a result of this:

https://github.com/dotnet/runtime/pull/43896/files#diff-a923a7746e08ef5df4bfd49f340d9f93055d66ba792770a7e1c227b3eaf3d4efL69

Because this FileSignInfo is under the SignBinaries target, if we sign the dac outside of the SignBinaries phase (as appears to be the case in the baseline for Linux), it would get the default dll signature (Microsof400). But the rest get MicrosoftSHA2. Which is correct here?

hoyosjs · 2020-11-09T18:43:44Z

@mmitche Baseline as in before PBSign=true? Do you happen to know which ones? The idea is the DAC should always be signed with MicrosoftSHA2.

mmitche · 2020-11-09T18:52:33Z

Yeah, I did a PBSign=true with this change, and compared against the official build of the baseline sha. It's only the DAC, for the reasons above.

Note that this doesn't affect RTM (I spot checked some packages there), because I removed the targets and hoisted https://github.com/dotnet/runtime/blob/release/5.0/eng/Signing.props#L39 out.

hoyosjs · 2020-11-09T23:49:48Z

+      AzureDevOpsCollectionUri="$(SYSTEM_TEAMFOUNDATIONCOLLECTIONURI)"
+      AzureDevOpsProject="$(SYSTEM_TEAMPROJECT)"
+      AzureDevOpsBuildId="$(BUILD_BUILDID)"
+      ItemsToSign="@(ItemsToSignPostBuild)"


I didn't know specifying a parameter to a task twice was valid MSBuild. TIL

Which one do you see as duplicated?

Ah, not at all. I was like he ran this and it passed. Just some behavior I didn't know worked. But they are different. There's ItemsToPush and ItemsToSign.

hoyosjs · 2020-11-09T23:51:39Z

+    <DownloadedSymbolPackages Include="$(DownloadDirectory)**\*.symbols.nupkg" />
+    <DownloadedSymbolPackagesWithoutPaths Include="@(DownloadedSymbolPackages->'%(Filename)%(Extension)')" />
+    <!-- https://github.com/dotnet/arcade/issues/6192 -->
+    <!-- <FileSignInfo Include="@(DownloadedSymbolPackagesWithoutPaths->Distinct())" CertificateName="None" /> -->


This was fixed. Can we try uncommenting this line?

hoyosjs · 2020-11-09T23:53:52Z

+    <FileSignInfo Include="Mono.Cecil.Pdb.dll" CertificateName="3PartySHA2" />
+    <FileSignInfo Include="Mono.Cecil.Rocks.dll" CertificateName="3PartySHA2" />
+
+    <FileSignInfo Include="mscordaccore.dll" CertificateName="MicrosoftSHA2" />


I think we hit interesting behavior of signtool here. This says the short name dac signs with sha2, but the long name is microsoft400.

Signtool seems to match on exact name and extension. This code would need to explicitly handle the long name DAC by either calculating the name or globbing it.

Yes, the match for FileSignInfo is the name without path. I had tried at some point to glob the mscordaccore produced and then take the file name. This won't work when signing in packages which is what we want. For that we'd probably need the versioning target to have run and then add something ugly like:

<FileSignInfo Include="mscordaccore.dll" CertificateName="MicrosoftSHA2" /> + <FileSignInfo Include="mscordaccore_x86_x86_$(FileVersion).dll CertificateName="MicrosoftSHA2" /> + <FileSignInfo Include="mscordaccore_x86_arm_$(FileVersion).dll CertificateName="MicrosoftSHA2" /> + <FileSignInfo Include="mscordaccore_x64_x64_$(FileVersion).dll CertificateName="MicrosoftSHA2" /> + <FileSignInfo Include="mscordaccore_x64_arm64_$(FileVersion).dll CertificateName="MicrosoftSHA2" /> + <FileSignInfo Include="mscordaccore_arm_arm_$(FileVersion).dll CertificateName="MicrosoftSHA2" /> + <FileSignInfo Include="mscordaccore_arm64_arm64_$(FileVersion).dll CertificateName="MicrosoftSHA2" />

The interesting behavior I thought I was going to see was that the file contents are the same, but the names were different. One suggested signing with 400, the other with SHA2. I thought SignTool uses the hash of the content to only sign things once, so I was unsure which one it'd pick. However, but it looks like it uses name and hash so this isn't a concern. It just means the long name has always been signed using 400 since 3.1.

Signtool uses hash of content + name. So if you have foo.nupkg and foo.symbols.nupkg and they happen to be identical you can make different signing decisions. This is by design.

hoyosjs · 2020-11-10T00:31:33Z

+  </Choose>
+
+  <ItemGroup>
+    <ItemsToSign Update="@(ItemsToSign)" Authenticode="$(CertificateId)" />


@mmitche Will this get an override by FileSignInfo?

I actually don't think that does anything at all unless calling the Microbuild targets directly. I'll remove this and see what happens.

Yeah, SignTool itself doesn't read this property, but it would be have been read by the Microbuild targets at one point.

mmitche · 2020-11-10T14:57:58Z

https://dev.azure.com/dnceng/internal/_build/results?buildId=880901&view=results - Post build signing
https://dev.azure.com/dnceng/internal/_build/results?buildId=880902&view=results - Normal signing with this build

hoyosjs · 2020-11-11T07:11:22Z

I did a quick spot check. And as you said, the only weird case is the long name DAC for PBSign=false.

For PBSign=true I can see that the cross-bit DAC (you can find them in the runtime pack under tools/<host_arch>_<target_arch>) I thought we'd fixed this with #43500, and it's working with no PBSign changes.

mmitche · 2020-11-11T15:05:56Z

@hoyosjs Looks like not included here: https://github.com/dotnet/runtime/pull/43896/files#diff-a923a7746e08ef5df4bfd49f340d9f93055d66ba792770a7e1c227b3eaf3d4efR164

I think I fixed it. I also removed the original itemgroup to make it a little more clear.

mmitche · 2020-11-11T17:27:43Z

@hoyosjs I spot checked the cross dacs and I think they look signed now.

mmitche · 2020-11-11T21:46:48Z

Newest build with no PBSign: https://dev.azure.com/dnceng/internal/_build/results?buildId=882618&view=results
With PBSign: https://dev.azure.com/dnceng/internal/_build/results?buildId=882621&view=results

hoyosjs · 2020-11-11T22:43:25Z

@mmitche everything looks good. Now in PB sign I see that the long name DAC is 400 signed. Not any concerns for now, it mimics behavior that already existed in master.

mmitche · 2020-11-11T23:15:51Z

@hoyosjs I'll port that one tweak for PBSign to release/5.0

Post build signing

5764163

Dotnet-GitSync-Bot added the area-Setup label Oct 27, 2020

mmitche requested a review from hoyosjs October 27, 2020 17:15

mmitche added 2 commits October 30, 2020 07:06

Merge remote-tracking branch 'upstream/master' into cherry-pick-pbs

293dd23

Add PostBuildSign flag

d331e09

runfoapp bot mentioned this pull request Oct 30, 2020

Inability to unzip assets during build on Unix x64 #32805

Closed

This was referenced Nov 6, 2020

[release/5.0-rc2] Add signing information to asset manifest #41889

Merged

[master] Add signing information to asset manifest #44363

Closed

hoyosjs approved these changes Nov 9, 2020

View reviewed changes

hoyosjs reviewed Nov 10, 2020

View reviewed changes

Feedback

a7c0086

Sign cross dacs

93751c6

No need for * in mscordbi

46b4e3c

runfoapp bot mentioned this pull request Nov 11, 2020

Many Android JIT tests failing in CI #44306

Closed

mmitche merged commit 905a367 into dotnet:master Nov 11, 2020

mmitche deleted the cherry-pick-pbs branch November 11, 2020 23:15

ghost locked as resolved and limited conversation to collaborators Dec 12, 2020

Conversation

mmitche commented Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoyosjs commented Oct 29, 2020

Uh oh!

mmitche commented Oct 29, 2020

Uh oh!

jkoritzinsky commented Nov 6, 2020

Uh oh!

mmitche commented Nov 6, 2020

Uh oh!

jkoritzinsky commented Nov 6, 2020

Uh oh!

mmitche commented Nov 6, 2020

Uh oh!

jkoritzinsky commented Nov 6, 2020

Uh oh!

hoyosjs commented Nov 6, 2020

Uh oh!

mmitche commented Nov 9, 2020

Uh oh!

hoyosjs commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmitche commented Nov 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hoyosjs Nov 10, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

hoyosjs Nov 10, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmitche commented Nov 10, 2020

Uh oh!

hoyosjs commented Nov 11, 2020

Uh oh!

mmitche commented Nov 11, 2020

Uh oh!

mmitche commented Nov 11, 2020

Uh oh!

mmitche commented Nov 11, 2020

Uh oh!

hoyosjs commented Nov 11, 2020

Uh oh!

mmitche commented Nov 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mmitche commented Oct 27, 2020 •

edited

Loading

hoyosjs commented Nov 9, 2020 •

edited

Loading

hoyosjs Nov 10, 2020 •

edited

Loading

hoyosjs Nov 10, 2020 •

edited

Loading