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

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.

lzybkr

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

lzybkr · 2017-01-23T20:12:07Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-StringHash.Tests.ps1

+        $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.

lzybkr · 2017-01-23T20:13:00Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-StringHash.Tests.ps1

+
+    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.

lzybkr · 2017-01-23T20:15:29Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-StringHash.Tests.ps1

+            $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.

lzybkr · 2017-01-23T20:16:04Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-StringHash.Tests.ps1

+
+        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.

lzybkr · 2017-01-23T20:18:19Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-StringHash.Tests.ps1

@@ -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.)

lzybkr · 2017-01-23T20:45:55Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetHash.cs

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


Use lowercase string.

lzybkr · 2017-01-23T20:47:06Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetHash.cs

+        /// </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 @SteveL-MSFT

@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.

lzybkr · 2017-01-23T20:59:59Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-FileHash.Tests.ps1

-	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.

lzybkr · 2017-01-23T21:01:01Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-FileHash.Tests.ps1

+        }

-	    $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.

lzybkr · 2017-01-23T21:04:33Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-FileHash.Tests.ps1

-	    $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.

lzybkr · 2017-01-24T18:10:52Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-FileHash.Tests.ps1

-            @{ 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 ...?

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

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.

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.

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 @rkeithhill of the Get-StringHash idea to start this RFC?

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 @rkeithhill is up for it, I assume he'll jump in.

Removed for future RFC.

iSazonov · 2017-01-27T16:04:18Z

@lzybkr I create new PR with RFC PowerShell/PowerShell-RFC#63

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

iSazonov · 2017-01-30T03:51:17Z

@lzybkr Should I do something else in the PR?

iSazonov · 2017-02-04T06:44:05Z

@lzybkr Thanks!

Add Get-StringHash and Get-FileHash

0c09cb5

msftclas added the cla-already-signed label Jan 20, 2017

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

lzybkr suggested changes Jan 23, 2017

View reviewed changes

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

Fixes after code review

ffe5dad

lzybkr reviewed Jan 24, 2017

View reviewed changes

Fix 'FileNotFound'

970a976

+ $testDocument' in test

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

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

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.

iSazonov mentioned this pull request Jan 27, 2017

Add Get-StringHash PowerShell/PowerShell-RFC#63

Merged

lzybkr added 3 commits February 3, 2017 17:48

Remove Get-StringHash

d61fd6a

Remove Get-StringHash

2052e9d

Remove Get-StringHash

be61534

lzybkr approved these changes Feb 4, 2017

View reviewed changes

lzybkr self-assigned this Feb 4, 2017

lzybkr merged commit 2981806 into PowerShell:master Feb 4, 2017

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

matt9ucci mentioned this pull request Nov 18, 2017

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

Merged

8 tasks

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


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

Conversation

iSazonov commented Jan 20, 2017

Uh oh!

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!

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!

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!

iSazonov commented Jan 25, 2017

Uh oh!

lzybkr commented Jan 26, 2017

Uh oh!

lzybkr commented Jan 26, 2017

Uh oh!

iSazonov commented Jan 26, 2017

Uh oh!

lzybkr commented Jan 26, 2017

Uh oh!

iSazonov commented Jan 27, 2017

Uh oh!

iSazonov commented Jan 30, 2017

Uh oh!

iSazonov Jan 24, 2017 •

edited

Loading

iSazonov Jan 24, 2017 •

edited

Loading