Sign tool explicit files #410

JohnTortugo · 2018-08-01T19:09:06Z

Closes: #396

SignToolTask will receive a list of files to be signed ExplicitSignItems, a list of files to be ignored ExplicitExcludeItems
SignToolTask will receive a mapping of PublicKeyToken + CertificateName + StrongName
The SignTool can operate using only these new parameters or with the usual SignToolData.json or both methods; Once these changes are stable and working we'll probably disable support to SignToolData.json
Adding support for overriding the signing certificate for specific File+TargetFramework combinations.
Removing support for SignToolData.json file
Adding unit test cases

src/Microsoft.DotNet.SignTool/src/SignToolTask.cs

@@ -70,14 +87,30 @@ private void ExecuteImpl()
                }
            }

+            var mapTokenToSignInfo = new Dictionary<string, SignInfo>();


src/Microsoft.DotNet.SignTool/src/SignToolTask.cs

@@ -43,6 +44,22 @@ public class SignToolTask : Task
        [Required]
        public string MicroBuildCorePath { get; set; }

+        /// <summary>
+        /// Explicit list of containers / files to be signed.


src/Microsoft.DotNet.SignTool/src/Configuration.cs

-            var map = new Dictionary<string, SignInfo>(StringComparer.OrdinalIgnoreCase);
-            foreach (var item in fileJson.SignList)
+            var mapFileToSignInfo = new Dictionary<string, SignInfo>(StringComparer.OrdinalIgnoreCase);
+            List<string> excludeList = new List<string>();


src/Microsoft.DotNet.SignTool/src/Configuration.cs

+            {
+                if (IsManaged(filePath) == true)
+                {
+                    var fileAsm = System.Reflection.Assembly.LoadFile(filePath);


src/Microsoft.DotNet.SignTool/src/Configuration.cs

+                    else
+                    {
+                        log.LogError($"SignInfo for Public Key Token {publicKeyToken} not found.");
+                        return new SignInfo(null, null);


jaredpar

Need to address the LoadFile issue.

src/Microsoft.DotNet.SignTool/src/SignToolTask.cs

+        /// <summary>
+        /// Explicit list of containers / files to NOT be signed.
+        /// </summary>
+        public string[] ExplicitExcludeItems { get; set; }


JohnTortugo · 2018-08-03T00:15:55Z

PTAL

src/Microsoft.DotNet.SignTool/src/Configuration.cs

 using Microsoft.DotNet.SignTool.Json;
 using Microsoft.Build.Utilities;
+using System.Reflection.Metadata;


src/Microsoft.DotNet.SignTool/src/SignToolTask.cs

+
+        /// <summary>
+        /// Mapping relating PublicKeyToken, Certificate and Strong Name. 
+        /// During signing Certificate and Strong Name will be looked up here based on PublicKeyToken.


src/Microsoft.DotNet.SignTool/src/SignToolTask.cs

+        /// Mapping relating PublicKeyToken, Certificate and Strong Name. 
+        /// During signing Certificate and Strong Name will be looked up here based on PublicKeyToken.
+        /// </summary>
+        public ITaskItem[] StrongNameSignInfo { get; set; }


src/Microsoft.DotNet.SignTool/src/SignToolTask.cs

+
+                    mapTokenToSignInfo.Add(publicKeyToken, signInfo);
+                }
+            }


src/Microsoft.DotNet.SignTool/src/SignToolTask.cs

+
+                    mapFileAndTargetFrameworkToCertificate.Add(new Tuple<string, string>(fileName, targetFramework), certificateName);
+                }
+            }


src/Microsoft.DotNet.SignTool/src/SignToolTask.cs

+                    var targetFramework = item.GetMetadata("TargetFramework").ToLower();
+                    var certificateName = item.GetMetadata("CertificateName").ToLower();
+
+                    mapFileAndTargetFrameworkToCertificate.Add(new Tuple<string, string>(fileName, targetFramework), certificateName);


src/Microsoft.DotNet.SignTool/src/SignToolTask.cs

+                }
+            }
+
+            var mapFileAndTargetFrameworkToCertificate = new Dictionary<Tuple<string, string>, string>();


src/Microsoft.DotNet.SignTool/src/SignToolTask.cs

+                foreach (var item in FileSignInfo)
+                {
+                    var fileName = item.ItemSpec;
+                    var targetFramework = item.GetMetadata("TargetFramework").ToLower();


src/Microsoft.DotNet.SignTool/src/SignToolTask.cs

+                {
+                    var fileName = item.ItemSpec;
+                    var targetFramework = item.GetMetadata("TargetFramework").ToLower();
+                    var certificateName = item.GetMetadata("CertificateName").ToLower();


src/Microsoft.DotNet.SignTool/src/Configuration.cs

-        internal static BatchSignInput TryReadConfigFile(TaskLoggingHelper log, TextReader configReader, string outputPath)
+        internal static BatchSignInput ParseConfigInfo(TaskLoggingHelper log, TextReader configReader, string outputPath, string[] explicitSignList, 
+            Dictionary<string, SignInfo> mapTokenToSignInfo,
+            Dictionary<Tuple<string, string>, string> mapFileAndTargetFrameworkToCertificate)


src/Microsoft.DotNet.SignTool/src/Configuration.cs

-            foreach (var item in fileJson.SignList)
+            var mapFileToSignInfo = new Dictionary<string, SignInfo>(StringComparer.OrdinalIgnoreCase);
+            var excludeList = new List<string>();
+            var publishUrl = "unset";


tmat · 2018-08-03T01:10:41Z

    public string ConfigFilePath { get; set; }

Remove this. The json file is no longer needed.

Refers to: src/Microsoft.DotNet.SignTool/src/SignToolTask.cs:27 in fea3a1b. [](commit_id = fea3a1b, deletion_comment = False)

src/Microsoft.DotNet.SignTool/src/Configuration.cs

+            var excludeList = new List<string>();
+            var publishUrl = "unset";
+
+            if (configReader != null)


src/Microsoft.DotNet.SignTool/src/SignToolTask.cs

+            {
+                foreach (var item in FileSignInfo)
+                {
+                    var fileName = item.ItemSpec;


src/Microsoft.DotNet.SignTool/src/Configuration.cs

                    {
-                        log.LogError($"Duplicate file entry: {relativeFileName}");
+                        log.LogError($"Duplicate file entry: {filePath}");
                    }
                    else


src/Microsoft.DotNet.SignTool/src/BatchSignInput.cs

        /// </summary>
-        internal ImmutableArray<FileName> AssemblyNames { get; }
+        private readonly Dictionary<(string, string, string), string> _fileAndTokenToOverridingInfos;


src/Microsoft.DotNet.SignTool/src/BatchSignInput.cs

+
+                    /// Do we need to override the default certificate this file ?
+                    if (_fileAndTokenToOverridingInfos.TryGetValue(keyForAllTargets, out overridingCertificate) ||
+                        _fileAndTokenToOverridingInfos.TryGetValue(keyForSpecificTarget, out overridingCertificate))


src/Microsoft.DotNet.SignTool/src/BatchSignInput.cs

+                        _fileAndTokenToOverridingInfos.TryGetValue(keyForSpecificTarget, out overridingCertificate))
+                    {
+                        /// If has overriding info, is it for ignoring the file?
+                        if (overridingCertificate != null && overridingCertificate.Equals(SignToolConstants.IgnoreFileCertificateSentinel))


src/Microsoft.DotNet.SignTool/src/BatchSignInput.cs

        /// </summary>
-        internal ImmutableArray<FileName> ZipContainerNames { get; }
+        private readonly Dictionary<string, SignInfo> _mapTokenToSignInfo;


src/Microsoft.DotNet.SignTool/src/SignInfo.cs

 namespace Microsoft.DotNet.SignTool
 {
-    internal struct SignInfo
+    internal class SignInfo


src/Microsoft.DotNet.SignTool/src/BatchSignInput.cs

+                        return signInfo;
+                    }
+                    else
+                    {


src/Microsoft.DotNet.SignTool/src/BatchSignInput.cs


-                if (!string.IsNullOrEmpty(matchingFile))
+                if (IsManaged(fileFullPath) == true)


tests/Microsoft.DotNet.SignTool.Tests/SignToolTests.cs

+            var util = new BatchSignUtil(task.BuildEngine, task.Log, signTool, signingInput, null);
+
+            /// There is a validation inside this method that checks that the files where actually signed
+            /// so it's not duplicated here.


tests/Microsoft.DotNet.SignTool.Tests/SignToolTests.cs

+            {
+                foreach (var item in ((FakeBuildEngine)task.BuildEngine).LogErrorEvents)
+                {
+                    Console.WriteLine(item.Message);


tests/Microsoft.DotNet.SignTool.Tests/SignToolTests.cs

+                    candidate.SignInfo.Certificate == expected.SignInfo.Certificate && 
+                    candidate.SignInfo.StrongName == expected.SignInfo.StrongName))
+                {
+                    task.Log.LogError($"Expected this file ({expected.FullPath}) to be signed with this " +


tests/Microsoft.DotNet.SignTool.Tests/SignToolTests.cs

+                }
+            }
+
+            if (expectedToBeSigned.Count != signingInput.FilesToSign.Count)


- Added Assert for checking the .proj that is generated as side-effect of the signing. - Renamed some variables and some general refactoring. - Added new test case where a overriding certificate is informed.

JohnTortugo · 2018-08-17T19:05:58Z

Hey, I'd like to merge this PR today, can you PTAL and advise if there are any blocking issues? I'm willing to work on other changes but I'd like to merge this one so I can start working on some other things.

I've just tested the tool by patching Sign.proj as suggested by @tmat #464 and it is able to find all files that we have listed to be signed in our current SignToolData.json file.

src/Microsoft.DotNet.SignTool/src/BatchSignInput.cs

        /// </summary>
-        internal ImmutableArray<string> ExternalFileNames { get; }
+        internal Dictionary<FileName, ZipData> ZipDataMap { get; set; }


src/Microsoft.DotNet.SignTool/src/BatchSignInput.cs

-        internal ImmutableDictionary<FileName, FileSignInfo> FileSignInfoMap { get; }
-
-        private ContentUtil contentUtil = new ContentUtil();
+        public List<FileName> FilesToSign = new List<FileName>();


jaredpar · 2018-08-17T20:58:37Z

    internal string GetChecksum(string filePath)

This is an instance method reading from a static cache (_filePathCache). This cache should be made instance as well.

Refers to: src/Microsoft.DotNet.SignTool/src/ContentUtil.cs:26 in e1693b8. [](commit_id = e1693b8, deletion_comment = False)

src/Microsoft.DotNet.SignTool/src/ExplicitCertificateKey.cs

+namespace Microsoft.DotNet.SignTool
+{
+    struct ExplicitCertificateKey
+    {


src/Microsoft.DotNet.SignTool/src/ExplicitCertificateKey.cs

+    {
+        public string FileName;
+        public string PublicKeyToken;
+        public string TargetFramework;


src/Microsoft.DotNet.SignTool/src/FileName.cs


 namespace Microsoft.DotNet.SignTool
 {
-    internal struct FileName : IEquatable<FileName>
+    internal class FileName : IEquatable<FileName>


- Adding back Configuration file/class - Making BatchSignInput a wrapper around Immutable properties - Fixes around coding convention

JohnTortugo · 2018-08-20T17:41:54Z

Hey, I've addressed all your comments already. I'm planning to merge this today after lunch, unless there is a blocking concern/comment. All non-blocking concerns I'll address in a future PR. @jaredpar @tmat

Looks like most of my feedback was addressed

src/Microsoft.DotNet.SignTool/src/SignToolTask.cs

                {
-                    Log.LogError($"File '{MSBuildPath}' not found.");
+                    Log.LogError($"MSBuild was not found at this path: '{MSBuildPath}'.");


src/Microsoft.DotNet.SignTool/src/SignToolTask.cs

+                foreach (var item in FileSignInfo)
+                {
+                    var fileName = item.ItemSpec;
+                    var targetFramework = item.GetMetadata("TargetFramework");


JohnTortugo self-assigned this Aug 1, 2018

JohnTortugo requested review from tmat, jaredpar, MattGal, maririos and chcosta August 1, 2018 19:09

jaredpar reviewed Aug 1, 2018

View reviewed changes

src/Microsoft.DotNet.SignTool/src/SignToolTask.cs Outdated

@@ -70,14 +87,30 @@ private void ExecuteImpl()

}

}

var mapTokenToSignInfo = new Dictionary<string, SignInfo>();

This comment was marked as spam.

Sign in to view

jaredpar reviewed Aug 1, 2018

View reviewed changes

src/Microsoft.DotNet.SignTool/src/Configuration.cs Outdated

else

{

log.LogError($"SignInfo for Public Key Token {publicKeyToken} not found.");

return new SignInfo(null, null);

This comment was marked as spam.

Sign in to view

jaredpar previously requested changes Aug 1, 2018

View reviewed changes

maririos mentioned this pull request Aug 2, 2018

GPG Signing Tool using Azure Key Vault #412

Open

tmat reviewed Aug 2, 2018

View reviewed changes

src/Microsoft.DotNet.SignTool/src/SignToolTask.cs Outdated

/// <summary>

/// Explicit list of containers / files to NOT be signed.

/// </summary>

public string[] ExplicitExcludeItems { get; set; }

This comment was marked as spam.

Sign in to view

tmat reviewed Aug 3, 2018

View reviewed changes

src/Microsoft.DotNet.SignTool/src/Configuration.cs Outdated

using Microsoft.DotNet.SignTool.Json;

using Microsoft.Build.Utilities;

using System.Reflection.Metadata;

This comment was marked as spam.

Sign in to view

tmat reviewed Aug 3, 2018

View reviewed changes

src/Microsoft.DotNet.SignTool/src/SignToolTask.cs Outdated

/// <summary>

/// Mapping relating PublicKeyToken, Certificate and Strong Name.

/// During signing Certificate and Strong Name will be looked up here based on PublicKeyToken.

This comment was marked as spam.

Sign in to view

tmat reviewed Aug 3, 2018

View reviewed changes

src/Microsoft.DotNet.SignTool/src/SignToolTask.cs Outdated

mapTokenToSignInfo.Add(publicKeyToken, signInfo);

}

}

This comment was marked as spam.

Sign in to view

tmat reviewed Aug 3, 2018

View reviewed changes

src/Microsoft.DotNet.SignTool/src/SignToolTask.cs Outdated

mapFileAndTargetFrameworkToCertificate.Add(new Tuple<string, string>(fileName, targetFramework), certificateName);

}

}

This comment was marked as spam.

Sign in to view

tmat reviewed Aug 3, 2018

View reviewed changes

src/Microsoft.DotNet.SignTool/src/SignToolTask.cs Outdated

}

}

var mapFileAndTargetFrameworkToCertificate = new Dictionary<Tuple<string, string>, string>();

This comment was marked as spam.

Sign in to view

tmat reviewed Aug 3, 2018

View reviewed changes

src/Microsoft.DotNet.SignTool/src/SignToolTask.cs Outdated

foreach (var item in FileSignInfo)

{

var fileName = item.ItemSpec;

var targetFramework = item.GetMetadata("TargetFramework").ToLower();

This comment was marked as spam.

Sign in to view

tmat reviewed Aug 3, 2018

View reviewed changes

src/Microsoft.DotNet.SignTool/src/SignToolTask.cs Outdated

{

var fileName = item.ItemSpec;

var targetFramework = item.GetMetadata("TargetFramework").ToLower();

var certificateName = item.GetMetadata("CertificateName").ToLower();

This comment was marked as spam.

Sign in to view

tmat reviewed Aug 3, 2018

View reviewed changes

src/Microsoft.DotNet.SignTool/src/Configuration.cs Outdated

var excludeList = new List<string>();

var publishUrl = "unset";

if (configReader != null)

This comment was marked as spam.

Sign in to view

tmat reviewed Aug 3, 2018

View reviewed changes

src/Microsoft.DotNet.SignTool/src/SignToolTask.cs

{

foreach (var item in FileSignInfo)

{

var fileName = item.ItemSpec;

This comment was marked as spam.

Sign in to view

tmat reviewed Aug 3, 2018

View reviewed changes

src/Microsoft.DotNet.SignTool/src/Configuration.cs Outdated

{

log.LogError($"Duplicate file entry: {relativeFileName}");

log.LogError($"Duplicate file entry: {filePath}");

}

else

This comment was marked as spam.

Sign in to view

tmat reviewed Aug 15, 2018

View reviewed changes

src/Microsoft.DotNet.SignTool/src/BatchSignInput.cs Outdated

/// </summary>

internal ImmutableArray<FileName> ZipContainerNames { get; }

private readonly Dictionary<string, SignInfo> _mapTokenToSignInfo;

This comment was marked as spam.

Sign in to view

tmat reviewed Aug 15, 2018

View reviewed changes

src/Microsoft.DotNet.SignTool/src/SignInfo.cs Outdated

namespace Microsoft.DotNet.SignTool

{

internal struct SignInfo

internal class SignInfo

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

tmat reviewed Aug 15, 2018

View reviewed changes

src/Microsoft.DotNet.SignTool/src/BatchSignInput.cs Outdated

return signInfo;

}

else

{

This comment was marked as spam.

Sign in to view

tmat reviewed Aug 15, 2018

View reviewed changes

src/Microsoft.DotNet.SignTool/src/BatchSignInput.cs Outdated

if (!string.IsNullOrEmpty(matchingFile))

if (IsManaged(fileFullPath) == true)

This comment was marked as spam.

Sign in to view

tmat reviewed Aug 15, 2018

View reviewed changes

tests/Microsoft.DotNet.SignTool.Tests/SignToolTests.cs Outdated

{

foreach (var item in ((FakeBuildEngine)task.BuildEngine).LogErrorEvents)

{

Console.WriteLine(item.Message);

This comment was marked as spam.

Sign in to view

tmat reviewed Aug 15, 2018

View reviewed changes

tests/Microsoft.DotNet.SignTool.Tests/SignToolTests.cs Outdated

}

}

if (expectedToBeSigned.Count != signingInput.FilesToSign.Count)

This comment was marked as spam.

Sign in to view

Changes after PR feedback:

e1693b8

- Added Assert for checking the .proj that is generated as side-effect of the signing. - Renamed some variables and some general refactoring. - Added new test case where a overriding certificate is informed.

jaredpar reviewed Aug 17, 2018

View reviewed changes

src/Microsoft.DotNet.SignTool/src/BatchSignInput.cs Outdated

/// </summary>

internal ImmutableArray<string> ExternalFileNames { get; }

internal Dictionary<FileName, ZipData> ZipDataMap { get; set; }

This comment was marked as spam.

Sign in to view

jaredpar reviewed Aug 17, 2018

View reviewed changes

src/Microsoft.DotNet.SignTool/src/ExplicitCertificateKey.cs Outdated

namespace Microsoft.DotNet.SignTool

{

struct ExplicitCertificateKey

{

This comment was marked as spam.

Sign in to view

jaredpar reviewed Aug 17, 2018

View reviewed changes

src/Microsoft.DotNet.SignTool/src/ExplicitCertificateKey.cs Outdated

{

public string FileName;

public string PublicKeyToken;

public string TargetFramework;

This comment was marked as spam.

Sign in to view

jaredpar reviewed Aug 17, 2018

View reviewed changes

src/Microsoft.DotNet.SignTool/src/FileName.cs Outdated

namespace Microsoft.DotNet.SignTool

{

internal struct FileName : IEquatable<FileName>

internal class FileName : IEquatable<FileName>

This comment was marked as spam.

Sign in to view

JohnTortugo mentioned this pull request Aug 17, 2018

[Signing] Update Signing documentation #387

Closed

Changes after PR feedback:

939de6f

- Adding back Configuration file/class - Making BatchSignInput a wrapper around Immutable properties - Fixes around coding convention

JohnTortugo merged commit 569feb9 into dotnet:master Aug 20, 2018

JohnTortugo deleted the SignToolExplicitFiles branch August 20, 2018 20:57

weshaggard mentioned this pull request Aug 20, 2018

Make SignTool accept a list of files to be signed #396

Closed

weshaggard reviewed Aug 20, 2018

View reviewed changes

src/Microsoft.DotNet.SignTool/src/SignToolTask.cs

foreach (var item in FileSignInfo)

{

var fileName = item.ItemSpec;

var targetFramework = item.GetMetadata("TargetFramework");

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This was referenced Aug 20, 2018

Documentation for refactored SignTool #488

Merged

Port GenFacades, Microsft.Cci.Extensions to Arcade #477

Merged


		if (!string.IsNullOrEmpty(matchingFile))
		if (IsManaged(fileFullPath) == true)

Sign tool explicit files #410

Sign tool explicit files #410

Conversation

JohnTortugo commented Aug 1, 2018 • edited Loading

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

jaredpar left a comment • edited Loading

Choose a reason for hiding this comment

This comment was marked as spam.

JohnTortugo commented Aug 3, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

tmat commented Aug 3, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

JohnTortugo commented Aug 17, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

jaredpar commented Aug 17, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

JohnTortugo commented Aug 20, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

JohnTortugo commented Aug 1, 2018 •

edited

Loading

jaredpar left a comment •

edited

Loading