Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Conversation

ArmaanMcleod
Copy link
Contributor

@ArmaanMcleodArmaanMcleod commentedJan 3, 2025
edited
Loading

PR Summary

Related to:#24714.

Similar to Subject Alternative Name extensions, we should also replace the string parsing forCN= Common name when adding toDnsNameList for Subject Distinguished names usingX500DistinguishedName.EnumerateRelativeDistinguishedNames

This is to mainly keep consistent with new .NET APIs fromSystem.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 withEnumerateRelativeDistinguishedNames and check for Common distinguished name using2.5.4.3 OID.

I've also included handling forX500RelativeDistinguishedName.HasMultipleElements, which does the following:

I've also refactored all the DnsNameList tests to be in oneTestCases so we can test multiple certificate combination to ensure this is stable for release.

This is all included in experimental featurePSDnsNameSubjectNameCertificateParser.

PR Context

PR Checklist

@ArmaanMcleodArmaanMcleodforce-pushed thex509certificate2-subject-distinguished-name-dns-fix branch from00f7f0e tod638b44CompareJanuary 3, 2025 08:02
@ArmaanMcleodArmaanMcleodforce-pushed thex509certificate2-subject-distinguished-name-dns-fix branch fromd638b44 to4b19e13CompareJanuary 3, 2025 08:31
@ArmaanMcleodArmaanMcleod marked this pull request as ready for reviewJanuary 3, 2025 08:40
@ArmaanMcleod
Copy link
ContributorAuthor

@KalleOlaviNiemitalo WouldEnumerateRelativeDistinguishedNames be the best way to extract common name CN instead of string parsing? Just checking since I was going to try and replace all the prefix string parsing in this part of the code 😄.

Mobility-Inc-Marq reacted with eyes emoji

@ArmaanMcleodArmaanMcleodforce-pushed thex509certificate2-subject-distinguished-name-dns-fix branch fromf45d906 to64b93a8CompareJanuary 4, 2025 11:28
@iSazonoviSazonov reopened thisJan 4, 2025
@ArmaanMcleodArmaanMcleod marked this pull request as draftJanuary 5, 2025 00:14
@ArmaanMcleodArmaanMcleod marked this pull request as ready for reviewJanuary 5, 2025 14:39
@ArmaanMcleodArmaanMcleodforce-pushed thex509certificate2-subject-distinguished-name-dns-fix branch fromb80b491 tocbf4c2aCompareJanuary 6, 2025 03:38
@ArmaanMcleod
Copy link
ContributorAuthor

@iSazonov@KalleOlaviNiemitalo Thanks for your help. I have added support for multi-value RDN incbf4c2a.

Seems to work fine when we useAsnReader. Let me know if you have any concerns here or more edge cases we should support.

@iSazonoviSazonov added the CL-GeneralIndicates that a PR should be marked as a general cmdlet change in the Change Log labelMar 12, 2025
@iSazonov
Copy link
Collaborator

@ArmaanMcleod Please look CI errors.

@ArmaanMcleod
Copy link
ContributorAuthor

@iSazonov This one is very tricky. Locally tests pass for windows but fail in CI. For LinuxDnsNameList is set tonull.

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.

@iSazonov
Copy link
Collaborator

Before you added the experimental feature the tests passed, yes?

@ArmaanMcleodArmaanMcleodforce-pushed thex509certificate2-subject-distinguished-name-dns-fix branch fromadf27d6 toa55308bCompareMarch 15, 2025 01:47
@ArmaanMcleod
Copy link
ContributorAuthor

ArmaanMcleod commentedMar 15, 2025
edited
Loading

Before you added the experimental feature the tests passed, yes?

@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-serviceMicrosoft GitHub Policy Service
Copy link
Contributor

microsoft-github-policy-servicebot commentedMar 15, 2025
edited by unfurl-linksbot
Loading

📣 Hey@ArmaanMcleod, how did we do? We would love to hear your feedback with the link below! 🗣️

🔗https://aka.ms/PSRepoFeedback

@iSazonov
Copy link
Collaborator

@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
Copy link
ContributorAuthor

ArmaanMcleod commentedMar 15, 2025
edited
Loading

@iSazonov Yep I believe it is because ofopenssl versions on different OS. I have moved the certificates toTestData/Certificates path to see if CI behaves better with certificates without generating them usingopenssl.

@ArmaanMcleod
Copy link
ContributorAuthor

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.04

PS/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 isopenssl problem or .NET API problem.

@iSazonov
Copy link
Collaborator

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.

@iSazonov
Copy link
Collaborator

CIs fail on Unix-s. Weird! We have two paths. Leave the old code as it is or find the truth.

ArmaanMcleod reacted with thumbs up emoji

@ArmaanMcleod
Copy link
ContributorAuthor

ArmaanMcleod commentedMar 15, 2025
edited
Loading

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.

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 😄.

@iSazonov
Copy link
Collaborator

I'll try my best though as this is very interesting problem

You have an option to create simple repro code and report the issue to .Net repo.

@ArmaanMcleod
Copy link
ContributorAuthor

I'll try my best though as this is very interesting problem

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:

bash: line 1: 37357 Segmentation fault (core dumped) ''/home/armaan/.vscode-server/extensions/ms-dotnettools.csharp-2.69.22-linux-x64/.debugger/vsdbg --interpreter=vscode --connection=/tmp/CoreFxPipe_vsdbg-ui-664483ba64104302836de1684d455c93

Very strange. This is when trying to do anything with X509Certificate2 objects in Xterm.

@ArmaanMcleodArmaanMcleod marked this pull request as draftMarch 15, 2025 14:51
@ArmaanMcleod
Copy link
ContributorAuthor

@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 🙂.

@ArmaanMcleodArmaanMcleod changed the titleUpdateDnsNameList forX509Certificate2 to useX500DistinguishedName.EnumerateRelativeDistinguishedNames for Subject Distinguished NameWIP: UpdateDnsNameList forX509Certificate2 to useX500DistinguishedName.EnumerateRelativeDistinguishedNames for Subject Distinguished NameMar 15, 2025
@iSazonov
Copy link
Collaborator

You 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-servicemicrosoft-github-policy-servicebot added the Waiting on AuthorThe PR was reviewed and requires changes or comments from the author before being accept labelMar 19, 2025
@microsoft-github-policy-serviceMicrosoft GitHub Policy Service
Copy link
Contributor

microsoft-github-policy-servicebot commentedMar 22, 2025
edited by unfurl-linksbot
Loading

📣 Hey@ArmaanMcleod, how did we do? We would love to hear your feedback with the link below! 🗣️

🔗https://aka.ms/PSRepoFeedback

KalleOlaviNiemitalo reacted with confused emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@doctordnsdoctordnsdoctordns left review comments

@KalleOlaviNiemitaloKalleOlaviNiemitaloAwaiting requested review from KalleOlaviNiemitalo

@iSazonoviSazonovAwaiting requested review from iSazonov

@jshigetomijshigetomiAwaiting requested review from jshigetomijshigetomi will be requested when the pull request is marked ready for reviewjshigetomi is a code owner

Assignees
No one assigned
Labels
CL-GeneralIndicates that a PR should be marked as a general cmdlet change in the Change LogWaiting on AuthorThe PR was reviewed and requires changes or comments from the author before being accept
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@ArmaanMcleod@KalleOlaviNiemitalo@iSazonov@daxian-dbw@doctordns

[8]ページ先頭

©2009-2025 Movatter.jp