[API Proposal]: Make GenericNameAsn (and related) available via X509SubjectAlternativeNameExtension

[glatzert]

Background and motivation

I'm the maintainer of a small ACME compatible server software, that connects ACME to ADCS.
Currently the implementation uses CERTENROLLLib to validate data around CSRs.
I now tried to move that CSR validation into managed code, to have only small code snippets that need COM interop at all and else have a generic ACME server, that might support multiple backends.

While src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509SubjectAlternativeNameExtension.cs would contain lots of the code, that is necessary to read CSRs (and especially the SANs), all besides DnsNames and IPAddresses is marked internal, which leads to duplicating code from some of those classes into my own project.

API Proposal

namespace System.Security.Cryptography.X509Certificates
{
    public sealed class X509SubjectAlternativeNameExtension : X509Extension
    {
        // existing API omitted
        
        public IEnumerable<GeneralNameAsn> EnumerateAlternativeNames();
    }
}

API Usage

var certificateRequest = CertificateRequest.LoadSigningRequest(
    Convert.FromBase64String(order.CertificateSigningRequest),
    HashAlgorithmName.SHA256,
    CertificateRequestLoadOptions.UnsafeLoadCertificateExtensions 
    );

var alternativeNames = certificateRequset.CertificateExtensions.OfType<X509SubjectAlternativeNameExtension>().SelectMany(x => x.AlternativeNames);

Alternative Designs

Since there are already methods for DnsNames and IPAddresses EnumerateDnsNames(), EnumerateIPAddresses()
There could be enumeration by type

namespace System.Security.Cryptography.X509Certificates
{
    public sealed class X509SubjectAlternativeNameExtension : X509Extension
    {
        // existing API omitted
        
        public IEnumerable<OtherNameAsn> EnumerateOtherNames();
        public IEnumerable<string> EnumerateRfc822Names();
        public IEnumerable<byte[]> EnumerateX400Addresses();
        public IEnumerable<byte[]> EnumerateDirectoryNames();
        public IEnumerable<EdiPartyNameAsn> EnumerateEdiPartyNames();
        public IEnumerable<string> EnumerateUris();
        public IEnumerable<string> EnumerateRegisteredIds();
    }
}

Risks

This entails making a bunch of classes public, that currently are internal

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

[bartonjs]

public IEnumerable EnumerateAlternativeNames();

To quote from #112069 (comment):

No, because GeneralNameAsn is not public. None of the types suffixed with Asn are, nor will they ever be.

That also knocks out the alternative, public IEnumerable<OtherNameAsn> EnumerateOtherNames();

In the context of processing a CSR, I assume you don't actually support any of the names that we don't, so I'd, personally, implement it as making a new extension instance that uses only the iPAddress and dNSName values that you validated. Since you're a middleman component that might not work, since you'd invalidate the signature. In that case, I'd recommend just using AsnReader to verify that the count of GeneralName values is equal to the count of iPAddress and dNSName values returned by our enumerator.

From an API perspective we could expose the total entry count, I suppose. Again, assuming that all you really want is to make sure no other alternative names were listed.

[glatzert]

Your assumption is not entirely correct.
The ACME protocol want's the server to validate the CSR, so it contains only allowed SANs. A SAN will be allowed, if there's an authorized identifier in the ACME order. If there are SANs that it cannot validate, the CSR is to be rejected.
In RFC 8555, there's only dns - here your assumption would be correct - accessing all dnsName SANs and the count would suffice.
But there are extensions to ACME, which have new identifiers: specifically ip (all good here), email, permanent-identifier and hardware-module. I didn't read about the email specs yet, but the permanent-identifier and hardware-module do come with additional SANs contained in "otherName", e.g. id-on-permanentIdentifier.

Currently I'm reading those via CERTENROLLLib (a COM library), which works and is okay-ish, but I saw, that .net already has everything I'm going to need, so I started copying (the decoding parts) of some of the *Asn classes and AsnValueReader and such - and to be honest that feels bad - I was also contemplating, if I should just throw some reflection at it, to be able to access what I need.

[bartonjs]

EmailAddress and UPN are already permitted on the builder half, so they'd both be reasonable to add to the reader half.

PermanentIdentifier's optional assigner field makes it less trivial. The builder can easily do AddPermanentIdentifier(string identifierValue, string? assignerOid = null), so the reader becomes, I guess, a tuple:

public IEnumerable<(string IdentifierValue, string? AssignerOid)> EnumeratePermanentIdentifiers();

HardwareModuleName, would then be a tuple of (string TypeOid, byte[] SerialNumber).

They're not the nicest, but we can do it.

[vcsjones]

I wonder if we should treat OtherName more or less like how we treat extensions today. Something like

public class OtherName
{
    public Oid Oid { get; }
    public byte[] RawData { get; }

    public OtherName(Oid oid, ReadOnlySpan<byte> rawData);
}

public sealed class PermanentIdentifierOtherName : OtherName
{
    public string IdentifierValue { get; }
    public Oid? AssignerOid { get; }
    public PermanentIdentifierOtherName(string identifierValue, Oid? assignerOid);
}

The builder just takes an OtherName and makes a sequence out of Oid and RawData.

The reader will be something like public IEnumerable<OtherName> EnumerateOtherNames() and for ones we have strongly typed implementations for, we return those, otherwise you get the base type of bytes.

[glatzert]

I made a EnumerateAlternativeNames, which uses a simple class hierarchy throughout. The base class is AlternativeName and then there are 8 direct inheritors (OtherAlternativeName, DnsAlternativeName, etc.). I also prototyped a PermanentIdentifier class, that inhterits from OtherAlternativeName.

Where there's no "common" way to interpret the data, I threw in a byte[] RawData, but that should probably go to the base class.
https://github.com/dotnet/runtime/compare/main...glatzert:dotnet-runtime:ottenhus/x509AlternativeNames?expand=1

[bartonjs]

I'm not a huge fan of how extensions work, so it wouldn't have occurred to me. It feels a bit chatty, but it works.

So, more or less as prototyped by @glatzert, except:

• The classes should use ctors to convey what is required, not required properties.
• Don't use init properties. Either get/set, or get-only+ctor.
• Don't use nested classes.
• Move RawData down to the base class. Everything has raw data, some things are capable of interpreting it.
  • But it feels like since this is The Future, we should expose it as ReadOnlyMemory, not byte[].
  • And I guess the data-interpreting ctor should use something like (ReadOnlyMemory<byte> rawData, bool skipCopy = false)?

[glatzert]

I fiddled around a little and made a new branch to see how it might work with ctors and having the raw data everywhere, but I did not want to implement all 8 classes just to see what happens.

For me it's non trivial to see, what I would have to do in GeneralNameAsn to access the RawData. If I read the code right, it might even not exist until someone calls Encode(), since GeneralNameAsn might be used on other occasions? But calling encode to create the RawData from a given GeneralNameAsn does not seem right ...

[bartonjs]

it's non trivial to see, what I would have to do in GeneralNameAsn to access the RawData

Basically... not have a GeneralNameAsn to start with.

public IEnumerable<GeneralName> EnumerateAll()
{
    AsnReader reader = new AsnReader(RawData, AsnEncodingRules.DER);
    reader = reader.ReadSequence();
    int i = 0;

    while (reader.HasData)
    {
        Asn1Tag tag = reader.PeekTag();
        Debug.Assert(tag.Class == TagClass.ContextSpecific);
        ReadOnlyMemory<byte> encoded = reader.ReadEncodedValue();

        yield return tag.Value switch
        {
            0 => ProcessOtherName(_decoded[i], encoded),
            1 => new Rfc822Name(encoded),
            ...
        };

        i++;
    }
}

It'd work the same if it was just for OtherName values, which I think was @vcsjones original suggestion. The 1-8 values don't feel like they need wrapper objects... we either understand them, or we don't. OtherName is where it gets more nuanced (and the values start having fields). (And, again, the presence/absence of those could be conveyed by public int TotalEntries { get; })

[glatzert]

I implemented the EnumerateAllNames (and also wrote some preliminary test case to validate it, by extracting the Extensions from a OpenSSL CSR).
Having classes makes it simple to do a single enumeration over all entries, so I stayed with that approach.
I put those classes into a namespace to minimize naming collisions and called all fields that contain the values Value.

4 SAN types could be simple strings, 1 is IPAddress, but somehow building a method for each enumeration case doesn't feel right.
OtherName brings TypeId and EncodedValue (as opposed to EncodedData in GeneralName), which is probably horrible naming.
HardwareModuleName and PermanentIdentifier are there as well, but as of now unvalidated via test.

Having AsnValueReader at my disposal really made reading that Extension data pretty simple, so even if this enumeration cannot come to .net, having the value reader public would be a great help.