Add support to match domainComponent (DC) in RDNSequence with TLS Auth #1386
Conversation
|
To generate the certs with DCs: cd ./test/configs/certs/rdns/
openssl genrsa -out ca.key 2048
openssl req -new -x509 -days 1826 -key ca.key -out ca.pem -subj "/C=US/ST=California/L=Los Angeles/O=NATS/OU=NATS/CN=localhost"
# create client certs: A
openssl genrsa -out client-a.key 2048
openssl req -new -key client-a.key -out client-a.csr -subj "/C=US/ST=California/L=Los Angeles/O=NATS/OU=NATS/CN=localhost/DC=foo1/DC=foo2"
openssl x509 -req -days 3650 -in client-a.csr -CA ca.pem -CAkey ca.key -set_serial 01 -out client-a.pem
# create client certs: B
openssl genrsa -out client-b.key 2048
openssl req -new -key client-b.key -out client-b.csr -subj "/C=US/ST=California/L=Los Angeles/O=NATS/OU=NATS/CN=localhost"
openssl x509 -req -days 3650 -in client-b.csr -CA ca.pem -CAkey ca.key -set_serial 01 -out client-b.pem
# create client certs: C
openssl genrsa -out client-c.key 2048
openssl req -new -key client-c.key -out client-c.csr -subj "/C=US/ST=California/L=Los Angeles/O=NATS/OU=NATS/CN=localhost/DC=foo3/DC=foo4"
openssl x509 -req -days 3650 -in client-c.csr -CA ca.pem -CAkey ca.key -set_serial 01 -out client-c.pem
# create server certs
openssl genrsa -out server.key 2048
openssl req -new -key server.key -out server.csr -subj "/C=US/ST=California/L=Los Angeles/O=NATS/OU=NATS/CN=localhost"
openssl x509 -req -days 3650 -in server.csr -CA ca.pem -CAkey ca.key -set_serial 01 -out server.pem |
|
|
||
| authorization { | ||
| users = [ | ||
| { user = "CN=localhost,OU=NATS,O=NATS,L=Los Angeles,ST=California,C=US,DC=foo1,DC=foo2" } |
There was a problem hiding this comment.
So the issue was with two DC entries not showing up properly?
There was a problem hiding this comment.
None of the DC parts would be included the current way that the Go stdlib generates this string so it would become
CN=localhost,OU=NATS,O=NATS,L=Los Angeles,ST=California,C=US
instead of
CN=localhost,OU=NATS,O=NATS,L=Los Angeles,ST=California,C=US,DC=foo1,DC=foo2
There was a problem hiding this comment.
Myself, I'd avoid the fmt.Sprintf() deep in comparator conversions when you just want to join two strings together, but otherwise LGTM for technical correctness.
I'm concerned that if folks are already working around the DC being missing, this will cause their existing working ACL rules to start failing because we're changing the identity that they have to match. In this case, I think we just need to call it out as an upgrade note to beware of.
|
thanks @philpennock think you are right, should just concat instead |
| if len(dcs) > 0 { | ||
| u := strings.Join([]string{rdn, dcs}, ",") | ||
| if fn(u) { | ||
| c.Debugf("Using RDNSequence for auth [%q]", u) |
There was a problem hiding this comment.
Could that be a security risk to display those (even if in debug)?
There was a problem hiding this comment.
think ok since not credentials themselves, but maybe could revisit and change these into being verbose instead
There was a problem hiding this comment.
That's fine if they are not creds per-se.
Currently when using TLS based authentication, any domain components that could be present in the cert will be omitted since Go's ToRDNSequence is not including them: https://github.com/golang/go/blob/202c43b2ad3fca2cdcaff0d0720de5c99030b638/src/crypto/x509/pkix/pkix.go#L226-L245 This commit adds support to include the domain components in case present, also roughly following the order suggested at: https://tools.ietf.org/html/rfc2253 Signed-off-by: Waldemar Quevedo <wally@synadia.com>
|
Updated to remove the sprintf just in case, thanks everyone for checking. |
| @@ -527,6 +529,26 @@ func (s *Server) processClientOrLeafAuthentication(c *client) bool { | |||
| return false | |||
| } | |||
|
|
|||
| func getTLSAuthDCs(rdns *pkix.RDNSequence) string { | |||
| dcOID := asn1.ObjectIdentifier{0, 9, 2342, 19200300, 100, 1, 25} | |||
There was a problem hiding this comment.
const DcOID = ...
and outside the funciont?
There was a problem hiding this comment.
think these cannot be made into const
Currently when using TLS based authentication, any domain components that could be present in the cert will be omitted since Go's ToRDNSequence is not including them:
https://github.com/golang/go/blob/202c43b2ad3fca2cdcaff0d0720de5c99030b638/src/crypto/x509/pkix/pkix.go#L226-L245
This commit adds support to include the domain components in case present, also roughly following the order suggested at: https://tools.ietf.org/html/rfc2253
Signed-off-by: Waldemar Quevedo wally@synadia.com
git pull --rebase origin master)Changes proposed in this pull request:
/cc @nats-io/core