Add path exclusion to mTLS and BasicAuth authentication

[rzetelskik]

This PR introduces support for path exclusion from authentication for both mTLS and BasicAuth.
The main issue here was allowing for excluding the requests, which request a URL matching an excepted path, from
mTLS authentication. This PR achieves this by lowering the requirements in TLSConfig and delegating the certificate verification to a dedicated middleware - an approach heavily based on how kubernetes/apiserver handles authentication and authorization. To match this approach and allow for path exclusion for both types of authentication, basic authentication is also moved to a dedicated middleware.

As discussed in prometheus/prometheus#9166, the need for this comes from kubelet, the Kubernetes node agent, not allowing the use of certificates or safely providing credentials for liveness/readiness probes. See also prometheus-operator/prometheus-operator#5419.

This PR also takes into account the comments in the previous attempts to add support for this: #106 and #70.

Fixes prometheus/prometheus#9166

Edit:
Some implementation details, as requested by @bboreham :
This PR introduces an Authenticator interface.

type Authenticator interface {
	Authenticate(*http.Request) (bool, string, error)
}

The Authenticate method takes an *http.Request and decides whether the given request passes authentication.
The func WithAuthentication(handler http.Handler, authenticator Authenticator, logger log.Logger) http.Handler middleware is also introduced. It authenticates every request and passes it to the next handler on success.

The ChainAuthenticator is also implemented to conjunct multiple Authenticators. Its implementation of Authenticate fails to authenticate the request unless it authenticates against all chained Authenticators successfully.

There are two instantiations of the interface implemented in this PR: BasicAuthAuthenticator and X509Authenticator.

BasicAuthAuthenticator isolates all the code responsible for basic auth authentication that was there before in webHandler without any changes to the logic.

X509Authenticator implements the part of mTLS server handshake that is normally executed for connections with ClientAuth higher than VerifyClientCertIfGiven (https://cs.opensource.google/go/go/+/master:src/crypto/tls/handshake_server.go;l=878).
The crux of this PR is the following: one can't just exclude certain URLs from mTLS authentication. There's simply no access to this information at that level of encapsulation. Therefore ClientAuth in tlsConfig is always capped at RequestClientCert, and the actual certificate verification is delegated to the x509 middleware (ab8e6f5#diff-ee4d3f619ee85cfe4114c969b9777f1936391e90d3198df35fc6f0145077badeR246).

For the actual path exclusion an Exceptor interface is introduced.

type Exceptor interface {
	IsExcepted(r *http.Request) bool
}

func WithExceptor(handler http.Handler, authenticator Authenticator, exceptor Exceptor, logger log.Logger) http.Handler middleware calls IsExcepted and checks, based on a requested URL, whether a request is excepted from authentication. If so, it passes it directly to the provided handler, otherwise the handler is preceded with the authenticator.

[rzetelskik]

cc @roidelapluie @SuperQ

[rzetelskik]

@roidelapluie @SuperQ any chance to have someone review it?

[bboreham]

I don't know much about mTLS, but the title and description seem way too small for a +1200-line change.

[rzetelskik]

@bboreham I've added a section on implementation details, I hope it's clear enough. Also bear in mind that a bulk of that code are unit tests - I've tried to cover the changes quite extensively. The actual essence of these changes is only about 200-300 LoC, although I wouldn't really consider LoC a good indicator of complexity...

[bboreham]

The ChainAuthenticator is also implemented to conjunct multiple Authenticators.

Why? Maybe you are doing a large-scale refactoring? Please explain.

It seems possible that this line should go higher up in the explanation:

The crux of this PR is the following: one can't just exclude certain URLs from mTLS authentication.

[rzetelskik]

It seems possible that this line should go higher up in the explanation:

The crux of this PR is the following: one can't just exclude certain URLs from mTLS authentication.

Good point. I changed the main part of the description to include this.

The ChainAuthenticator is also implemented to conjunct multiple Authenticators.

Why?

I'm not sure I understand what your question is here. Why is the ChainAuthenticator introduced? I believe it's just cleaner than conditionally nesting the handlers with authenticators. It also allows for adding path exception to a collection of authenticators at once.

Maybe you are doing a large-scale refactoring? Please explain.

I don't think I'm doing much refactoring in this PR besides moving the code responsible for basic auth authentication to a middleware, but I've explained above the reasons for doing so. Other than that I've tried to keep the changes in the existing code minimal, so I don't understand where this is coming from.

[bboreham]

I don't think I'm doing much refactoring in this PR
only about 200-300 LoC

OK, we have a different view on that. The whole project is currently 1823 lines.

Thank you, the rationale to create a middleware and chain makes sense, it just wasn't in the description first time around.

[rzetelskik]

ping @SuperQ @roidelapluie

[rzetelskik]

ping @SuperQ @roidelapluie

[roidelapluie]

I do not feel confident writing TLS code ourselves is benefitting for the project.

[rzetelskik]

First and foremost, I don't agree that this approach includes "writing TLS code" at all. As explained in the PR description, all it does is it lowers certificate requirements on handshake and delegates the required certificate verification to a middleware. And for the verification itself it uses functions from a standard library, which is nowhere near to actually implementing the protocol. And even the code used to verify the certificates comes from the stdlib implementation: https://cs.opensource.google/go/go/+/master:src/crypto/tls/handshake_server.go;l=858.

Having said that, I believe you @roidelapluie were the one to suggest extending path exclusion to mTLS in one of the previous attempts to do so: #106 (comment), so I'd be happy to hear the approach you had in mind.

[rzetelskik]

@roidelapluie ping

[rzetelskik]

@roidelapluie ping

[ashish1099]

any update on this one ?

[rzetelskik]

any update on this one ?

My team still needs this for prometheus-operator and I'm happy to work on merging this. As you can see, the maintainers don't seem particularly interested though.

[francesco-furnari]

any update ?

[bboreham]

I have added this for discussion at a future Prometheus Dev Summit
https://docs.google.com/document/d/11LC3wJcVk00l8w5P3oLQ-m3Y37iom6INAMEu2ZAGIIE/edit

[rzetelskik]

Thanks @bboreham!
Just to comment on the entry in the doc - describing this PR as "refactoring of mTLS code" doesn't do it justice imho. It adds a feature as described in [1], and it does so in a way proposed by one of the Prometheus team members in a comment on one of the earlier PRs trying to solve this [2]. I asked for confirmation if this would be the desired approach but didn't get any responses then.

The use cases behind it: [3], [4].

[1] - prometheus/prometheus#9166
[2] - #106 (comment)
[3] - prometheus-operator/prometheus-operator#5419
[4] - prometheus-operator/prometheus-operator#4200

[matthiasr]

Hey, sorry for the delay in getting around to this. As per the Dev Summit, I am picking up the review hat for this. Please bear with me as I work my way through it 😄

[matthiasr]

Sorry for the long delay, it did take a while to get through everything with the care it deserves. Overall this looks good! Thank you for adding all the tests 🙏 they really helped me understand and trust what is going on.

My main ask is documentation, documentation, documentation. Both for the end user, at the "how and why to configure my exporter" level, and for users of the library. If we export it, someone will use it, and they need to know the semantics and what exactly happens when. This is particularly dicy around the TLS options – with no documentation in the toolkit to get them started, we cannot expect developers who use this to jump through and parse all the details of the Go TLS configuration options we are now exposing them to. In general, if we don't need to export something, I would err on the side of keeping it private – it's easy to later make things public when there's a clear use case, but hard to keep track of the big library surface we are opening up here.

Could you please rebase the PR?

[rzetelskik]

@matthiasr thanks for the detailed review! I'll try to get to rebasing it and going through your comments in the following weeks, bear with me as it's been quite a while since I submitted the PR, so I'll probably have to reiterate on some of the decisions I made.

[rzetelskik]

I rebased the PR on top of the current master and addressed all the comments.
I also added a couple of additional unit tests and changed the authentication middleware's implementation a bit to better align with the current behaviour so that none of the existing unit tests have to be modified.

As for exporting the authentication package - perhaps we could just move it to internal/?

@matthiasr please have a look

[matthiasr]

As for exporting the authentication package - perhaps we could just move it to internal/?

Oh yeah that's a good idea! Let's make anything that's not supposed to be used outside of the exporter-toolkit itself internal. Other than that, this looks ready to go for me!

[rzetelskik]

@matthiasr thanks! I moved it to /web/internal/authentication.

[matthiasr]

@SuperQ this is ready to go!

[matthewbjones]

This is great, as Kamal (https://kamal-deploy.org/) uses kamal-proxy and that relies upon a healthcheck endpoint and does not currently support HTTP Basic Auth, so in order to deploy Prometheus using the Kamal ecosystem, it requires that the healthcheck endpoint served by Prometheus to be exempt from the HTTP Basic Auth.

[rzetelskik]

@SuperQ is there anything you need from my side to merge this one?

[SuperQ]

One question, what about including additional paths in code?

For example, if I'm implementing an exporter with a health endpoint that I want to bypass auth, wouldn't I want to be able to do that by default so that users don't have to manually modify their local configs?