NETOBSERV-855 add authentication checks #277
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| } | ||
| hlog.Debug("Checking auth: kube config created") | ||
|
|
||
| _, err = client.CoreV1().Namespaces().List(ctx, v1.ListOptions{}) |
There was a problem hiding this comment.
what about relying on client.AuthorizationV1().SelfSubjectAccessReviews() here ?
There was a problem hiding this comment.
or just client.AuthenticationV1().TokenReviews() if you don't want to check any role
There was a problem hiding this comment.
Checking if a token is valid by trying to list namespaces with it looks a bit hacky.
As a first quick fix this is fine but we should look for a better solution IMO.
There was a problem hiding this comment.
I'll check what Julien suggests
There was a problem hiding this comment.
So, I think for a quick fix we should go ahead with GetNamespaces, bc using SSAR / TokenReviews requires permission from the console plugin to create these objects (so need changes in the operator etc.)
Also, I remember to have seen a similar issue and the GetNamespaces solution (call it a hack if you want :) ) was used because working with more k8s distributions (including ones that don't necessarily have RBAC out of the box)
There was a problem hiding this comment.
maybe just add a TODO comment in that case
| ) | ||
|
|
||
| func setupRoutes(cfg *Config) *mux.Router { | ||
| r := mux.NewRouter() | ||
| r.Use(func(orig http.Handler) http.Handler { |
There was a problem hiding this comment.
If I understand correctly, all endpoints will be behind authentication.
We should exclude from authentication the /metrics endpoint and the fronted static files.
There was a problem hiding this comment.
hmm I think we still need to do that? We still don't want metrics or whatever being accessible from unauthenticated users?
There was a problem hiding this comment.
@OlivierCazade I've removed the metrics endpoint for the time being
| } | ||
| hlog.Debug("Checking auth: kube config created") | ||
|
|
||
| _, err = client.CoreV1().Namespaces().List(ctx, v1.ListOptions{}) |
There was a problem hiding this comment.
Checking if a token is valid by trying to list namespaces with it looks a bit hacky.
As a first quick fix this is fine but we should look for a better solution IMO.
* NETOBSERV-855 add authentication checks * Update tests * temporary removal of metrics endpoint
* NETOBSERV-855 add authentication checks * Update tests * temporary removal of metrics endpoint
No description provided.