GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Port CmsMessage cmdlets and Get-PfxCertificate to powershell core by daxian-dbw · Pull Request #3224 · PowerShell/PowerShell

Conversation

@daxian-dbw

@daxian-dbw Dongbo Wang (daxian-dbw) commented Feb 28, 2017

Copy link
Copy Markdown
Member

Fix #2452
CmsMessage depends on Cert store, so they are not enabled in non-windows. Only Get-PfxCertificate is enabled on non-windows.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Dongbo Wang (daxian-dbw) added 4 commits February 23, 2017 17:54
@daxian-dbw
Port *-CmsMessage cmdlets and Get-PfxCertificate
56dddcd
@daxian-dbw
Fix build
3bbfa7b
@daxian-dbw
Port tests for CMS cmdlets
c49df23
@daxian-dbw
enable 'Get-PfxCertificate' on non-Windows and add tests
b7715bc
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Feb 28, 2017
@daxian-dbw
Dongbo Wang (daxian-dbw) requested a review from Steve Lee (SteveL-MSFT) February 28, 2017 00:34
@daxian-dbw Dongbo Wang (daxian-dbw) assigned Travis Plunk (TravisEz13) Feb 28, 2017
@daxian-dbw
Fix an issue with the test
638fc7f
iSazonov
Ilya (iSazonov) reviewed Feb 28, 2017
View reviewed changes
Comment thread src/System.Management.Automation/utils/ClrFacade.cs
/// <summary>
/// Facade for Convert.ToBase64String(bytes, Base64FormattingOptions.InsertLineBreaks)
/// Inserts line breaks after every 76 characters in the string representation.
/// </summary>

@iSazonov Ilya (iSazonov) Feb 28, 2017 •
edited by daxian-dbw
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't [convert]::ToBase64String($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#.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarify!
Closed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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) {

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Closed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1
}
}


@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove unneeded empty line.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1 Outdated
$PSdefaultParameterValues = $defaultParamValues
}

if ($importedCert)

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe safer check isWindows?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1 Outdated

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

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add -Force -ErrorAction SilentlyContinue

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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"

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use new pattern from #3161 ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commented in that PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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 {

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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 {

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same about silently skip.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Closed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1 Outdated
}

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

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Will change it to $errors.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$errors is already busy by PowerShell :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to sleep :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
iSazonov
Ilya (iSazonov) reviewed Feb 28, 2017
View reviewed changes
Comment thread src/Microsoft.PowerShell.Security/security/CertificateCommands.cs

string clearTextPassword = Utils.GetStringFromSecureString(password);

var cert = new X509Certificate2(path, clearTextPassword, X509KeyStorageFlags.DefaultKeySet);

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Closed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1

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

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we use Out-Null?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clear (no "It" block).
Closed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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"

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1

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

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this block import PKI module?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a hack to import a certificate using 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clear.
Closed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add the comment to the code?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a comment there:

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1
}

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

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we reuse $dataEnciphermentCert?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to keep this Base64 encoded cert for

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clear. Ok.
Closed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1 Outdated

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

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we combine three Should Match in one?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Use Should Be instead.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1 Outdated

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

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use platform independent NewLine for future?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Will do.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1 Outdated
$virtualEventLog.Message = $protected

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

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use platform independent NewLine for future?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1 Outdated

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

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use platform independent NewLine for future?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1
}

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

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe replace Get-Process on something more fast?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Closed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@daxian-dbw
Address review comments in tests
85fdd7b
iSazonov
Ilya (iSazonov) reviewed Feb 28, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1 Outdated
{
if ($importedCert)
{
Remove-Item (Join-Path Cert:\CurrentUser\My $importedCert.Thumbprint)

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😦

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Feb 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-ErrorAction SilentlyContinue added.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Dongbo Wang (daxian-dbw) added 2 commits February 28, 2017 12:19
@daxian-dbw
Address some more comments in tests
563983e
@daxian-dbw
Fix another comment in tests
844b5e7
@iSazonov

Ilya (iSazonov) commented Feb 28, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

Tests LGTM.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@daxian-dbw
Dongbo Wang (daxian-dbw) requested a review from Mike Richmond (mirichmo) February 28, 2017 21:46
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Feb 28, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

+Mike Richmond (@mirichmo) to Reviewers.
Note that the product code changes have been checked in SD for quite a while to light up DSC scenarios in NanoServer. This PR moves the code changes to Github and adds more tests.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mirichmo
Mike Richmond (mirichmo) reviewed Mar 1, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1 Outdated
}

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

@mirichmo Mike Richmond (mirichmo) Mar 1, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Mar 1, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mirichmo
Mike Richmond (mirichmo) reviewed Mar 1, 2017
View reviewed changes
Comment thread src/System.Management.Automation/utils/ClrFacade.cs
return encodedRawString;

StringBuilder builder = new StringBuilder(encodedRawString.Length);
int index = 0, remainingLen = encodedRawString.Length;

@mirichmo Mike Richmond (mirichmo) Mar 1, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Mar 1, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Mar 1, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Mar 1, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #3235 to track this category of issues.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@daxian-dbw
Split a test case to 3 It blocks
a12e394
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Mar 1, 2017

Copy link
Copy Markdown
Member Author

Mike Richmond (@mirichmo) I have addressed your comments, can you please take another look?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mirichmo
Mike Richmond (mirichmo) approved these changes Mar 1, 2017
View reviewed changes
@daxian-dbw
Dongbo Wang (daxian-dbw) merged commit fc30ae1 into PowerShell:master Mar 1, 2017
@daxian-dbw
Dongbo Wang (daxian-dbw) deleted the cms branch March 1, 2017 18:51
@SteveL-MSFT Steve Lee (SteveL-MSFT) mentioned this pull request Mar 4, 2017
Investigate Get/Protect/Unprotect-CmsMessage cmdlets #471
Closed
@bgever Bart Verkoeijen (bgever) mentioned this pull request May 6, 2017
Get-pfxCertificate cmdlet not working #473
Closed
@mrhockeymonkey Scott Matthews (mrhockeymonkey) mentioned this pull request Jan 22, 2020
PSDesiredStateConfiguration Cannot Encrypt Credentials on Linux #11656
Closed
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@daxian-dbw
Port CmsMessage cmdlets and Get-PfxCertificate to powershell core (Po…
a6fbbf7 
…werShell#3224)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov Ilya (iSazonov) iSazonov left review comments
@SteveL-MSFT Steve Lee (SteveL-MSFT) Awaiting requested review from SteveL-MSFT
+1 more reviewer
@mirichmo Mike Richmond (mirichmo) mirichmo approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@TravisEz13 Travis Plunk (TravisEz13)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@daxian-dbw @iSazonov @mirichmo @msftclas @TravisEz13
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.