Skip to content

Add Password parameter to Get-PfxCertificate cmdlet #6113

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
iSazonov merged 10 commits into from
Feb 23, 2018
Merged

Add Password parameter to Get-PfxCertificate cmdlet #6113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
15bf04e
Add Password parameter to Get-PfxCertificate cmdlet
maybe-hello-world Feb 6, 2018
b02f60c
Auto-Implemented Property used instead of manual implemention
maybe-hello-world Feb 6, 2018
b3955f5
Small comments changes
maybe-hello-world Feb 7, 2018
6312afd
Change behaviour to non-breaking and write tests
maybe-hello-world Feb 8, 2018
8592a2e
Fix test
maybe-hello-world Feb 8, 2018
64bd54a
Make test crossplatfrom
maybe-hello-world Feb 9, 2018
415b92e
Tabs to spaces cleanup
maybe-hello-world Feb 9, 2018
f46750f
Rename Force switch
maybe-hello-world Feb 9, 2018
17c1c41
Change generated certificate to existing
maybe-hello-world Feb 14, 2018
fd5d9f0
Rename switch according to committee decision
maybe-hello-world Feb 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 37 additions & 43 deletions src/Microsoft.PowerShell.Security/security/CertificateCommands.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,18 @@ public string[] LiteralPath
}
private bool _isLiteralPath = false;

/// <summary>
/// Gets or sets the password for unlocking the certificate.
/// </summary>
[Parameter(Mandatory = false)]
public SecureString Password { get; set; }

/// <summary>
/// Do not prompt for password if not given.
/// </summary>
[Parameter(Mandatory = false)]
public SwitchParameter NoPromptForPassword { get; set; }

//
// list of files that were not found
//
Expand Down Expand Up @@ -130,47 +142,35 @@ protected override void ProcessRecord()
}
else
{
if (Password == null && !NoPromptForPassword.IsPresent)
{
try
{
cert = GetCertFromPfxFile(resolvedProviderPath, null);
WriteObject(cert);
continue;
}
catch (CryptographicException)
{
Password = SecurityUtils.PromptForSecureString(
Host.UI,
CertificateCommands.GetPfxCertPasswordPrompt);
}
}

try
{
cert = GetCertFromPfxFile(resolvedProviderPath);
cert = GetCertFromPfxFile(resolvedProviderPath, Password);
}
catch (CryptographicException)
catch (CryptographicException e)
{
//
// CryptographicException is thrown when any error
// occurs inside the crypto class library. It has a
// protected member HResult that indicates the exact
// error but it is not available outside the class.
// Thus we have to assume that the above exception
// was thrown because the pfx file is password
// protected.
//
SecureString password = null;

string prompt = null;
prompt = CertificateCommands.GetPfxCertPasswordPrompt;

password = SecurityUtils.PromptForSecureString(Host.UI, prompt);
Copy link
Member

@TravisEz13 TravisEz13 Feb 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We should not change the behavior to prompt using the Host.UI when a password is not supplied.

try
{
cert = GetCertFromPfxFile(resolvedProviderPath,
password);
}
catch (CryptographicException e)
{
//
// since we cannot really figure out the
// meaning of a given CryptographicException
// we have to use NotSpecified category here
//
ErrorRecord er =
new ErrorRecord(e,
"GetPfxCertificateUnknownCryptoError",
ErrorCategory.NotSpecified,
null);
WriteError(er);
continue;
}
ErrorRecord er =
new ErrorRecord(e,
"GetPfxCertificateUnknownCryptoError",
ErrorCategory.NotSpecified,
null);
WriteError(er);
continue;
}

WriteObject(cert);
Expand Down Expand Up @@ -210,12 +210,6 @@ protected override void ProcessRecord()
}
}

private static X509Certificate2 GetCertFromPfxFile(string path)
{
X509Certificate2 cert = new X509Certificate2(path);
return cert;
}

private static X509Certificate2 GetCertFromPfxFile(string path, SecureString password)
{
var cert = new X509Certificate2(path, password, X509KeyStorageFlags.DefaultKeySet);
Expand Down
22 changes: 18 additions & 4 deletions test/powershell/Modules/Microsoft.PowerShell.Security/CmsMessage.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
Import-Module (Join-Path -Path $PSScriptRoot 'certificateCommon.psm1') -Force

Describe "CmsMessage cmdlets and Get-PfxCertificate basic tests" -Tags "CI" {

BeforeAll {
$certLocation = New-GoodCertificate
$certLocation | Should Not BeNullOrEmpty | Out-Null

$protectedCertLocation = New-ProtectedCertificate
$protectedCertLocation | Should Not BeNullOrEmpty | Out-Null
}

It "Verify Get-PfxCertificate -FilePath" {
Expand All @@ -22,6 +25,17 @@ Describe "CmsMessage cmdlets and Get-PfxCertificate basic tests" -Tags "CI" {
$cert.Subject | Should Be "CN=MyDataEnciphermentCert"
}

It "Verify Get-PfxCertificate right password" {
$pass = ConvertTo-SecureString "password" -AsPlainText -Force
$cert = Get-PfxCertificate $protectedCertLocation -Password $pass
$cert.Subject | Should Be "CN=localhost"
}

It "Verify Get-PfxCertificate wrong password" {
$pass = ConvertTo-SecureString "wrongpass" -AsPlainText -Force
$e = { Get-PfxCertificate $protectedCertLocation -Password $pass -ErrorAction Stop } | ShouldBeErrorId "GetPfxCertificateUnknownCryptoError,Microsoft.PowerShell.Commands.GetPfxCertificateCommand"
}

It "Verify CMS message recipient resolution by path" -Skip:(!$IsWindows) {
$errors = $null
$recipient = [System.Management.Automation.CmsMessageRecipient] $certLocation
Expand Down Expand Up @@ -71,9 +85,9 @@ Describe "CmsMessage cmdlets thorough tests" -Tags "Feature" {
# Skip for non-Windows platforms
$defaultParamValues = $PSdefaultParameterValues.Clone()
$PSdefaultParameterValues = @{ "it:skip" = $true }
}
}
}

AfterAll {
if($IsWindows)
{
Expand Down Expand Up @@ -318,7 +332,7 @@ Describe "CmsMessage cmdlets thorough tests" -Tags "Feature" {

# Validate they all match the EKU
$correctMatching = $foundCerts | Where-Object {
($_.EnhancedKeyUsageList.Count -gt 0) -and
($_.EnhancedKeyUsageList.Count -gt 0) -and
($_.EnhancedKeyUsageList[0].ObjectId -eq '1.3.6.1.4.1.311.80.1')
}
# "All Document Encryption Cert should have had correct EKU"
Expand Down
17 changes: 16 additions & 1 deletion test/powershell/Modules/Microsoft.PowerShell.Security/certificateCommon.psm1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,21 @@ OksttXT1kXf+aez9EzDlsgQU4ck78h0WTy01zHLwSKNWK4wFFQM=
return $certLocation
}

Function New-ProtectedCertificate
Copy link
Contributor

@markekraus markekraus Feb 9, 2018

Choose a reason for hiding this comment

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

We do have 2 password protected PFX in the project already

Maybe they could just be copied to the test drive or accessed directly?

Copy link
Contributor Author

@maybe-hello-world maybe-hello-world Feb 9, 2018

Choose a reason for hiding this comment

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

Well, but is it good to rely on WebListener's location and certs? If WebListener change in future you'll have to find what it'll break in rather unusual places like this.

Copy link
Contributor

@markekraus markekraus Feb 9, 2018

Choose a reason for hiding this comment

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

While that seems like a logical point.. that change would break the tests here and we would know about it. it's not like moving the certs would leave this tests here passing. Yes, we would have to update the tests at that time. But WebListener is a part of the test suite so we might as well reuse what we can form it.

There is an open issue for consolidating all certs for all tests anyway as they exists in far to many places. My comment is an attempt to keep the complexity of certs in our tests at the current level without increasing it further.

Copy link
Member

@SteveL-MSFT SteveL-MSFT Feb 22, 2018

Choose a reason for hiding this comment

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

I agree that we should try to have the test certs used commonly rather than having different ones. However, I would be fine having an issue open to refactor this as a separate PR.

{
<#
.SYNOPSIS
Return existing password-protected pfx certificate

.NOTES
Password: "password"
#>

$certLocation = ".\test\tools\Modules\WebListener\ServerCert.pfx"

return $certLocation
}

Function New-BadCertificate
{
$codeSigningCert = "
Expand Down Expand Up @@ -164,4 +179,4 @@ function Remove-TestCertificates
else {
throw 'Not supported on non-windows platforms'
}
}
}