Clear TLS client headers if TLSMutualAuth is optional

[stffabi]

What does this PR do?

If no client peer certificates could be found always clear the headers so no request could pass them to backends, e.g. if TLSMutualAuth is optional.

Motivation

Using an optional TLSMutualAuth we've seen that clients could pass the headers into the request and the final request to the backend may look like it has been TLS mutual authenticated.

More

• Added/updated tests
• Added/updated documentation

Additional Notes

[jbdoumenjou]

Hi @stffabi, thanks for your interest in the project.

Your pull request description seems confusing to me.
The code you submit clears the passTLSClientCert headers and prevent from adding it to the requests without activating the option.

Is that what you want ?

[stffabi]

Hi @jbdoumenjou, thanks for your reply.

It should prevent that an incoming request which has the passTLSClientCert headers set, but no TLS ClientCert has been provided, will be sent to the backend with these headers still set.

Assuming an entrypoint with the following config, CA:/myca.cert CA.Optional:true and having an frontend with traefik.frontend.passTLSClientCert.pem=true. Therefore the entrypoint will have a TLSConfig with ClientAuth = tls.VerifyClientCertIfGiven. As a result if no ClientAuth has been done, the list r.TLS.PeerCertificates will be empty and ModifyRequestHeaders won't write the headers.

More precisely it won't touch the request. Let's assume the incoming request already had the X-Forwarded-Tls-Client-Cert header set, this will be just forwarded to the backend and the backend can't distinguish between an authenticated or an unauthenticated request and might trick the backend to work on an unauthenticated request.

One could discuss if the headers should always be removed, even if e.g. traefik.frontend.passTLSClientCert.pem=false or just in the true case. Maybe it's more clear to have them only removed if the corresponding option is set. So I would vote for removing them only if the option has been set and I've updated the PR accordingly.

[jbdoumenjou]

It is a good point to avoid someone to send unwanted passTLSClientCert header to the backend.
Your PR is very useful, it needs only one other thing.
When we want to pass the header without setting the passTLSClientCert, we need to explicitly accept to forward the header. So, I think we can add the X-Forwarded-Tls-Client-Cert to the X-Forwarded-XXX trust mechanism (you can check the header rewriter middleware).
WDYT?

Anyway, I totally agree with the need of the PR and change the status accordingly.
Feel free to ask for help/support, we will be happy to help you.

[stffabi]

That makes perfectly sense to remove the headers from untrusted sources if passTLSClientCert is not set.
I've updated the PR accordingly to remove the passTLSClientCert from untrusted sources.

[stffabi]

The header rewriter middleware comes after all other middlewares, so the current implementation removes the header even if the tlsClientHeaders middleware has added it before.

@jbdoumenjou any idea what would be the best way to handle this? Isn't that also a problem for the fowardAuth middleware? If forwardAuth has the flag trustForwardHeader activated and a request comes from an untrusted IP, then the forwardHeaders from the untrusted IP will be sent to the forward auth endpoint.
Shouldn't the rewriteHeader middleware be one of the first middlewares in the middleware chain? At least the logic which handles trustedIPs and the removal of the headers.

[jbdoumenjou]

You’re right.

Finally, we can consider that the last modification solve an edge case and it's not worth the cost.

[stffabi]

I've removed the changes from the rewriter middleware, the PR is now ready for another review.

Traefik 2.0 has the "rewriter middleware" (forwarded_header middleware) as one of the first middleware in the chain defined. So we could implement the logic for 2.0. If you are interested I could open another PR for the change in 2.0.

[jbdoumenjou]

LGTM

[jbdoumenjou]

We were discussing about the PR when we realized a misconception on the middleware.
It is not related to your PR, but thanks to you, we detect the problem and fix it accordingly.

[ldez]

Thanks 👍

[stffabi]

Glad I could help 😄

[mmatur]

LGTM