Bring back Certificate provider parameters by iSazonov · Pull Request #10622 · PowerShell/PowerShell

iSazonov · 2019-09-25T10:16:40Z

PR Summary

Bring back Certificate provider parameters:
- DNSName
- DocumentEncryptionCert
- EKU
- ExpiringInDays
- SSLServerAuthentication
Add new tests and update existing tests. (There are many style issues - will fix in follow PR.)
Update test certificate for EKU and DNSName tests.
Remove old unneeded code.

PR Context

Related #3847

After removing undocumented certificate API in PR #3818 we lost some parameters in Certificate provider.

We need to review documentation because the parameters is not all documented and it seems they do not always documented correctly.
(For reference https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.security/about/about_certificate_provider?view=powershell-6)

PR Checklist

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

iSazonov · 2019-09-25T13:59:28Z

It seems Get-PfxCertificate crush the pwsh process if open "GoodCertificate" which I updated. All works well on Windows and Linux but fail on MacOs. Looks like a bug on MacOs. At least pwsh should return an error and doesn't crush.
I haven't MacOs and can not investigate in depth. I need help to resolve the issue.

TravisEz13

This change requires a security review

TravisEz13 · 2019-09-25T21:37:59Z

@iSazonov I don't have time to review now. Blocking this PR, until I can organize a security review.

iSazonov · 2019-09-26T03:10:51Z

Add WIP while waiting security review.

TravisEz13 · 2019-10-04T17:27:21Z

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

Have you verified this will work with punycode?

Do you mean that Windows PowerShell support also punycode in dnsname filter? I did not find this in docs https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.security/about/about_certificate_provider?view=powershell-6

What is a scenario where it could be used?

When the name in the cert is in punycode. This is a compliance requirement. you deleted the code to do this.

Clear about compliance. Not clear what code do you mean.
Also docs say:

DnsName <Microsoft.PowerShell.Commands.DnsNameRepresentation>
This parameter gets certificates that have the specified domain name or name pattern in the DNSNameList property of the certificate. The value of this parameter can either be "Unicode" or "ASCII". Punycode values are converted to Unicode. Wildcard characters (*) are permitted.

It is not clear how punycode converted to Unicode. It seems this did unpublic code (in P/Invoke) which we removed long ago.

I looked original code which was removed and see only one difference - DNSName parameter was DnsNameRepresentation type, now string.
In both cases DnsNameRepresentation is initialized by one constructor with string parameter which assigned to DNSname and punycode - no conversion. So new code works for punycode.
Question is should we change type from string to DnsNameRepresentation?

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

src/System.Management.Automation/security/SecuritySupport.cs

PaulHigin

LGTM

TravisEz13 · 2019-10-07T22:32:06Z

macOS tests are hanging

  Describing CmsMessage cmdlets and Get-PfxCertificate basic tests
Certificate written to /var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/protectedCert.pfx
Enter password:                                                                                                                                                                                                                                                                                                                                 
Execution of { & $powershell $PSFlags -c $command } by build.psm1: line 1231 failed with exit code 1
At /Users/vsts/agent/2.158.0/work/1/s/build.psm1:2074 char:17
+                 throw $errorMessage
+                 ~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : OperationStopped: (Execution of { & $p\u2026ed with exit code 1:String) [], RuntimeException
+ FullyQualifiedErrorId : Execution of { & $powershell $PSFlags -c $command } by build.psm1: line 1231 failed with exit code 1

iSazonov · 2019-10-08T03:07:36Z

@TravisEz13

macOS tests are hanging

I did not change anything except the certificate. I tend to think it's a bug outside PowerShell :-(
I haven't MacOs and I only can revert to old certificate and generate new for new tests to
minimize area.

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

TravisEz13 · 2020-05-21T22:58:01Z

@PoshChan Please remind me in 24 hours

PoshChan · 2020-05-22T22:58:48Z

@TravisEz13, this is the reminder you requested 24 hours ago

ghost · 2020-05-30T02:00:14Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Mainainer, Please provide feedback and/or mark it as Waiting on Author

TravisEz13 · 2020-05-30T17:35:51Z

Can you resolve the conflicts?

TravisEz13 · 2020-06-01T19:57:52Z

@PoshChan Please remind me in 1 hour

PoshChan · 2020-06-01T20:58:05Z

@TravisEz13, this is the reminder you requested 1 hour ago

ghost · 2020-06-25T19:06:53Z

🎉v7.1.0-preview.4 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

iSazonov added Documentation Needed in this repo Documentation is needed in this repo CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log labels Sep 25, 2019

iSazonov added this to the 7.0.0-preview.5 milestone Sep 25, 2019

iSazonov requested review from SteveL-MSFT, TravisEz13, adityapatwardhan and daxian-dbw September 25, 2019 10:16

iSazonov assigned TravisEz13 Sep 25, 2019

iSazonov force-pushed the add-expiringindays branch from edb9a44 to 4e915bc Compare September 25, 2019 12:11

PoshChan reviewed Sep 25, 2019

View reviewed changes

src/Microsoft.PowerShell.Security/security/CertificateProvider.cs Outdated Show resolved Hide resolved

PoshChan reviewed Sep 25, 2019

View reviewed changes

src/Microsoft.PowerShell.Security/security/CertificateProvider.cs Outdated Show resolved Hide resolved

PoshChan reviewed Sep 25, 2019

View reviewed changes

src/Microsoft.PowerShell.Security/security/CertificateProvider.cs Outdated Show resolved Hide resolved

PoshChan reviewed Sep 25, 2019

View reviewed changes

src/Microsoft.PowerShell.Security/security/CertificateProvider.cs Outdated Show resolved Hide resolved

iSazonov force-pushed the add-expiringindays branch from 79ea18d to a24b03b Compare September 25, 2019 13:52

iSazonov requested a review from rjmholt September 25, 2019 14:00

TravisEz13 requested changes Sep 25, 2019

View reviewed changes

ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Sep 25, 2019

iSazonov changed the title ~~Bring back Certificate provider parameters~~ WIP: Bring back Certificate provider parameters Sep 26, 2019

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Sep 26, 2019

TravisEz13 reviewed Oct 4, 2019

View reviewed changes

src/Microsoft.PowerShell.Security/security/CertificateProvider.cs Outdated Show resolved Hide resolved

PaulHigin suggested changes Oct 4, 2019

View reviewed changes

src/Microsoft.PowerShell.Security/security/CertificateProvider.cs Outdated Show resolved Hide resolved

src/System.Management.Automation/security/SecuritySupport.cs Outdated Show resolved Hide resolved

src/System.Management.Automation/security/SecuritySupport.cs Outdated Show resolved Hide resolved

ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Oct 4, 2019

PaulHigin approved these changes Oct 7, 2019

View reviewed changes

TravisEz13 added this to the 7.1.0-preview.1 milestone Nov 23, 2019

Add ExpiringInDays filter in Certificate provider

c4f735e

iSazonov force-pushed the add-expiringindays branch from 30c1b06 to c4f735e Compare March 29, 2020 18:42

PoshChan reviewed Mar 29, 2020

View reviewed changes

src/Microsoft.PowerShell.Security/security/CertificateProvider.cs Show resolved Hide resolved

Fix test

ee6f752

iSazonov modified the milestones: 7.1.0-preview.1, 7.1.0-preview.2 Mar 29, 2020

TravisEz13 modified the milestones: 7.1.0-preview.2, 7.1.0-preview.3 Apr 24, 2020

TravisEz13 modified the milestones: 7.1.0-preview.3, 7.1.0-preview.4 May 14, 2020

ghost added the Review - Needed The PR is being reviewed label May 30, 2020

ghost removed the Review - Needed The PR is being reviewed label May 30, 2020

TravisEz13 added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label May 30, 2020

Merge branch 'master' into add-expiringindays

430fa2a

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label May 31, 2020

TravisEz13 closed this Jun 1, 2020

TravisEz13 reopened this Jun 1, 2020

TravisEz13 approved these changes Jun 2, 2020

View reviewed changes

TravisEz13 merged commit 73e8427 into PowerShell:master Jun 2, 2020

iSazonov deleted the add-expiringindays branch June 3, 2020 02:57

iSazonov mentioned this pull request Jun 3, 2020

Need to document some Certificate provider parameters MicrosoftDocs/PowerShell-Docs#6059

Closed

12 tasks

iSazonov removed the Documentation Needed in this repo Documentation is needed in this repo label Jun 3, 2020

Conversation

iSazonov commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iSazonov commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TravisEz13 left a comment

Choose a reason for hiding this comment

Uh oh!

TravisEz13 commented Sep 25, 2019

Uh oh!

iSazonov commented Sep 26, 2019

Uh oh!

TravisEz13 Oct 4, 2019

Choose a reason for hiding this comment

Uh oh!

iSazonov Oct 4, 2019 • 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!

TravisEz13 Oct 4, 2019

Choose a reason for hiding this comment

Uh oh!

iSazonov Oct 4, 2019

Choose a reason for hiding this comment

Uh oh!

iSazonov Oct 8, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PaulHigin left a comment

Choose a reason for hiding this comment

Uh oh!

TravisEz13 commented Oct 7, 2019

Uh oh!

iSazonov commented Oct 8, 2019

Uh oh!

Uh oh!

TravisEz13 commented May 21, 2020

Uh oh!

PoshChan commented May 22, 2020

Uh oh!

ghost commented May 30, 2020

Uh oh!

TravisEz13 commented May 30, 2020

Uh oh!

TravisEz13 commented Jun 1, 2020

Uh oh!

PoshChan commented Jun 1, 2020

Uh oh!

ghost commented Jun 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

iSazonov commented Sep 25, 2019 •

edited

Loading

iSazonov commented Sep 25, 2019 •

edited

Loading

iSazonov Oct 4, 2019 •

edited

Loading