Add GetFileHash and VerifyFileHash tasks by natemcmaster · Pull Request #3398 · dotnet/msbuild

natemcmaster · 2018-06-11T15:44:28Z

Add tasks for computing the file hash and verifying an expected file hash using SHA256, SHA384, or SHA512. These tasks were originally part of aspnet/buildtools, then moved to dotnet/arcade. I've polished them up and added better tests/logging. I'm not set on the API names so let me know if there is a better MSBuild convention for the task and property names.

Resolves #3369

cc @jeffkl

jeffkl

Overall looks great, just a few minor comments.

jeffkl · 2018-06-11T15:52:06Z

I think you should just move HashHelper's stuff to GetFileHash and call it from VerifyFileHash.

jeffkl · 2018-06-11T15:56:27Z

Although I doubt this will be instantiated a lot, I think I'd prefer to see these be some sort of Lazy so that we don't end up with N instances of these classes when you might only be using one. Lazy<T> still allocates, perhaps a factory method with a switch? Is there a benefit of having them as static and would it be better to just instantiate them when needed?

jeffkl · 2018-06-11T16:01:06Z

Best practice is to return !Log.HasLoggedErrors;

jeffkl · 2018-06-11T16:03:25Z

-MSB3954: failed to compute file hash '{0}'. File not found. +MSB3954: Failed to compute hash for file '{0}' because it does not exist or is inaccessible.

natemcmaster · 2018-06-11T16:37:00Z

Updated with PR feedback.

AndyGerlicher

Looks good thanks for contributing this!

natemcmaster · 2018-06-11T17:20:21Z

@dotnet-bot test this please

Add tasks for computing the file hash and verifying an expected file hash using SHA256, SHA384, or SHA512.

natemcmaster · 2018-06-11T22:07:56Z

Microsoft.Build.UnitTests.GetSDKReferenceFiles_Tests.GetSDKReferenceFilesTestFixture.VerifyReferencesLoggedX64

Test failure seems unrelated to my change. https://ci2.dot.net/job/Microsoft_msbuild/job/master/job/innerloop_Windows_NT_Full_prtest/597/testReport/junit/Microsoft.Build.UnitTests.GetSDKReferenceFiles_Tests/GetSDKReferenceFilesTestFixture/VerifyReferencesLoggedX64/

@dotnet-bot test Windows_NT Build for Full please

cdmihai · 2018-06-12T16:01:01Z

+        public string Algorithm { get; set; } = DefaultFileHashAlgorithm;
+
+        /// <summary>
+        /// The metadata name where the hash is store in each item. File hash is in hex. Defaults to "FileHash".


typo: "where the hash is stored in each item"

cdmihai · 2018-06-12T16:05:01Z

+                if (!File.Exists(file.ItemSpec))
+                {
+                    Log.LogErrorFromResources("FileHash.FileNotFound", file.ItemSpec);
+                    continue;


Is it worth continuing if a file does not exist?

no, good point. We could run a separate loop first to check File.Exists before ever computing a hash.

👍
Two loops might regress spinning disks a bit, but hopefully it's an ever decreasing problem. :)

cdmihai · 2018-06-12T16:14:40Z

+
+                var hash = ComputeHash(Algorithm, file.ItemSpec);
+                file.SetMetadata("FileHashAlgoritm", Algorithm);
+                file.SetMetadata(MetadataName, ConversionUtilities.ConvertByteArrayToHex(hash));


Should the user be able to choose which of the two representations to use? I assume that they would only use one of them, so the other becomes wasted memory.

Would it be better if we changed the tasks to support a property such as UseBase64Encoding="true" instead? That would allow us to operating in hex or base64 mode and avoid wasting memory.

Maybe an input property called "HashEncoding" with two valid options, hex and base64? And then single metadata / output would be either one or the other.

cdmihai · 2018-06-12T16:19:56Z

+            switch (algorithmName.ToUpperInvariant())
+            {
+                case "SHA256":
+                    return SHA256.Create();


Can these objects be reused for the entire task execution or do they carry state and need to be created for each ComputeHash call?

Aren't task instances recreated?

Yes, but hash algorithm objects are created for each file to hash. Probably not a problem unless thousands of files get hashed.

IIUC HashAlgorithm objects carry state so need to be recreated for each file they are hashing.

cdmihai · 2018-06-12T16:26:55Z

@jeffkl @rainersigwald @natemcmaster
Are these tasks expensive enough to have them yield and reacquire the engine?

jeffkl · 2018-06-12T16:31:25Z

Are these tasks expensive enough to have them yield and reacquire the engine?

I'd imagine it depends on the size of the file. I don't think it would be used to hash huge files, but perhaps it could check the file size and then yield?

natemcmaster · 2018-06-12T19:25:07Z

Are these tasks expensive enough to have them yield and reacquire the engine?

I guess it depends how far you want to go to optimize for hashing large files and/or large numbers of files. I'd have to explore the scenrios more since I'm not sure what the balance is between IO/CPU time. Do we already have a good way to run benchmarks?

cdmihai · 2018-06-12T22:04:23Z

Do we already have a good way to run benchmarks?

It's not easy yet to add arbitrary tests. Probably easiest way would be to create github repos where hashing / checking hook up in the design time build target graph. Then you could create a couple of repos with increasing file size / number of files hashed, run the design time build test against them, and that will give you a csv with times and memory.

cdmihai · 2018-06-12T22:08:42Z

But maybe it's only worth doing this when we actually observe slow run times. :)

…users to select hex or base64 encoding of file hashes

natemcmaster · 2018-06-13T02:11:57Z

I've updated with PR feedback.

CZEMacLeod · 2018-06-13T11:09:34Z

-                    Log.LogErrorFromResources("VerifyFileHash.HashMismatch", File, Algorithm, HashBase64, actualHash);
-                    return false;
-                }
+                var actualHash = encoding == Tasks.HashEncoding.Hex


This should probably use the same EncodeHash function from GetFileHash. Perhaps it could be an extension method of HashEncoding? It would mean that if another encoding method were added it would be less places to change.

CZEMacLeod · 2018-06-13T11:15:57Z

+            }
+        }
+
+        internal static bool TryParseHashEncoding(string value, out HashEncoding encoding)


Is there a particular reason you are using switch with all the hardcoded text strings here rather than just return Enum.TryParse<HashEncoding>(value, true, out encoding);

I am thinking about ensuring that if new encoding methods are added in the future, that it doesn't require changes in lots of places.

No particular reason. Enum.TryParse is better so switched to that.

natemcmaster · 2018-06-13T16:40:43Z

Linux failures appear to be unrelated to my changes. Is this a known issue?

"/mnt/j/w/Microsoft_msbuild/master/innerloop_Ubuntu16.04_CoreCLR_prtest/artifacts/Debug/bootstrap/netcoreapp2.1/MSBuild/MSBuild.dll". Beginning retry 1 in 1000ms. The process cannot access the file '/mnt/j/w/Microsoft_msbuild/master/innerloop_Ubuntu16.04_CoreCLR_prtest/artifacts/Debug/bin/MSBuild.Bootstrap/publish/MSBuild.dll' because it is being used by another process. [/mnt/j/w/Microsoft_msbuild/master/innerloop_Ubuntu16.04_CoreCLR_prtest/src/MSBuild.Bootstrap/MSBuild.Bootstrap.csproj]

@dotnet-bot test Ubuntu16.04 Build for CoreCLR please

jeffkl · 2018-06-13T16:43:30Z

Linux failures appear to be unrelated to my changes. Is this a known issue?

I'll take a look if any fail on your latest commit

natemcmaster · 2018-06-13T17:07:31Z

It must have just been flakiness. Re-running Ubuntu passed. But if you want to investigate that failure more, here's a link: https://ci2.dot.net/job/Microsoft_msbuild/job/master/job/innerloop_Ubuntu16.04_CoreCLR_prtest/619/

jeffkl requested changes Jun 11, 2018

View reviewed changes

jeffkl approved these changes Jun 11, 2018

View reviewed changes

AndyGerlicher approved these changes Jun 11, 2018

View reviewed changes

Add GetFileHash and VerifyFileHash tasks

159e979

Add tasks for computing the file hash and verifying an expected file hash using SHA256, SHA384, or SHA512.

cdmihai reviewed Jun 12, 2018

View reviewed changes

natemcmaster mentioned this pull request Jun 13, 2018

Remove obsolete MSBuild tasks: DownloadFile, ZipArchive, UnzipArchive dotnet/arcade#244

Merged

Add HashEncoding property to GetFileHash and VerifyFileHash to allow …

f770d35

…users to select hex or base64 encoding of file hashes

CZEMacLeod reviewed Jun 13, 2018

View reviewed changes

Address PR feedback

206383a

cdmihai approved these changes Jun 13, 2018

View reviewed changes

cdmihai merged commit c3b2db5 into dotnet:master Jun 13, 2018

natemcmaster mentioned this pull request Jun 13, 2018

Delete the Microsoft.DotNet.Build.Tasks.IO task dotnet/arcade#254

Merged

natemcmaster deleted the filehash-tasks branch June 13, 2018 22:54

rainersigwald mentioned this pull request Dec 10, 2018

Expose GetFileHash & VerifyFileHash for .NET Core #3999

Merged

rainersigwald mentioned this pull request Jan 28, 2019

Add GetFileHash and VerifyFileHash tasks to XSD (for IntelliSense) #4103

Closed

Conversation

natemcmaster commented Jun 11, 2018

Uh oh!

jeffkl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natemcmaster commented Jun 11, 2018

Uh oh!

AndyGerlicher left a comment

Choose a reason for hiding this comment

Uh oh!

natemcmaster commented Jun 11, 2018

Uh oh!

natemcmaster commented Jun 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdmihai Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdmihai Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdmihai commented Jun 12, 2018

Uh oh!

jeffkl commented Jun 12, 2018

Uh oh!

natemcmaster commented Jun 12, 2018

Uh oh!

cdmihai commented Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdmihai commented Jun 12, 2018

Uh oh!

natemcmaster commented Jun 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natemcmaster commented Jun 13, 2018

Uh oh!

jeffkl commented Jun 13, 2018

Uh oh!

natemcmaster commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

cdmihai Jun 12, 2018 •

edited

Loading

cdmihai Jun 12, 2018 •

edited

Loading

cdmihai commented Jun 12, 2018 •

edited

Loading

natemcmaster commented Jun 13, 2018 •

edited

Loading