pki should not alphabetically sort OUs when using the /issue/:role path

[pfjason-bbmus]

Describe the bug
Issue #6596 resolves sorted OUs when they come in via the CSR, but when a policy is created with multiple OUs they are also sorted, and then when using the /issue path to generate a certificate without a CSR certificates are created with OUs in alphabetical order, not in the order they were given to the policy. They appear to be written in the correct order when viewing the policy details in the web UI, but when a certificate is generated they are alphabetical.

To Reproduce
Steps to reproduce the behavior:

1. Create a new PKI secret store
2. Create a new default policy containing multiple OUs
3. Generate a new certificate using vault write pki/issue/rolename
4. inspect certificate with your preferred tool
5. notice OUs are now out of order

Expected behavior
Generated certificates should have OUs in the policy order, not alphabetical order

Environment:

• Vault Server Version (retrieve with vault status): 1.8.1
• Vault CLI Version (retrieve with vault version): Vault v1.8.1 (4b0264f)
• Server Operating System/Architecture: Docker / Ubuntu

Vault server configuration file(s):

N/A

Additional context
It shouldn't matter but we're generating ec-256 and ec-521 certificates, not RSA.

[cipherboy]

\o Hello @pfjason-bbmus,

This is currently unavoidable due to our reliance on crypto/x509 and in particular, crypto/x509/pkix's pkix.Name type.

Via the role issue/ endpoint, we build the bundle using the correct Subject.

However, when calling subject.ToRDNSequence, this places all attributes of a given type in a single RDN (a DN is a SEQUENCE of RDNs; a RDN is a SET of Attribute Pairs; each attribute pair is a SEQEUNCE of a type and value).

When using vault to issue a certificate on this endpoint, you can confirm this cause matches the generated (example) cert:

MIIDcDCCAligAwIBAgIUUgAJ8+keBN6YBU9fGKFpQDfdxeYwDQYJKoZIhvcNAQEL
BQAwFjEUMBIGA1UEAxMLZXhhbXBsZS5jb20wHhcNMjIwMjAyMjEyODIwWhcNMjIw
MjAyMjEyODU5WjBDMS0wCgYDVQQLEwNzZWMwDQYDVQQLEwZjcnlwdG8wEAYDVQQL
EwljcnlwdG9zZWMxEjAQBgNVBAMTCWxvY2FsaG9zdDCCASIwDQYJKoZIhvcNAQEB
BQADggEPADCCAQoCggEBAKTLBKr55QL8rnzKSxcz04xHL07vOgb6LbTfJDn5Dri3
khKb6hlMwWXcR77WnGVtW52Bstt2WZhyXP6B2vsArToQS6y20AawCPIUjrkgX7n7
R+0faOtFHGx17bZRrJsF7kNxnTcULcHkHXTOYwhjSjdg8lrGEldDURWaACyNEu/G
wko6JKF5fJZtUGreE14SM0LiW4c63IDIFpGX1/A/hvzrHmYoisvG0fQFkKMxayey
W20oFtGojpsp0SHyEgIYJpqja1FgPzl7asODuXTonTO83jg4pb/a+5OmmqlSO1wF
/myy62lZKvxU0ZXWSWFsx2z/tVpIFoeDhu5ngx3sdecCAwEAAaOBiDCBhTAOBgNV
HQ8BAf8EBAMCA6gwHQYDVR0lBBYwFAYIKwYBBQUHAwEGCCsGAQUFBwMCMB0GA1Ud
DgQWBBROtqzG+iikBzyNXOfEV5Wak/zNKjAfBgNVHSMEGDAWgBSkJa56KE3pFmQb
XSlpsF75ynuirjAUBgNVHREEDTALgglsb2NhbGhvc3QwDQYJKoZIhvcNAQELBQAD
ggEBAKvnwPdrKYfZ1wuPhJVE3OhCFqmKzMRMQYAfEMDbDMk/xM4NQZi6qE/w8uiA
yQExJwk+sPF/c/GG9qZBsKNexr3/hthOoczz2BLoZ0EfIzb5LpGihNhRgNkVIXb6
AMv89zTmufB3onkXRKgLnPExW9KSIINZ0hcMvnTyKHy93k+B22BvrJecDSFqtHEh
tHkTc5pwJy3GntWlSi3oMPdNr41YIKTlcsulOXUGBNgIRhiTqhFncj+5+N81a3IQ
AGD9YkDa8FUEMCBnfepK50+R4ptouOdRHjzGs+E9TFxF2SGpYzU+N2+53bpdkHwe
QsnAqi6X/kPAT3Kk+qwtQ5cN6TQ=

And decode it, you see:

SEQUENCE (2 elem)
      SET (3 elem)
        SEQUENCE (2 elem)
          OBJECT IDENTIFIER 2.5.4.11 organizationalUnitName (X.520 DN component)
          PrintableString sec
        SEQUENCE (2 elem)
          OBJECT IDENTIFIER 2.5.4.11 organizationalUnitName (X.520 DN component)
          PrintableString crypto
        SEQUENCE (2 elem)
          OBJECT IDENTIFIER 2.5.4.11 organizationalUnitName (X.520 DN component)
          PrintableString cryptosec
      SET (1 elem)
        SEQUENCE (2 elem)
          OBJECT IDENTIFIER 2.5.4.3 commonName (X.520 DN component)
          PrintableString localhost

From a playground example we can additionally confirm this analysis:

Code:

package main

import (
	"crypto/x509/pkix"
	"encoding/asn1"
	"encoding/base64"
	"fmt"
)

func main() {
	var name pkix.Name
	name.Organization = append(name.Organization, "alpha")
	name.Organization = append(name.Organization, "delta")
	name.Organization = append(name.Organization, "beta")

	fmt.Println("Original name: " + name.String())

	raw_name, _ := asn1.Marshal(name.ToRDNSequence())
	fmt.Println("Base64 of name.ToRDNSequence(): " + base64.StdEncoding.EncodeToString(raw_name))

	var raw_sequence pkix.RDNSequence
	var round_tripped pkix.Name
	asn1.Unmarshal(raw_name, &raw_sequence)
	round_tripped.FillFromRDNSequence(&raw_sequence)

	fmt.Println("Round-tripped name: " + round_tripped.String())
}

Output:

Original name: O=alpha+O=delta+O=beta
Base64 of name.ToRDNSequence(): MCsxKTALBgNVBAoTBGJldGEwDAYDVQQKEwVhbHBoYTAMBgNVBAoTBWRlbHRh
Round-tripped name: O=beta+O=alpha+O=delta

Which when decoded matches the above sequence types.

Because all OUs are within a single RDN (which iself is a ASN.1 SET element), order is not necessarily preserved or guaranteed.

Per RFC 5280 Section 7.1:

Two naming attributes match if the attribute types are the same and
the values of the attributes are an exact match after processing with
the string preparation algorithm.> Two relative distinguished names
RDN1 and RDN2 match if they have the same number of naming attributes
and for each naming attribute in RDN1 there is a matching naming
attribute in RDN2.> Two distinguished names DN1 and DN2 match if they
have the same number of RDNs, for each RDN in DN1 there is a matching
RDN in DN2, and the matching RDNs appear in the same order in both
DNs.> A distinguished name DN1 is within the subtree defined by the
distinguished name DN2 if DN1 contains at least as many RDNs as DN2,
and DN1 and DN2 are a match when trailing RDNs in DN1 are ignored.

So clients should treat these as equivalent, regardless of OU order, given that they're in a single OU.

In order to work around this in the future (where this == putting OUs in the same SET and thus convert them to many single-valued RDNs), we'd have to add support for the RawSubject field (including when signing CSRs directly), and allow the role to specify the RawSubject encoding scheme (including which elements belong in a SET together and which belong in separate SETs).

That gets non-trivial as we'd (likely) have to create a replacement scheme (wherein the role specifies which values from the request it'll accept and where they'll go), or allow the request itself to specify the raw subject. Due to the risk of the latter, perhaps it is best to introduce a role parameter use_csr_raw_subject and limit this to CSR signing.

But if you (or anyone else) has thoughts about designing an interface over RawSubject, happy to hear them. :-)

---

Vault reproducer for the future:

vault secrets enable pki
vault write pki/root/generate/internal common_name=example.com
vault write pki/roles/example-dot-com ou=cryptosec,crypto,sec
vault write pki/issue/example-dot-com common_name=localhost ttl=10s

[dhduvall]

Here's a hot take: introduce an rdn attribute for the endpoint, which would itself take key-value pairs of X.509 attribute and value. For example:

vault write pki/roles/danek \
    rdn=o="My Company" \
    rdn=ou=Software \
    rdn=ou="Bugs Division",l="My House" \
    rdn=cn="Mi a Name I Call Myself"

• the ordering matters, which means the nesting of the OU RDNs is preserved;
• there's an example of a multi-part RDN (you'll need a way to escape the comma, though, or whatever separator character you choose);
• you eliminate a potential explosion in X.509 attribute names at the Vault PKI role attribute level (if you want to add more than the ones you currently support);
• you can easily extend it to use the OID for unrecognized attributes (e.g.,rdn=2.5.4.44=Jr), giving people a big escape hatch;
• introducing a new attribute lets you bypass any backwards-compatibility issues that would arise if you reused the existing ones.

I think there's precedent for this kind of two-level structure, but I'm not sure. And of course you'd need to work around golang/go#40876.

I'm sure there's something I've missed, but this could tackle #10281 and parts of #4562 and #4560.

[cipherboy]

I like this take, but I think still missing is how do we validate this.

Some thoughts:

• Existing role's cn validations should probably still apply to the rdn=cn components...
• We need a little more flexible rule engine to support restricting and potentially overriding/setting values the user hasn't set or set incorrectly.
• We probably need a way of limiting quantities of components and structure. I'm fine with forcing alternative structures into separate roles (e.g,. rdn=o,cn vs rdn=ou,cn).

&c. I think this mostly pushes us towards something like HCL or perhaps a JSON-type rule language. We'll also need to be able to pull values off the auth method/templates like we do presently.

In short, while I like this proposal, I'm not sure how the validation side should work today and that's what needs a little more thought.

(n.b.: this is definitely more than a bug, so I've reassigned the labels -- in particular, this just the behavior of the Go library, so it is hard to classify it as a bug. They do provide an alternative form (RawSubject), but this requires us to build the RDN structure ourselves.)