make wildcarded feature work on more patterns #250
Conversation
|
Your Render PR Server URL is https://chproxy-pr-250.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-cd652lsgqg418ep9ji3g. |
proxy.go
Outdated
| found = false | ||
| case rp.hasWildcarded: | ||
| // checking if we have wildcarded users and if username matches one 3 possibles patterns | ||
| for _, user := range rp.users { |
There was a problem hiding this comment.
What happens if we have the following wildcarded users:
business_*
*_analyst
*
And somebody queries with business_analyst? It seems it now depends on how rp.users is loaded for what user settings are used.
There was a problem hiding this comment.
good point, I will add in the doc this behavior when the admin defines overlapping patterns
proxy_test.go
Outdated
| expStatusCode: http.StatusUnauthorized, | ||
| f: func(p *reverseProxy) *http.Response { | ||
| req := httptest.NewRequest("POST", fakeServer.URL, nil) | ||
| req.Header.Set("X-ClickHouse-User", "default") |
There was a problem hiding this comment.
Small nitpick: We should extract the header names to const to avoid typo's in the tests.
| cu.name = name | ||
| cu.password = password | ||
| case name == "" || name == defaultUser: | ||
| // default user can't work with the wildcarded feature for security reasons |
There was a problem hiding this comment.
Can you elaborate in the comment what are the security scenarios we're talking about? I'm going to forget it soon as I know myself 😄
There was a problem hiding this comment.
if the clickhouse server is ill-configured and the default user is usable it's not a big deal currently since "external" users have to use chproxy which doesn't allow the default user unless the chproxy admin created a default user with no password.
But, with the wildcarded feature and more specifically the * case, an external user/hacker could try to connect to clickhouse using the usr/pwd "default"/"" and access data is not supposed to access.
There was a problem hiding this comment.
Could you add that to the doc or comment?
proxy.go
Outdated
| s := strings.Split(user.name, "*") | ||
| // cf a validation in config.go, the names must contains either a prefix, a suffix or a wildcard | ||
| switch { | ||
| case s[0] == "" && s[1] == "": | ||
| // the wildcarded user is "*" | ||
| return getUserInformations(rp, user, name, password) | ||
| case s[0] == "": | ||
| // the wildcarded user is "*[suffix]" | ||
| suffix := s[1] | ||
| if strings.HasSuffix(name, suffix) { | ||
| return getUserInformations(rp, user, name, password) | ||
| } | ||
| case s[1] == "": | ||
| // the wildcarded user is "[prefix]*" | ||
| prefix := s[0] | ||
| if strings.HasPrefix(name, prefix) { | ||
| return getUserInformations(rp, user, name, password) | ||
| } |
There was a problem hiding this comment.
Could we extract it out to dedicated function? This one gets biggy
|
|
||
| The current implementation of wildcarded doesn't work with some limits on `out-users` (like the max_concurrent_queries) but works well with all the limits on `in-users`. The limits for `in_users` and `out-users` works as if all the users matching the pattern were the same user. | ||
|
|
||
| If the wildcarded users are overlapping, the real users will be attached randomly to one of the wildcarded users. For example, let's say: |
There was a problem hiding this comment.
Will it remain consistent for the same user? Meaning if analyst_john-UK is routed to analyst1 out user in first place, will it remain the same for the second query launched by him?
There was a problem hiding this comment.
Technically, while chproxy is up then yes it will be consistent.
If chproxy is restarted, it should be consistent (but I'm not sure).
If we change something and the admin updates chproxy then the behavior is unknown.
This is why I decided to put "randomly" and don't dive into details so that chproxy admins are warned that they should be extra careful when the wildcarded users are overlapping
proxy.go
Outdated
| return found, u, c, cu | ||
| } | ||
|
|
||
| func getUserInformations(rp *reverseProxy, user *user, name string, password string) (found bool, u *user, c *cluster, cu *clusterUser) { |
There was a problem hiding this comment.
I don't know how to better name it but initial naming hides the fact that it relates to wildcarded users. Maybe we could completely extract wildcarded users logic away to a dedicated file with possibly dedicated struct if needed?
| func getUserInformations(rp *reverseProxy, user *user, name string, password string) (found bool, u *user, c *cluster, cu *clusterUser) { | |
| func (rp *reverseProxy) overrideWildcardedUser(user *user, name string, password string) (found bool, u *user, c *cluster, cu *clusterUser) { |
There was a problem hiding this comment.
I found a more explictit (but longer) name: generateWildcardedUserInformation
Regarding putting the wildcarded logic in a specfic file, I looked at the implementation and currently there are:
- 2 lines on config.go
- 37 lines of code in 2 specific function (I created a specific function like you ask) so I'm not sure it's worth creating a dedicated file. It would make sens if all the code was like that (for example if we were using a middleware pattern)
| @@ -579,6 +579,23 @@ type clusterUser struct { | |||
| isWildcarded bool | |||
| } | |||
|
|
|||
| func deepCopy(cu *clusterUser) *clusterUser { | |||
There was a problem hiding this comment.
Can you explain why is it needed?
There was a problem hiding this comment.
because the current implementation doesn't do a deepcopy and only modify the name & pwd of the cu user.
So, if analyst_john-UK & analyst_jane-UK are doing a query at the same time we can end-up in this situation:
- cu user name = john & password = pwd1
- cu user name = jane & password = pwd2
- the query of john is executed in clickhouse with the credentials jane/pwd2
- the query of jane is executed in clickhouse with the credentials jane/pwd2
proxy.go
Outdated
| return found, u, c, cu | ||
| } | ||
|
|
||
| func findWildcardedUserInformation(rp *reverseProxy, name string, password string) (found bool, u *user, c *cluster, cu *clusterUser) { |
There was a problem hiding this comment.
I'd still make it a method receiver to reverseProxy, instead of a function accepting reverseProxy as parameter, but up to you the final choice
Description
cf #249
the aim of this PR is to make the wildcarded feature work on usernames that matche one of the 3 following patterns
instead of having the feature working only for usernames that matches the pattern [prefix]_[wildcard]
Pull request type
Please check the type of change your PR introduces:
Checklist
Does this introduce a breaking change?
Further comments