Merged
Conversation
cb2a1a5 to
50f0cb5
Compare
8f430f0 to
4ce58fa
Compare
fa200c0 to
66cda66
Compare
This change makes the legacy behavior contingent on the runtime of the compiler. When the compiler is running on the desktop runtime, the legacy signing code will be used. When running on CoreCLR, the new portable signing code will be used. This fixes key container signing for almost all scenarios (signing using a key container when the compiler is running on the desktop framework). Signing using a key container on Windows using a compiler running on CoreCLR is still broken, but that is a far rarer case. This change also fixes delaysign, which would full sign the file if the keyfile passed to the compiler contains a private key. Fixes dotnet#25340 Fixes dotnet#25424
66cda66 to
44f1388
Compare
jaredpar
approved these changes
Mar 14, 2018
Member
Consider making this a local function, since used by a single test Refers to: src/Compilers/CSharp/Test/Emit/Attributes/InternalsVisibleToAndStrongNameTests.cs:1594 in e1a84e2. [](commit_id = e1a84e2, deletion_comment = False) |
jcouv
reviewed
Mar 14, 2018
| var header = metadata.Module.PEReaderOpt.PEHeaders.CorHeader; | ||
| //confirm header has expected SN signature size | ||
| Assert.Equal(256, header.StrongNameSignatureDirectory.Size); | ||
| Assert.Equal(CorFlags.ILOnly, header.Flags); |
Member
There was a problem hiding this comment.
Consider adding a comment ("although a private key was provided, the assembly is still not signed" or something)
jcouv
approved these changes
Mar 14, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Customer scenario(s)
There are two signing bugs. In the first, the compiler fails to sign an assembly if signing is specific using the AssemblyKeyContainer attribute and nothing else. Specifically, if key signing is requested using a key file or using a key container but provided through command line or MSBuild option, signing succeeds. In the second, the
-delaysignoption will fully sign an assembly if provided a key file that contains both a public and private key.Bugs this fixes
#25340
#25424
Tracking bug https://devdiv.visualstudio.com/DevDiv/_workitems/edit/580955
Workarounds, if any
There are three possible workarounds for the key container bug for 15.6:
For the delaysign bug, you can use the UseLegacyStrongNameProvider flag on Windows, or you can create a key file that contains only the public key, with no private key.
Risk
This change mostly moves us back to using the old signing behavior. The only change not related is the fix for delaysign on CoreCLR, which is a targeted fix to prevent us from full-signing when it was not requested. This change has been validated by unit tests.
Performance impact
Almost none -- an additional null check in the working case and only a handful of allocations.
Is this a regression from a previous update?
This is a regression, but not from another update. Key container signing using attributes previously worked.
Root cause analysis
The tests only tried to test key containers with the desktop strong name provider, assuming that only a desktop strong name provider would be provided if we require key container signing. This is incorrect. This change moves us back to the old signing code, which has proper tests for these scenarios. For the delaysign fix, appropriate tests have been added for the CoreCLR code.
How was the bug found?
Customer reported.