- Notifications
You must be signed in to change notification settings - Fork7.7k
WIP: UpdateDnsNameList
forX509Certificate2
to useX500DistinguishedName.EnumerateRelativeDistinguishedNames
for Subject Distinguished Name#24733
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
00f7f0e
tod638b44
Compare…edNames and added tests
d638b44
to4b19e13
Compare@KalleOlaviNiemitalo Would |
src/Microsoft.PowerShell.Security/security/CertificateProvider.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Microsoft.PowerShell.Security/security/CertificateProvider.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Microsoft.PowerShell.Security/security/CertificateProvider.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Microsoft.PowerShell.Security/security/CertificateProvider.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/Microsoft.PowerShell.Security/security/CertificateProvider.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
f45d906
to64b93a8
Comparesrc/Microsoft.PowerShell.Security/security/CertificateProvider.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Microsoft.PowerShell.Security/security/CertificateProvider.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
b80b491
tocbf4c2a
Compare@iSazonov@KalleOlaviNiemitalo Thanks for your help. I have added support for multi-value RDN incbf4c2a. Seems to work fine when we use |
src/Microsoft.PowerShell.Security/security/CertificateProvider.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Microsoft.PowerShell.Security/security/CertificateProvider.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
@ArmaanMcleod Please look CI errors. |
@iSazonov This one is very tricky. Locally tests pass for windows but fail in CI. For Linux Perhaps we limit this feature to Windows? I don't remember it being this strange but results seem to vary quite a bit between OS. |
Before you added the experimental feature the tests passed, yes? |
adf27d6
toa55308b
CompareArmaanMcleod commentedMar 15, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@iSazonov Even before tests which worked locally did not work in CI. I think this experimental feature is not worth it, too unstable and I can't even test it properly. I had to make tests only work in CI and they would fail locally. I am not sure if there is differences with how these APIs behave across Operating systems. It seems even drastic differences exist between Windows and Linux. I will move on and close this PR. |
microsoft-github-policy-servicebot commentedMar 15, 2025 • edited by unfurl-linksbot
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by unfurl-linksbot
Uh oh!
There was an error while loading.Please reload this page.
📣 Hey@ArmaanMcleod, how did we do? We would love to hear your feedback with the link below! 🗣️ 🔗https://aka.ms/PSRepoFeedback |
@ArmaanMcleod If the tests work on Windows locally and in CIs I guess openssl works different on Unix. I suggest to create test certificate, put it in assert forlder and run tests with the same certificate on all CIs. If tests pass, we will know root of the issue. |
ArmaanMcleod commentedMar 15, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@iSazonov Yep I believe it is because of |
Right now on Windows vs Ubuntu 20.04: Windows> [ExperimentalFeature]::IsEnabled('PSDnsNameSubjectNameCertificateParser')True>$certPath="./test/powershell/Modules/Microsoft.PowerShell.Security/TestData/Certificates/certificate5.pfx">$cert=New-Object System.Security.Cryptography.X509Certificates.X509Certificate2((Resolve-Path$certPath))>$cert.DnsNameListPunycode Unicode---------------longexample.comlongexample.com Ubuntu 20.04PS/home/armaan/PowerShell> [ExperimentalFeature]::IsEnabled('PSDnsNameSubjectNameCertificateParser')TruePS/home/armaan/PowerShell>$certPath="./test/powershell/Modules/Microsoft.PowerShell.Security/TestData/Certificates/certificate5.pfx"PS/home/armaan/PowerShell>$cert=New-Object System.Security.Cryptography.X509Certificates.X509Certificate2($certPath)PS/home/armaan/PowerShell>$cert.DnsNameListPS/home/armaan/PowerShell> So there is something really strange going on between the OS's. Can never get Linux to work properly. Although@iSazonov maybe best to kick off CI to see if somehow it works. We can at least drill down if it is |
Do other certificate values read correctly on Linux? If so, I can only suggest looking in the debugger to rule out that the error is in our code. |
CIs fail on Unix-s. Weird! We have two paths. Leave the old code as it is or find the truth. |
ArmaanMcleod commentedMar 15, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I will try to see in debugger why this is so strange on Unix. If not and it goes nowhere it might be best to leave code as is and close PR. I'll try my best though as this is very interesting problem 😄. The other option as well is we enable this experimental feature for Windows. I guess we have to make a call if this is even worth including. I am not convinced anymore with how unpredictable the tests are 😄. |
You have an option to create simple repro code and report the issue to .Net repo. |
I would be able to but when trying to debug with WSL I get segmentation fault:
Very strange. This is when trying to do anything with X509Certificate2 objects in Xterm. |
@iSazonov I will put this one in draft and figure out how to get debugger working with Linux to see what is going on. Probably not worth keeping this PR open until the behaviour is confirmed. Will let you know how it goes if I get a decent resolution or way forward. Appreciate the reviews and help so far 🙂. |
DnsNameList
forX509Certificate2
to useX500DistinguishedName.EnumerateRelativeDistinguishedNames
for Subject Distinguished NameDnsNameList
forX509Certificate2
to useX500DistinguishedName.EnumerateRelativeDistinguishedNames
for Subject Distinguished NameYou can try to create simple C# app to read the certificate on Linux. If you get null for the property you can report to .Net repo. |
microsoft-github-policy-servicebot commentedMar 22, 2025 • edited by unfurl-linksbot
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by unfurl-linksbot
Uh oh!
There was an error while loading.Please reload this page.
📣 Hey@ArmaanMcleod, how did we do? We would love to hear your feedback with the link below! 🗣️ 🔗https://aka.ms/PSRepoFeedback |
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
Related to:#24714.
Similar to Subject Alternative Name extensions, we should also replace the string parsing for
CN=
Common name when adding toDnsNameList
for Subject Distinguished names usingX500DistinguishedName.EnumerateRelativeDistinguishedNames
This is to mainly keep consistent with new .NET APIs from
System.Security.Cryptography.X509Certificates
and reduce need to string parsing in favour of using the methods provided. Currently as it stands there is nothing wrong with existing code, but we should probably use the .NET Certificate APIs across the code so it is consistent.Some background:dotnet/runtime#33914 for how it gets used.
In This PR I have replaced the string parsing with
EnumerateRelativeDistinguishedNames
and check for Common distinguished name using2.5.4.3
OID.I've also included handling for
X500RelativeDistinguishedName.HasMultipleElements
, which does the following:HasMultipleElements
isfalse
, use .NET API single attribute methodsX500RelativeDistinguishedName.GetSingleElementType
andX500RelativeDistinguishedName.GetSingleElementValue
directly. These handle most certificates and are the common case which .NET handles very easily.HasMultipleElements
istrue
, fall back toAsnReader
fromSystem.Formats.Asn1
to read theX500RelativeDistinguishedName.RawData
ourselves from the RDN and parse to parse the Common Name (CN) ourselves. This is an edge case but something we want to keep ensuring no breaking changes or regressions occur with older RFC compliant certificates.I've also refactored all the DnsNameList tests to be in one
TestCases
so we can test multiple certificate combination to ensure this is stable for release.This is all included in experimental feature
PSDnsNameSubjectNameCertificateParser
.PR Context
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.- [ ] Issue filed:
(which runs in a different PS Host).