Update DnsNameList for X509Certificate2 to use X509SubjectAlternativeNameExtension.EnumerateDnsNames Method
#24714
Conversation
test/powershell/Modules/Microsoft.PowerShell.Security/CertificateProvider.Tests.ps1
Outdated
Show resolved
Hide resolved
iSazonov
added
the
CL-General
|
@MaurizioFocareta Did you want to test these changes using a non-english locale to see if using .NET runtime approach fixes your problem? I'll keep looking to see if we can get the tests to verify it but for now we can probably do a manual test off this branch to see if it works for you 🙂. |
| string parsedSubjectDistinguishedDnsName = cert.Subject.Substring(distinguishedNamePrefix.Length); | ||
| DnsNameRepresentation dnsName = GetDnsNameRepresentation(parsedSubjectDistinguishedDnsName); |
There was a problem hiding this comment.
@iSazonov I wonder if this can also be replaced with https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.x509certificates.x500distinguishedname.enumeraterelativedistinguishednames?view=net-9.0.
I don't think even distinguished name needs string parsing anymore.
There was a problem hiding this comment.
Actually will park this, just realised this will include all distinguished names outside of CN=. dotnet runtime probably needs to expose something for just common name without too much extra work. Right now you'd need to still check the OID during enumeration which is not ideal and probably leads to more complex code in its current state.
There was a problem hiding this comment.
I think it would end up being something like:
private const string CommonNameOid = "2.5.4.3";
// Extract DNS name from Subject distinguished name
foreach (X500RelativeDistinguishedName distinguishedName in cert.SubjectName.EnumerateRelativeDistinguishedNames())
{
if (!distinguishedName.HasMultipleElements &&
distinguishedName.GetSingleElementType().Value.Equals(CommonNameOid, StringComparison.OrdinalIgnoreCase))
{
DnsNameRepresentation dnsName = GetDnsNameRepresentation(distinguishedName.GetSingleElementValue());
_dnsList.Add(dnsName);
}
}
There was a problem hiding this comment.
The change is not related the PR. Please revert.
If you think original code has an issue, please open new issue to discuss.
There was a problem hiding this comment.
Yeah so this was just referring to how distinguished names are handled. I have reverted that change and will open a new issue to discuss. This is resolved for this PR.
There was a problem hiding this comment.
I force pushed and removed the changes.
ArmaanMcleod
force-pushed
the
x509Certificate2-enumerate-dns-names-fix
branch
from
442149a to
18a73cd
Compare
That makes sense; X509Certificate2.RawData is the entire certificate including all extensions, but the byte[] rawData parameter of the X509SubjectAlternativeNameExtension constructor must be only the single extension.
Because the prefix was added by unmanaged code in Windows, I expect it respects SetThreadPreferredUILanguages rather than CultureInfo.CurrentUICulture. The corresponding language pack for Windows might also have to be installed. |
Thanks @KalleOlaviNiemitalo. That makes a lot of sense. |
PR Summary
Updated
DnsNameListforX509Certificate2to useX509SubjectAlternativeNameExtension.EnumerateDnsNamesmethod. This replaces theDNS Name=prefix + OID 2.5.29.17 checking code which is error prone when using different language clients.Using the
public X509SubjectAlternativeNameExtension (byte[] rawData, bool critical = false)constructor withcert.RawDatadid not work and resulted inCryptographicExceptionexceptions. Instead, what was much more robust was just casting the extension toX509SubjectAlternativeNameExtensionand then using theEnumerateDnsNames()method.Also testing previous code with different culture/locale I could not get DNS name prefix to output in a different language, so that case is a bit difficult to reproduce. Although it seems like this is supposed to just work moving to the .NET
EnumerateDnsNames()method. I am not even sure switching cultures is good enough to expose this problem and how it can even be validated in the tests.PR Context
Fixes #24594
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.- [ ] Issue filed:
(which runs in a different PS Host).