Add support for x.509 Name Serial Number attribute in subject of certificates #4694
Conversation
| AllowAnyName: true, | ||
| AllowIPSANs: true, | ||
| EnforceHostnames: false, | ||
| AllowedSerialNumbers: []string{"*"}, |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
builtin/logical/pki/cert_util.go
Outdated
| } | ||
| if cn == "" { | ||
| cn = data.apiData.Get("common_name").(string) | ||
| if cn == "" && data.role.RequireCN { | ||
| return errutil.UserError{Err: `the common_name field is required, or must be provided in a CSR with "use_csr_common_name" set to true, unless "require_cn" is set to false`} | ||
| } | ||
| } | ||
| if ridSerialNumber == "" { | ||
| ridSerialNumberRaw, ok := data.apiData.GetOk("serial_number") |
There was a problem hiding this comment.
As it's a defined to be a string, you can actually just do a Get here directly.
There was a problem hiding this comment.
This was failing otherwise since it's not a field common to CA generation/signing.
There was a problem hiding this comment.
Feels like it should be? I don't see a reason to not allow specifying the serial number when generating a CA.
builtin/logical/pki/cert_util.go
Outdated
| dnsNames := []string{} | ||
| emailAddresses := []string{} | ||
| { | ||
| if data.csr != nil && data.role.UseCSRCommonName { | ||
| cn = data.csr.Subject.CommonName | ||
| ridSerialNumber = data.csr.Subject.SerialNumber |
There was a problem hiding this comment.
I'm not sold on this change. The serial number isn't the common name, and that field isn't defined outside of purely the common name.
|
Looking good, just two minor comments. |
| "pki": Factory, | ||
| }, | ||
| } | ||
| cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{ |
There was a problem hiding this comment.
You're one of the first outside contributors to actually use NewTestCluster. Kudos!
There was a problem hiding this comment.
I gotta admit, most of this was copy/paste from existing tests 😳
There was a problem hiding this comment.
Yes, but, many people copy/paste from existing tests using framework.TestCase and friends, and...we don't like that. (It was a good idea at the time when Vault was very, very simple, and has not at all scaled.) So it's sad when someone comes with a whole ton of TestCase code and we have to decide whether to tell them to redo it all or continue feeding the beast.
builtin/logical/pki/cert_util.go
Outdated
| if data.csr != nil && len(data.role.AllowedSerialNumbers) > 0 { | ||
| ridSerialNumber = data.csr.Subject.SerialNumber | ||
| } | ||
| if ridSerialNumber == "" { |
There was a problem hiding this comment.
I like the check that you added but think the order should be reversed -- API-specified values should override CSR values. Part of the reason is that a lot of people are not careful when constructing CSRs and may pick the same value over and over (e.g. just set something in a template) or may not use a sufficiently random source if they aren't intending for it to be something specific.
|
We pushed back 0.10.2 slightly so should be able to get this in for it! Just need that one check to be reversed and I think it's 🌈 |
|
Can I just say that I hate that pkix subjects and certs themselves both have serial numbers. |
|
Thank you! |
This PR adds support for specifying X.509 Name serialNumber attributes in subjects. It is possible to already to do this using a custom OID (2.5.4.5) with `allowed_other_sans="2.5.4.5;utf8:*" in the role configuration, however this field is not regularly parsed by various applications. It's also possible to issue certificates by passing a CSR to the sign-verbatim endpoint, but as the documents suggest it is a highly trusted and potentially dangerous endpoint. However, the sign-verbatim endpoint does not respect role specific requirements (such as EKU restrictions) and submitted CSR's may override/bypass them.
Discussion here: #4562 (comment)