[WIP] Add path exclusion to mTLS and BasicAuth authentication #1
Conversation
|
cc @tnozicka, let's clear up the details here before I send it for review upstream |
web/authentication.go
Outdated
| if len(certs) == 0 && isClientCertRequired(x.clientAuth) { | ||
| return false, "A certificate is required to be sent by the client.", nil | ||
| } |
There was a problem hiding this comment.
doesn't x509 authenticator require client certs by definition? (otherwise you'd not use the authenticator)
There was a problem hiding this comment.
there's VerifyClientCertIfGiven and I think it's nicer to have the logic of the authenticator handle all cases
There was a problem hiding this comment.
usually, the authenticator answers the question who is this user -> (username, group) then authorizer decides whether the user/group can access the resource - putting clientAuth (simple authorizer) into it makes in also an authorizer
There was a problem hiding this comment.
usually, the authenticator answers the question who is this user -> (username, group)
We discussed already that the toolkit uses authentication for authorization only, even with basic auth it doesn't care about the identity but only if the kv pair matches. That's why I named the interface an authorizer initially.
web/authentication.go
Outdated
|
|
||
| chains, err := certs[0].Verify(opts) | ||
| if err != nil { | ||
| return false, fmt.Sprintf("Bad certificate: %v", err), nil |
There was a problem hiding this comment.
nit: for debugging it's good to have a way to identify which cert failed to verify (e.g. to identify the client from logs)
There was a problem hiding this comment.
it's good to have a way to identify which cert failed to verify
What exactly would you log to get any valuable info? Request's RemoteAddr?
There was a problem hiding this comment.
usually a cert serial number + issues (look at kube)
There was a problem hiding this comment.
Added a copy from kube
web/authentication.go
Outdated
| return f(r) | ||
| } | ||
|
|
||
| func WithAuthenticationExceptor(handler http.Handler, authenticator Authenticator, exceptor AuthenticationExceptor, logger log.Logger) http.Handler { |
There was a problem hiding this comment.
I see this more as one layer above (at the mux) where you don't wrap the handlers with authentication, if you don't want authentication.
There was a problem hiding this comment.
e.g. at the mux level you already have a way to specify which path go to which handles and you can make richer rules then inside another layer of exact path matching
There was a problem hiding this comment.
It would definitely be a nicer approach, but would you be able to handle changes to the config file in that way? It's supported with the current implementation (albeit very poorly - by reading the config file on every request... multiple times) so I suppose it's expected to support it.
There was a problem hiding this comment.
but would you be able to handle changes to the config file in that way
Is this asking about dynamically changing which paths require auth? I don't expect that to by dynamic at all. (There is a fixes set of unauthorized endpoints like /readyz and then the rest that can either have auth enabled or disabled.)
There was a problem hiding this comment.
I don't expect that to by dynamic at all.
Yeah, but currently exporter-toolkit "reloads" the config file for the entire configuration. I suppose it has to be consistent for the excluded paths as well.
There is a fixes set of unauthorized endpoints like /readyz and then the rest that can either have auth enabled or disabled.
This (maybe) applies to prometheus, but it won't necessarily be the case for the entire toolkit.
There was a problem hiding this comment.
I suppose it has to be consistent for the excluded paths as well.
I was proposing not to put the paths into the config at all but rather into the code
There was a problem hiding this comment.
I agree that it's a better way to do it, but it looks like this is what the prometheus maintainers were aiming for: prometheus/prometheus#9166 (comment)
web/testdata/tls_config_noAuth.requireandverifyclientcert.authexcludedpaths.good.yml
Show resolved
Hide resolved
| switch clientAuth { | ||
| case tls.RequireAnyClientCert, tls.VerifyClientCertIfGiven, tls.RequireAndVerifyClientCert: | ||
| // Cert verification is delegated to the authentication middleware. | ||
| cfg.ClientAuth = tls.RequestClientCert |
There was a problem hiding this comment.
There's NoClientCert, in which case the certificate shouldn't be requested
// NoClientCert indicates that no client certificate should be requested
// during the handshake, and if any certificates are sent they will not
// be verified.
NoClientCert = iota
There was a problem hiding this comment.
I suppose this switch also depends on ClientCAs!="" so in that case this would be ruled out.
I general if you get a client CA you need to request the client cert, if you don't have the client CA there is no point in requesting that.
|
@tnozicka thanks, I've sent a draft commit with some of the requested changes. I'll clean it up once I have the answers to some of the questions in the comments. |
rzetelskik
force-pushed
the
path-exclusion-auth
branch
9 times, most recently
from
20ed165
to
673797d
Compare
rzetelskik
force-pushed
the
path-exclusion-auth
branch
from
673797d
to
9705f1a
Compare
|
Closing in favor of prometheus#151. For any further comments let's move upstream. cc @tnozicka @zimnx |
This PR introduces support for path exclusion from authentication for both mTLS and BasicAuth.
The approach to exclusion from mTLS is heavily based on how kubernetes/apiserver handles authentication and authorization, by lowering the requirements in TLSConfig and enforcing it by using a 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: prometheus#106 and prometheus#70.
Fixes prometheus/prometheus#9166