Adds correct support for TLS Muthual autentication #235

rikatz · 2017-02-06T18:39:47Z

This PR adds support for TLS muthual autenthication by making the following changes:

Creates a new function in 'ssl.go' for dealing with secrets that contain 'ca.crt' but doesn't contain tls.key or tls.crt
Creating a conditional call in backend_ssl.go that calls each function described above, depending the in the condition the Secret was created
Changes the nginx.tmpl to call the correct field (CAFileName), and also adds a default 'huge' ssl_verify_depth to deal with the intermediary CA chains (while this is not solved), and also pass to the backends a Header containing the 'authenticated' certs, so the app itself can authorize or not this.

TODO (on future PR):

Make the ssl_verify_client configurable
Create a directive that allows specifying also a CRL file
Check if this is possible, and create a configurable 'redirect' directive when authentication is not succesful, as the following (an example is the following):

 if ($ssl_client_verify != SUCCESS) {
            redirect http://nossl.ingress.com/certerror.html;
        }

Hope this helps in this feature.

k8s-reviewable · 2017-02-06T18:39:52Z

This change is

coveralls · 2017-02-06T19:00:11Z

Coverage decreased (-0.4%) to 44.014% when pulling 39643bc on rikatz:ingress-ssl-auth into 016f3a2 on kubernetes:master.

coveralls · 2017-02-07T11:33:30Z

Coverage decreased (-0.4%) to 44.043% when pulling e1a52d1 on rikatz:ingress-ssl-auth into 016f3a2 on kubernetes:master.

coveralls · 2017-02-07T14:39:09Z

Coverage decreased (-0.5%) to 43.933% when pulling 0518f47 on rikatz:ingress-ssl-auth into 016f3a2 on kubernetes:master.

coveralls · 2017-02-07T17:43:12Z

Coverage decreased (-0.5%) to 43.933% when pulling ef7be11 on rikatz:ingress-ssl-auth into 016f3a2 on kubernetes:master.

bprashanth

Thanks for the pr! I haven't looked at the code yet, just making sure I understand the feature so I left some comments on the example.

bprashanth · 2017-02-08T22:10:54Z

examples/tls-authentication/nginx/README.md

+
+## Prerequisites
+
+You need a valid CA File, composed of a group of valid enabled CAs. This MUST be in PEM Format.


Please add somewhere a doc of how to set this up using openssl, maybe in the prerequisites section, and link to it from here? https://github.com/kubernetes/ingress/blob/master/examples/PREREQUISITES.md

## Generating a CA Some examples might require an explicty CA. You can act as your own CA by running... ## Generating Client certs Some examples require client certs. You can generate your own, and sign them using the CA generated above, as follows...

bprashanth · 2017-02-08T22:11:18Z

examples/tls-authentication/nginx/README.md

+## Prerequisites
+
+You need a valid CA File, composed of a group of valid enabled CAs. This MUST be in PEM Format.
+Also the Ingress must terminate TLS, otherwise this makes no sense ;)


Can you clarify why only tls termination, can't the ingress re-encrypt too?

Used the wrong words. You must have a TLS/HTTPs ingress, but no termination is really needed.

bprashanth · 2017-02-08T22:24:00Z

examples/tls-authentication/nginx/README.md

+
+You need a valid CA File, composed of a group of valid enabled CAs. This MUST be in PEM Format.
+Also the Ingress must terminate TLS, otherwise this makes no sense ;)
+


Please add a Terminology section and define (something like, feel free to make corrections or use your own text and add links where necessary):

CA cert: Certificate Authority public key. Client certs must chain back to this cert, meaning the Issuer field of some certificate in the chain leading up to the client cert must contain the name of this CA. For purposes of this example, this is a self signed certificate.

Client certs: Certificate used by clients to authenticate themselves with the loadbalancer/backends.

CA: Certificate authority signing the client cert, in this example we will play the role of a CA. You can generate a CA cert as show in this doc.

CA chains: A chain of certificates where the parent has a Subject field matching the Issuer field of the child, except for the root, which has Issuer == Subect.

Your terminology is fine! No changes. Will just write an adittional doc explaining how to generate you're very own CA (or point to CoreOS documentation!)

bprashanth · 2017-02-08T22:26:34Z

examples/tls-authentication/nginx/README.md

+## Deployment
+
+The following command instructs the controller to enable the TLS Authentication using
+the secret containing the valid CA chains.


The following command instructs the controller to enalbe TLS authentication using the secret from the ingress.kubernetes.io/auth-tls-secret annotation on the Ingress. Clients must present this cert to the loadbalancer, or they will receive a HTTP 400 response.

bprashanth · 2017-02-08T22:27:57Z

examples/tls-authentication/nginx/README.md

+  7s		7s		1	{nginx-ingress-controller }			Warning		MAPPING	Ingress rule 'default/nginx-test' contains no path definition. Assuming /
+
+
+$ curl -k https://ingress.test.com


Can you use the host header for this or do you need to use the full DNS name in the command line?

bprashanth · 2017-02-08T22:28:27Z

examples/tls-authentication/nginx/nginx-tls-auth.yaml

+metadata:
+  annotations:
+    # Create this with kubectl create secret generic caingress --from-file=ca.crt --namespace=default
+    ingress.kubernetes.io/auth-tls-secret: default/caingress


please add the ingress.class annotation as described in the PREREQUISITES doc

rikatz · 2017-02-09T11:45:46Z

@bprashanth Corrected as you suggested

coveralls · 2017-02-09T12:03:35Z

Coverage decreased (-0.4%) to 43.973% when pulling 1738cbd on rikatz:ingress-ssl-auth into 016f3a2 on kubernetes:master.

bprashanth

@rikatz good change overall, just some stylistic nits and refactor suggestions

bprashanth · 2017-02-10T00:04:14Z

examples/PREREQUISITES.md

+You can act as your very own CA, or use an existing one. As an exercise / learning, we're going to generate our
+own CA, and also generate a client certificate.
+
+These instructions are based in CoreOS OpenSSL instructions: https://coreos.com/kubernetes/docs/latest/openssl.html


please embed this link like so [instructions](https://coreos)

bprashanth · 2017-02-10T00:06:07Z

examples/PREREQUISITES.md

+-----END CERTIFICATE-----
+``` 
+
+You can have as many certificates as you wan't. If they're in the binary DER format, 


s/wan't/want

bprashanth · 2017-02-10T00:21:10Z

examples/tls-authentication/nginx/README.md

@@ -0,0 +1,73 @@
+# TLS termination


This title is wrong, and actually I think a better place to keep this example would be in a auth directory, so I'd imagine something like:

auth oauth password client-certs

Wdyt?
We could also instead create a tls directory like

tls termination client-certs reencrypt ...

Hum, I think that inside the auth directory is better. Will change here as suggested.

bprashanth · 2017-02-10T00:21:36Z

examples/tls-authentication/nginx/README.md

+
+* Client Cert: Certificate used by the clients to authenticate themselves with the loadbalancer/backends.
+
+* CA: Certificate authority signing the client cert, in this example we will play the role of a CA. 


Suggest defining CA before CA Certificate

bprashanth · 2017-02-10T00:21:58Z

examples/tls-authentication/nginx/README.md

+## Prerequisites
+
+You need a valid CA File, composed of a group of valid enabled CAs. This MUST be in PEM Format.
+The instructions are described here: https://github.com/kubernetes/ingress/blob/master/examples/PREREQUISITES.md#ca-authentication


Please embed the link

bprashanth · 2017-02-10T00:31:22Z

core/pkg/net/ssl/ssl.go

+		return nil, fmt.Errorf("could not close CA temp pem file %v: %v", tempCAPemFile.Name(), err)
+	}
+
+	pemCACerts, err := ioutil.ReadFile(tempCAPemFile.Name())


why write it then read it then validate, can you validate the raw CA before even creating the file so we can exit early if it's invalid? i.e something like

// AddCA validates then writes the given CA to a pem file located in the DefaultSSLDirectory (where is this coming from?) // and returns an SSLCert struct containing the location the CA was written to. AddCA(name, ca []bytes) (SSLCert, error) { if err := validate(ca) != nil { return nil, err } // write CA to path return ingress.SSLCerts{}, err }

Have changed the function, so don't need to write into a temp and move, but also don't need to use a new function :)

bprashanth · 2017-02-10T00:36:32Z

core/pkg/ingress/controller/controller.go

+		ic.secretQueue.Enqueue(key)
+	}
+	// Enough time to enqueue the new certificate
+	time.Sleep(5 * time.Second)


do we need this 5 second sleep or can we rely on the enqueue backing off with each failure? (I'd really prefer to not sleep, we can instead document that the secret must exist, and if it dosn't the controller will throw some events and possibly enter exponential backoff)

@bprashanth Had some failure here because of this, but can't remember right now. So I'm removing this sleep to see what happens

bprashanth · 2017-02-10T00:40:04Z

core/pkg/ingress/controller/backend_ssl.go

+		glog.V(3).Infof("Found certificate and private key, configuring %v as a TLS Secret", secretName)
+		s, err = ssl.AddOrUpdateCertAndKey(nsSecName, cert, key, ca)
+	} else if ca != nil {
+		glog.V(3).Infof("Found only ca.crt, configuring %v as an AuthCert secret", secretName)


This is clumsy but I guess you don't have any other way to do it?
IIUC you're assuming that if there's no key, it must be a ca.crt?
The log message is also a little confusing, please clarify what you mean by "configuring as an AuthCert"? (you don't seem to have defined what an AuthCert is anywhere, though I'm assuming you mean a client cert used for authentication. Suggest spelling that out.)

Yeap, the message is a bit confusing. Changed here to 'Found only ca.crt, configuring %v as an Certificate Authentication secret'

Also, this is the followed workflow:

If it contains a cert and a key, assume this is a keypair and configure it as the keypair from the Ingress (even if it contains a ca.crt also)

It it contains only the ca.crt, configure it as a CA only file

Otherwise, no valid secret, no file.

bprashanth · 2017-02-10T00:41:28Z

core/pkg/ingress/controller/backend_ssl.go

+	var s *ingress.SSLCert
+	if okcert && okkey {
+		glog.V(3).Infof("Found certificate and private key, configuring %v as a TLS Secret", secretName)
+		s, err = ssl.AddOrUpdateCertAndKey(nsSecName, cert, key, ca)


Is it possible to have a single function replace AddOrUpdateCertAndKey/AddOrUpdateCertAuth, called AddTLSKeyPair(nsSecName, cert, key, ca), which if cert and key are non-empty, appends ca to cert like AddOrUpdateCertAndKey and if they're empty, writes ca to file directly like AddOrUpdateCertAuth ? (I haven't though too much about the refactor so feel free to tell me it's too hard, just seems confusing to have 2 similar functions)

@bprashanth We can work on this. In the first moments I though it would be the best way. But looking at both functions, we would have to insert a lot of 'ifs' in it, to keep the same behaviour and only one function.

We can assume both ways, no restriction about that. Just followed the easiest one :)

bprashanth · 2017-02-10T00:49:30Z

controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl

        # PEM sha: {{ $location.CertificateAuth.PemSHA }}
        ssl_client_certificate              {{ $location.CertificateAuth.CAFileName }};
        ssl_verify_client on;
+        ssl_verify_depth 10;


For my own education, what is the average depth in the wild? do most people just have 1 CA that signs the 1 cert

2 is the most common depth for normal public-ca-minted certs, though 3 isn't uncommon (e.g. cross-signed certs, CAs that have second layers of intermediates).
Out of the alexa top 100 sites, 80% are depth=2, 9% depth=1, and 11% didn't feel like responding on 443.

However, for client cert auth, public CAs wouldn't generally be used. In that case, 2 and 1 are both quite common. 1 is common for organizations that have a root CA and directly issue client certs from that, while organizations that try to practice something akin to what public CAs do (e.g. keeping the root CA in offline storage and signing from intermediates on HSMs) will have a depth of 2.

It's worth noting that libnss (firefox?), iirc, has a max verify_depth of 8 before it gives up.

@euank You're right, i've been pretty lazy here. I'll add another annotation instead of hardcoding this, so the user can choose what it's verify depth.

Just FYI, here we use ssl_verify_depth 3 as he have the structure root -> intermediate -> final -> client :)

rikatz · 2017-02-10T16:01:18Z

@bprashanth Have to leave to a meeting now, will keep reviewing in the weekend

coveralls · 2017-02-10T16:20:48Z

Coverage increased (+1.2%) to 45.571% when pulling dbdfbf2 on rikatz:ingress-ssl-auth into 016f3a2 on kubernetes:master.

bprashanth · 2017-02-10T22:58:39Z

@rikatz no hurry, please take your time.
We should be fine as long as this doesn't languish to the point where it needs a huge rebase, because then it'll be wasted work.

rikatz · 2017-02-17T19:47:46Z

Jenkins error is valid, will take a look at this this weekend

coveralls · 2017-02-17T23:49:21Z

Coverage decreased (-0.3%) to 45.584% when pulling 95cf6cd on rikatz:ingress-ssl-auth into 3d9f89b on kubernetes:master.

rikatz · 2017-02-21T14:24:58Z

OK, corrected all the errors. Waiting for review to improve the code :)

aledbf · 2017-02-25T00:00:42Z

@rikatz please rebase

…tion modified: controllers/nginx/configuration.md modified: controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl modified: core/pkg/ingress/annotations/authtls/main.go modified: core/pkg/ingress/controller/backend_ssl.go modified: core/pkg/ingress/controller/controller.go modified: core/pkg/ingress/controller/util_test.go modified: core/pkg/ingress/resolver/main.go modified: core/pkg/ingress/types.go modified: core/pkg/net/ssl/ssl.go modified: examples/PREREQUISITES.md new file: examples/auth/client-certs/nginx/README.md new file: examples/auth/client-certs/nginx/nginx-tls-auth.yaml

rikatz · 2017-02-25T01:51:16Z

@aledbf Did it as asked. Don't know if this is correct, but here it is :)

coveralls · 2017-02-25T02:12:00Z

Coverage decreased (-0.3%) to 45.767% when pulling a342c0b on rikatz:ingress-ssl-auth into f5e005f on kubernetes:master.

aledbf · 2017-02-25T23:34:25Z

/lgtm

aledbf · 2017-02-25T23:34:34Z

@rikatz thanks!

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 6, 2017

rikatz mentioned this pull request Feb 7, 2017

When using auth-tls-secret, ssl_verify_depth might also be needed #110

Closed

bprashanth reviewed Feb 8, 2017

View reviewed changes

bprashanth reviewed Feb 10, 2017

View reviewed changes

bprashanth mentioned this pull request Feb 10, 2017

Add health checking from NGINX to individual endpoints. #146

Closed

rikatz force-pushed the ingress-ssl-auth branch from 95cf6cd to a342c0b Compare February 25, 2017 01:50

aledbf self-assigned this Feb 25, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 25, 2017

aledbf merged commit 0aabfba into kubernetes:master Feb 25, 2017

This was referenced Mar 8, 2017

Nginx Ingress Controller [0.8.3] ConfigMap - Client certificate verification not supported #42

Closed

Invalid Cert Auth does not get updated when changed #201

Closed

gianrubio mentioned this pull request Apr 7, 2017

Duplicate ssl_client_certificate directive when using ingress host with multiple paths #571

Closed


		## Prerequisites

		You need a valid CA File, composed of a group of valid enabled CAs. This MUST be in PEM Format.


		You need a valid CA File, composed of a group of valid enabled CAs. This MUST be in PEM Format.
		Also the Ingress must terminate TLS, otherwise this makes no sense ;)

		7s 7s 1 {nginx-ingress-controller } Warning MAPPING Ingress rule 'default/nginx-test' contains no path definition. Assuming /


		$ curl -k https://ingress.test.com


		* Client Cert: Certificate used by the clients to authenticate themselves with the loadbalancer/backends.

		* CA: Certificate authority signing the client cert, in this example we will play the role of a CA.

Adds correct support for TLS Muthual autentication #235

Adds correct support for TLS Muthual autentication #235

Conversation

rikatz commented Feb 6, 2017 • edited Loading

k8s-reviewable commented Feb 6, 2017

coveralls commented Feb 6, 2017

coveralls commented Feb 7, 2017

coveralls commented Feb 7, 2017

coveralls commented Feb 7, 2017

bprashanth 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

rikatz commented Feb 9, 2017

coveralls commented Feb 9, 2017

bprashanth 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

euank Feb 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rikatz commented Feb 10, 2017

coveralls commented Feb 10, 2017

bprashanth commented Feb 10, 2017

rikatz commented Feb 17, 2017

coveralls commented Feb 17, 2017

rikatz commented Feb 21, 2017

aledbf commented Feb 25, 2017

rikatz commented Feb 25, 2017

coveralls commented Feb 25, 2017

aledbf commented Feb 25, 2017

aledbf commented Feb 25, 2017

rikatz commented Feb 6, 2017 •

edited

Loading

euank Feb 13, 2017 •

edited

Loading