[ADDED] verify_cert_and_check_known_urls to tie subject alt name to url in cfg #1727

matthiashanel · 2020-11-19T15:03:17Z

Only works for gateways and routes. When true the subject alt DNS name
must match one url in the corresponding configuration

As discussed. At the very least have a look at the unit test cases in TestDNSAltNameMatching. Looking at certs to write my other tests I started to wonder if we should extend this to IPs too?

        X509v3 extensions:
            X509v3 Subject Alternative Name:
                DNS:localhost, IP Address:127.0.0.1

Also look for 127.0.0.1 and it's IPv6 variant. Opinions?

If yes, I'd be open to suggestions on which urls to provide in test/tls_test.go line 823, 891, 892

What I intend to include in the doc
verify_and_implicit_allow| Only settable in a non client context where verify=true is the default (gateways/cluster). The incoming connections certificate's X509v3 Subject Alternative Name DNS entries will be matched against all urls in the configuration context that contains this tls map. If a match is found the connection is accepted, otherwise rejected. Meaning for gateways we will match all DNS entries in the certificate against all gateway urls. For cluster we will match against all route urls. As a consequence of this, dynamic cluster growth may require config changes in other cluster where this flag is true. DNS name checking is performed according to https://tools.ietf.org/html/rfc6125#section-6.4.1 Only the full wildcard * is supported for the left most label. This would be a way to keep cluster growth flexible.

Signed-off-by: Matthias Hanel mh@synadia.com

kozlovic

Some tests changes.

server/opts.go

test/tls_test.go

kozlovic · 2020-11-19T15:45:10Z

server/auth.go

-			return "", opts.Gateway.Username == user
+	if opts.Gateway.TLSMap || opts.Gateway.TLSImplicitAllow {
+		return checkClientTLSCertSubject(c, func(user string, _ *ldap.DN, isDNSAltName bool) (string, bool) {
+			if user == "" {


In original code, is that possible that user and Gateway.Username be empty in TLSMap context? If so, this is changing the behavior. Is that intended?

I don't think that user should ever be empty. In that case the CA would have signed a cert with empty fields.
I'm also not sure. if you can even configure an empty user name.

This is more like a guard I added out of habit for my code.
Out of habit I also placed it at the top of the function.
Remove?

I was just asking, because maybe we need this check and it wasn't there before. Actually, looking at isGatewayAuthorized() top of func we already return true if opts.Gateway.Username == ""

I think this function (outside of tests at least) is always called with a user, so we should not need this test.
But if we think that cert.EmailAddress, etc.. may contain (legally or maliciously) empty values and if that would cause a failure for dnsAltNameLabels(), then you could leave the check.

I just guard against something weird coming out of the cert. that said. I realized that it is missing for the cluster

kozlovic · 2020-11-19T15:46:54Z

server/auth.go

+					if gw != nil && dnsAltNameMatches(labels, gw.URLs) {
+						return "", true
+					}
+				}


Help me understand if we are in this if statement, and we did not return true yet, should then we fail here? Or is it intended that we fall back to TLSMap check.

I have to go through all gateways.
Just because GW1 didn't contain a matching url does not mean that GW2 wouldn't.
Your comment would apply to the cluster case as well, there only one list exists.

I decided to support setting both flags (verify_and_map verify_and_implicit_allow) to true.
I did so primarily to avoid having to write the check to cause a config error if both are set.

change?

My question is more: if opts.Gateway.TLSImplicitAllow is true, but isDNSAltName is false, it means what? That we are not in a client context and we should not perform this check anyway. But if isDNSAltName is true and we did not find a match, does that makes sense to fallback to tls_map check only? I don't know.

if isDNSAltName is false then the argument user does not contain a value coming out of a DNS entry, hence it can't be used for matching, even if isDNSAltName allow is true.

When presented with an email for example, we shouldn't try to tokenize the ip address.

X509v3 extensions: X509v3 Subject Alternative Name: IP Address:127.0.0.1

We get a call where the name is 127.0.0.1 and isDNSAltName = false.
If tls_map happens to be true we check if 127.0.0.1 is configured as user.

if isDNSAltName is true and say the user is localhost, and we fail to find localhost in our config and if the user also enabled tls_map, why not see if the user localhost is configured?
if tls_map is not false, we'll default to false at the end of the function.

server/auth.go

kozlovic · 2020-11-19T15:54:37Z

server/opts.go

+	TLSTimeout       float64           `json:"-"`
+	TLSConfig        *tls.Config       `json:"-"`
+	TLSMap           bool              `json:"-"`
+	TLSImplicitAllow bool              `json:"-"`


To be honest, it is not clear to me what this option does. Default will be false, so can you describe to me what setting this boolean to true will mean?

the doc will say:
verify_and_implicit_allow| Only settable in a non client context where verify=true is the default (gateways/cluster). The incoming connections certificate's X509v3 Subject Alternative Name DNS entries will be matched against all urls in the configuration context that contains this tls map. If a match is found the connection is accepted, otherwise rejected. Meaning for gateways we will match all DNS entries in the certificate against all gateway urls. For cluster we will match against all route urls. As a consequence of this, dynamic cluster growth may require config changes in other cluster where this flag is true. DNS name checking is performed according to https://tools.ietf.org/html/rfc6125#section-6.4.1 Only the full wildcard * is supported for the left most label. This would be a way to keep cluster growth flexible.

The explanation seem clear to me, but I am not sure about the option name. Maybe @philpennock and @davidkemper can comment on that?

matthiashanel · 2020-11-20T00:56:34Z

@kozlovic @philpennock @davidkemper

Talked to @philpennock , let's hold off on IPs for now.
As for the name, here are more suggestions, any (partly/fully) sound particular appealing or descriptive?:
verify_and_inherit_allowed_names
verify_and_map_listed_names
verify_and_map_existing_hostnames
verify_and_map_names

kozlovic · 2020-11-20T02:03:39Z

Maybe verify_and_map_listed_names or verify_and_map_existing_hostnames? But I am very bad with names, so I would let maybe @davidkemper weigh in.

kozlovic

LGTM.. let's see if others have some input for the new config option name.

Only works for gateways and routes. When true the subject alt DNS name must match one url in the corresponding configuration Signed-off-by: Matthias Hanel <mh@synadia.com>

had to change failing tests to use insecure as to not fail due to the outgoing connection being not trusted. Signed-off-by: Matthias Hanel <mh@synadia.com>

Signed-off-by: Matthias Hanel <mh@synadia.com>

…pt_known_urls This follows the suggestion by phil. I added the and to be similar to verify_and_map. I fixed a minor issue where the implicit verify could be overwriting an explicitly configured one. Signed-off-by: Matthias Hanel <mh@synadia.com>

kozlovic · 2020-11-20T20:19:54Z

server/opts.go

+	TLSConfig          *tls.Config          `json:"-"`
+	TLSTimeout         float64              `json:"tls_timeout,omitempty"`
+	TLSMap             bool                 `json:"-"`
+	TLSAcceptKnownUrls bool                 `json:"-"`


For some reason, I feel like this name sounds less restrictive when enabled than when it is disabled, which is not the case. Meaning that the default is false, which sounds like the server is not accepting something, while in reality, when false it means that it may accept things that it should not.

Would TLSCheckKnownUrls be better? I know we agreed on verify_and_accept_known_urls but maybe we could also change that one with verify_and_check_known_urls. If the verify_and_check_ seem strange, maybe having verify_cert_and_check_known_urls lays out explicitly what this is about.

Signed-off-by: Matthias Hanel <mh@synadia.com>

kozlovic

LGTM

matthiashanel requested review from philpennock, kozlovic and davidkemper November 19, 2020 15:03

kozlovic requested changes Nov 19, 2020

View reviewed changes

kozlovic approved these changes Nov 20, 2020

View reviewed changes

matthiashanel added 4 commits November 20, 2020 13:57

[Added] verify_and_implicit_allow to tie subject alt name to url in cfg

74642e0

Only works for gateways and routes. When true the subject alt DNS name must match one url in the corresponding configuration Signed-off-by: Matthias Hanel <mh@synadia.com>

Incorporating comments and fixed an issue where code was not invoked

f24aec4

had to change failing tests to use insecure as to not fail due to the outgoing connection being not trusted. Signed-off-by: Matthias Hanel <mh@synadia.com>

Speeding up unit test fail case

06e4e93

Signed-off-by: Matthias Hanel <mh@synadia.com>

matthiashanel force-pushed the tls-verify-and-impliict-allow branch from 10c0759 to eda80ff Compare November 20, 2020 19:30

kozlovic reviewed Nov 20, 2020

View reviewed changes

Renaming to verify_cert_and_check_known_urls

8fd1b66

Signed-off-by: Matthias Hanel <mh@synadia.com>

kozlovic approved these changes Nov 20, 2020

View reviewed changes

kozlovic changed the title ~~[Added] verify_and_implicit_allow to tie subject alt name to url in cfg~~ [ADDED] verify_and_implicit_allow to tie subject alt name to url in cfg Nov 20, 2020

kozlovic changed the title ~~[ADDED] verify_and_implicit_allow to tie subject alt name to url in cfg~~ [ADDED] verify_cert_and_check_known_urls to tie subject alt name to url in cfg Nov 20, 2020

kozlovic merged commit 4d51a41 into master Nov 20, 2020

kozlovic deleted the tls-verify-and-impliict-allow branch November 20, 2020 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ADDED] verify_cert_and_check_known_urls to tie subject alt name to url in cfg #1727

[ADDED] verify_cert_and_check_known_urls to tie subject alt name to url in cfg #1727

matthiashanel commented Nov 19, 2020 •

edited

Loading

kozlovic left a comment

kozlovic Nov 19, 2020

matthiashanel Nov 19, 2020 •

edited

Loading

kozlovic Nov 19, 2020

kozlovic Nov 19, 2020

matthiashanel Nov 19, 2020

kozlovic Nov 19, 2020

matthiashanel Nov 19, 2020 •

edited

Loading

kozlovic Nov 19, 2020

matthiashanel Nov 19, 2020

kozlovic Nov 19, 2020

matthiashanel Nov 19, 2020 •

edited

Loading

matthiashanel Nov 19, 2020

kozlovic Nov 19, 2020

matthiashanel commented Nov 20, 2020

kozlovic commented Nov 20, 2020

kozlovic left a comment

kozlovic Nov 20, 2020

kozlovic left a comment

[ADDED] verify_cert_and_check_known_urls to tie subject alt name to url in cfg #1727

[ADDED] verify_cert_and_check_known_urls to tie subject alt name to url in cfg #1727

Conversation

matthiashanel commented Nov 19, 2020 • edited Loading

kozlovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiashanel Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiashanel Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiashanel Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiashanel commented Nov 20, 2020

kozlovic commented Nov 20, 2020

kozlovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kozlovic left a comment

Choose a reason for hiding this comment

matthiashanel commented Nov 19, 2020 •

edited

Loading

matthiashanel Nov 19, 2020 •

edited

Loading

matthiashanel Nov 19, 2020 •

edited

Loading

matthiashanel Nov 19, 2020 •

edited

Loading