Add cmd flag to skip http paths from authentication #1637
Conversation
328090a
to
bceb05e
Compare
pkg/api/router.go
Outdated
@@ -163,8 +164,22 @@ func authHandler(cfg *Config, handler http.Handler) http.Handler { | |||
return handler | |||
} | |||
|
|||
isIgnoredPath := func(r *http.Request) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can add this as a method to Auth
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this is getting into too much auth logic for the router file. It should probably go into the common.go file, have the Auth method encapsulate all this internal logic about authorization methods and ignored paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might even make sense to break it up into its own auth file since its kinda overgrown the common file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, a bit of organization would benefit us here with the rise of complexity in the auth code.
pkg/api/router.go
Outdated
@@ -163,8 +164,22 @@ func authHandler(cfg *Config, handler http.Handler) http.Handler { | |||
return handler | |||
} | |||
|
|||
isIgnoredPath := func(r *http.Request) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this is getting into too much auth logic for the router file. It should probably go into the common.go file, have the Auth method encapsulate all this internal logic about authorization methods and ignored paths.
pkg/api/router.go
Outdated
@@ -163,8 +164,22 @@ func authHandler(cfg *Config, handler http.Handler) http.Handler { | |||
return handler | |||
} | |||
|
|||
isIgnoredPath := func(r *http.Request) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might even make sense to break it up into its own auth file since its kinda overgrown the common file.
9f48f88
to
a28b24f
Compare
I will pass this review to other reviewers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with a small ask to add a TODO for some future refactoring.
@@ -29,7 +28,7 @@ import ( | |||
"github.com/timescale/promscale/pkg/telemetry" | |||
) | |||
|
|||
func GenerateRouter(apiConf *Config, promqlConf *query.Config, client *pgclient.Client, store *jaegerStore.Store, reload func() error) (*mux.Router, error) { | |||
func GenerateRouter(apiConf *Config, promqlConf *query.Config, client *pgclient.Client, store *jaegerStore.Store, authWrapper mux.MiddlewareFunc, reload func() error) (*mux.Router, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is obviously a smell, having so many input parameters on a function. The function itself is too big as well.
Lets put a TODO on this to refactor it in the future so when somebody goes to add another parameter, they can see it and not do it 😃
a28b24f
to
05b5269
Compare
This commit adds a flag `--web.auth.ignore-path` which takes a http path and passed path would be ignored from authentication. The flag shall be repeated to pass array of ignored paths. e.g. ``` ./dist/promscale --web.auth.ignore-path=/heathz --web.auth.ignore-path=/api/query_range ``` Fixes timescale#1636 Signed-off-by: Arunprasad Rajkumar <ar.arunprasad@gmail.com>
Signed-off-by: Arunprasad Rajkumar <ar.arunprasad@gmail.com>
05b5269
to
c9981cb
Compare
Signed-off-by: Arunprasad Rajkumar ar.arunprasad@gmail.com
Description
This PR adds a flag named
--web.auth.ignore-path
which takes a http path and passed path would be ignored from authentication. The flag shall be repeated to pass array of ignored paths.e.g.
Fixes #1636
Code changes are inspired from https://github.com/brancz/kube-rbac-proxy/blob/fc1ca4f969941340a8adb66932bd64dc5773d37a/main.go#L298-L321
Merge requirements
Please take into account the following non-code changes that you may need to make with your PR: