Implement X500DistinguishedNameBuilder

[vcsjones]

Closes #44738

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

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

Issue Details

---

Closes #44738

Author:	vcsjones
Assignees:	-
Labels:	area-System.Security, new-api-needs-documentation
Milestone:	-

[vcsjones]

Draft until docs are done.

[bartonjs]

If you can come up with a sane way to describe the order of operations here, that'd probably make for a good remarks section.

Assuming that we went with the user-sanity implementation over the code-sanity implementation, that'd be finding some way to explain something like

For an X500DistinguishedName value whose Name property prints as "CN=widgets.example.org, OU=Widget Processing Center, O=Example Org" you would call AddCommonName("widgets.example.org"), then AddOrganizationalUnit("Widget Processing Center"), then AddOrganization("Example Org"), and finally Encode().

Or maybe it's only confusing when you're over-burdened with knowledge. That this reverses, and X500DN reverses, so everything just works like anyone who doesn't know how all this actually works thinks it does.

[vcsjones]

I came up with a way, though I'll let you be the judge of it's sanity.

[bartonjs]

Conversation from my head:

> Ask Kevin where the tests are for multi-valued.
< But the code didn't handle that.
> Yeah, that's why it's a good question.
< But the code didn't handle that.
> Are you listening to me?
< Yeah. The code doesn't handle it, I don't think it's possible. Maybe you want to rephrase the question.
> Fine, as Kevin where the escape valve for multi-valued RDNs is.
< Sure. I think it got cut.
> What?!
< Yeah. Something about anyone who would know what a multi-valued RDN is can probably figure out AsnWriter on their own.
> Oh, yeah. That sounds familiar.
< Happy now?
> Yeah.  Note to self, don't ask Kevin about multi-valued RDNs.

[vcsjones]

Conversation from my head:

Ask Kevin where the tests are for multi-valued.

What a coincidence. I had the same conversation with the same guy earlier this morning.

[vcsjones]

@bartonjs thank you for the thorough feedback. I think I have addressed all of your feedback, with the exception of "Is what AddEncoded taking really DER?" which, to my understanding, is non-trivial to do correctly, unless I am missing something. If I am not missing something, we can:

1. Allow AddEncoded to be relaxed, as its current state, with the validation being limited to length encoding and no trailing data. We take the stance that this API is meant to take pre-encoded, pre-validated data, and document it as such.
2. Block this PR on new functionality in System.Formats.Asn1 that does the DER validation we want.
3. Add the DER validation in X500DistinguishedNameBuilder.
4. Remove AddEncoded from the new API set until we have some validation available to us, if ever.

My order of preference is 1, 4, 2, 3.

[bartonjs]

Ignoring my 01 01 01 testcase, as that ended up worrying about the payload, I think the subconscious intent behind my question was really more about the acceptable tags.

https://github.com/dotnet/runtime/blob/39fb7f7826270f00b856e3e9a13165c07dec87cc/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X500DictionaryStringHelper.cs limits the outputs to Universal-class character string values. Reading through https://datatracker.ietf.org/doc/html/rfc4517 shows that almost everything looks like text... until I realized that it is also providing textual output for non-textual attribute values... so I think that means our X500DN.Format is missing a bunch of RFC 4517 rules (and probably Windows is, too, or maybe we just need some more test cases to improve our compatibility if they have more support than I recall).

Forcing something to use a particular encoding, e.g. OU to use BMPEncoding, is solvable by Add. I forget why we wanted AddEncoded... maybe it was exactly for "this is something other than a text string" (unless it was "I'm walking through things using AsnReader, but want to replace one element... here, this is already encoded, just take it... don't make me turn it into a string for you to turn it back into bytes). I think I've talked myself in a circle as to whether we want to limit the type to character strings only or not.

[vcsjones]

forget why we wanted AddEncoded... maybe it was exactly for "this is something other than a text string"

I think there was more utility back when we thought we would have multi-RDN value support.

I don’t recall ever seeing non-text inputs, even for things where they could be better-than-text, they are usually strings. e.g. I’ve seen OIDs in subjects, but they are an OID as a PrintableString, not an OID as an OBJECT IDENTIFIER.

I suppose AddEncoded could be removed under the same pretense that multi-RDN was removed: you are doing something interesting / unique and you can probably just use AsnWriter.

[bartonjs]

I think there was more utility back when we thought we would have multi-RDN value support.

Yeah, but we have the wrong shape for that. The one multi-valued test we have (

runtime/src/libraries/System.Security.Cryptography.X509Certificates/tests/X500DistinguishedNameTests.cs

Lines 23 to 33 in e74c3e8

	[Theory]
	[InlineData(false)]
	[InlineData(true)]
	public static void PrintMultiComponentRdn(bool fromSpan)
	{
	byte[] encoded = (
	"30223120300C060355040313054A616D65733010060355040A13094D6963726F" +
	"736F6674").HexToByteArray();
	const string expected = "CN=James + O=Microsoft";
	X500DistinguishedName dn;

) shows the encoding:

SEQUENCE (1 elem)
  SET (2 elem)
    SEQUENCE (2 elem)
      OBJECT IDENTIFIER 2.5.4.3 commonName (X.520 DN component)
      PrintableString James
    SEQUENCE (2 elem)
      OBJECT IDENTIFIER 2.5.4.10 organizationName (X.520 DN component)
      PrintableString Microsoft

If it was a multi-RDN escape hatch then we'd've had just AddEncoded which required the input to be a SET( OF).

I suppose AddEncoded could be removed under the same pretense that multi-RDN was removed: you are doing something interesting / unique and you can probably just use AsnWriter.

Yeah. Let's just cut it. If we remember why we wanted it it's easy enough to add back (and already API-Approved)

[vcsjones]

If it was a multi-RDN escape hatch

I didn’t intend to imply that I thought it was doing that, just that back when we were shaping out the API, it was an escape hatch for that. But then we threw an OID on there (so it can’t take the SET OF) so it didn’t make sense and we should have cut it instead.

[vcsjones]

I yanked AddEncoded and spun off #66622 to think about ASN.1 validation.