Add Get-StringHash and Get-FileHash cmdlets by iSazonov · Pull Request #3024 · PowerShell/PowerShell

Ilya (iSazonov) · 2017-01-20T14:07:14Z

Rewrite Get-FileHash on C# and refactoring tests
The original Get-FileHash output type is Microsoft.Powershell.Utility.FileHash. The type was defined by ExtendedTypeDefinition. It is not C# type and reported as PSCustomObject.
Therefore, the output type was changed and now it is native C# type Microsoft.PowerShell.Commands.FileHashInfo.
Add new cmdlet Get-StringHash and add tests
The original idea was to add a InputString parameter to the common cmdlet but due to a conflict with the Path parameter (paths is strings too!) I had to make a new cmdlet to provide a pipeline support for the InputString parameter.

Jason Shirk (lzybkr)

@PowerShell/powershell-committee - needs to review the new cmdlet - parameters, output type, allowing null.

Jason Shirk (lzybkr) · 2017-01-23T20:12:07Z

+        $hashSHA256forEmptyString = "E3B0C44298FC1C149AFBF4C8996FB92427AE41E4649B934CA495991B7852B855"
+
+        # Keep "sHA1" below! It is for testing that the cmdlet accept a hash algorithm name in any case!
+        $testcases =


With the use of a common variable name, this should be moved closer to the use to limit the scope.

Jason Shirk (lzybkr) · 2017-01-23T20:13:00Z

+
+    Context "Default result tests" {
+        It "Should default to correct hash (SHA256)" {
+


Consider removing some of the blank lines just after or just before opening/closing braces.

Jason Shirk (lzybkr) · 2017-01-23T20:15:29Z

+            $result[1] | Should Be $hashSHA256forEmptyString
+        }
+
+        It "Should be able to set the default hash algorithm" {


This test is unnecessary - there is no reason to test PSDefaultParameterValues on specific cmdlets, the generic tests for PSDefaultParameterValues are sufficient.

It is from original tests.
Removed.

Jason Shirk (lzybkr) · 2017-01-23T20:16:04Z

+
+        It "Should not throw for empty or null input string" {
+            Get-StringHash -InputString "" | Should Be $hashSHA256forEmptyString
+            Get-StringHash -InputString $null | Should Be $null


Is this really desirable? Allowing $null?

I thought about this example:

$stringarray | Get-StringHash

If the string array contains a null string member then we throw - is this the desired behavior?
The following example does not cause a exception:

"a",$null | Select-String -Pattern ".*"

$null is not passed through the pipeline and Select-String allows $null anyway, so that's not a good example.

Yes, but key is that null string don't throw an exception here.

Jason Shirk (lzybkr) · 2017-01-23T20:18:19Z

@@ -0,0 +1,62 @@
+Describe "Get-StringHash" -Tags "CI" {


I would expect very few changes to this feature once merged, so maybe we don't need the tests in CI.

We can change it after the merger. (Or just before.)

Jason Shirk (lzybkr) · 2017-01-23T20:45:55Z

+        /// <summary>
+        /// Create FileHashInfo object and output it
+        /// </summary>
+        private void WriteHashResult(String Algorithm, String hash, String path)


Use lowercase string.

Jason Shirk (lzybkr) · 2017-01-23T20:47:06Z

+        /// </summary>
+        /// <value></value>
+        [Parameter(Position = 1)]
+        [ValidateSet(HashAlgorithmNames.SHA1,


The PowerShell version supports additional algorithms - why are they not included here?

It is from original cmdlet and HashAlgorithmNames class.
If there are other useful algorithms, please give a link.
And whether we should do something specifically for Mac?

According to the ValidateSet use here, the original function has:

MACTripleDES

MD5

RIPEMD160

SHA1

SHA256

SHA384

SHA512

I suppose it's not surprising that a couple of these are missing for .Net Core.

We should probably add the missing algorithms for Windows PowerShell or explore alternative implementations to that we can have completely portable scripts.

/cc Steve Lee (@SteveL-MSFT)

Jason Shirk (@lzybkr) if .Net Core doesn't support those, we should treat it as a separate issue (not block this PR). however, we should see if .Net Core will implement them or why they no longer support it (patents? no longer secure?)

I believe these are hashing algorithms, so security isn't the issue. My guess is less demand.

But the real question here - are we OK with a PR that would be a breaking change to Windows PowerShell if we took this code back to Windows as is?

The security concern is that hashes are used to determine if something (like a file/package) has been modified. However, SHA1, for example, has been proven that you can generate collisions meaning that it is feasible someone could modify a file and produce the same resulting SHA1 hash so it appears to be legit.

We can discuss this at the Committee meeting.

Jason Shirk (lzybkr) · 2017-01-23T20:59:59Z

-	It "Should default to correct hash algorithm" {
-	    $result = Get-FileHash $testDocument
+    BeforeAll {
+        New-Variable testDocument -Value (Join-Path -Path (Join-Path -Path $PSScriptRoot -ChildPath assets) -ChildPath testablescript.ps1) -Scope Global -Force


A test shouldn't normally create a variable in global scope.

Jason Shirk (lzybkr) · 2017-01-23T21:01:01Z

+        }

-	    $result.Algorithm | Should Be "MD5"
+        It "Should be able to set the default hash algorithm" {


This test should not be necessary, PSDefaultParameterValues is already well tested.

Jason Shirk (lzybkr) · 2017-01-23T21:04:33Z

-	    $result.Algorithm | Should Be "SHA256"
-	}
+        # Keep "sHA1" below! It is for testing that the cmdlet accept a hash algorithm name in any case!
+        $testcases =


Should be moved closer to the use to limit the scope of the variable.

Jason Shirk (lzybkr) · 2017-01-24T18:10:52Z

-            @{ algorithm = "SHA384"; hash = "656215B6A07011E625206F43E57873F49AD7B36DFCABB70F6CDCE2303D7A603E55D052774D26F339A6D80A264340CB8C" },
-            @{ algorithm = "SHA512"; hash = "C688C33027D89ACAC920545471C8053D8F64A54E21D0415F1E03766DDCDA215420E74FAFD1DC399864C6B6B5723A3358BD337339906797A39090B02229BF31FE" },
-            @{ algorithm = "MD5";    hash = "7B09811D1631C9FD46B39D1D35522F0A" }
+        New-Variable testDocument -Value (Join-Path -Path (Join-Path -Path $PSScriptRoot -ChildPath assets) -ChildPath testablescript.ps1) -Force


Can't this just use $testDocument = Join-Path ...?

Ilya (iSazonov) · 2017-01-25T04:55:47Z

@PowerShell/powershell-committeeTeam
Should we introduce a new type 'Microsoft.PowerShell.Commands.StringHashInfo' for Get-StringHash output similar to Get-FileHash output?

+ $testDocument' in test

Jason Shirk (lzybkr) · 2017-01-26T01:06:43Z

@PowerShell/powershell-committee has decided we need an RFC for the Get-StringHash cmdlet.

The biggest concern was how PowerShell will convert anything to string, so the cmdlet will happily hash objects in a potentially unexpected way.

The output type was also of some concern when combined with allowing null because it will be difficult to match input with output. We think the output needs the original object in addition to the hash.

Jason Shirk (lzybkr) · 2017-01-26T01:08:15Z

The committee also agreed that we do not need any updates with regards to the algorithms supported - we've marked the PR wit Breaking Change so we can document the difference between Windows PowerShell and PowerShell Core.

Ilya (iSazonov) · 2017-01-26T06:28:32Z

What will be the road map for the PR?
Could I remove Get-StringHash code from the PR?

I do not have the forces for the RFC to write the big text correctly in English.
Can we ask the author Keith Hill (@rkeithhill) of the Get-StringHash idea to start this RFC?

Jason Shirk (lzybkr) · 2017-01-26T16:59:48Z

By all means - remove Get-StringHash from the PR.

Your English seems more than sufficient to write a RFC, but if Keith Hill (@rkeithhill) is up for it, I assume he'll jump in.

Removed for future RFC.

Ilya (iSazonov) · 2017-01-27T16:04:18Z

Jason Shirk (@lzybkr) I create new PR with RFC PowerShell/PowerShell-RFC#63

Now Get-StringHash was removed from the PR.
Travis again temporary failed 😕

Ilya (iSazonov) · 2017-01-30T03:51:17Z

Jason Shirk (@lzybkr) Should I do something else in the PR?

Ilya (iSazonov) · 2017-02-04T06:44:05Z

Jason Shirk (@lzybkr) Thanks!

Add Get-StringHash and Get-FileHash

0c09cb5

Microsoft Contribution License Agreements (msftclas) added the cla-already-signed label Jan 20, 2017

PowerShellTeam added the Review - Needed The PR is being reviewed label Jan 20, 2017

Jason Shirk (lzybkr) suggested changes Jan 23, 2017

View reviewed changes

Jason Shirk (lzybkr) added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Jan 23, 2017

Fixes after code review

ffe5dad

Jason Shirk (lzybkr) reviewed Jan 24, 2017

View reviewed changes

Fix 'FileNotFound'

970a976

+ $testDocument' in test

Ilya (iSazonov) force-pushed the gethash branch from 65483d2 to 970a976 Compare January 25, 2017 09:01

Steve Lee (SteveL-MSFT) added the Breaking-Change breaking change that may affect users label Jan 26, 2017

Jason Shirk (lzybkr) added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Jan 26, 2017

Remove Get-StringHash and tests

43c61a8

Removed for future RFC.

Ilya (iSazonov) mentioned this pull request Jan 27, 2017

Add Get-StringHash PowerShell/PowerShell-RFC#63

Merged

Jason Shirk (lzybkr) added 3 commits February 3, 2017 17:48

Remove Get-StringHash

d61fd6a

Remove Get-StringHash

2052e9d

Remove Get-StringHash

be61534

Jason Shirk (lzybkr) approved these changes Feb 4, 2017

View reviewed changes

Jason Shirk (lzybkr) self-assigned this Feb 4, 2017

Jason Shirk (lzybkr) merged commit 2981806 into PowerShell:master Feb 4, 2017

Ilya (iSazonov) removed the Review - Needed The PR is being reviewed label Mar 27, 2017

Matt Gucci (matt9ucci) mentioned this pull request Nov 18, 2017

Remove hash algorithms unsupported in v6.0 MicrosoftDocs/PowerShell-Docs#1873

Merged

2 of 8 tasks

Ilya (iSazonov) deleted the gethash branch April 17, 2020 12:19

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026

Convert Get-FileHash to a cmdlet (PowerShell#3024)

5ec5e7b


		Context "Default result tests" {
		It "Should default to correct hash (SHA256)" {

Conversation

Ilya (iSazonov) commented Jan 20, 2017

Uh oh!

Jason Shirk (lzybkr) 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!

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!

Ilya (iSazonov) Jan 24, 2017 • 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!

Ilya (iSazonov) Jan 24, 2017 • 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!

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!

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!

Ilya (iSazonov) commented Jan 25, 2017

Uh oh!

Jason Shirk (lzybkr) commented Jan 26, 2017

Uh oh!

Jason Shirk (lzybkr) commented Jan 26, 2017

Uh oh!

Ilya (iSazonov) commented Jan 26, 2017

Uh oh!

Jason Shirk (lzybkr) commented Jan 26, 2017

Uh oh!

Ilya (iSazonov) commented Jan 27, 2017

Uh oh!

Ilya (iSazonov) commented Jan 30, 2017

Uh oh!

Ilya (iSazonov) Jan 24, 2017 •

edited

Loading

Ilya (iSazonov) Jan 24, 2017 •

edited

Loading