Skip to content
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

Make it safer and easier to build an X500DistinguishedName #44738

Closed
Tracked by #64488
vcsjones opened this issue Nov 16, 2020 · 40 comments · Fixed by #66509
Closed
Tracked by #64488

Make it safer and easier to build an X500DistinguishedName #44738

vcsjones opened this issue Nov 16, 2020 · 40 comments · Fixed by #66509
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Security needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Milestone

Comments

@vcsjones
Copy link
Member

vcsjones commented Nov 16, 2020

What / Why

The CertificateRequest functionality takes an X500DistingishedName type as input for the subject of the certificate request in many of its constructor arguments. However, (correctly) and safely building one of these is a bit difficult at the moment, and requires either input sanitation, escaping, or a combination of both. Consider code that tries to do something like this:

string userEmailInput = "[email protected], DC=contoso";
X500DistinguishedName dn = new X500DistinguishedName($"E = {userEmailInput}");
CertificateRequest req = new CertificateRequest(dn, ECDsa.Create(), HashAlgorithmName.SHA512);

A program that does no or little email validation input would end up allowing someone to inject a component in to the distinguished name.

Ideally, something like SubjectAlternativeNameBuilder would exist that makes it easier and safer to encode an X500DistinguishedName.

Proposal

Update from before:

  • The Add* methods no longer take an encoding option, they will just do the optimal thing for their data type.
  • Instead of using Asn1Tag or UniversalTagNumber from System.Formats.Asn1 add a new nested enum for the power-user Add(OID, value, encoding) method.
    • This is because there were complexities in an inbox-only contract referencing a semi-OOB
namespace System.Security.Cryptography.X509Certificates
{
    public sealed partial class X500DistinguishedNameBuilder
    {
        public void AddEmailAddress(string emailAddress); // IA5String
        public void AddDomainComponent(string domainComponent); // IA5String, needs to use IdnMapping.
        public void AddLocalityName(string localityName); // UnboundDirectoryString (encode as UTF8)
        public void AddCommonName(string commonName);  // UnboundDirectoryString (encode as UTF8)
        public void AddCountryOrRegion(string twoLetterCode); // Always encoded as PrintableString
        public void AddOrganizationName(string organizationName); // UnboundDirectoryString (encode as UTF8)
        public void AddOrganizationalUnitName(string organizationalUnitName); // UnboundDirectoryString (encode as UTF8)
        public void AddStateOrProvinceName(string stateOrProvinceName); // UnboundDirectoryString (encode as UTF8)
    
        public void Add(string oidValue, string value, X500DistinguishedNameBuilder.EncodingType stringEncodingType = default);
        public void Add(Oid oid, string value, X500DistinguishedNameBuilder.EncodingType stringEncodingType = default);
    
        public void AddEncoded(Oid oid, byte[] encodedValue);
        public void AddEncoded(Oid oid, ReadOnlySpan<byte> encodedValue);
        public void AddEncoded(string oidValue, byte[] encodedValue);
        public void AddEncoded(string oidValue, ReadOnlySpan<byte> encodedValue);
    
        public X500DistinguishedName Build();
    
        public enum EncodingType
        {
            Default = 0, // Will be treated like UTF8String but is a distinct value of "0" so that default(EncodingType) has a sane value
            UTF8String = 12,
            PrintableString = 19,
            TeletexString = 20,
            BMPString = 30,
        }
    }
}

Previous proposal:

namespace System.Security.Cryptography.X509Certificates
{
    public sealed class X500DistinguishedNameBuilder
    {
        public void AddEmailAddress(ReadOnlySpan<char> emailAddress, Asn1Tag? stringEncodingType = null);
        public void AddDomainComponent(ReadOnlySpan<char> domainComponent, Asn1Tag? stringEncodingType = null);
        public void AddLocalityName(ReadOnlySpan<char> localityName, Asn1Tag? stringEncodingType = null);
        public void AddCommonName(ReadOnlySpan<char> commonName, Asn1Tag? stringEncodingType = null);
        public void AddCountryOrRegion(ReadOnlySpan<char> twoLetterCode, Asn1Tag? stringEncodingType = null);
        public void AddOrganizationName(ReadOnlySpan<char> organizationName, Asn1Tag? stringEncodingType = null);
        public void AddOrganizationalUnitName(ReadOnlySpan<char> organizationalUnitName, Asn1Tag? stringEncodingType = null);
        public void AddStateOrProvinceName(ReadOnlySpan<char> stateOrProvinceName, Asn1Tag? stringEncodingType = null);
    
        public void Add(Oid oid, ReadOnlySpan<char> value, Asn1Tag? stringEncodingType = null);
        public void Add(ReadOnlySpan<char> oidValue, ReadOnlySpan<char> value, Asn1Tag? stringEncodingType = null);
    
        public void AddEncoded(Oid oid, ReadOnlySpan<byte> encodedValue);
        public void AddEncoded(ReadOnlySpan<char> oidValue, ReadOnlySpan<byte> encodedValue);
    
        public X500DistinguishedName Build();
    }
}

We can add the common cases as methods, and for the uncommon / custom cases folks can use the Add method that takes an Oid.

Alternatives

Can be built yourself using AsnWriter, but, that feels a bit too close to the metal for my taste.

@vcsjones vcsjones added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 16, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Nov 16, 2020
@vcsjones
Copy link
Member Author

/cc @bartonjs

@bartonjs
Copy link
Member

A couple of questions:

What does this do?

X509DistinguishedNameBuilder builder = new X509DistinguishedNameBuilder();
builder.AddEmail("[email protected]");
builder.AddCommonName("example.org");
string whatAmI = builder.Build().Name;

If it's E = [email protected], CN = example.org that probably matches user expectations from the .NET API perspective, but that technically means that "Add" is "Push", since the Name property prints reversed. Conversely, CN = example.org, E = [email protected] probably makes sense to someone who understands X.500 names more than .NET.

My initial thought was that Build and BuildReversed were both warranted, but I think that this is probably something that someone can easily debug one way or the other.

Is there validation?

https://www.itu.int/ITU-T/formal-language/itu-t/x/x520/2008/SelectedAttributeTypes.html says that CountryName (C / 2.5.4.6) is a 2 character PrintableString that corresponds to an ISO 3166 country code. How much, if any, of that is validated? "日本" is two characters, but it falls outside the PrintableString character set. "ZQ" is two characters, and a PrintableString, but not a current two letter ISO 3166 identifier.

I think my preference would be to restrict AddCountryName to two-character PrintableString values (not trying to keep alignment with ISO 3166). I could be wrong, and maybe "no validation" is best.

We might also have to name AddCountryName AddCountryOrRegionName. It doesn't match what the spec calls it, but it solves the problem with the TW identifier.

Are all of the MultiValue things needed?

Multi-Valued RDNs are pretty rare. Leaving the OID-keyed one as an escape valve seems reasonable, but do the others provide enough value (basically to tests) to warrant the confusion they introduce (namely, that they're useful).

If Multi-Value things are needed, should it be a suffix?

Assuming classic IntelliSense ordering, should the multi-value methods be grouped together, or grouped with their single-value counterpart?

Prefix logical grouping:

    public void AddDomainComponent(string domainComponent);
    public void AddEmailAddress(string emailAddress);

    public void AddMultiValueDomainComponent(string[] domainComponents);
    public void AddMultiValueEmailAddress(string[] emailAddresses);

Suffix logical grouping:

    public void AddDomainComponent(string domainComponent);
    public void AddDomainComponentMultiValued(string[] domainComponents);

    public void AddEmailAddress(string emailAddress);
    public void AddEmailAddressMultiValued(string[] emailAddresses);

Should we accept pre-encoded values?

public void Add(Oid oid, byte[] encodedValue);
public void AddEncoded(Oid oid, byte[] encodedValue);

Should we accept string oidValue as an alternative to Oid oid?

Probably 😄 It can keep the Oid-typed one, just add overloads.

@vcsjones
Copy link
Member Author

vcsjones commented Nov 16, 2020

My initial thought was that Build and BuildReversed were both warranted

That makes sense, I also considered making this more ICollection like with Insert and such... but I tend to agree that once the behavior is understood, it's fairly straight forward.

Is there validation?

The case of countryName is fairly straight forward, so I could see validating it as simple as two letters (so there is no need to debate which country codes are valid...).

CountryName I think is also restricted to not having multiple values, but I agree the multi-value could go away anyway.

Are all of the MultiValue things needed?

I like keeping the generic Add one around for it, but agree that the common case ones can go away. I also think we can make the single value cases accept ROS now. Nope.

@vcsjones
Copy link
Member Author

vcsjones commented Nov 16, 2020

A thought: does it need an option to specify how things get encoded? e.g. to force UTF8String or BMPString? If we do, does it belong on Build or for individual Add* calls?

@bartonjs
Copy link
Member

does it need an option to specify how things get encoded?

Something like

/// <param name="stringEncodingType">An optional ASN.1 tag for the string encoding type to use for the value. The default value uses the preferred encoding for the OID, when known, or UTF-8.</param>
/// <exception cref="WhateverAsnWriterThrows"><paramref name="value"/> cannot be validly encoded under the specified encoding type.</exception>
public void Add(Oid oid, string value, Asn1Tag? stringEncodingType = null);

?

If we do, does it belong on Build or for individual Add* calls?

Clearly my thought was Add. If we build things using the encoded order (the opposite of the .NET strings) the add methods can just immediately go into an AsnWriter (though that presumably closes itself out when you call Build, we'd need to codify if Build closes the builder or you can call Add again and then Build again for a "and one more thing" approach.

If we build them in the friendly way then an AsnWriter can still be used immediately which then Encode()s into a Stack<byte[]> and Reset()s, then Build builds up the macro structure reading from the stack... not hyper-efficient, but not really the end of the world.

I also think we can make the single value cases accept ROS now. Nope.

Why can't the add methods take ReadOnlySpan<char> for the value parameter as an overload to the string version?

@vcsjones
Copy link
Member Author

Taking the AsnTag seems reasonable and provides the most flexibility. Presumably this would stop you from doing something nonsensical like using a AsnNull tag?

Why can't the add methods take ReadOnlySpan<char> for the value parameter as an overload to the string version?

I had not planned on having Add immediately write to the AsnWriter but to defer it until Encode. As you have just explained, it seems doable now.

@vcsjones
Copy link
Member Author

Updated proposal.

The Old overloads looks a bit redundant to me now. I don't really have a preference one way or the other, but if there is strong belief that the ROS overloads are good enough I wouldn't mind omitting them.

@KalleOlaviNiemitalo
Copy link

How would you use that API to add a multi-value RDN like in one of the examples in RFC 4514 section 4:

OU=Sales+CN=J. Smith,DC=example,DC=net

If public void AddMultiValue(Oid oid, string[] values, Asn1Tag? stringEncodingType = null) adds a whole RDN, then it cannot be used here, because the attribute types don't match.

Perhaps the API could work like this instead (assuming big-endian):

X509DistinguishedNameBuilder builder = new X509DistinguishedNameBuilder();
builder.AddDomainComponent("net");
builder.AddDomainComponent("example");
builder.StartMultiValueRdn();
builder.AddOrganizationUnitName("Sales");
builder.AddCommonName("J. Smith");
builder.EndMultiValueRdn();
builder.Build();

@ghost
Copy link

ghost commented Nov 17, 2020

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @jeffhandley
See info in area-owners.md if you want to be subscribed.

Issue Details
Description:

What / Why

The CertificateRequest functionality takes an X500DistingishedName type as input for the subject of the certificate request in many of its constructor arguments. However, (correctly) and safely building one of these is a bit difficult at the moment, and requires either input sanitation, escaping, or a combination of both. Consider code that tries to do something like this:

string userEmailInput = "[email protected], DC=contoso";
X500DistinguishedName dn = new X500DistinguishedName($"E = {userEmailInput}");
CertificateRequest req = new CertificateRequest(dn, ECDsa.Create(), HashAlgorithmName.SHA512);

A program that does no or little email validation input would end up allowing someone to inject a component in to the distinguished name.

Ideally, something like SubjectAlternativeNameBuilder would exist that makes it easier and safer to encode an X500DistinguishedName.

Proposal

Maybe something like this to start a conversation:

public sealed class X500DistinguishedNameBuilder
{
    public void AddEmailAddress(ReadOnlySpan<char> emailAddress, Asn1Tag? stringEncodingType = null);
    public void AddDomainComponent(ReadOnlySpan<char> domainComponent, Asn1Tag? stringEncodingType = null);
    public void AddLocalityName(ReadOnlySpan<char> localityName, Asn1Tag? stringEncodingType = null);
    public void AddCommonName(ReadOnlySpan<char> commonName, Asn1Tag? stringEncodingType = null);
    public void AddCountryOrRegionName(ReadOnlySpan<char> countryOrRegionName, Asn1Tag? stringEncodingType = null);
    public void AddOrganizationName(ReadOnlySpan<char> organizationName, Asn1Tag? stringEncodingType = null);
    public void AddOrganizationUnitName(ReadOnlySpan<char> organizationUnitName, Asn1Tag? stringEncodingType = null);
    public void AddStateOrProvince(ReadOnlySpan<char> stateOrProvince, Asn1Tag? stringEncodingType = null);

    public void Add(Oid oid, ReadOnlySpan<char> value, Asn1Tag? stringEncodingType = null);
    public void Add(ReadOnlySpan<char> oid, ReadOnlySpan<char> value, Asn1Tag? stringEncodingType = null);

    public void AddEncoded(Oid oid, ReadOnlySpan<byte> encodedValue);
    public void AddEncoded(ReadOnlySpan<char> oid, ReadOnlySpan<byte> encodedValue);

    public void AddMultiValue(Oid oid, string[] values, Asn1Tag? stringEncodingType = null);
    public void AddMultiValue(ReadOnlySpan<char> oid, string[] values, Asn1Tag? stringEncodingType = null);

    public X500DistinguishedName Build();
}

We can add the common cases as methods, and for the uncommon / custom cases folks can use the Add method that takes an Oid.

Alternatives

Can be built yourself using AsnWriter, but, that feels a bit too close to the metal for my taste.

Author: vcsjones
Assignees: -
Labels:

api-suggestion, area-System.Security, untriaged

Milestone: -

@bartonjs
Copy link
Member

Rather than have state with Start/End multi, probably just something like

// Easy CN+CN
public void AddMultiValue(string oidValue, string[] values, Asn1Tag? stringEncodingType = null);
// OU+CN with common/default encoding
public void AddMultiValue(IEnumerable<(string OidValue, string Value)> valueSegments, Asn1Tag? stringEncodingType = null);
// OU with IA5String and CN with UTF8String, 'cuz why not?
public void AddMultiValue(IEnumerable<(string OidValue, string Value, Asn1Tag? StringEncodingType)> valueSegments);

Or just cut multi-valued altogether, because the ordering is weird. The example of OU=Sales+CN=J. Smith,DC=example,DC=net (which is the .NET default order, aka "Reversed" (RFC 4514 section 2.1 says last-prints-first)) is

SEQUENCE OF
    SET OF
        SEQUENCE
            OBJECT IDENTIFIER id-at-domainComponent
            SOME SORT OF STRING "net"
    SET OF
        SEQUENCE
            OBJECT IDENTIFIER id-at-domainComponent
            SOME SORT OF STRING "example"
    SET OF
        SEQUENCE
            OBJECT IDENTIFIER id-at-organizationalUnit
            SOME SORT OF STRING "Sales"
        SEQUENCE
            OBJECT IDENTIFIER id-at-commonName
            SOME SORT OF STRING "Smith"

Because of how strings display, the calls would actually be (I think)

X509DistinguishedNameBuilder builder = new X509DistinguishedNameBuilder();
builder.AddMultiValue(new[] { (Oids.OU, "Sales"), (Oids.CN, "Smith") });
builder.AddDomainComponent("example");
builder.AddDomainComponent("net");
return builder.Build().Name;

Since we're maintaining "the API call order matches the string order" it might make sense, but it's definitely a bit of a trip for anyone who knows how the final output is encoded (ok, maybe just me).

@vcsjones
Copy link
Member Author

Or just cut multi-valued altogether

At this point I'm inclined to agree. My motivation for the proposal was CertificateRequest, which accepts an X500DN as input, whereas most of the places it has been used is as some kind of output.

I suppose if someone wanted All The Flexibility they can use AsnWriter to encode it.

@vcsjones
Copy link
Member Author

Or just cut multi-valued altogether

Removed from proposal.

@bartonjs
Copy link
Member

A couple of musings from this morning.

  • Per https://www.itu.int/ITU-T/formal-language/itu-t/x/x520/2008/SelectedAttributeTypes.html#SelectedAttributeTypes.organizationalUnitName, there should be an "al" in "Organization[al]Name"
  • While X.520 calls it "countryName", I'd remove "Name" since it's the ISO two letter code.
    • And maybe call the parameter twoLetterCode to better suggest the desired value. RegionInfo's property for this is "TwoLetterISORegionName", which feels wordy for a parameter.
    • The method name could gain "Code" as a suffix, but I'm not sure how I feel about that.
  • StateOrProvince probably wants Name tacked on because X.520 calls it stateOrProvinceName, to suggest it's not the two letter state code, and for consistency.
  • I believe everywhere we take a string-like for an OID we call the parameter oidValue to suggest it's equal to Oid.Value.

@vcsjones
Copy link
Member Author

there should be an "al" in "Organization[al]Name"

Updated, but note that "al" is only in "OrganizationalUnitName", not "OrganizationName"

The rest of the feedback seems sensible.

@bartonjs
Copy link
Member

Updated, but note that "al" is only in "OrganizationalUnitName", not "OrganizationName"

Yep, that's what I meant. Apparently I didn't have enough coffee-power to type Unit. At least I linked to the right thing 😄.

@bartonjs bartonjs added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner labels Nov 18, 2020
@bartonjs bartonjs added this to the 6.0.0 milestone Nov 18, 2020
@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Nov 24, 2020
@terrajobst
Copy link
Member

terrajobst commented Nov 24, 2020

Video

  • Looks good as proposed
  • We considered fluent pattern but concluded "meh"
  • Consider overriding ToString() (or adding a DebuggerDisplayAttribute) so that during debugging people can see what's already in the builder.
  • Let's use string, we can always add ReadOnlySpan<char> in the future. But we should overload the byte one.
namespace System.Security.Cryptography.X509Certificates
{
    public sealed class X500DistinguishedNameBuilder
    {
        public void AddEmailAddress(string emailAddress, Asn1Tag? stringEncodingType = null);
        public void AddDomainComponent(string domainComponent, Asn1Tag? stringEncodingType = null);
        public void AddLocalityName(string localityName, Asn1Tag? stringEncodingType = null);
        public void AddCommonName(string commonName, Asn1Tag? stringEncodingType = null);
        public void AddCountryOrRegion(string twoLetterCode, Asn1Tag? stringEncodingType = null);
        public void AddOrganizationName(string organizationName, Asn1Tag? stringEncodingType = null);
        public void AddOrganizationalUnitName(string organizationalUnitName, Asn1Tag? stringEncodingType = null);
        public void AddStateOrProvinceName(string stateOrProvinceName, Asn1Tag? stringEncodingType = null);

        public void Add(Oid oid, string value, Asn1Tag? stringEncodingType = null);
        public void Add(string oidValue, string value, Asn1Tag? stringEncodingType = null);

        public void AddEncoded(Oid oid, byte[] encodedValue);
        public void AddEncoded(Oid oid, ReadOnlySpan<byte> encodedValue);
        public void AddEncoded(string oidValue, byte[] encodedValue);
        public void AddEncoded(string oidValue, ReadOnlySpan<byte> encodedValue);

        public X500DistinguishedName Build();
    }
}

@vcsjones
Copy link
Member Author

vcsjones commented Feb 3, 2021

@bartonjs Spent a bit of lunch time trying to get something done for this one since I thought it would be quick but it raised a few questions.

  1. "can we just append to an AsnWriter as stuff is added" is a little tricky since it means Build can only be called once. That also has the effect of making it difficult to override ToString or do a DebuggerDisplay. Since this whole thing is sequence, we have to close the sequence when Build is called. You can't copy an AsnWriter while a sequence is open as far as I can tell.

    Is it preferable to have this be a "once you build, you're done", or should it be something more like the SAN builder where encoded values are kept in a List. Or maybe there is a way to copy an AsnWriter with an open sequence that I am not seeing.

  2. Some RDNs require specific encodings. For example, emailAddress must be IA5 string (It is not a UnboundedDirectoryString). Does it make sense to accept stringEncodingType there? We can have it be the default but I wonder if it should be there at all, or allow it as "caller beware".

@vcsjones
Copy link
Member Author

vcsjones commented Feb 4, 2021

@bartonjs Another wall I hit, this API proposal exposes types from System.Formats.Asn1 in X509Certificates for the first time. So I do that an update ref code using the MSBuild task. That doesn't build because the ref project does not have a reference to the ref project of S.F.Asn1.

Okay, so I go and add the ref project like so:

    <ProjectReference Include="..\..\System.Formats.Asn1\ref\System.Formats.Asn1.csproj" />

New build error:

/code/personal/dotnet/runtime/src/libraries/System.Security.Cryptography.X509Certificates/ref/System.Security.Cryptography.X509Certificates.cs(118,97): error CS0012: The type 'ValueType' is defined in an assembly that is not referenced. You must add a reference to assembly 'netstandard, Version=2.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'. [/code/personal/dotnet/runtime/src/libraries/System.Security.Cryptography.X509Certificates/ref/System.Security.Cryptography.X509Certificates.csproj]
/code/personal/dotnet/runtime/src/libraries/System.Security.Cryptography.X509Certificates/ref/System.Security.Cryptography.X509Certificates.cs(118,155): error CS0012: The type 'ValueType' is defined in an assembly that is not referenced. You must add a reference to assembly 'netstandard, Version=2.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'. [/code/personal/dotnet/runtime/src/libraries/System.Security.Cryptography.X509Certificates/ref/System.Security.Cryptography.X509Certificates.csproj]

So the S.F.Asn1 project is netstandard and net461 only. I am guessing that has something to do with this, but I am now stuck.

@bartonjs
Copy link
Member

bartonjs commented Feb 4, 2021

Hm. We were only accepting the Asn1Tag as an encoding type hint anyways, right? Maybe we want to do

partial class X500DistinguishedNameBuilder
{
    public enum EncodingType
    {
        Default,
        Utf8String,
        IA5String,
        etc
    }

    public void AddCommonName(string commonName, EncodingType stringEncodingType = default);
}

to avoid exposing System.Formats.Asn1 through the X509Certificates ref.

@bartonjs
Copy link
Member

bartonjs commented Feb 4, 2021

Some RDNs require specific encodings. For example, emailAddress must be IA5 string

When the RDN type has a specific encoding we shouldn't have the extra parameter. If someone wants to build a bad string they can do AddEncoded.

"can we just append to an AsnWriter as stuff is added" is a little tricky since it means Build can only be called once.

Well, what's worse is we're prepending. So I think we're really just keeping a Stack<(string OidValue, string RdnValue, SomeAsnType Encoding)> and we get

public void Build()
{
    // throw if nothing was added?

    AsnWriter writer = new AsnWriter(DER);

    using (writer.PushSequence())
    {
        foreach (stuff in _stack)
        {
             using (writer.PushSet())
             using (writer.PushSequence())
             {
                  writer.WriteObjectIdentifier(oidValue);
                  writer.WriteCharacterString(...);
             }
        }
    }

    return writer.Build();
}

private string DebuggerDisplay()
{
    return _display ??= (_stack.Count == 0 ? "" : new X500DistinguishedName(Build()).Name;
}

private string PushRdn(string oidValue, string rdnValue, ...)
{
    _display = null;
    ...
}

@vcsjones
Copy link
Member Author

vcsjones commented Feb 4, 2021

@bartonjs Okay. It seems that this still needs a bit of work. If you don't mind, I think this should go back to [api-needs-work] and I will get a proposal diff up as soon as I get a clearer understanding of the different RDN encoding requirements. Apologies for this needing around round of review.

@terrajobst terrajobst added the api-approved API was approved in API review, it can be implemented label Feb 16, 2021
@terrajobst
Copy link
Member

terrajobst commented Feb 16, 2021

Video

  • Let's just drop the nested type and replace it with an int. It'a a power API and users likely end up hard casting the Asn1 enums.
namespace System.Security.Cryptography.X509Certificates
{
    public sealed partial class X500DistinguishedNameBuilder
    {
        public void AddEmailAddress(string emailAddress);
        public void AddDomainComponent(string domainComponent);
        public void AddLocalityName(string localityName);
        public void AddCommonName(string commonName); 
        public void AddCountryOrRegion(string twoLetterCode);
        public void AddOrganizationName(string organizationName);
        public void AddOrganizationalUnitName(string organizationalUnitName);
        public void AddStateOrProvinceName(string stateOrProvinceName);
    
        public void Add(string oidValue, string value, int stringEncodingType = 0);
        public void Add(Oid oid, string value, int stringEncodingType = 0);
    
        public void AddEncoded(Oid oid, byte[] encodedValue);
        public void AddEncoded(Oid oid, ReadOnlySpan<byte> encodedValue);
        public void AddEncoded(string oidValue, byte[] encodedValue);
        public void AddEncoded(string oidValue, ReadOnlySpan<byte> encodedValue);
    
        public X500DistinguishedName Build();    
    }
}

@vcsjones
Copy link
Member Author

@bartonjs I'm probably going to miss this for .NET 6 so I can wrap up other things. Unless you have objections this should be moved to .NET 7.

@bartonjs bartonjs modified the milestones: 6.0.0, 7.0.0 Jul 7, 2021
@vcsjones
Copy link
Member Author

@bartonjs Okay, for .NET 7 we can go back to the original proposal as the original issue does not seem to exist since we added a configuration to System.Formats.Asn1 package for .NET 6 in addition to .NET Standard.

To jog everyone's memory: the first proposal's public methods had AsnTag parameters. I couldn't get this working because of how System.Formats.Asn1 was being built, so the proposal was amended to change those parameters to int, a more weakly-typed version of AsnTag. But that problem does not exist anymore.

I would propose we go back to the first approved API. If we do, do we need a 3rd API approval, or does the first one stick?

@bartonjs
Copy link
Member

Well, we wouldn't need the full AsnTag value, right, just a UniversalTagNumber, right? (Since it would throw for anything other than a Universal)

@vcsjones
Copy link
Member Author

vcsjones commented Sep 14, 2021

Well, we wouldn't need the full AsnTag value, right, just a UniversalTagNumber, right? (Since it would throw for anything other than a Universal)

Y..es. So. Another proposal then:

namespace System.Security.Cryptography.X509Certificates
{
    public sealed partial class X500DistinguishedNameBuilder
    {
        public void AddEmailAddress(string emailAddress);
        public void AddDomainComponent(string domainComponent);
        public void AddLocalityName(string localityName);
        public void AddCommonName(string commonName); 
        public void AddCountryOrRegion(string twoLetterCode);
        public void AddOrganizationName(string organizationName);
        public void AddOrganizationalUnitName(string organizationalUnitName);
        public void AddStateOrProvinceName(string stateOrProvinceName);
  
-        public void Add(string oidValue, string value, int stringEncodingType = 0);
-        public void Add(Oid oid, string value, int stringEncodingType = 0);  
+        public void Add(string oidValue, string value, UniversalTagNumber? stringEncodingType = null);
+        public void Add(Oid oid, string value, UniversalTagNumber? stringEncodingType = null);
    
        public void AddEncoded(Oid oid, byte[] encodedValue);
        public void AddEncoded(Oid oid, ReadOnlySpan<byte> encodedValue);
        public void AddEncoded(string oidValue, byte[] encodedValue);
        public void AddEncoded(string oidValue, ReadOnlySpan<byte> encodedValue);
    
        public X500DistinguishedName Build();    
    }
}

The default(UniversalTagNumber) value is defined in the enum so I don't think defaulting it makes sense. Rather than using a default value I just got rid of the default for the one place it was being used.

@bartonjs
Copy link
Member

The original proposal (which took a tag) was nullable. Presumably we thought it was useful to have the default value for known tag types (a copy-enumerator, once we ever get around to making an enumerator?)

@vcsjones
Copy link
Member Author

The original proposal (which took a tag) was nullable.

We could make it a nullable value type again. We got rid of the optionals / nullable on the convince helpers where I saw them being the most useful. I suppose we could make stringEncodingType nullable, so updated.

@bartonjs bartonjs added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-approved API was approved in API review, it can be implemented labels Sep 21, 2021
@bartonjs
Copy link
Member

bartonjs commented Sep 21, 2021

Video

Looks good as proposed (for the third time!)

namespace System.Security.Cryptography.X509Certificates
{
    public sealed partial class X500DistinguishedNameBuilder
    {
        public void AddEmailAddress(string emailAddress);
        public void AddDomainComponent(string domainComponent);
        public void AddLocalityName(string localityName);
        public void AddCommonName(string commonName); 
        public void AddCountryOrRegion(string twoLetterCode);
        public void AddOrganizationName(string organizationName);
        public void AddOrganizationalUnitName(string organizationalUnitName);
        public void AddStateOrProvinceName(string stateOrProvinceName);
  
        public void Add(string oidValue, string value, UniversalTagNumber? stringEncodingType = null);
        public void Add(Oid oid, string value, UniversalTagNumber? stringEncodingType = null);
    
        public void AddEncoded(Oid oid, byte[] encodedValue);
        public void AddEncoded(Oid oid, ReadOnlySpan<byte> encodedValue);
        public void AddEncoded(string oidValue, byte[] encodedValue);
        public void AddEncoded(string oidValue, ReadOnlySpan<byte> encodedValue);
    
        public X500DistinguishedName Build();    
    }
}

@bartonjs bartonjs removed the api-ready-for-review API is ready for review, it is NOT ready for implementation label Sep 21, 2021
@vcsjones
Copy link
Member Author

@bartonjs did this get approved and is just missing the label?

@bartonjs bartonjs added the api-approved API was approved in API review, it can be implemented label Sep 21, 2021
@bartonjs
Copy link
Member

bartonjs commented Sep 21, 2021

Clicking's hard, mm'kay? 😄

@amin1best
Copy link

According to the following links:

https://docs.microsoft.com/en-us/previous-versions/windows/desktop/ldap/distinguished-names

https://www.ietf.org/rfc/rfc2253.txt

If possible, I suggest:

  1. AddStreetAddress

  2. AddUserid

  3. In x520 and x509 for each attribute there is a field called LDAP-NAME which I use to build RDN.

x520 Sample
x520
x509 Sample
x509

Isn't there an easier way to add a RDN?
like:

public void Add(Oid oid, string LDAPName, string value);

@bartonjs , @vcsjones

@bartonjs
Copy link
Member

bartonjs commented Oct 1, 2021

@amin1best LDAPName is mostly redundant with oid. It largely informs what a string lookup table for OID to FriendlyName (and vice versa) should be, and/or a string-based lookup in LDAP. At any case, only the OID and string are actually encoded into the X.500 value.

Once we take that parameter out, its now an overload redundant to one we already have:

public void Add(Oid oid, string value, UniversalTagNumber? stringEncodingType = null);

@vcsjones vcsjones self-assigned this Nov 30, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 11, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 15, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Apr 23, 2022
@bartonjs bartonjs added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Aug 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Security needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants