diff --git a/src/Microsoft.DotNet.SignTool.Tests/FakeSignTool.cs b/src/Microsoft.DotNet.SignTool.Tests/FakeSignTool.cs index aca1cf628c0..893620354be 100644 --- a/src/Microsoft.DotNet.SignTool.Tests/FakeSignTool.cs +++ b/src/Microsoft.DotNet.SignTool.Tests/FakeSignTool.cs @@ -20,7 +20,7 @@ internal FakeSignTool(SignToolArgs args, TaskLoggingHelper log) { } - public override void RemovePublicSign(string assemblyPath) + public override void RemoveStrongNameSign(string assemblyPath) { } diff --git a/src/Microsoft.DotNet.SignTool.Tests/Microsoft.DotNet.SignTool.Tests.csproj b/src/Microsoft.DotNet.SignTool.Tests/Microsoft.DotNet.SignTool.Tests.csproj index 9fa752f7241..1b82bf4cb79 100644 --- a/src/Microsoft.DotNet.SignTool.Tests/Microsoft.DotNet.SignTool.Tests.csproj +++ b/src/Microsoft.DotNet.SignTool.Tests/Microsoft.DotNet.SignTool.Tests.csproj @@ -51,6 +51,7 @@ <_SnToolFiles Include="$(Pkgsn)\sn.exe.*"/> + <_SnToolFiles Include="$(Pkgsn)\1033\*"/> diff --git a/src/Microsoft.DotNet.SignTool.Tests/SignToolTests.cs b/src/Microsoft.DotNet.SignTool.Tests/SignToolTests.cs index 499ba4ce17a..32af62cb648 100644 --- a/src/Microsoft.DotNet.SignTool.Tests/SignToolTests.cs +++ b/src/Microsoft.DotNet.SignTool.Tests/SignToolTests.cs @@ -2622,33 +2622,50 @@ public void ValidStrongNameSignaturesValidateWithFallback() public void SigningSignsAsExpected(string file, string key, bool initiallySigned) { // Make sure this is unique - string resource = GetResourcePath(file, Guid.NewGuid().ToString()); - using (var stream = new FileStream(resource, FileMode.Open)) - { - StrongName.IsSigned(stream).Should().Be(initiallySigned); - stream.Position = 0; - StrongName.Sign(stream, GetResourcePath(key)); - stream.Position = 0; - StrongName.IsSigned(stream).Should().BeTrue(); - } + string resourcePath = GetResourcePath(file, Guid.NewGuid().ToString()); + StrongName.IsSigned(resourcePath).Should().Be(initiallySigned); + StrongName.Sign(resourcePath, GetResourcePath(key)); + StrongName.IsSigned(resourcePath).Should().BeTrue(); // Legacy sn verification works on on Windows only - StrongName.IsSigned_Legacy(resource, s_snPath).Should().Be( + StrongName.IsSigned_Legacy(resourcePath, s_snPath).Should().Be( RuntimeInformation.IsOSPlatform(OSPlatform.Windows)); } + [WindowsOnlyTheory] + [InlineData("OpenSigned.dll", "OpenSignedCorrespondingKey.snk", true)] + [InlineData("DelaySignedWithOpen.dll", "OpenSignedCorrespondingKey.snk", false)] + public void SigningSignsAsExpectedWithLegacyAndVerifiesWithNonLegacy(string file, string key, bool initiallySigned) + { + // Make sure this is unique + string resourcePath = GetResourcePath(file, Guid.NewGuid().ToString()); + StrongName.IsSigned_Legacy(resourcePath, s_snPath).Should().Be(initiallySigned); + // Unset the strong name bit first + StrongName.ClearStrongNameSignedBit(resourcePath); + StrongName.Sign_Legacy(resourcePath, GetResourcePath(key), s_snPath).Should().BeTrue(); + StrongName.IsSigned(resourcePath).Should().BeTrue(); + } + [Fact] public void CannotSignWithTheEcmaKey() { - Action shouldFail = () => StrongName.Sign(GetResourcePath("StrongNamedWithEcmaKey.dll"), GetResourcePath("OpenSignedCorrespondingKey.snk")); - shouldFail.Should().Throw(); + // Using stream variant so that legacy fallback doesn't swallow the exception. + using (var inputStream = File.OpenRead(GetResourcePath("StrongNamedWithEcmaKey.dll"))) + { + Action shouldFail = () => StrongName.Sign(inputStream, GetResourcePath("OpenSignedCorrespondingKey.snk")); + shouldFail.Should().Throw(); + } } [Fact] public void DelaySignedBinaryFailsToSignWithDifferentKey() { - Action shouldFail = () => StrongName.Sign(GetResourcePath("DelaySigned.dll"), GetResourcePath("OpenSignedCorrespondingKey.snk")); - shouldFail.Should().Throw(); + // Using stream variant so that legacy fallback doesn't swallow the exception. + using (var inputStream = File.OpenRead(GetResourcePath("DelaySigned.dll"))) + { + Action shouldFail = () => StrongName.Sign(inputStream, GetResourcePath("OpenSignedCorrespondingKey.snk")); + shouldFail.Should().Throw(); + } } } } diff --git a/src/Microsoft.DotNet.SignTool/src/BatchSignUtil.cs b/src/Microsoft.DotNet.SignTool/src/BatchSignUtil.cs index 790c1d61268..ac38980f1b6 100644 --- a/src/Microsoft.DotNet.SignTool/src/BatchSignUtil.cs +++ b/src/Microsoft.DotNet.SignTool/src/BatchSignUtil.cs @@ -60,8 +60,10 @@ internal void Go(bool doStrongNameCheck) return; } - // Next remove public signing from all of the assemblies; it can interfere with the signing process. - RemovePublicSign(); + // Remove strong name signing, as sn.exe would choke on already signed binaries. + // Our new signing infra is more resilient, but sn.exe may be used as a backup + // or when not signing locally. + RemoveStrongNameSigning(); // Next sign all of the files if (!SignFiles()) @@ -96,14 +98,14 @@ internal void Go(bool doStrongNameCheck) _log.LogMessage(MessageImportance.High, "Build artifacts signed and validated."); } - private void RemovePublicSign() + private void RemoveStrongNameSigning() { foreach (var fileSignInfo in _batchData.FilesToSign.Where(x => x.IsPEFile())) { if (fileSignInfo.SignInfo.ShouldStrongName) { - _log.LogMessage($"Removing public sign: '{fileSignInfo.FileName}'"); - _signTool.RemovePublicSign(fileSignInfo.FullPath); + _log.LogMessage($"Removing strong name signing from: '{fileSignInfo.FileName}'"); + _signTool.RemoveStrongNameSign(fileSignInfo.FullPath); } } } diff --git a/src/Microsoft.DotNet.SignTool/src/RealSignTool.cs b/src/Microsoft.DotNet.SignTool/src/RealSignTool.cs index 537b49b6f35..5a89f12e92f 100644 --- a/src/Microsoft.DotNet.SignTool/src/RealSignTool.cs +++ b/src/Microsoft.DotNet.SignTool/src/RealSignTool.cs @@ -79,20 +79,9 @@ public override bool RunMSBuild(IBuildEngine buildEngine, string projectFilePath return true; } - public override void RemovePublicSign(string assemblyPath) + public override void RemoveStrongNameSign(string assemblyPath) { - using (var stream = new FileStream(assemblyPath, FileMode.Open, FileAccess.ReadWrite, FileShare.Read)) - using (var peReader = new PEReader(stream)) - using (var writer = new BinaryWriter(stream)) - { - if (!ContentUtil.IsPublicSigned(peReader)) - { - return; - } - - stream.Position = peReader.PEHeaders.CorHeaderStartOffset + OffsetFromStartOfCorHeaderToFlags; - writer.Write((UInt32)(peReader.PEHeaders.CorHeader.Flags & ~CorFlags.StrongNameSigned)); - } + StrongName.ClearStrongNameSignedBit(assemblyPath); } public override bool VerifySignedPEFile(Stream assemblyStream) @@ -113,15 +102,6 @@ public override bool VerifyStrongNameSign(string fileFullPath) return true; } - -/* Unmerged change from project 'Microsoft.DotNet.SignTool (net472)' -Before: - return ContentUtil.IsStrongNameSigned(fileFullPath); - } -After: - return DotNet.SignTool.StrongName.IsStrongNameSigned(fileFullPath); - } -*/ return StrongName.IsSigned(fileFullPath); } diff --git a/src/Microsoft.DotNet.SignTool/src/SignTool.cs b/src/Microsoft.DotNet.SignTool/src/SignTool.cs index 9029c9f34da..76b7d648e34 100644 --- a/src/Microsoft.DotNet.SignTool/src/SignTool.cs +++ b/src/Microsoft.DotNet.SignTool/src/SignTool.cs @@ -28,7 +28,7 @@ internal SignTool(SignToolArgs args, TaskLoggingHelper log) _log = log; } - public abstract void RemovePublicSign(string assemblyPath); + public abstract void RemoveStrongNameSign(string assemblyPath); public abstract bool LocalStrongNameSign(IBuildEngine buildEngine, int round, IEnumerable files); @@ -187,32 +187,7 @@ private static void AppendLine(StringBuilder builder, int depth, string text) protected bool LocalStrongNameSign(FileSignInfo file) { - if (!File.Exists(_args.SNBinaryPath) || !_args.SNBinaryPath.EndsWith("sn.exe")) - { - _log.LogError($"Found file that needs to be strong-name sign ({file.FullPath}), but path to 'sn.exe' wasn't specified."); - return false; - } - - _log.LogMessage($"Strong-name signing {file.FullPath} locally."); - - // sn -R - var process = Process.Start(new ProcessStartInfo() - { - FileName = _args.SNBinaryPath, - Arguments = $@"-R ""{file.FullPath}"" ""{file.SignInfo.StrongName}""", - UseShellExecute = false, - WorkingDirectory = TempDir, - }); - - process.WaitForExit(); - - if (process.ExitCode != 0) - { - _log.LogError($"Failed to strong-name sign file {file.FullPath}"); - return false; - } - - return true; + return StrongName.Sign(file.FullPath, file.SignInfo.StrongName, _args.SNBinaryPath, _log); } } } diff --git a/src/Microsoft.DotNet.SignTool/src/StrongName.cs b/src/Microsoft.DotNet.SignTool/src/StrongName.cs index a306f115273..0e7f419ef91 100644 --- a/src/Microsoft.DotNet.SignTool/src/StrongName.cs +++ b/src/Microsoft.DotNet.SignTool/src/StrongName.cs @@ -219,17 +219,81 @@ internal static bool IsSigned_Legacy(string file, string snPath) return process.ExitCode == 0; } + /// + /// Unset the strong name signing bit from a file. This is required for sn + /// + /// + public static void ClearStrongNameSignedBit(string file) + { + using (var stream = new FileStream(file, FileMode.Open, FileAccess.ReadWrite, FileShare.Read)) + using (var peReader = new PEReader(stream)) + using (var writer = new BinaryWriter(stream)) + { + if (!ContentUtil.IsPublicSigned(peReader)) + { + return; + } + + stream.Position = peReader.PEHeaders.CorHeaderStartOffset + FlagsOffsetInCorHeader; + writer.Write((UInt32)(peReader.PEHeaders.CorHeader.Flags & ~CorFlags.StrongNameSigned)); + } + } + /// /// Strong names an existing previously signed or delay-signed binary with keyfile. + /// Fall back to legacy signing if available and new signing fails. /// /// Path to file to sign /// Path to key pair. - public static void Sign(string file, string keyFile) + /// Optional logger + /// Optional path to sn.exe + /// True if the file was signed successfully, false otherwise + public static bool Sign(string file, string keyFile, string snPath = null, TaskLoggingHelper log = null) + { + try + { + using (var metadata = new FileStream(file, FileMode.Open)) + { + Sign(metadata, keyFile); + } + return true; + } + catch (Exception e) + { + if (log != null) + { + log.LogMessage(MessageImportance.High, $"Failed to sign PE file {file} with strong name {keyFile}: {e}"); + } + + if (!string.IsNullOrEmpty(snPath)) + { + // Fall back to the old method of checking for a strong name signature, but only on Windows. + // Otherwise, return false: + return Sign_Legacy(file, keyFile, snPath); + } + } + + return false; + } + + internal static bool Sign_Legacy(string file, string keyfile, string snPath) { - using (var metadata = new FileStream(file, FileMode.Open)) + // sn -R + var process = Process.Start(new ProcessStartInfo() { - Sign(metadata, keyFile); + FileName = snPath, + Arguments = $@"-R ""{file}"" ""{keyfile}""", + UseShellExecute = false + }); + + process.WaitForExit(); + + if (process.ExitCode != 0) + { + return false; } + + return true; } /// @@ -239,7 +303,7 @@ public static void Sign(string file, string keyFile) /// /// /// - public static void Sign(Stream peStream, string keyFile) + internal static void Sign(Stream peStream, string keyFile) { // This process happens as follows: // 1. Open the PE and read into a file. diff --git a/src/Microsoft.DotNet.SignTool/src/ValidationOnlySignTool.cs b/src/Microsoft.DotNet.SignTool/src/ValidationOnlySignTool.cs index 9c540daf669..4423c51089c 100644 --- a/src/Microsoft.DotNet.SignTool/src/ValidationOnlySignTool.cs +++ b/src/Microsoft.DotNet.SignTool/src/ValidationOnlySignTool.cs @@ -25,15 +25,6 @@ internal ValidationOnlySignTool(SignToolArgs args, TaskLoggingHelper log) public override bool LocalStrongNameSign(IBuildEngine buildEngine, int round, IEnumerable files) { - // On non-Windows, we skip strong name signing because sn.exe is not available. - // We could skip it always in the validation sign tool, but it is useful to - // get some level of validation. - - if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) - { - return true; - } - foreach (var file in files) { if (file.SignInfo.ShouldLocallyStrongNameSign) @@ -48,7 +39,7 @@ public override bool LocalStrongNameSign(IBuildEngine buildEngine, int round, IE return true; } - public override void RemovePublicSign(string assemblyPath) + public override void RemoveStrongNameSign(string assemblyPath) { }