Support other names in SANs #3889
Conversation
jefferai
changed the title
builtin/logical/pki/backend_test.go
Outdated
| // checks to verify that the OID SAN logic doesn't screw up other SANs, then | ||
| // will spit out the PEM. This can be validated independently. | ||
| // | ||
| // You want the hex dump of the octet string corresponding tot he X509v3 |
| cert.DNSNames[2] != "foo.foobar.com" { | ||
| t.Fatalf("unexpected DNS SANs %v", cert.DNSNames) | ||
| } | ||
| t.Logf("certificate 1 to check:\n%s", certStr) |
There was a problem hiding this comment.
You didn't read the comment carefully enough :-)
| cert.DNSNames[2] != "foo.foobar.com" { | ||
| t.Fatalf("unexpected DNS SANs %v", cert.DNSNames) | ||
| } | ||
| t.Logf("certificate 2 to check:\n%s", certStr) |
builtin/logical/pki/cert_util.go
Outdated
| case len(badName) != 0: | ||
| return errutil.UserError{Err: fmt.Sprintf( | ||
| "other SAN %s not allowed for OID %s by this role", badName, badOID)} | ||
| case len(badOID) > 0: |
There was a problem hiding this comment.
Can we have len(badOID) !=0 here as well, just for readability consistency with the previous case statement?
builtin/logical/pki/cert_util.go
Outdated
|
|
||
| func stringToOid(in string) (asn1.ObjectIdentifier, error) { | ||
| ret := []int{} | ||
| split := strings.Split(in, ".") |
There was a problem hiding this comment.
Can we do a make([]int,0,len(split)) after this line to avoid resizing of ret during append?
builtin/logical/pki/cert_util.go
Outdated
| @@ -432,31 +444,84 @@ func validateNames(req *logical.Request, names []string, role *roleEntry) string | |||
| return "" | |||
| } | |||
|
|
|||
| func validateOtherSANs(data *dataBundle, requested map[string][]string) (string, string, error) { | |||
There was a problem hiding this comment.
Can we have a comment for this method mentioning that this will performing the regex check on the allowed names on the role?
There was a problem hiding this comment.
Additionally it wasn't clear, until looking at the caller, what the return values represent. Could you document that?
builtin/logical/pki/cert_util.go
Outdated
|
|
||
| // Marshal and add to ExtraExtensions | ||
| ext := pkix.Extension{ | ||
| Id: []int{2, 5, 29, 17}, |
There was a problem hiding this comment.
Can we make this asn1.ObjectIdentifier{2, 5, 29, 17}?
builtin/logical/pki/cert_util.go
Outdated
|
|
||
| // Marshal and add to ExtraExtensions | ||
| ext := pkix.Extension{ | ||
| Id: asn1.ObjectIdentifier{2, 5, 29, 17}, |
There was a problem hiding this comment.
Can your provide some context in a comment where these values are coming from?
There was a problem hiding this comment.
Or alternatively use the constant values https://golang.org/pkg/encoding/asn1/#pkg-constants
…#3992) and CA generation/signing endpoints.
jefferai
dismissed stale reviews from vishalnayak and briankassouf
via
a8d62af
Allows specifying custom OID/UTF8 string SANs. This was harder than expected because Go's built-in asn1 library is, in the words of its author, too magical. So instead this pulls in cryptobytes, which the asn1 library's author wrote after writing something similar for BoringSSL.
If other and non-other SANs are specified we encode them all with cryptobytes (we have to) but we fall back to normal stdlib handling of SANs if not. I'm not sure if we should just always use cryptobytes or not, but we can always switch it later; all tests currently point to it working fine either way.
Because things were getting rather out of hand, this also bundles in a fairly significant refactor of cert_util.go, which is much of the actual diff in this PR.