Remove refactoring

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)
         {
         }
 

src/Microsoft.DotNet.SignTool.Tests/Microsoft.DotNet.SignTool.Tests.csproj

@@ -51,6 +51,7 @@
   <Target Name="_CopySnTool" AfterTargets="ResolveProjectReferences" Condition="$([MSBuild]::IsOSPlatform('windows'))">
     <ItemGroup>
       <_SnToolFiles Include="$(Pkgsn)\sn.exe.*"/>
+      <_SnToolFiles Include="$(Pkgsn)\1033\*"/>
     </ItemGroup>
     <ItemGroup>
       <Content Include="@(_SnToolFiles)" CopyToOutputDirectory="PreserveNewest" Link="tools\sn\%(RecursiveDir)%(Filename)%(Extension)" />

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<NotImplementedException>();
+            // 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<NotImplementedException>();
+            }
         }
 
         [Fact]
         public void DelaySignedBinaryFailsToSignWithDifferentKey()
         {
-            Action shouldFail = () => StrongName.Sign(GetResourcePath("DelaySigned.dll"), GetResourcePath("OpenSignedCorrespondingKey.snk"));
-            shouldFail.Should().Throw<InvalidOperationException>();
+            // 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<InvalidOperationException>();
+            }
         }
     }
 }

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);
                 }
             }
         }

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);
         }
 

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<FileSignInfo> 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 <path_to_file> <path_to_snk>
-            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);
         }
     }
 }

src/Microsoft.DotNet.SignTool/src/StrongName.cs

@@ -219,17 +219,81 @@ internal static bool IsSigned_Legacy(string file, string snPath)
             return process.ExitCode == 0;
         }
 
+        /// <summary>
+        /// Unset the strong name signing bit from a file. This is required for sn
+        /// </summary>
+        /// <param name="file"></param>
+        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));
+            }
+        }
+
         /// <summary>
         /// Strong names an existing previously signed or delay-signed binary with keyfile.
+        /// Fall back to legacy signing if available and new signing fails.
         /// </summary>
         /// <param name="file">Path to file to sign</param>
         /// <param name="keyFile">Path to key pair.</param>
-        public static void Sign(string file, string keyFile)
+        /// <param name="log">Optional logger</param>
+        /// <param name="snPath">Optional path to sn.exe</param>
+        /// <returns>True if the file was signed successfully, false otherwise</returns>
+        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 <path_to_file> <path_to_snk>
+            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;
         }
 
         /// <summary>
@@ -239,7 +303,7 @@ public static void Sign(string file, string keyFile)
         /// <param name="keyFile"></param>
         /// <exception cref="InvalidOperationException"></exception>
         /// <exception cref="Exception"></exception>
-        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.

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<FileSignInfo> 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)
         {
         }