-
Notifications
You must be signed in to change notification settings - Fork 7.2k
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.
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
Bring back Certificate provider parameters #10622
Conversation
edb9a44
to
4e915bc
Compare
79ea18d
to
a24b03b
Compare
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. |
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 change requires a security review
@iSazonov I don't have time to review now. Blocking this PR, until I can organize a security review. |
Add WIP while waiting security review. |
/// <param name="cert">Certificate object.</param> | ||
/// <param name="pattern">Wildcard pattern for DNS name to search.</param> | ||
/// <returns>True on success, false otherwise.</returns> | ||
internal static bool CertContainsName(X509Certificate2 cert, WildcardPattern pattern) |
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.
Have you verified this will work with punycode?
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.
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?
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.
When the name in the cert is in punycode. This is a compliance requirement. you deleted the code to do this.
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 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.
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 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
Outdated
Show resolved
Hide resolved
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.
LGTM
macOS tests are hanging
|
I did not change anything except the certificate. I tend to think it's a bug outside PowerShell :-( |
30c1b06
to
c4f735e
Compare
@PoshChan Please remind me in 24 hours |
@TravisEz13, this is the reminder you requested 24 hours ago |
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
Can you resolve the conflicts? |
@PoshChan Please remind me in 1 hour |
@TravisEz13, this is the reminder you requested 1 hour ago |
🎉 Handy links: |
PR Summary
Bring back Certificate provider parameters:
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
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.