From e2fc8d5ea7d155519eee965354629cb48c6b6ae7 Mon Sep 17 00:00:00 2001 From: Andy Gocke Date: Wed, 14 Mar 2018 17:07:57 -0700 Subject: [PATCH] Key container signing fix (#25426) 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 #25340 Fixes #25424 --- .../Test/CommandLine/CommandLineTests.cs | 4 +- .../InternalsVisibleToAndStrongNameTests.cs | 73 ++++++++----------- src/Compilers/CSharp/csc/csc.csproj | 3 - .../Core/Portable/CodeAnalysis.csproj | 1 + .../CommandLine/CommonCommandLineArguments.cs | 4 +- .../Core/Portable/PEWriter/PeWriter.cs | 9 +-- .../Server/VBCSCompiler/VBCSCompiler.csproj | 3 - .../Test/CommandLine/CommandLineTests.vb | 4 +- .../InternalsVisibleToAndStrongNameTests.vb | 10 +-- src/Compilers/VisualBasic/vbc/vbc.csproj | 3 - src/Scripting/Core/Scripting.csproj | 3 - .../Utilities/Portable/TestUtilities.csproj | 3 - 12 files changed, 44 insertions(+), 76 deletions(-) diff --git a/src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs b/src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs index bffaf067ebc..49c7c161ca6 100644 --- a/src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs +++ b/src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs @@ -5864,7 +5864,7 @@ public void EmittedSubsystemVersion() Assert.Equal(1, peHeaders.PEHeader.MinorSubsystemVersion); } - [Fact(Skip = "https://github.com/dotnet/roslyn/pull/23529")] + [Fact] public void CreateCompilationWithKeyFile() { string source = @" @@ -5883,7 +5883,7 @@ public static void Main() var cmd = new MockCSharpCompiler(null, dir.Path, new[] { "/nologo", "a.cs", "/keyfile:key.snk", }); var comp = cmd.CreateCompilation(TextWriter.Null, new TouchedFileLogger(), NullErrorLogger.Instance); - Assert.Equal(comp.Options.StrongNameProvider.GetType(), typeof(PortableStrongNameProvider)); + Assert.IsType(comp.Options.StrongNameProvider); } [Fact] diff --git a/src/Compilers/CSharp/Test/Emit/Attributes/InternalsVisibleToAndStrongNameTests.cs b/src/Compilers/CSharp/Test/Emit/Attributes/InternalsVisibleToAndStrongNameTests.cs index 23d9c08c16e..9b5d8568063 100644 --- a/src/Compilers/CSharp/Test/Emit/Attributes/InternalsVisibleToAndStrongNameTests.cs +++ b/src/Compilers/CSharp/Test/Emit/Attributes/InternalsVisibleToAndStrongNameTests.cs @@ -1145,16 +1145,8 @@ private void ConfirmModuleAttributePresentAndAddingToAssemblyResultsInSignedOutp using (var metadata = ModuleMetadata.CreateFromStream(moduleContents)) { var flags = metadata.Module.PEReaderOpt.PEHeaders.CorHeader.Flags; - if (legacyStrongName) - { - //confirm file does not claim to be signed - Assert.Equal(0, (int)(flags & CorFlags.StrongNameSigned)); - } - else - { - // It should have been signed in the stream - Assert.NotEqual(0, (int)(flags & CorFlags.StrongNameSigned)); - } + //confirm file does not claim to be signed + Assert.Equal(0, (int)(flags & CorFlags.StrongNameSigned)); EntityHandle token = metadata.Module.GetTypeRef(metadata.Module.GetAssemblyRef("mscorlib"), "System.Runtime.CompilerServices", "AssemblyAttributesGoHere"); Assert.False(token.IsNil); //could the type ref be located? If not then the attribute's not there. @@ -1599,52 +1591,49 @@ public class C Assert.True(emitResult.Success); } - private void DelaySignWithAssemblySignatureKeyHelper(StrongNameProvider provider) + [Fact] + public void DelaySignWithAssemblySignatureKey() { - //Note that this SignatureKey is some random one that I found in the devdiv build. - //It is not related to the other keys we use in these tests. + DelaySignWithAssemblySignatureKeyHelper(s_defaultPortableProvider); + DelaySignWithAssemblySignatureKeyHelper(s_defaultDesktopProvider); - //In the native compiler, when the AssemblySignatureKey attribute is present, and - //the binary is configured for delay signing, the contents of the assemblySignatureKey attribute - //(rather than the contents of the keyfile or container) are used to compute the size needed to - //reserve in the binary for its signature. Signing using this key is only supported via sn.exe + void DelaySignWithAssemblySignatureKeyHelper(StrongNameProvider provider) + { + //Note that this SignatureKey is some random one that I found in the devdiv build. + //It is not related to the other keys we use in these tests. - var options = TestOptions.ReleaseDll - .WithDelaySign(true) - .WithCryptoKeyFile(s_keyPairFile) - .WithStrongNameProvider(provider); + //In the native compiler, when the AssemblySignatureKey attribute is present, and + //the binary is configured for delay signing, the contents of the assemblySignatureKey attribute + //(rather than the contents of the keyfile or container) are used to compute the size needed to + //reserve in the binary for its signature. Signing using this key is only supported via sn.exe - var other = CreateCompilation( - @" + var options = TestOptions.ReleaseDll + .WithDelaySign(true) + .WithCryptoKeyFile(s_keyPairFile) + .WithStrongNameProvider(provider); + + var other = CreateCompilation( + @" [assembly: System.Reflection.AssemblyDelaySign(true)] [assembly: System.Reflection.AssemblySignatureKey(""002400000c800000140100000602000000240000525341310008000001000100613399aff18ef1a2c2514a273a42d9042b72321f1757102df9ebada69923e2738406c21e5b801552ab8d200a65a235e001ac9adc25f2d811eb09496a4c6a59d4619589c69f5baf0c4179a47311d92555cd006acc8b5959f2bd6e10e360c34537a1d266da8085856583c85d81da7f3ec01ed9564c58d93d713cd0172c8e23a10f0239b80c96b07736f5d8b022542a4e74251a5f432824318b3539a5a087f8e53d2f135f9ca47f3bb2e10aff0af0849504fb7cea3ff192dc8de0edad64c68efde34c56d302ad55fd6e80f302d5efcdeae953658d3452561b5f36c542efdbdd9f888538d374cef106acf7d93a4445c3c73cd911f0571aaf3d54da12b11ddec375b3"", ""a5a866e1ee186f807668209f3b11236ace5e21f117803a3143abb126dd035d7d2f876b6938aaf2ee3414d5420d753621400db44a49c486ce134300a2106adb6bdb433590fef8ad5c43cba82290dc49530effd86523d9483c00f458af46890036b0e2c61d077d7fbac467a506eba29e467a87198b053c749aa2a4d2840c784e6d"")] public class C { static void Goo() {} }", - new MetadataReference[] { MscorlibRef_v4_0_30316_17626 }, - options: options); + new MetadataReference[] { MscorlibRef_v4_0_30316_17626 }, + options: options); - using (var metadata = ModuleMetadata.CreateFromImage(other.EmitToArray())) - { - var header = metadata.Module.PEReaderOpt.PEHeaders.CorHeader; - //confirm header has expected SN signature size - Assert.Equal(256, header.StrongNameSignatureDirectory.Size); + using (var metadata = ModuleMetadata.CreateFromImage(other.EmitToArray())) + { + var header = metadata.Module.PEReaderOpt.PEHeaders.CorHeader; + //confirm header has expected SN signature size + Assert.Equal(256, header.StrongNameSignatureDirectory.Size); + // Delay sign should not have the strong name flag set + Assert.Equal(CorFlags.ILOnly, header.Flags); + } } } - [Fact] - public void DelaySignWithAssemblySignatureKey() - { - DelaySignWithAssemblySignatureKeyHelper(s_defaultPortableProvider); - } - - [Fact] - public void DelaySignWithAssemblySignatureKey_Legacy() - { - DelaySignWithAssemblySignatureKeyHelper(s_defaultDesktopProvider); - } - [WorkItem(545720, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/545720")] [WorkItem(530050, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/530050")] [Fact] diff --git a/src/Compilers/CSharp/csc/csc.csproj b/src/Compilers/CSharp/csc/csc.csproj index fb4d49ad96c..0b3a1af6639 100644 --- a/src/Compilers/CSharp/csc/csc.csproj +++ b/src/Compilers/CSharp/csc/csc.csproj @@ -35,9 +35,6 @@ CoreClrAnalyzerAssemblyLoader.cs - - CoreClrShim.cs - DesktopBuildClient.cs diff --git a/src/Compilers/Core/Portable/CodeAnalysis.csproj b/src/Compilers/Core/Portable/CodeAnalysis.csproj index 003ee69192f..a82c23815ec 100644 --- a/src/Compilers/Core/Portable/CodeAnalysis.csproj +++ b/src/Compilers/Core/Portable/CodeAnalysis.csproj @@ -27,6 +27,7 @@ + DesktopShim.cs diff --git a/src/Compilers/Core/Portable/CommandLine/CommonCommandLineArguments.cs b/src/Compilers/Core/Portable/CommandLine/CommonCommandLineArguments.cs index 74c2cdeea27..2431fc04b44 100644 --- a/src/Compilers/Core/Portable/CommandLine/CommonCommandLineArguments.cs +++ b/src/Compilers/Core/Portable/CommandLine/CommonCommandLineArguments.cs @@ -286,7 +286,9 @@ public CompilationOptions CompilationOptions StrongNameFileSystem fileSystem, string tempDirectory) { - bool fallback = ParseOptionsCore.Features.ContainsKey("UseLegacyStrongNameProvider") || + bool fallback = + !CoreClrShim.IsRunningOnCoreClr || + ParseOptionsCore.Features.ContainsKey("UseLegacyStrongNameProvider") || CompilationOptionsCore.CryptoKeyContainer != null; return fallback ? new DesktopStrongNameProvider(KeyFileSearchPaths, tempDirectory, fileSystem) : diff --git a/src/Compilers/Core/Portable/PEWriter/PeWriter.cs b/src/Compilers/Core/Portable/PEWriter/PeWriter.cs index 7e38260b2af..07d075a75b4 100644 --- a/src/Compilers/Core/Portable/PEWriter/PeWriter.cs +++ b/src/Compilers/Core/Portable/PEWriter/PeWriter.cs @@ -201,13 +201,7 @@ internal static class PeWriter } var strongNameProvider = context.Module.CommonCompilation.Options.StrongNameProvider; - var corFlags = properties.CorFlags; - if (privateKeyOpt != null) - { - Debug.Assert(strongNameProvider.Capability == SigningCapability.SignsPeBuilder); - corFlags |= CorFlags.StrongNameSigned; - } var peBuilder = new ExtendedPEBuilder( peHeaderBuilder, @@ -228,8 +222,9 @@ internal static class PeWriter PatchModuleVersionIds(mvidFixup, mvidSectionFixup, mvidStringFixup, peContentId.Guid); - if (privateKeyOpt != null) + if (privateKeyOpt != null && corFlags.HasFlag(CorFlags.StrongNameSigned)) { + Debug.Assert(strongNameProvider.Capability == SigningCapability.SignsPeBuilder); strongNameProvider.SignPeBuilder(peBuilder, peBlob, privateKeyOpt.Value); FixupChecksum(peBuilder, peBlob); } diff --git a/src/Compilers/Server/VBCSCompiler/VBCSCompiler.csproj b/src/Compilers/Server/VBCSCompiler/VBCSCompiler.csproj index 2fe5a80c0de..08fda4ae9db 100644 --- a/src/Compilers/Server/VBCSCompiler/VBCSCompiler.csproj +++ b/src/Compilers/Server/VBCSCompiler/VBCSCompiler.csproj @@ -36,9 +36,6 @@ CoreClrAnalyzerAssemblyLoader.cs - - CoreClrShim.cs - DesktopAnalyzerAssemblyLoader.cs diff --git a/src/Compilers/VisualBasic/Test/CommandLine/CommandLineTests.vb b/src/Compilers/VisualBasic/Test/CommandLine/CommandLineTests.vb index 57919525606..a1edda264c8 100644 --- a/src/Compilers/VisualBasic/Test/CommandLine/CommandLineTests.vb +++ b/src/Compilers/VisualBasic/Test/CommandLine/CommandLineTests.vb @@ -120,7 +120,7 @@ End Module Assert.Equal("", output.ToString().Trim()) End Sub - + Public Sub CreateCompilationWithKeyFile() Dim source = " Public Class C @@ -136,7 +136,7 @@ End Class" Dim cmd = New MockVisualBasicCompiler(dir.Path, {"/nologo", "a.vb", "/keyfile:key.snk"}) Dim comp = cmd.CreateCompilation(TextWriter.Null, New TouchedFileLogger(), NullErrorLogger.Instance) - Assert.True(TypeOf comp.Options.StrongNameProvider Is PortableStrongNameProvider) + Assert.IsType(Of DesktopStrongNameProvider)(comp.Options.StrongNameProvider) End Sub diff --git a/src/Compilers/VisualBasic/Test/Emit/Attributes/InternalsVisibleToAndStrongNameTests.vb b/src/Compilers/VisualBasic/Test/Emit/Attributes/InternalsVisibleToAndStrongNameTests.vb index 94036e9e149..03faa79bc13 100644 --- a/src/Compilers/VisualBasic/Test/Emit/Attributes/InternalsVisibleToAndStrongNameTests.vb +++ b/src/Compilers/VisualBasic/Test/Emit/Attributes/InternalsVisibleToAndStrongNameTests.vb @@ -1214,6 +1214,7 @@ End Class ' confirm header has expected SN signature size Dim peHeaders = New PEHeaders(other.EmitToStream()) Assert.Equal(256, peHeaders.CorHeader.StrongNameSignatureDirectory.Size) + Assert.Equal(CorFlags.ILOnly, peHeaders.CorHeader.Flags) End Sub ''' @@ -1260,13 +1261,8 @@ End Class Using metadata = ModuleMetadata.CreateFromStream(moduleContents) Dim flags = metadata.Module.PEReaderOpt.PEHeaders.CorHeader.Flags - If legacyStrongName Then - ' confirm file does not claim to be signed - Assert.Equal(0, CInt(flags And CorFlags.StrongNameSigned)) - Else - ' portable signing should sign the module - Assert.Equal(CorFlags.StrongNameSigned, CInt(flags And CorFlags.StrongNameSigned)) - End If + ' confirm file does not claim to be signed + Assert.Equal(0, CInt(flags And CorFlags.StrongNameSigned)) Dim token As EntityHandle = metadata.Module.GetTypeRef(metadata.Module.GetAssemblyRef("mscorlib"), "System.Runtime.CompilerServices", "AssemblyAttributesGoHere") Assert.False(token.IsNil) ' could the magic type ref be located? If not then the attribute's not there. diff --git a/src/Compilers/VisualBasic/vbc/vbc.csproj b/src/Compilers/VisualBasic/vbc/vbc.csproj index 46b2dcc00c6..e075a2b414a 100644 --- a/src/Compilers/VisualBasic/vbc/vbc.csproj +++ b/src/Compilers/VisualBasic/vbc/vbc.csproj @@ -34,9 +34,6 @@ CoreClrAnalyzerAssemblyLoader.cs - - CoreClrShim.cs - DesktopBuildClient.cs diff --git a/src/Scripting/Core/Scripting.csproj b/src/Scripting/Core/Scripting.csproj index 7cc32015e8d..5f6c1bbdce5 100644 --- a/src/Scripting/Core/Scripting.csproj +++ b/src/Scripting/Core/Scripting.csproj @@ -45,9 +45,6 @@ Hosting\Resolvers\RelativePathResolver.cs - - CoreClrShim.cs - Hosting\Resolvers\ClrGlobalAssemblyCache.cs diff --git a/src/Test/Utilities/Portable/TestUtilities.csproj b/src/Test/Utilities/Portable/TestUtilities.csproj index 6fc1f83c688..ad72da1e7dc 100644 --- a/src/Test/Utilities/Portable/TestUtilities.csproj +++ b/src/Test/Utilities/Portable/TestUtilities.csproj @@ -67,9 +67,6 @@ - - Assert\CoreClrShim.cs - DesktopAnalyzerAssemblyLoader.cs -- GitLab