Port CmsMessage cmdlets and Get-PfxCertificate to powershell core #3224

daxian-dbw · 2017-02-28T00:34:36Z

iSazonov · 2017-02-28T05:24:08Z

src/System.Management.Automation/utils/ClrFacade.cs

+        /// Facade for Convert.ToBase64String(bytes, Base64FormattingOptions.InsertLineBreaks)
+        /// Inserts line breaks after every 76 characters in the string representation.
+        /// </summary>
+        internal static string ToBase64StringWithLineBreaks(byte[] bytes)


Haven't we still Base64FormattingOptions in CoreCLR? 😕

Workarround:

PS C:\Program Files\PowerShell\6.0.0.16> $test="0123456789" *10 PS C:\Program Files\PowerShell\6.0.0.16> $test 0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789 PS C:\Program Files\PowerShell\6.0.0.16> [convert]::ToBase64String($test.ToCharArray(), 0, 100,1) MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTIzNDU2 Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTIzNDU2Nzg5MDEyMzQ1Njc4OQ== PS C:\Program Files\PowerShell\6.0.0.16> [convert]::ToBase64String($test.ToCharArray(), 0, 100,0) MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTIzNDU2Nzg5MDEyMzQ1Njc4OQ==

So they expose the 'Base64FormattingOptions' overload in runtime assembly but not in contract assembly, interesting. Since this overload is not in contract, we cannot use it in C# code.

Sorry, I add workarround later. Cannot we use it too?

Isn't [convert]::T 8000 oBase64String($test.ToCharArray(), 0, 100,1) your workaround in powershell? You can do this in powershell because the API ToBase64String(byte[] inArray, int offset, int length, System.Base64FormattingOptions) is exposed in runtime assembly and thus powershell is able to call it using reflection. However, the contract assembly doesn't contain this API. C# code are compiled against the contract assembly, and thus this won't work in C#.

Thanks for clarify!
Closed.

iSazonov · 2017-02-28T05:26:54Z

test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1

+        $cert.Subject | Should Be "CN=MyDataEnciphermentCert"
+    }
+
+    It "Verify CMS message recipient resolution by path" -Skip:(!$IsWindows) {


Maybe better put Windows only tests in separate Describe? (Get-PfxCertificate tests in separate Describe)

I did that initially, but then I had to setup the testing certificate file twice in both Describe. It feels a waste of time given that there are merely 3 Get-PfxCertificate tests and 3 CI level CMS message tests.

Ok. Closed.

iSazonov · 2017-02-28T05:27:50Z

test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1

+    }
+}
+
+


Please remove unneeded empty line.

Oh, I intentionally put 2 empty lines between Describe blocks, and 1 empty line between It blocks.

iSazonov · 2017-02-28T05:30:21Z

test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1

+            $PSdefaultParameterValues = $defaultParamValues
+        }
+
+        if ($importedCert)


Maybe safer check isWindows?

iSazonov · 2017-02-28T05:31:24Z

test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1

+
+        if ($importedCert)
+        {
+            Remove-Item (Join-Path Cert:\CurrentUser\My $importedCert.Thumbprint)


Add -Force -ErrorAction SilentlyContinue

I want to make sure the certificate is removed after the test run. The test should try to not change the system state as much as possible.

iSazonov · 2017-02-28T05:32:53Z

test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1

+            "Hello World" | Protect-CmsMessage -To "SomeThumbprintThatDoesNotExist" -ErrorAction Stop
+            throw "No Exception!"
+        } catch {
+            $_.FullyQualifiedErrorId | Should Be "NoCertificateFound,Microsoft.PowerShell.Commands.ProtectCmsMessageCommand"


Could you use new pattern from #3161 ?

Sure thing.

Just learned that ShouldBeErrorId is not a Pester function. I'm a little worried that when using this function, I won't be able to just run Invoke-Pester .\CmsMessage.Tests.ps1 anymore, unless all test files that use this function explicitly import the helper module.

I put the same comment in that PR. I will postpone changing to this pattern for now.

Preferred way to run tests is to use Start-PSPester. In that PR I already changed Start-PSPester to import the helper module.

well, I don't want to lose the flexibility to just run a single test case using Invoke-Pester 😄

I commented in that PR.

iSazonov · 2017-02-28T05:36:09Z

test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1

+
+            $decrypted = Unprotect-CmsMessage -Path $encryptedPath -To $certLocation
+            $decrypted | Should Be "Hello World`r`nHow are you?`r`n"
+        } finally {


Without Catch can we silently skip the test if there is a throw?

If there is an exception thrown in the try block, then the test will fail:

PS:20> describe "abc" { >> it "def" { >> try { >> $null.Get() | Should Be "yeah" >> } finally { >> Write-Host 'Blah' >> } >> } >> } Describing abc Blah [-] def 34ms You cannot call a method on a null-valued expression. at line: 4 in

Clear. (It is in It block!)
Closed.

iSazonov · 2017-02-28T05:36:56Z

test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1

+            # Decrypt using $importedCert in the Cert store
+            $decrypted = Unprotect-CmsMessage -Path $tempPath
+            $decrypted | Should Be "Hello World"
+        } finally {


The same about silently skip.

iSazonov · 2017-02-28T05:42:39Z

test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1

+    }
+
+    It "Verify CMS message recipient resolution by cert" -Skip:(!$IsWindows) {
+        $ers = $null


Confuse $ers <-> $cert :-) Below too.

Good point. Will change it to $errors.

$errors is already busy by PowerShell :-)

$Error is used by powershell, $errors is good :)

I am going to sleep :-)

iSazonov · 2017-02-28T16:46:24Z

src/Microsoft.PowerShell.Security/security/CertificateCommands.cs

-
+            string clearTextPassword = Utils.GetStringFromSecureString(password);
+
+            var cert = new X509Certificate2(path, clearTextPassword, X509KeyStorageFlags.DefaultKeySet);


I see SecureString in corefx but not in alfa.16 😕

PS C:\Program Files\PowerShell\6.0.0.16> [X509Certificate2]::New OverloadDefinitions ------------------- System.Security.Cryptography.X509Certificates.X509Certificate2 new() System.Security.Cryptography.X509Certificates.X509Certificate2 new(byte[] rawData) System.Security.Cryptography.X509Certificates.X509Certificate2 new(byte[] rawData, string password) System.Security.Cryptography.X509Certificates.X509Certificate2 new(byte[] rawData, string password, System.Security.Cryptography.X509Certificates.X509KeyStorageFlags keyStorageFlags) System.Security.Cryptography.X509Certificates.X509Certificate2 new(System.IntPtr handle) System.Security.Cryptography.X509Certificates.X509Certificate2 new(string fileName) System.Security.Cryptography.X509Certificates.X509Certificate2 new(string fileName, string password) System.Security.Cryptography.X509Certificates.X509Certificate2 new(string fileName, string password, System.Security.Cryptography.X509Certificates.X509KeyStorageFlags keyStorageFlags)

Thanks for noticing this. We need to update our code to use the new APIs once we move to a newer version of .NET Core packages. I opened this issue #3228 to track this effort as well as the list of similar code instances. Please feel free to update the list when you spot one.

iSazonov · 2017-02-28T17:05:44Z

test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1

+
+    BeforeAll {
+        $certLocation = Create-TestCertificate
+        $certLocation | Should Not BeNullOrEmpty | Out-Null


Why we use Out-Null?

It will output True to the console without Out-Null.

PS:18> describe "abc" { BeforeAll { "abc" | Should Not BeNullOrEmpty } } Describing abc True

Clear (no "It" block).
Closed.

iSazonov · 2017-02-28T17:13:33Z

test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1

+        $protected.IndexOf("-----BEGIN CMS-----") | Should Be 0
+
+        $message = $protected | Get-CmsMessage
+        $message.Recipients[0].IssuerName | Should Be "CN=MyDataEnciphermentCert"


Non-informative exception may be here. Can we use $message.Recipients.IssuerName ?

iSazonov · 2017-02-28T17:18:38Z

test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1

+
+                try {
+                    $modulePathCopy = $env:PSMODULEPATH
+                    $env:PSMODULEPATH = $null


Isn't this block import PKI module?

It's a hack to import a certificate u 1CF5 sing PKI module from powershell core. PKI module is not carried by powershell core, but we only run those tests on windows platform, which makes this hack possible.

Clear.
Closed.

Maybe add the comment to the code?

There is a comment there:

PKI module is not available for PowerShell Core, so we need to use Windows PowerShell to import the cert

iSazonov · 2017-02-28T17:21:48Z

test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1

+    }
+
+    It "Verify message recipient resolution by Base64Cert" {
+        $certContent = "


Can we reuse $dataEnciphermentCert?

I would like to keep this Base64 encoded cert for

to use a Base64 encoded cert directly for CmsMessageRecipient, ascii armor "-----BEGIN CERTIFICATE-----" and "-----END CERTIFICATE-----" need to be present to surround the cert text.

this test is to verify if the passed-in text can be properly decoded (remove armor and then decode Base64), and thus it chooses the "Encryption" purpose because that doesn't require the private key in the cert. That's why the cert text here is smaller in size.

Clear. Ok.
Closed.

iSazonov · 2017-02-28T17:33:35Z

test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1

+
+        $decryptedWithContext | Should Match "Pre content"
+        $decryptedWithContext | Should Match "World"
+        $decryptedWithContext | Should Match "Post content"


Can we combine three Should Match in one?

Fixed. Use Should Be instead.

iSazonov · 2017-02-28T17:34:32Z

test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1

+
+    It "Verify Unprotect-CmsMessage lets you include context" {
+        $protected = "Hello World" | Protect-CmsMessage -To $certLocation
+        $adjustedProtected = "Pre content`r`n" + $protected + "`r`nPost content"


Maybe use platform independent NewLine for future?

Good point. Will do.

iSazonov · 2017-02-28T17:35:31Z

test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1

+        $virtualEventLog.Message = $protected
+
+        $decrypted = $virtualEventLog | Unprotect-CmsMessage -To $certLocation
+        $decrypted | Should Be "Encrypted Message1`r`nEncrypted Message2"


Maybe use platform independent NewLine for future?

iSazonov · 2017-02-28T17:35:50Z

test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1

+
+        $processed = $virtualEventLog | Unprotect-CmsMessage -To $certLocation -IncludeContext
+        $processed.Id | Should Be $savedId
+        $processed.Message | Should Be "Encrypted Message1`r`nEncrypted Message2"


Maybe use platform independent NewLine for future?

iSazonov · 2017-02-28T17:37:48Z

test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1

+    }
+
+    It "Verify protect message using OutString" {
+        $protected = Get-Process -Id $pid | Protect-CmsMessage -To $certLocation


Maybe replace Get-Process on something more fast?

I think it's OK. This test is with feature tag, and only run in nightly build.

iSazonov · 2017-02-28T20:09:25Z

test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1

+        {
+            if ($importedCert)
+            {
+                Remove-Item (Join-Path Cert:\CurrentUser\My $importedCert.Thumbprint)


Sorry, I don't understand why not add -Force -ErrorAction SilentlyContinue

Ah, -Force is desired, but not -EA SilentlyConteinue. I will update the test. If it fails to remove, then I want the test to fail.

If we want the test maybe make explicit test? Why use cleanup code for test?

Good point. Maybe we should have some Cert:\ provider tests to cover this. I just did a quick search and we have zero Cert provider test 😦

-ErrorAction SilentlyContinue added.

iSazonov · 2017-02-28T20:28:47Z

daxian-dbw · 2017-02-28T21:50:39Z

mirichmo · 2017-03-01T05:07:51Z

test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1

+        }
+
+        # Should have round-tripped content
+        $result = "Hello World" | Unprotect-CmsMessage -IncludeContext


This looks like it should be a separate It. The errors are similar, but the tests are different.

mirichmo · 2017-03-01T05:11:58Z

src/System.Management.Automation/utils/ClrFacade.cs

+
+            StringBuilder builder = new StringBuilder(encodedRawString.Length);
+            int index = 0, remainingLen = encodedRawString.Length;
+            while (remainingLen > 76)


Was this copied from somewhere in CoreCLR or is it new implementation? I'm curious if it needs additional testing or if it is sufficiently exercised by your tests.

This is new implementation based on the base64LineBreakPosition from the source code of [System.Convert]::ToBase64String(byte[] inArray, Base64FormattingOptions options).
This method is used in Protect-CmsMessage through CmsUtils.Encrypt. I believe it's well exercised by the tests.

@daxian-dbw Perhaps it makes sense to open new Issue to migrate to .Net Core method when it become available?

Opened #3235 to track this category of issues.

daxian-dbw · 2017-03-01T06:42:38Z

daxian-dbw added 4 commits February 23, 2017 17:54

Port *-CmsMessage cmdlets and Get-PfxCertificate

56dddcd

Fix build

3bbfa7b

Port tests for CMS cmdlets

c49df23

enable 'Get-PfxCertificate' on non-Windows and add tests

b7715bc

msftclas added the cla-not-required label Feb 28, 2017

daxian-dbw requested a review from SteveL-MSFT February 28, 2017 00:34

daxian-dbw assigned TravisEz13 Feb 28, 2017

Fix an issue with the test

638fc7f

iSazonov reviewed Feb 28, 2017

View reviewed changes

Address review comments in tests

85fdd7b

iSazonov reviewed Feb 28, 2017

View reviewed changes

daxian-dbw added 2 commits February 28, 2017 12:19

Address some more comments in tests

563983e

Fix another comment in tests

844b5e7

daxian-dbw requested a review from mirichmo February 28, 2017 21:46

mirichmo reviewed Mar 1, 2017

View reviewed changes

Split a test case to 3 It blocks

a12e394

mirichmo approved these changes Mar 1, 2017

View reviewed changes

daxian-dbw merged commit fc30ae1 into PowerShell:master Mar 1, 2017

daxian-dbw deleted the cms branch March 1, 2017 18:51

SteveL-MSFT mentioned this pull request Mar 4, 2017

Investigate Get/Protect/Unprotect-CmsMessage cmdlets #471

Closed

bgever mentioned this pull request May 6, 2017

Get-pfxCertificate cmdlet not working #473

Closed

mrhockeymonkey mentioned this pull request Jan 22, 2020

PSDesiredStateConfiguration Cannot Encrypt Credentials on Linux #11656

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port CmsMessage cmdlets and Get-PfxCertificate to powershell core #3224

Port CmsMessage cmdlets and Get-PfxCertificate to powershell core #3224

Port CmsMessage cmdlets and Get-PfxCertificate to powershell core #3224

Port CmsMessage cmdlets and Get-PfxCertificate to powershell core #3224

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment