crypto/x509: accept non-string pkix.Name attributes

[lukevalenta]

Relative OID tag support is currently missing from the encoding/asn1 and x/crypto/cryptobyte packages.

The motivation is that RELATIVE-OID is used in both https://datatracker.ietf.org/doc/html/draft-ietf-tls-trust-anchor-ids-01 and https://datatracker.ietf.org/doc/draft-davidben-tls-merkle-tree-certs/.

In particular, attempting to parse a draft07 Merkle Tree Certificate with x509.ParseCertificate gives the following error: x509: invalid RDNSequence: invalid attribute value: unsupported string type: 13.

[seankhliao]

What does support look like? API changes, behaviour changes, etc.

cc @FiloSottile merkle tree certs coauthor

[lukevalenta]

What does support look like? API changes, behaviour changes, etc.

cc @FiloSottile merkle tree certs coauthor

Looks like there are two asn1 libraries to add support to: x/crypto/cryptobyte and encoding/asn1.

For the MTC case to allow x509.ParseCertificate to parse certificates where the subject name contains a relative OID we'd need to add support for relative OIDs to cryptobyte.ReadASN1 at https://cs.opensource.google/go/go/+/refs/tags/go1.25.1:src/crypto/x509/parser.go;l=145, which would mean adding a new constant to the list of accepted tags at https://cs.opensource.google/go/x/crypto/+/master:cryptobyte/asn1/asn1.go;l=37.

It would also be nice to have support in encoding/asn1, which would involve adding the tag to the list of consts at https://cs.opensource.google/go/go/+/refs/tags/go1.25.1:src/encoding/asn1/common.go;l=30 along with associated support in that package.

I'd probably call this a behavior change, as the asn1 libraries would no longer throw an error when encountering the relative OID ASN.1 tag.

[davidben]

Attribute values are an ANY, so in principle an X.509 parser is supposed to accept all ASN.1 values. Particular attribute types will, of course, have known types and they're usually strings, but an unknown attribute type could be anything.

Here's how Name is defined in RFC 5280:

   Name ::= CHOICE { -- only one possibility for now --
     rdnSequence  RDNSequence }

   RDNSequence ::= SEQUENCE OF RelativeDistinguishedName

   RelativeDistinguishedName ::=
     SET SIZE (1..MAX) OF AttributeTypeAndValue

   AttributeTypeAndValue ::= SEQUENCE {
     type     AttributeType,
     value    AttributeValue }

   AttributeType ::= OBJECT IDENTIFIER

   AttributeValue ::= ANY -- DEFINED BY AttributeType

Looks like the issue isn't in cryptobyte, rather in parseASN1String, which assumes every attribute is one of T61String, PrintableString, UTF8String, BMPString, IA5String, or NumericString. So at minimum I think support means being spec-compliant and tolerating unknown types in X.509 names. Doesn't have to do anything useful with them, but the certificate should still be parsed.

[davidben]

Interestingly, the pkix.Name structure seems to be able to represent all this just fine. pkix.Name.FillFromRDNSequence seems to even account for attribute values that aren't string.

It's just that the parser doesn't handle it. Possibly unknown types should be bound to an asn1.RawValue or something? Not sure.

[FiloSottile]

Hmm, the main question is indeed how to represent ANY values in AttributeTypeAndValue.Value. asn1.RawValue feels like the easiest answer, but while this is not a breaking change now because we are going from certificates that don't parse to certificates that parse, we can't go back later and change the type. Still, I can't think of any reason to get fancy. We can say that string types go to string, and the rest go to asn1.RawValue.

We have been moving parsers away from encoding/asn1 so I think we can probably add this only to the x/crypto/cryptobyte path. As long as encoding/asn1 can still encode the structure with asn1.RawValue.

[davidben]

I guess the other option is to skip them, and only introduce new values in the parse path when/if you all feel the need to report them to the caller. It's already the case that pkix.Name doesn't capture the full space of what an X.509 name is because the SEQUENCE OF SET OF AttributeTypeAndValue is flatted into a single list. It also loses which string type was used. Though dropping attributes feels more off than those other changes, so RawValue is probably better?

The other fun thing is that later promoting a value from RawValue to known type is actually compatibility-breaking.

(Why, oh why, is X.509 so pointlessly complicated? Has anyone ever even seen a multi-attribute RDN? I'm not sure I've ever encountered one outside of things constructed for tests.)

FWIW, MTC only used it because it seemed tidy, but in another context, I was asked to make OCTET STRING work in BoringSSL's X.509 Name parser because some protocol decided to stuck those in X.509 names. (openssl/openssl#27576, since fixed in BoringSSL.)

[davidben]

Fascinating. So this actually used to work, but not in the way I'd have expected. Here's a test program, with a test certificate that contains a bunch of funny attribute values:

parse_cert.go

package main

import (
	"crypto/x509"
	"encoding/pem"
	"fmt"
)

const certPEM = `
-----BEGIN CERTIFICATE-----
MIIB0DCCAXagAwIBAgIJAIZft7jy3RcpMAoGCCqGSM49BAMCMA8xDTALBgNVBAMM
BFRlc3QwHhcNMjUwOTAyMTg0MzE3WhcNMjUxMDAyMTg0MzE3WjCByzEeMBwGDSqG
SIb3EgQBhLcJAgEMC3V0Zjgtc3RyaW5nMR8wHQYNKoZIhvcSBAGEtwkCAgQMb2N0
ZXQtc3RyaW5nMRgwFgYNKoZIhvcSBAGEtwkCAw0FAQIDBAUxEzARBg0qhkiG9xIE
AYS3CQIEBQAxITAfBg0qhkiG9xIEAYS3CQIFMA4MBWhlbGxvDAV3b3JsZDEUMBIG
DSqGSIb3EgQBhLcJAgYCASoxIDAeBg0qhkiG9xIEAYS3CQIHBg0qhkiG9xIEAYS3
CQIHMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE7M4zoqQtXbvGsudKaM5gd8em
xyk68AFTjIYU4PO1AtiYX3wyL89kwbHjxgvmh/9aBg1LOj6kfsJIxULUmUpdzTAK
BggqhkjOPQQDAgNIADBFAiBvKz3oALhCqKrRFLUbax6+tI1s1B14IPVk2ZHbBEou
5gIhAOpvJRNj5qluPXKLXmZvIK8uOjUhiZoowYvborSS1EBK
-----END CERTIFICATE-----
`

func main() {
	block, _ := pem.Decode([]byte(certPEM))
	cert, err := x509.ParseCertificate(block.Bytes)
	if err != nil {
		panic(err)
	}
	for _, attr := range cert.Subject.Names {
		fmt.Printf("Type = %s\n", attr.Type)
		fmt.Printf("Value = (%T) %s\n", attr.Value, attr.Value)
	}
}

der-ascii output of test name

    SEQUENCE {
      SET {
        SEQUENCE {
          OBJECT_IDENTIFIER { 1.2.840.113554.4.1.72585.2.1 }
          UTF8String { "utf8-string" }
        }
      }
      SET {
        SEQUENCE {
          OBJECT_IDENTIFIER { 1.2.840.113554.4.1.72585.2.2 }
          OCTET_STRING { "octet-string" }
        }
      }
      SET {
        SEQUENCE {
          OBJECT_IDENTIFIER { 1.2.840.113554.4.1.72585.2.3 }
          RELATIVE_OID { .1.2.3.4.5 }
        }
      }
      SET {
        SEQUENCE {
          OBJECT_IDENTIFIER { 1.2.840.113554.4.1.72585.2.4 }
          NULL {}
        }
      }
      SET {
        SEQUENCE {
          OBJECT_IDENTIFIER { 1.2.840.113554.4.1.72585.2.5 }
          SEQUENCE {
            UTF8String { "hello" }
            UTF8String { "world" }
          }
        }
      }
      SET {
        SEQUENCE {
          OBJECT_IDENTIFIER { 1.2.840.113554.4.1.72585.2.6 }
          INTEGER { 42 }
        }
      }
      SET {
        SEQUENCE {
          OBJECT_IDENTIFIER { 1.2.840.113554.4.1.72585.2.7 }
          OBJECT_IDENTIFIER { 1.2.840.113554.4.1.72585.2.7 }
        }
      }
    }

(Edit: updated it to also test INTEGERs)

In Go today, parsing fails. But in Go 1.16.15, before https://go-review.googlesource.com/c/go/+/274234, it actually succeeded! This is because Go used to just asn1.Unmarshal the RDN sequence, and that was already an any field.
https://cs.opensource.google/go/go/+/refs/tags/go1.16.15:src/crypto/x509/x509.go;drc=35334caf18cb35ecc7a43082b7bfcc7ce8d0de8f;l=1316

But it parsed not the way I would have expected:

Type = 1.2.840.113554.4.1.72585.2.1
Value = (string) utf8-string
Type = 1.2.840.113554.4.1.72585.2.2
Value = ([]uint8) octet-string
Type = 1.2.840.113554.4.1.72585.2.3
Value = (<nil>) %!s(<nil>)
Type = 1.2.840.113554.4.1.72585.2.4
Value = (<nil>) %!s(<nil>)
Type = 1.2.840.113554.4.1.72585.2.5
Value = (<nil>) %!s(<nil>)
Type = 1.2.840.113554.4.1.72585.2.6
Value = (int64) %!s(int64=42)
Type = 1.2.840.113554.4.1.72585.2.7
Value = (asn1.ObjectIdentifier) 1.2.840.113554.4.1.72585.2.7

Before, types that encoding/asn1 recognized like OCTET STRING and OBJECT IDENTIFIER would work fine and get parsed as you might expected. (Go 1.17 broke this.) But other types would... cleanly parse and then bound to nil! Looks like it's because that's just how encoding/asn1 works:
https://cs.opensource.google/go/go/+/refs/tags/go1.16.15:src/encoding/asn1/asn1.go;drc=665def2c11bb49749b075d612e98b6db293266a7;l=727

(NB: That certificate won't parse in OpenSSL either, because of openssl/openssl#27576. That said, OpenSSL does handle RELATIVE-OID. It's the other types I was testing in there that OpenSSL can't handle.)

[davidben]

Something else we ran into exploring all this is that Go doesn't have any information-preserving X.509 name representations, which makes all the String() methods return confusing things. If the attribute type is not recognized, String() will use the generic # syntax. This is correct, but it encodes the result of asn1.Marshal on the value and the value loses information. All string types are parsed as string and so asn1.Marshal has to guess.

The docs do say:

Name represents an X.509 distinguished name. This only includes the common elements of a DN. Note that Name is only an approximation of the X.509 structure. If an accurate representation is needed, asn1.Unmarshal the raw subject or issuer as an RDNSequence.

But the alternative doesn't work either because it still runs into encoding/asn1's information loss from string. Mostly it deals with pkix.Name flattening the two-level SEQUENCE OF SET OF AttributeTypeAndValue structure.

When constructing a certificate, Go callers are able to force a particular string type by binding an asn1.RawValue to the any field though. You're allowed to bind things to the any that encoding/asn1 would never output. Though this isn't obvious.

I'm not sure what the right thing to do with all this is, if anything. X.509 names are horrid. I mention it just because, if you all are going to revisit how to represent names, it's possibly worth pondering this fiasco. Or not. (In hindsight, perhaps AttributeTypeAndValue.Value should have been a asn1.RawValue and then there are just methods to get a string out of it. Ah well.)