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

Fix signing in PR runs #31926

Merged
merged 1 commit into from
Feb 10, 2020
Merged

Fix signing in PR runs #31926

merged 1 commit into from
Feb 10, 2020

Conversation

trylek
Copy link
Member

@trylek trylek commented Feb 7, 2020

Make URCT copying unconditional to unblock installer tasks in debug
mode.

Thanks

Tomas

Fixes: #1026

Make URCT copying unconditional to unblock installer tasks in debug
mode.

Thanks

Tomas
@@ -20,7 +20,7 @@
<UcrtFilesToCopy Include="$(UniversalCRTSDKDir)Redist\ucrt\DLLs\$(BuildArch)\*.dll" />
Copy link
Member

Choose a reason for hiding this comment

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

In the case for debug, don't we need ucrtbased.dll?

Copy link
Member Author

Choose a reason for hiding this comment

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

@dagood - could you please take a look at the below run? I think I see some installer runs passing but I don't know for sure. @hoyoys - I must admit I have no idea, nor does Matt whom I asked as he originally added this bit. Can you please elaborate? I was under the impression the the redistributable Universal CRT used to be a replacement for the Win32 API DLLs that appeared in later Windows versions but I may be certainly missing something.

Copy link
Member

@hoyosjs hoyosjs Feb 8, 2020

Choose a reason for hiding this comment

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

I must admit I have no idea, nor does Matt whom I asked as he originally added this bit.

We added the UCRT and API shims as a way to support older windows versions (for example API* is only needed on win7). If it's a debug build, we'd need ucrtbased.dll and such variants. Given that we statically link the CRT files no issues, other than UCRT. As for why we sign them here, if we don't resign them ourselves here, we'd be shipping an API shim that's not Authenticode signed.

Copy link
Member

Choose a reason for hiding this comment

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

That being said, the only one that needs to be signed is that one API shim.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, you know I'm no expert here. Given we're not actually publishing / shipping anything in the PR runs, just doing dry runs of some installer signing magic presumably using some test certificates - what additional changes would you suggest?

Copy link
Member

Choose a reason for hiding this comment

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

If this is only going to be for test-signing, then it doesn't matter if the file is there unless we turn on signing validation or test on windows 7

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Juan for your additional feedback. I defer to Davis to comment on this in greater detail as in this particular case I'm just a trained monkey mechanically trying out a combo of two code changes. Have a great weekend!

@dagood
Copy link
Member

dagood commented Feb 10, 2020

The PR run looks reasonable, specifically:

  SignToolTask starting.
  DryRun: True
  Signing mode: Test
  MicroBuild signing logs will be in (Signing*.binlog): F:\workspace\_work\1\s\artifacts\log\Debug\
  MicroBuild signing configuration will be in (Round*.proj): F:\workspace\_work\1\s\artifacts\tmp\Debug\
  Signing Round 0: 186 files to sign.
  Build artifacts signed and validated.
  SignToolTask execution finished.
  SignBinaries -> completed in 00:00:02.4659135

I don't know for certain how deep DryRun: True is... if it means it wouldn't have caught the official build break.

The ultimate test would be to add a commit that reverts #31807 and see if the test signing catches the error. 😄 (Edit: Test being performed at #32044)

@trylek trylek changed the title WIP: Fix signing in PR runs Fix signing in PR runs Feb 10, 2020
@trylek trylek requested a review from dagood February 10, 2020 22:20
@trylek trylek merged commit 5c6f5b1 into dotnet:master Feb 10, 2020
@trylek trylek deleted the SigningInPRRuns branch February 10, 2020 23:23
Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks Tomas

@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable test signing during PR validation jobs
5 participants