enable TLS client forwarding #1446

drewwells · 2017-04-17T15:33:27Z

Copys the incoming TLS client certificate to the outgoing
request. The backend can then use this certificate for
client authentication ie. k8s client cert authentication

fixes #1392

timoreimann

Left some comments. I mostly focused on Go-specific issues since I'm not very familiar with Go's TLS library/package. Someone with more domain expertise will need to have another look.

Could you please add tests as well?

timoreimann · 2017-04-22T21:29:41Z

server.go

+				return nil, err
+			}
+			ok := pool.AppendCertsFromPEM(data)
+			if !ok {


I suppose this can be merged with the previous line and simplified like

if !pool.AppendCertsFromPEM(data) {

timoreimann · 2017-04-22T21:30:53Z

server.go

+			}
+		}
+		config.RootCAs = pool
+		//config.ClientAuth = tls.RequireAndVerifyClientCert


Please remove.

timoreimann · 2017-04-22T21:36:51Z

server.go

+						if frontend.PassTLSCert {
+							tlsConfig, err = createClientTLSConfig(entryPoint.TLS)
+							if err != nil {
+								log.Errorf("failed to create TLS config %s: %v", frontendName, err)


Please capitalize the first letter of the log message.

timoreimann · 2017-04-22T21:37:19Z

server.go

+						if frontend.PassTLSCert {
+							tlsConfig, err = createClientTLSConfig(entryPoint.TLS)
+							if err != nil {
+								log.Errorf("failed to create TLS config %s: %v", frontendName, err)


s/config/config for frontend/

timoreimann · 2017-04-22T21:43:01Z

server.go

@@ -418,6 +418,35 @@ func (server *Server) listenSignals() {
 	server.Stop()
 }

+func createClientTLSConfig(tlsOption *TLS) (*tls.Config, error) {
+	if tlsOption == nil {
+		return nil, errors.New("no TLS provided")


IIUC, then we only hit this case if no TLS was configured on the entry point.

Could we handle this error further up the stack, e.g., in loadConfig?

since it's a pointer, a tlsOption(nil) will panic. It should really be checked prior to access

emilevauge · 2017-05-19T13:56:41Z

Ping @drewwells ?

drewwells · 2017-05-19T14:16:43Z

there were some non-trivial changes to server.go, not sure I will get these conflicts merged correctly.

zyclonite · 2017-05-23T15:26:11Z

would it be enough to simply forward the subject of the certificate...

i did a pull request for the underlying proxy library to enable this
vulcand/oxy#68

emilevauge · 2017-06-06T15:38:42Z

@timoreimann do you validate this PR finally (even with the conflict)?

timoreimann

Sorry, this one fell off my radar.

LGTM, thanks! 👍

samgurtman-zz · 2017-06-10T06:47:56Z

Any time frame for merge?

drewwells · 2017-06-10T12:38:05Z

@zyclonite that MR turns off SSL verification

drewwells · 2017-06-10T12:39:02Z

This MR conflicts again, is there any interest in supporting this feature? We have moved on and no longer use Traefik b/c this feature was missing.

emilevauge · 2017-06-10T21:46:01Z

@drewwells @samgurtman sorry if it takes some time. We are a small team, half of the maintainers team does this on his spare time. We will merge it for sure, but be patient.

emilevauge · 2017-06-10T21:47:21Z

Ping @timoreimann

timoreimann · 2017-06-11T04:46:57Z

@emilevauge I approved a few days ago. Your LGTM is still missing. 😀

emilevauge · 2017-06-11T07:58:42Z

@timoreimann oops, sorry. Was looking from my mobile phone, and reviews are not visible 😊

emilevauge

Thanks @drewwells
LGTM!

emilevauge · 2017-06-11T12:34:35Z

Closing & reopening to enable Semaphore CI

Copys the incoming TLS client certificate to the outgoing request. The backend can then use this certificate for client authentication ie. k8s client cert authentication

drewwells · 2017-06-11T16:52:37Z

Great, thanks!

…

On Sun, Jun 11, 2017 at 8:24 AM Ludovic Fernandez ***@***.***> wrote: Merged #1446 <#1446>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1446 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOmE85fX_i27sP-B_L0U-pok5EN12yGks5sC-qTgaJpZM4M_P_F> .

samgurtman-zz · 2017-06-11T16:54:36Z

Fantastic! Thanks for this! We can fully transition to Traefik now.

samgurtman-zz · 2017-06-21T04:33:02Z

I'm trying this on Docker Swarm with a build off of master, but adding the label "traefik.frontend.passTLSCert=true" has no effect. Is this to be expected?

emilevauge · 2017-06-21T07:49:50Z

@samgurtman, in fact, @drewwells didn't add any label in this PR. We would love if you could help us on this in a new PR ❤️

samgurtman-zz · 2017-06-21T09:22:51Z

Could you point me to an example of an existing label implementation I can base it off of? I don't have any Go experience so that may be a problem.

emilevauge · 2017-06-21T10:15:37Z

@samgurtman you can have a look at traefik.frontend.passHostHeader in https://github.com/containous/traefik/blob/master/provider/docker/docker.go which should be similar :)

drewwells · 2017-06-21T13:22:16Z

Well originally this was a POC, but it took months to get the code merged. Maybe somebody with write access can add the other labels. Unless you want labels sometime in September :)

emilevauge · 2017-06-21T13:32:29Z

@drewwells

Maybe somebody with write access can add the other labels. Unless you want labels sometime in September :)

Maintainers (who have write access) are also making PR and it can take some time to get merged. We are a small team dealing with many contributions. We prefer carefully reviewing every PR than merging on the hoof.

And by the way, you were not that responsive either !

samgurtman-zz · 2017-06-22T03:44:43Z

So I added the label handling for docker, and I can confirm the passTLSCert stuff is being triggered, but I see no effects in the headers passed to my downstream server. Has this been smoke tested, and can someone confirm it is actually working with traefik.toml configuration?

ldez · 2017-06-22T07:48:14Z

This PR is merged. Please open a new issue or discuss this in :

Stack Overflow using the "traefik" tag
the Traefik community Slack channel

samgurtman-zz · 2017-06-22T08:48:54Z

Ok, I guess I won't contribute a PR then if you can't confirm whether this existing PR actually has any effect.

emilevauge · 2017-06-22T09:10:33Z

@samgurtman we just asked you to come discuss we us somewhere else as this PR is closed. If you think there may be an issue on this, we would love to hear you on Slack instead.

samgurtman-zz · 2017-06-22T09:16:52Z

I have indeed posted on slack already. I'd love to here from you there.

…

On 22/06/2017 9:10 PM, "Emile Vauge" ***@***.***> wrote: @samgurtman <https://github.com/samgurtman> we just asked you to come discuss we us somewhere else as this PR is closed. If you think there may be an issue on this, we would love to hear you on Slack instead. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1446 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGIKX4mgVQZktgXVM_VOZkYO2ElFExZWks5sGi-SgaJpZM4M_P_F> .

dac388 · 2017-06-28T19:04:44Z

Pretty confused about how I find out if this issue is resolved, or rather, how to implement the resolution, and by traefik documentation in general, tbh.

emilevauge · 2017-06-28T19:09:57Z

Thank you for your support ! 👍
This is a closed pull request, please open an issue or come on the slack channel to talk with the community.

MichaelSp · 2017-07-10T13:37:28Z

I'm also confused if this suppose to work.
Google leads me here, so maybe you (eg. @emilevauge) can comment on the current state of the implementation to not confuse others as well.
I'd expect:

the latest version to have that feature included: https://store.docker.com/images/traefik
to set label traefik.frontend.passTLSCert=true on my backend container
is any of that assumptions wrong?

ldez · 2017-07-10T14:19:52Z

@MichaelSp

this feature will be in the next release (1.4)
the label must be set on the frontend not the backend.

Thanks for your interest in Traefik 😃

This issue PR closed. Please open a new issue or discuss this in :

Stack Overflow using the "traefik" tag
the Traefik community Slack channel

timoreimann suggested changes Apr 22, 2017

View reviewed changes

ldez added contributor/waiting-for-corrections priority/P1 need to be fixed in next release labels Apr 24, 2017

ldez modified the milestone: 1.4 May 10, 2017

ldez added status/2-needs-review and removed priority/P1 need to be fixed in next release labels May 10, 2017

drewwells force-pushed the forwardclient branch from 92907df to 9291994 Compare May 19, 2017 15:25

timoreimann approved these changes Jun 6, 2017

View reviewed changes

ldez added kind/enhancement a new or improved feature. and removed contributor/waiting-for-corrections labels Jun 7, 2017

emilevauge approved these changes Jun 11, 2017

View reviewed changes

emilevauge added contributor/needs-resolve-conflicts status/3-needs-merge and removed status/2-needs-review labels Jun 11, 2017

emilevauge force-pushed the forwardclient branch from 9291994 to 34c9024 Compare June 11, 2017 12:34

emilevauge closed this Jun 11, 2017

emilevauge reopened this Jun 11, 2017

enable TLS client forwarding

22a4433

Copys the incoming TLS client certificate to the outgoing request. The backend can then use this certificate for client authentication ie. k8s client cert authentication

emilevauge force-pushed the forwardclient branch from 34c9024 to 22a4433 Compare June 11, 2017 12:41

ldez removed the status/3-needs-merge label Jun 11, 2017

ldez merged commit 3048509 into traefik:master Jun 11, 2017

ldez mentioned this pull request Jun 12, 2017

does traefik support complete ssl pass-through? #1376

Closed

drewwells deleted the forwardclient branch June 21, 2017 13:21

ldez removed the contributor/needs-resolve-conflicts label Jul 10, 2017

ldez added the area/tls label Aug 14, 2017

enable TLS client forwarding #1446

enable TLS client forwarding #1446

Conversation

drewwells commented Apr 17, 2017

timoreimann left a comment

Choose a reason for hiding this comment

timoreimann Apr 22, 2017

Choose a reason for hiding this comment

timoreimann Apr 22, 2017

Choose a reason for hiding this comment

timoreimann Apr 22, 2017

Choose a reason for hiding this comment

timoreimann Apr 22, 2017

Choose a reason for hiding this comment

timoreimann Apr 22, 2017

Choose a reason for hiding this comment

drewwells May 19, 2017

Choose a reason for hiding this comment

emilevauge commented May 19, 2017

drewwells commented May 19, 2017

zyclonite commented May 23, 2017

emilevauge commented Jun 6, 2017

timoreimann left a comment

Choose a reason for hiding this comment

samgurtman-zz commented Jun 10, 2017

drewwells commented Jun 10, 2017

drewwells commented Jun 10, 2017

emilevauge commented Jun 10, 2017

emilevauge commented Jun 10, 2017

timoreimann commented Jun 11, 2017

emilevauge commented Jun 11, 2017

emilevauge left a comment

Choose a reason for hiding this comment

emilevauge commented Jun 11, 2017

drewwells commented Jun 11, 2017 via email

samgurtman-zz commented Jun 11, 2017

samgurtman-zz commented Jun 21, 2017 • edited Loading

emilevauge commented Jun 21, 2017

samgurtman-zz commented Jun 21, 2017 • edited Loading

emilevauge commented Jun 21, 2017

drewwells commented Jun 21, 2017

emilevauge commented Jun 21, 2017 • edited Loading

samgurtman-zz commented Jun 22, 2017

ldez commented Jun 22, 2017

samgurtman-zz commented Jun 22, 2017 • edited Loading

emilevauge commented Jun 22, 2017

samgurtman-zz commented Jun 22, 2017 via email

dac388 commented Jun 28, 2017

emilevauge commented Jun 28, 2017 • edited by ldez Loading

MichaelSp commented Jul 10, 2017

ldez commented Jul 10, 2017 • edited by emilevauge Loading

samgurtman-zz commented Jun 21, 2017 •

edited

Loading

samgurtman-zz commented Jun 21, 2017 •

edited

Loading

emilevauge commented Jun 21, 2017 •

edited

Loading

samgurtman-zz commented Jun 22, 2017 •

edited

Loading

emilevauge commented Jun 28, 2017 •

edited by ldez

Loading

ldez commented Jul 10, 2017 •

edited by emilevauge

Loading