Add endpoint option to authenticate by client tls cert. #461

andersbetner · 2016-06-15T21:00:21Z

I have added authentication by client certificate as per #402
I haven't added any tests yet.

Does this pullrequest look all right?
I did not add an option for ClientAuthType. I just picked and hardcoded the most sensible default value: RequireAndVerifyClientCert
Should I add tests by basically duplicating the TestWithSNIConfigHandshake and TestWithSNIConfigRoute and then add client certification + client cert and CA cert in fixtures?

emilevauge · 2016-06-17T08:14:54Z

Hi @andersbetner, thanks for your contribution :)
I have few comments:

as we are in the process of releasing the 1.0.0 version, this PR will land in the 1.1.0 version (in few weeks).
I think we need to be able to have multiple client SSL certs. It would be interesting to change type of ClientCAFile.ClientCAFile from string to Certificates (and rename it then):

type TLS struct {
    Certificates          Certificates
    ClientCertificates    Certificates
}

WDYT?

/cc @vdemeester @samber

andersbetner · 2016-06-18T20:02:55Z

@emilevauge
Actually the cert-file can contain multiple certs.
I have added tests and clarified the documentation. (and changed the config parameter to ClientCAsFile).

The name ClientCAs is a bit of a misnomer, it doesn't hold client certificates. It contains the public key of one or more certificate authorities that you trust to sign client certs.
The client has to present a certificate that is signed by one of the CA:s in the ClientCAsFile.
I created two test cases where one has two CA:s in the ClientCAsFile.

Since this PR involves security it would be great if more than my pair of eyes could have a look at the code.

vdemeester · 2016-06-20T10:25:35Z

Seems good to me 😉
I kinda agree with @emilevauge on the fact that we should allow multiple values for ClientCAs (that each could have more public key in each 😛).

emilevauge · 2016-06-20T11:39:23Z

@andersbetner

The name ClientCAs is a bit of a misnomer, it doesn't hold client certificates. It contains the public key of one or more certificate authorities that you trust to sign client certs.
The client has to present a certificate that is signed by one of the CA:s in the ClientCAsFile.
I created two test cases where one has two CA:s in the ClientCAsFile.

True, obviously we cannot reuse Certificates type here.

Actually the cert-file can contain multiple certs.

Even if a file can contain multiple keys, I think it would be simpler to not force users to do that. Besides, the std library as been designed with that in mind: CertPool.AddCert https://golang.org/pkg/crypto/x509/#CertPool.AddCert. A slice of keys would allow both use case :)

Since this PR involves security it would be great if more than my pair of eyes could have a look at the code.

As soon as we agree on design, we will make a deep code review :)

andersbetner · 2016-06-20T18:47:39Z

@emilevauge , @vdemeester
I have no problem with a slice, I just didn't think of it. I was so colored by how Nginx ssl_client_certificate and Apache SSLCACertificateFile wants all CA:s in one file.

I had a look around and it seems like Caddy does it like you suggested.

Just to make sure I understand.
Is this what you meant?

type TLS struct {
    Certificates  Certificates
    ClientCAFiles []string
}

[entryPoints.https]
address = ":443"
ClientCAFiles = ["ca1.pem", "ca2.pem"]
[entryPoints.https.tls]
  [[entryPoints.https.tls.certificates]]
    CertFile = "integration/fixtures/https/snitest.com.crt"
    KeyFile = "integration/fixtures/https/snitest.com.key"

It is a pity that the authentication is forced on all SNI-certs on the same listener (by the go library), but I guess we are not alone there: Caddy issue #849

emilevauge · 2016-06-21T08:32:12Z

@andersbetner: Seems good.
You will also have to modify the entryPoints command line parser to include these client certs:

--entrypoints Entrypoints definition using format: --entryPoints='Name:http Address::8000 Redirect.EntryPoint:https' --entryPoints='Name:https Address::4442 TLS:tests/traefik.crt,tests/traefik.key' (default "map[]")

emilevauge · 2016-07-08T13:07:04Z

Hi @andersbetner, I'm planning to get this PR in the 1.1 releases. Do you think it's possible ?

andersbetner · 2016-07-08T14:48:56Z

@emilevauge
Sure, I was waiting for the 1.0 release.
I will rebase and make the changes to the config and command line.
Should I squash all changes into just two commit one for the changes to the code and one for the documentation? (that it should I sqash my future changes into the existing commits or does it not matter?

emilevauge · 2016-07-08T15:10:42Z

@andersbetner it would be better to squash your commits yes :)

andersbetner · 2016-07-19T10:40:09Z

@emilevauge
Added support for multiple CA-files as discussed earlier.
Tell me if you would like anything changed.

emilevauge · 2016-07-19T13:02:11Z

docs/basics.md

@@ -32,6 +32,7 @@ They can be defined using:
 - a port (80, 443...)
 - SSL (Certificates. Keys...)
 - redirection to another entrypoint (redirect `HTTP` to `HTTPS`)
+- authentication with a client certificate signed by a trusted CA


I would add this to the existing line SSL (Certificates. Keys...) :)

andersbetner · 2016-07-19T14:50:57Z

@emilevauge
I managed to get the config right for the tests but not in the documentation...

Fixed according to your comments.
Squashed the commit.
Rebased on top of master.

emilevauge · 2016-07-19T18:26:46Z

Thanks @andersbetner !
Maybe we will change a bit https://github.com/containous/traefik/pull/461/files#diff-bee1b19602a9f84a06f44e111f79a479R96 in a futur PR. I'm not totally convinced by how we manage flags with entrypoints...
LGTM!

/cc @containous/traefik

andersbetner · 2016-07-19T20:57:39Z

@emilevauge
Great, yeah I just made my addition to the flag syntax up. I have no problem with it being changed to something clearer.

andersbetner · 2016-07-20T12:43:03Z

@emilevauge
Rebased

emilevauge · 2016-07-20T16:15:52Z

/cc @containous/traefik

vdemeester · 2016-07-21T07:06:31Z

LGTM 🐯

andersbetner force-pushed the client_tls_auth branch 2 times, most recently from cbafb7d to 8074718 Compare June 17, 2016 07:05

emilevauge added this to the 1.1 milestone Jun 17, 2016

andersbetner force-pushed the client_tls_auth branch from 8074718 to 2d06ba7 Compare June 18, 2016 19:52

emilevauge added the contributor/waiting-for-corrections label Jul 8, 2016

andersbetner force-pushed the client_tls_auth branch from 2d06ba7 to f4e4947 Compare July 19, 2016 10:15

emilevauge added status/2-needs-review and removed contributor/waiting-for-corrections labels Jul 19, 2016

emilevauge reviewed Jul 19, 2016
View reviewed changes

andersbetner force-pushed the client_tls_auth branch from f4e4947 to f8a2b15 Compare July 19, 2016 14:38

emilevauge added the contributor/needs-resolve-conflicts label Jul 20, 2016

andersbetner added 2 commits July 20, 2016 14:41

Support for client certificate authentication

959c7dc

Added tests for client certificate authentication

664bc9c

andersbetner force-pushed the client_tls_auth branch from f8a2b15 to 664bc9c Compare July 20, 2016 12:42

vdemeester merged commit 9be0c67 into traefik:master Jul 21, 2016

vdemeester removed the contributor/needs-resolve-conflicts label Jul 21, 2016

emilevauge mentioned this pull request Jul 21, 2016

Refactor CA in entrypoints flag #549

Closed

sprohaska mentioned this pull request Aug 22, 2016

TLS client cert conflicts with ACME #640

Closed

ldez added status/3-needs-merge and removed status/2-needs-review labels Apr 29, 2017

djtm mentioned this pull request May 5, 2017

Mutual authentication using client certificate #402

Closed

ldez removed the status/3-needs-merge label Nov 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add endpoint option to authenticate by client tls cert. #461

Add endpoint option to authenticate by client tls cert. #461

andersbetner commented Jun 15, 2016

emilevauge commented Jun 17, 2016

andersbetner commented Jun 18, 2016

vdemeester commented Jun 20, 2016

emilevauge commented Jun 20, 2016 •

edited

Loading

andersbetner commented Jun 20, 2016

emilevauge commented Jun 21, 2016

emilevauge commented Jul 8, 2016

andersbetner commented Jul 8, 2016

emilevauge commented Jul 8, 2016

andersbetner commented Jul 19, 2016

emilevauge Jul 19, 2016

andersbetner commented Jul 19, 2016

emilevauge commented Jul 19, 2016

andersbetner commented Jul 19, 2016

andersbetner commented Jul 20, 2016

emilevauge commented Jul 20, 2016

vdemeester commented Jul 21, 2016

Add endpoint option to authenticate by client tls cert. #461

Add endpoint option to authenticate by client tls cert. #461

Conversation

andersbetner commented Jun 15, 2016

emilevauge commented Jun 17, 2016

andersbetner commented Jun 18, 2016

vdemeester commented Jun 20, 2016

emilevauge commented Jun 20, 2016 • edited Loading

andersbetner commented Jun 20, 2016

emilevauge commented Jun 21, 2016

emilevauge commented Jul 8, 2016

andersbetner commented Jul 8, 2016

emilevauge commented Jul 8, 2016

andersbetner commented Jul 19, 2016

emilevauge Jul 19, 2016

Choose a reason for hiding this comment

andersbetner commented Jul 19, 2016

emilevauge commented Jul 19, 2016

andersbetner commented Jul 19, 2016

andersbetner commented Jul 20, 2016

emilevauge commented Jul 20, 2016

vdemeester commented Jul 21, 2016

emilevauge commented Jun 20, 2016 •

edited

Loading