Router support for mutual tls authentication #19891

ramr · 2018-05-31T03:01:54Z

Changes as per RFE: https://bugzilla.redhat.com/show_bug.cgi?id=1477349

Adds mutual tls authentication support. Verification is via client-side certificates and its
use can be mandated to be either required or optional.
Additionally, an env variable ROUTER_MUTUAL_TLS_AUTH_CN provides more fine-grain
control on access based on certificate common names.

Usage:
Start the router with the new command line options to specify the mutual tls options.
Example:

$ oc adm router --latest-images=true  --mutual-tls-auth=required  \
       --mutual-tls-auth-ca=_ramr/nodejs-header-echo/config/CA/cacert.pem  \
       --mutual-tls-auth-filter='^/CN=header.test/ST=CA/C=US/O=Security/OU=OpenShift3$'

In the above example, the regular expression for mutual-tls-auth-filter forces an exact match with a certificate's subject.

To Test:

curl -insecure --cert-type pem --cert  _ramr/nodejs-header-echo/config/cert.pem  \
       --key _ramr/nodejs-header-echo/config/key.pem  -k -vvv  \
       --resolve reencrypt.header.test:443:127.0.0.1 https://reencrypt.header.test/

And test without the certificate/key to check that access is not allowed.
Other test scenarios can use different combinations of the filter (and/or --mutual-tls-auth flag) to test access.

@knobunc PTAL Thx

Edited Updated usage and tests

mrogers950 · 2018-05-31T13:09:29Z

cc @openshift/sig-security

simo5

nits

simo5 · 2018-05-31T13:43:22Z

images/router/haproxy/conf/haproxy-config.template

+  {{- if ne (env "ROUTER_MUTUAL_TLS_AUTH" "none") "none" }}
+    {{- with (env "ROUTER_MUTUAL_TLS_AUTH_CN") }}
+  # If a mutual TLS auth CN is set, we deny requests if the common name doesn't
+  # match. A custom template can change this behavior (e.g. set custom headers).


Please specify exactly what name the cert needs to match

Common Name ... will update this to make this clearer.

I am not asking what CN is, I am asking what it should match, it is not clear to me what you have to set CN to.

So to make it a wee bit clearer:

If the DN set to /CN=header.test/ST=CA/C=US/O=Security/OU=OpenShift3.

A couple (example set) of the headers set in the request sent to the backend would be:
X-SSL-Client-DN=/CN=header.test/ST=CA/C=US/O=Security/OU=OpenShift3
X-SSL-Client-CN=header.test

And the ROUTER_MUTUAL_TLS_AUTH_CN environment variable value allows you to "filter" requests based on that CN value (header.test).

Example:
A. For ROUTER_MUTUAL_TLS_AUTH_CN=der.test or ROUTER_MUTUAL_TLS_AUTH_CN=header.te would match the value and the request would be sent to the backend.
B. For ROUTER_MUTUAL_TLS_AUTH_CN=MyClient, the request would be rejected.

This sounds wrong, we should probably have a ROUTER_MUTUAL_TLS_AUTH_DN variable and match the exact subject as a default.
In what case matching a substring of a random CN in the subject is useful ?

That's what the customer was using/asking - ref: https://bugzilla.redhat.com/show_bug.cgi?id=1477349

In any case, this is moot because I just changed this to be an auth filter (see latest commit) to filter based on the subject (substring) so you can run a router ala:

oc adm router --latest-images=true --mutual-tls-auth=optional \ --mutual-tls-auth-ca=_ramr/nodejs-header-echo/config/CA/cacert.pem \ --mutual-tls-auth-filter="CN=header.test"

As re: an exact match, see the other comment re: filtering on organizations or OUs.

simo5 · 2018-05-31T13:44:33Z

images/router/haproxy/conf/haproxy-config.template

+  {{- if ne (env "ROUTER_MUTUAL_TLS_AUTH" "none") "none" }}
+    {{- with (env "ROUTER_MUTUAL_TLS_AUTH_CN") }}
+  # If a mutual TLS auth CN is set, we deny requests if the common name doesn't
+  # match. A custom template can change this behavior (e.g. set custom headers).


same as above

simo5 · 2018-05-31T13:45:14Z

images/router/haproxy/conf/haproxy-config.template

+  http-request set-header X-SSL-Issuer           %{+Q}[ssl_c_i_dn]
+  http-request set-header X-SSL-Client-NotBefore %{+Q}[ssl_c_notbefore]
+  http-request set-header X-SSL-Client-NotAfter  %{+Q}[ssl_c_notafter]
+  {{- end }}


It would be nice to have access to the whole client cert, is it possible via env vars ?

Ok will do - there's a ssl_c_der that gives the client cert in DER format (will convert to base64 if possible [otherwise hex] as it is in binary form).

simo5 · 2018-05-31T13:46:37Z

pkg/oc/admin/router/router.go

+
+	// MutualTLSAuthCRL contains the certificate revocation list used to
+	// verify a client's certificate.
+	MutualTLSAuthCRL string
 }


Given we default to no mutual TLS, it may be nice to specify a field with the Subject the cert need to match so that we do not need custom templates but a simple configuration change.

Hmm, ok - let me check with @knobunc on today's call on that.
Reason is that I felt the ROUTER_MUTUAL_TLS_AUTH_CN might not even be needed.
For the specific customer use case, they need to add/remove custom headers, so they would need a custom template to filter on the CN and then do the custom header magic anyway.

Hmm, am not sure doing this based on the subject may be possible. I did just briefly glance at the haproxy docs (ref: https://cbonte.github.io/haproxy-dconv/1.8/configuration.html#7.3.4) and didn't see a way to get at the subject field easily.

The subject is in SSL_CLIENT_S_DN

Or ssl_c_s_dn in haproxy

Okay, will rework this a bit later this afternoon to pass that filtering value as a command line option. And to do a substring match on that field ssl_c_s_dn. Thx

you probably want an exact match

so the existing RFE from https://bugzilla.redhat.com/show_bug.cgi?id=1477349 which the customer has uses a substring match.
In general, an exact match will be too restrictive if for example you want to filter on just say organizations, departments/units in an organization etc.

simo5 · 2018-05-31T13:49:36Z

pkg/oc/admin/router/router.go

 // generateSecretsConfig generates any Secret and Volume objects, such
 // as SSH private keys, that are necessary for the router container.
-func generateSecretsConfig(cfg *RouterConfig, namespace string, defaultCert []byte, certName string) ([]*kapi.Secret, []kapi.Volume, []kapi.VolumeMount, error) {
+func generateSecretsConfig(cfg *RouterConfig, namespace string, certName string, defaultCert, mtlsAuthCA, mtlsAuthCRL []byte) ([]*kapi.Secret, []kapi.Volume, []kapi.VolumeMount, error) {


May as well also drop string after namespace if we are into reordering arguments

Doh!! Thanks for catching that.

mrogers950 · 2018-05-31T19:30:40Z

pkg/oc/admin/router/router.go

+	mtlsAuth := strings.TrimSpace(cfg.MutualTLSAuth)
+	if len(mtlsAuth) > 0 && mtlsAuth != defaultMutualTLSAuth {
+		env["ROUTER_MUTUAL_TLS_AUTH"] = cfg.MutualTLSAuth
+		if len(mtlsAuthCA) > 0 {


Does it use the system store if the CA flag is not set? Otherwise it should be required when mTLS is enabled

The defaults section in the haproxy-config.template sets the default ca-base to the system store which gets used if one is not specified on the bind directive.

lihongan · 2018-06-01T08:59:56Z

pkg/oc/admin/router/router.go

@@ -309,15 +334,25 @@ func NewCmdRouter(f *clientcmd.Factory, parentName, name string, out, errout io.
 	cmd.Flags().BoolVar(&cfg.StrictSNI, "strict-sni", cfg.StrictSNI, "Use strict-sni bind processing (do not use default cert). Not supported for F5.")
 	cmd.Flags().BoolVar(&cfg.Local, "local", cfg.Local, "If true, do not contact the apiserver")

+	cmd.Flags().StringVar(&cfg.MutualTLSAuth, "mutual-tls-auth", cfg.MutualTLSAuth, "Controls access to the router using mutually agreed upon TLS configuration (ala client certificates). You can choose one of 'required', 'optional', or 'none'. The default is none.")


(ala client certificates) ala a typo ?

Meaning as an example but I will "rework" that text along with any other new review comments. Thx

ala is fine, but slightly obscure -- https://en.wiktionary.org/wiki/ala#Etymology_2

Might be better to say i.e.

knobunc · 2018-06-05T16:15:30Z

/hold
Until master opens for new features

knobunc · 2018-06-05T16:17:55Z

images/router/haproxy/conf/haproxy-config.template

+    {{- with (env "ROUTER_MUTUAL_TLS_AUTH_FILTER") }}
+  # If a mutual TLS auth subject filter environment variable is set, we deny
+  # requests if the DN field in the client certificate doesn't match that value.
+  # Please note that this match is a subset (substring) match.


Can you give an example showing that "head" would also match? And if you can anchor it so that it is an exact match? Thanks

knobunc · 2018-06-05T16:19:51Z

images/router/haproxy/conf/haproxy-config.template

+  # If a mutual TLS auth subject filter environment variable is set, we deny
+  # requests if the DN field in the client certificate doesn't match that value.
+  # Please note that this match is a subset (substring) match.
+  # Example: For DN set to: /CN=header.test/ST=CA/C=US/O=Security/OU=OpenShift3,


Let's just lose these examples and refer them to the examples above?

Done and referred to the examples above.

knobunc · 2018-06-05T16:22:19Z

pkg/oc/admin/router/router.go

@@ -309,15 +334,25 @@ func NewCmdRouter(f *clientcmd.Factory, parentName, name string, out, errout io.
 	cmd.Flags().BoolVar(&cfg.StrictSNI, "strict-sni", cfg.StrictSNI, "Use strict-sni bind processing (do not use default cert). Not supported for F5.")
 	cmd.Flags().BoolVar(&cfg.Local, "local", cfg.Local, "If true, do not contact the apiserver")

+	cmd.Flags().StringVar(&cfg.MutualTLSAuth, "mutual-tls-auth", cfg.MutualTLSAuth, "Controls access to the router using mutually agreed upon TLS configuration (ala client certificates). You can choose one of 'required', 'optional', or 'none'. The default is none.")


ala is fine, but slightly obscure -- https://en.wiktionary.org/wiki/ala#Etymology_2

Might be better to say i.e.

knobunc · 2018-06-05T16:24:16Z

/retest

acomabon · 2018-06-14T11:46:02Z

Any news about this merge?

enj · 2018-06-14T12:22:42Z

Master is frozen from anything but critical fixes until the release-3.10 branch is cut.

knobunc

/lgtm

knobunc · 2018-06-27T14:51:47Z

/hold cancel

pravisankar

/lgtm

lihongan · 2018-07-06T10:37:53Z

images/router/haproxy/conf/haproxy-config.template

+  http-request set-header X-SSL-Client-NotAfter  %{+Q}[ssl_c_notafter]
+  http-request set-header X-SSL-Client-DER       %{+Q}[ssl_c_der,base64]
+  {{- end }}
+


Are these headers required for edge routes which use plain http backend ? seems just re-encrypt route need them.

No strong opinion either way. But the headers do reflect information about the connection as it was handled on the edge by the router, so it is still relevant request info (irrespective of the type of backend/route) much like the [X-]Forwarded[-*] and Via headers.

knobunc · 2018-07-09T14:18:38Z

@juanvallejo @soltysh @enj -- Can someone approve the completion changes please.

simo5 · 2018-07-09T15:26:20Z

pkg/oc/admin/router/router.go

+
+	// MutualTLSAuthFilter contains the value to filter requests based on
+	// a client certificate subject field substring match.
+	MutualTLSAuthFilter string


I still do not think this should be a substring match.
In what case would you want to match only part of a cert without explicitly specifying which part at least ?
Unless there is a very specific reason to use a substring match we should only do an exact match.
Otherwise we risk opening up users to easy attacks.

The intent here is to restrict an already validated set of requests. It is important to note here that the connections are already filtered based on the CA signing the certificate (and/or any revoked certificates).

So the ROUTER_MUTUAL_TLS_AUTH_CA and ROUTER_MUTUAL_TLS_AUTH_CRL control the first pass of which connections (or certificates) are allowed and the substring match here is the second pass that is restricting that first block of connections to a smaller subset.

If we restrict that to specific match, then we can really only allow one single certifying authority per router. Multiple orgs/depts/units cannot be serviced by a single router instance.

This allows us flexibility to support that and if you really wanted a further locked down environment, you could just as well make the substring match more restrictive ala:
ROUTER_MUTUAL_TLS_AUTH_FILTER="/CN=header.test/ST=CA/C=US/O=Security/OU=OpenShift3" which effectively becomes a specific match.

Won't this match also: /CN=blah/CN=blah/CN=more-blah/CN=header.test/ST=CA/C=US/O=Security/OU=OpenShift3 ?

Yes, it would string substring match that. But that said, is that a valid DN/subject?
From the looks of it the / is a separator in the subject, correct?
So shouldn't that be escaped if someone forces that into an input field? Otherwise, there's potential of other issues, no?

@simo5 also pertinent here is the fact that the ROUTER_MUTUAL_TLS_AUTH_FILTER is optional. Meaning if its not specified, we don't do any restrictive checks. This is a secondary check.

But if this is still a sticking point for you, we could potentially change the substring match to a regular expression ... so you could potentially do a ^<escaped-text>$ kind of expression for an exact match.
That said, it does make the user interface a bit more awkward as it requires the user to specify a regular expression. Not a back breaker though!
And it is worth noting that a regular expression without the positional anchor ... ala CN=foo would still match a /CN=blah/CN=foo/CN=blah/ string.

@knobunc / @ironcladlou any issues with that?

You can have as many CN elements as you want in a DN.
Changing it to a regular expression would be definitely better and I would accept as a solution because then you can do exact matches if you want or anything.

An example with exact match and one with subfiltering should be provided as doc then.

juanvallejo · 2018-07-09T15:54:57Z

completions lgtm

juanvallejo · 2018-07-10T18:16:05Z

/approve

for completions

ramr · 2018-07-10T23:40:10Z

@simo5 changed to use a regular expression. Commit sha: 06615ab

No changes to the usage/command line except that the string is now a regular expression ala:

$ oc adm router --latest-images=true  --mutual-tls-auth=required   \
     --mutual-tls-auth-ca=_ramr/nodejs-header-echo/config/CA/cacert.pem  \
     --mutual-tls-auth-filter='^/CN=header.test/ST=CA/C=US/O=Security/OU=OpenShift3$'

The mutual-tls-auth-filter is a regular expression representing an exact match for the certificate subject.

ramr · 2018-07-10T23:41:47Z

@juanvallejo completions didn't change but in case you want to re-verify - the last commit only changed the command line option help text. Thx

ramr · 2018-07-11T00:13:58Z

Rebased with upstream to fix conflict in images/router/haproxy/conf/haproxy-config.template.

ramr · 2018-07-11T04:55:49Z

/retest

simo5 · 2018-07-11T09:05:12Z

please squash commits and I'll apply lgtm

ramr · 2018-07-11T17:56:24Z

@simo5 squashed the commits.

knobunc

/lgtm

knobunc · 2018-07-11T17:59:36Z

@simo5 -- Over to you

simo5 · 2018-07-11T18:25:22Z

/lgtm

simo5 · 2018-07-11T18:26:01Z

/retest

ramr · 2018-07-11T22:23:21Z

Opened new issue - test flake #20295

ramr · 2018-07-11T22:23:33Z

/retest

…ient side certificates and its use can be mandated to be either required or optional. In addition, an env variable ROUTER_MUTUAL_TLS_AUTH_CN provides more fine-grain control on access (via regular expressions) based on certificate common names.

ramr · 2018-07-12T02:49:10Z

@knobunc may need a new /lgtm as there was a conflict in router.go after the --extended-logging option PR merged. So I had to rebase (and fix the conflict and squash) this PR. Thx

simo5 · 2018-07-12T11:01:17Z

/lgtm

simo5 · 2018-07-12T11:01:37Z

/retest

openshift-ci-robot · 2018-07-12T11:03:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juanvallejo, knobunc, pravisankar, ramr, simo5

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~contrib/completions/OWNERS~~ [juanvallejo]
~~images/router/OWNERS~~ [knobunc]
~~pkg/oc/admin/router/OWNERS~~ [knobunc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

knobunc · 2018-07-12T15:26:43Z

/retest

openshift-ci-robot · 2018-07-12T17:26:38Z

@ramr: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/unit	`770687a`	link	`/test unit`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

ramr · 2018-07-12T18:04:41Z

/test unit
/test end_to_end

openshift-ci-robot requested review from pecameron and smarterclayton May 31, 2018 03:01

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 31, 2018

ramr force-pushed the mtls branch from ed83def to d3894ad Compare May 31, 2018 03:33

openshift-ci-robot added the sig/security label May 31, 2018

simo5 reviewed May 31, 2018

View reviewed changes

mrogers950 reviewed May 31, 2018

View reviewed changes

ramr force-pushed the mtls branch from 567e915 to 770687a Compare May 31, 2018 20:49

lihongan reviewed Jun 1, 2018

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 5, 2018

knobunc requested changes Jun 5, 2018

View reviewed changes

knobunc self-assigned this Jun 5, 2018

knobunc requested review from rajatchopra and pravisankar June 5, 2018 16:23

knobunc approved these changes Jun 14, 2018

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2018

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 27, 2018

pravisankar approved these changes Jun 28, 2018

View reviewed changes

openshift-ci-robot assigned pravisankar Jun 28, 2018

lihongan reviewed Jul 6, 2018

View reviewed changes

simo5 suggested changes Jul 9, 2018

View reviewed changes

ramr force-pushed the mtls branch from 06615ab to f2d0381 Compare July 11, 2018 00:12

ramr force-pushed the mtls branch from f2d0381 to eebf8f5 Compare July 11, 2018 17:55

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2018

knobunc approved these changes Jul 11, 2018

View reviewed changes

openshift-ci-robot assigned simo5 Jul 11, 2018

simo5 approved these changes Jul 11, 2018

View reviewed changes

ramr force-pushed the mtls branch from eebf8f5 to e789a67 Compare July 12, 2018 02:42

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 12, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 12, 2018

openshift-merge-robot merged commit c997c81 into openshift:master Jul 12, 2018

Boran mentioned this pull request Jun 10, 2022

TLS authentication + controller openshift/router#404

Closed

Router support for mutual tls authentication #19891

Router support for mutual tls authentication #19891

Conversation

ramr commented May 31, 2018 • edited Loading

mrogers950 commented May 31, 2018

simo5 left a 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

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

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

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

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

Choose a reason for hiding this comment

knobunc commented Jun 5, 2018

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

knobunc commented Jun 5, 2018

acomabon commented Jun 14, 2018

enj commented Jun 14, 2018

knobunc left a comment

Choose a reason for hiding this comment

knobunc commented Jun 27, 2018

pravisankar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knobunc commented Jul 9, 2018

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

Choose a reason for hiding this comment

juanvallejo commented Jul 9, 2018

juanvallejo commented Jul 10, 2018

ramr commented Jul 10, 2018

ramr commented Jul 10, 2018

ramr commented Jul 11, 2018

ramr commented Jul 11, 2018

simo5 commented Jul 11, 2018

ramr commented Jul 11, 2018

knobunc left a comment

Choose a reason for hiding this comment

knobunc commented Jul 11, 2018

simo5 commented Jul 11, 2018

simo5 commented Jul 11, 2018

ramr commented Jul 11, 2018

ramr commented Jul 11, 2018

ramr commented Jul 12, 2018

simo5 commented Jul 12, 2018

simo5 commented Jul 12, 2018

openshift-ci-robot commented Jul 12, 2018

knobunc commented Jul 12, 2018

openshift-ci-robot commented Jul 12, 2018 • edited Loading

ramr commented Jul 12, 2018

ramr commented May 31, 2018 •

edited

Loading

openshift-ci-robot commented Jul 12, 2018 •

edited

Loading