Skip to content

[RFC] Add proxy users for distributed queries#11498

Closed
azat wants to merge 5 commits intoClickHouse:masterfrom
azat:dist-proxy-user
Closed

[RFC] Add proxy users for distributed queries#11498
azat wants to merge 5 commits intoClickHouse:masterfrom
azat:dist-proxy-user

Conversation

@azat
Copy link
Member

@azat azat commented Jun 7, 2020

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Add proxy users for distributed queries

Detailed description / Documentation draft:

Based on ideas from #9751

Usage example

For this the following had been added:

  • CREATE USER proxied_user ALLOW PROXYING VIA proxy_user (covered)
  • ALTER USER proxied_user [ALLOW|DENY] PROXYING VIA proxy_user (covered)
  • GRANT PROXY ON . TO prooxy_user (covered)
  • REVOKE PROXY ON . FROM proxy_user (covered)
  • allowed_proxy_user config directive
  • allowed_proxy_users into system.users table
  • proxy_user in remote_servers (covered)

Where:

  • proxy_user -- user that allows to proxy
  • proxied_user -- update list of users that are alloewd to proxy to it

Left:

  • documentation
  • non-clumsy ALTER/CREATE
  • system.processes uses current_user (and also log message from ProcessList.cpp)
  • Add row-level filter support to Distributed engine #8926 (row base filter)
  • do not reset per-user limits (memory, etc)
  • more testing
  • non-clumsy GRANT
  • exception constructor via fmt
  • remote() support

Revs:

  • v2: rename setting to proxied_user to avoid overlaps with client CLI
  • v3: rebase and fix conflicts

Conflicts: #11391 (outperform)
Fixes: #6843
Fixes: #9751
Cc: @filimonov
Cc: @abyss7

Details

HEAD:

  • 5c72b02f86a1e483c5220aac7d572809f53f4b78
  • 35bbc5da23e984faba27c7293424c654211672f7

@azat azat marked this pull request as draft June 7, 2020 23:35
@blinkov blinkov added the pr-improvement Pull request with some product improvements label Jun 7, 2020
@azat azat changed the title [WIP] Add proxy users for distributed queries [RFC] Add proxy users for distributed queries Jun 8, 2020
@azat
Copy link
Member Author

azat commented Jun 10, 2020

@abyss7 @filimonov can you take a look? This is more "complete" replacement for #11391
Note, that it has a little bit awkward syntax (I guess), you can find usage example in the PR description.

@abyss7 abyss7 self-assigned this Jun 11, 2020
@azat
Copy link
Member Author

azat commented Jun 16, 2020

@abyss7 did you have a chance to look? (I'm curious about your thoughts on the idea in general)

@filimonov
Copy link
Contributor

filimonov commented Jun 16, 2020

  1. does it work for users defined via users.xml?
  2. ALTER USER initial_user ADD PROXY default - i don't like the syntax - sounds like that proxy will be used always. Maybe smth like ALLOW PROXYING VIA xxx / GRANT IMPERSONATION or similar. Not clear where checked (on initiator or on target) and what if the user has no this permission (he can't fire any distributed queries, or he will run them but they will be rejected by target nodes?). How to allow that to all users by default (i think that will be the most needed / desired setup). Do we need it at all? It leads to extra configuration complexity and not very sure why needed. To preserve certain users as proxy? To reject them? Maybe GRANT PROXY is enough?

May be combine them GRANT PROXY ON 'employee'@'localhost' TO 'employee_ext'@'localhost'; like in mysql?

@filimonov
Copy link
Contributor

And definitely need an opinion of @vitlibar

@azat azat force-pushed the dist-proxy-user branch from e3b700f to 35bbc5d Compare June 18, 2020 20:42
@azat azat marked this pull request as ready for review June 19, 2020 22:39
@filimonov
Copy link
Contributor

filimonov commented Jun 22, 2020

Regarding our talk about the permissions of proxy users.

It still not clear for me if we should allow proxying per user or globally for all users. The use case when we need to limit the list of users in which name that proxy can talk is still not very clear for me.

Let's imagine:

We have user Alice logged in on node1.
Node1 has a cluster configured to connect to other nodes (node2...nodeN) using special user Faythe (proxy).
Alice doesn't know the credentials of user Faythe.

If Alice asks for local resources everything works as before.

If Alice want to go to another cluster node (node2) data then:

  1. node1 checks is Alice has access to Distributed table
  2. if yes node1 establishes a connection to node2 with Faythe credentials.
  3. Faythe on node2 says 'in the name of Alice execute that query'.
  4. node2 checks if Faythe can talk in the name of other users, and if user Alice exists locally
  5. if yes node2 executes request as it was coming from Alice (applying all ACL/restrictions of Alice).

If Trudy (bad guy) wants to execute the selects pretending that he's Alice, he will first need either to connect to node1 as Alice, either to connect to node2 with Faythe credentials and start running queries in the name of Alice. In both scenarios, he needs to steal the secrets (credentials of Alice or credentials of Faythe) first. If we expect that Trudy can get secrets - we can't build any safe system.

Faythe actually can have only single permission (proxy = talking in the name of other users), she can have zero access to data.

That pairing (Faythe can talk in the name of Alice, but he can't talk in the name of Bob) sound safer, but introduces more complexity, and duplicates an ACL / RBAC functionality - i.e. if we want to limit access of Bob to distributed queries we can create it on node1 but not on node2 or set an ACL for Bob on a Distributed table, or set an ACL for Bob on the underlying table. Having one more option - Bob exists and can access table but Faythe can't talk in his name doesn't have too much sense, because otherwise Bob can just connect to every single node in the cluster in the loop directly (w/o Faythe proxy) and do what he wants.

If we really need to make it very granular / expect the use case on non-homogeneous clusters (when users and their permissions differ significantly between nodes) we actually may need some mapping of users on node2 to users on node1 etc. So in that case task is more complex and it will have a lot of corner cases (which still can't be covered/resolved by ALLOW PROXYING VIA). Not sure if those complications are needed, as it's a normal expectation to have all nodes of cluster homogeneous.

But maybe I'm wrong, and missing something.
Can you try to explain why do you think ALLOW PROXYING VIA is needed (using introduced actor names)?

@filimonov filimonov requested a review from vitlibar June 22, 2020 11:29
@azat
Copy link
Member Author

azat commented Jun 23, 2020

The use case when we need to limit the list of users in which name that proxy can talk is still not very clear for me.

For me to, but I guess that there can be some potential use cases.

For example, suppose you have "local" user (the user who executes remote query) and "proxy" user (the user which is used to login from one clickhouse server to another, i.e. <user> from <remote_servers>).

And let's assume that "proxy" user has only PROXY grants, and nothing more, but it does makes much sense in terms of secure, since it can "login" into any user anyway, and it is pretty unsafe, since you can access any table in this case on the remote node.
For example let's image that you have some local table on the remote node named "salaries" and user "manager" that is allowed to query it. You can create local user "manager" and use cluster to access "salaries" on remote node (and looks pretty unsecure).

Thoughts? @vitlibar @abyss7 @filimonov

@filimonov
Copy link
Contributor

filimonov commented Jun 23, 2020

For example let's image that you have some local table on the remote node named "salaries" and user "manager" that is allowed to query it. You can create local user "manager" and use cluster to access "salaries" on remote node (and looks pretty unsecure).

My expectations is that when cluster admin create user manager on one cluster node he should also create it on other nodes of that cluster. And if cluster admin allowed node1 to connect to node2 with special proxy user he automatically give a right for all users to use that cluster resources (unless it's limited by some ACL), and users don't know the creds (login/password) of proxy user, and can't create new users (only admin can, but he anyway know all the secrets)

@filimonov
Copy link
Contributor

And let's assume that "proxy" user has only PROXY grants, and nothing more, but it does makes much sense in terms of secure, since it can "login" into any user anyway

That just to enforce identifucation, i.e. to forbid queries from proxy user if he don't send in which name he want to execute the command.

Actually we can make it a bit more secure by adding smth like a signature.

I.e. SELECT * from foo settings as_user=Alice, proxying_signature = sha256(username, proxy_username, query_id, signing_server, target_server, Secret) so the target server can check if the query was issues by the apropriate server.

@azat azat force-pushed the dist-proxy-user branch from 9c8d089 to effb170 Compare June 24, 2020 08:48
@azat
Copy link
Member Author

azat commented Jun 24, 2020

Ok, I agree that additional ALLOW PROXYING VIA can be removed (and it can be added later if there will be some issues/cases).

can't create new users (only admin can, but he anyway know all the secrets)

Good point

I.e. SELECT * from foo settings as_user=Alice, proxying_signature = sha256(username, proxy_username, query_id, signing_server, target_server, Secret) so the target server can check if the query was issues by the apropriate server.

Yes, something like this can be added, later.

azat added 5 commits June 24, 2020 22:13
For this the following had been added:
- CREATE USER proxied_user PROXY proxy_user (covered)
- ALTER USER proxied_user [ADD|REMOVE] PROXY proxy_user (covered)
- GRANT PROXY ON *.* TO prooxy_user (covered)
- REVOKE PROXY ON *.* FROM proxy_user (covered)
- allowed_proxy_user config directive
- allowed_proxy_users into system.users table
- proxy_user in remote_servers (covered)

Where:
- proxy_user   -- user that allows to proxy
- proxied_user -- update list of users that are alloewd to proxy to it

Left:
- documentation
- more testing
- remote() support
- exception constructor via fmt

v2: rename setting to proxied_user to avoid oerlaps with client CLI
v3: rebase and fix conflicts
@azat azat force-pushed the dist-proxy-user branch from effb170 to e5b621e Compare June 24, 2020 19:13
@vitlibar
Copy link
Member

vitlibar commented Jun 26, 2020

If I understand the problem correctly you'd like to use initial_user (the user who logins and executes a query, not cluster's user) to check access rights on cluster's nodes. It seems to me there is an easier solution: just add an option to cluster's definition like

<remote_servers>
    <test_cluster>
        <node>
                <host>h1</host>
                <port>9000</port>
                <use_initial_user>1</use_initial_user>
        </node>

I mean this option becomes an alternative to <user> and <password>, if you set <use_initial_user>, don't set <user> and <password> for that node. And then if the option <use_initial_user> is set just pass the initial user's name and password to cluster's node (instead of the name and password of cluster's user). It will work if the initial user exists on all nodes and the password is also the same on all nodes.

@azat
Copy link
Member Author

azat commented Jun 26, 2020

I mean this option becomes an alternative to and , if you set <use_initial_user>, don't set and for that node

Thanks! So, that was my initial way (like #11391 but it does not have flag to switch, and user/password is still there plus it has some issues)

But there are some issues with connecting via original user/password:

  • right now protocol does not support user change w/o reconnects, so you need to issue reconnect (reconnect is not a huge problem I guess, and also protocol can be expanded to support this, or connection pool can be created for each user)
  • not always there is user/password while executing distributed query (see [CANCELED] Ability to preserve user for distributed queries #11391 (comment)), also can be fixed
  • requires expanding support for new authentication methods (not a big problem per se)
  • AFAIR @filimonov was worrying about extra authentication requests for LDAP (LDAP support not merged yet, but will be eventually), although I'm not sure that this is a problem (since connections should not be established often, and besides interaction with LDAP server will be required even after connect, and I guess it will have some cache anyway)
  • There is similar concept in other DBMS - Impersonate users in cluster #9751 (comment)
  • possible issues with INSERT side, that can be a little bit trickier, since there is distributed asynchronous INSERT, and they do not have all the context of the INSERT query since the are performed in background

Personally I prefer use initial user credentials to login to remote servers over proxy user, and I can address all the issues above (that are issues right now, i.e. everything except LDAP), but before I will start another version of this PR it is better to came to the agreement on this

@vitlibar
Copy link
Member

  • right now protocol does not support user change w/o reconnects, so you need to issue reconnect (reconnect is not a huge problem I guess, and also protocol can be expanded to support this, or connection pool can be created for each user)
  • not always there is user/password while executing distributed query (see #11391 (comment)), also can be fixed

As you said it seems it can be solved. Separate connection pools sound better than reconnections at this point.

  • requires expanding support for new authentication methods (not a big problem per se)

For the currently available authentication methods it shouldn't be problem at all. For new authentication methods it might be of course.

  • possible issues with INSERT side, that can be a little bit trickier, since there is distributed asynchronous INSERT, and they do not have all the context of the INSERT query since the are performed in background

You can store any part of the context you need to perform asynchronous INSERT beforehand. This issue doesn't seem to be difficult to solve.

Why is it similar? Proxy users in MySQL seem to be a different thing, it's related more likely to authentication in general than to access rights on other nodes while we're executing distributed queries.

@filimonov
Copy link
Contributor

reconnect is not a huge problem I guess

  • Why do we use connection pool at all in that case? :)
  • There are always an extra cost for establishing the connection. Imagine thousands req/sec.

Separate connection pools sound better than reconnections at this point.

100 users and cluster 20 shards x 2 replicas = pool of 4000 connections (outcoming), and the ~ same incoming.
4000 is our hard limit now.

100 users is not a big deal in a big company. 20 shards too.

And then if the option <use_initial_user> is set just pass the initial user's name and password

  • That means we need to preserve users credentials unencrypted (!!!) serverside during the session to pass it forward. Very bad idea.
  • If some complicated authentication protocols will be used (like Kerberos) it will require asking for new tickets every time the user wants to go to further nodes. Again if he wants to run the query on Distributed table with 20 shards - he will need to generate 20 tickets for a single query. Sounds bad.

Why is it similar? Proxy users in MySQL seem to be a different thing, it's related more likely to authentication in general than to access rights on other nodes while we're executing distributed queries.

It is similar idea. But used another way, which sounds like fitting well to existing clickhouse concepts.

in genral the idea is the following: server which are the part of cluster should be able to 'talk' each other. They should not need to use some external user creds to connect each other (they may need that connection out of the context of user queries). They should share the common secret. For the user only single login should happen after that he should have full access to resources he allowed to access.

Comparing to MySQL i would point the concept of replication setup:

CHANGE MASTER TO
    ->     MASTER_HOST='master_host_name',
    ->     MASTER_USER='replication_user_name',
    ->     MASTER_PASSWORD='replication_password',
    ->     MASTER_LOG_FILE='recorded_log_file_name',
    ->     MASTER_LOG_POS=recorded_log_position;

@vitlibar
Copy link
Member

@filimonov Ok, it sounds reasonable, thanks for the explanation.

@vitlibar
Copy link
Member

I have another idea. Cluster nodes check two filters for row policies - both for cluster's user and for initial user. It seems all we have to do to solve this task is to check access rights in the same way, i.e. provide access if both initial user and cluster's user on that node have corresponding grants.

@filimonov
Copy link
Contributor

filimonov commented Jun 27, 2020

Cluster nodes check two filters for row policies - both for cluster's user and for initial user.

Interesting, i was not aware of that.

both initial user and cluster's user on that node have corresponding grants.

That means that cluster user should have a superset of rights of all possible initial_users (i think in most use cases it will end up with super user/root rights, and that will lead to the situation when we check grants for 2 users, one of them anyway have rights for everything).

That would prevent the possibility to raise the permissions (i.e. bad user can't get more rights by sending another initial_user header) but it still leaves the hole in user identification/trackability possibilities.

I.e. If users Alice and Bob both work in the same departament and have the same rights Alice could pretend that she is Bob and drop some tables, and it will be impossible (or hard) to track such situation in the logs (Bob could be fired by angry boss before Bob will able to prove that it was Alice).

So generally that sounds like admin should be able to decide if particular user is allowed to set up / use initial_user header. That corresponds to that PROXY grant introduced here.

provide access if both initial user and cluster's user

Generally in that idea i like that it reuse existing concept of initial_user instead of introducing new concept (proxied_user) but it sounds like it may be more complicated to maintain compatibility with existing code and bit harder for the version upgrade. But it sounds solvable - may be we can give proxy grant for all users initially for backward compatibility, and allow to change that (smth like all_users_have_proxy_grant=1 initially, and set the default to zero after few versions)

BTW: one of the things we need to think of / clarify is the 'transitivity' of initial_user. Distributed over Distributed, or just a user passing the initial_user by manipulating headers. I didn't think about that yet, but that subject will definitely pop up.

@azat
Copy link
Member Author

azat commented Jun 27, 2020

@vitlibar

You can store any part of the context you need to perform asynchronous INSERT beforehand. This issue doesn't seem to be difficult to solve.

Yes, but there were worries (from @abyss7 ) even about storing password in RAM, so storing password on disk looks questionable.

provide access if both initial user and cluster's user on that node have corresponding grants.

This is still not enough, since most of the time proxy user will have all grants, and since right now you can send another proxied_user it is not safe (ALLOW PROXYING VIA was a "hack" to address this). I think that better will be to do real user authorization with user:password (say by adding another type of message)

@filimonov

reuse existing concept of initial_user instead of introducing new concept (proxied_user)

initial_user (it is the initiator of the Distributed queyr) is not the same as proxied_user (user who executes local query) and should not be.
And even if we could use initial_user instead of proxied_user I'm pretty sure that this is a bad idea since this can introduce some bugs that will be hard to find.

BTW: one of the things we need to think of / clarify is the 'transitivity' of initial_user. Distributed over Distributed, or just a user passing the initial_user by manipulating headers. I didn't think about that yet, but that subject will definitely pop up.

Not sure that this is an issue, since you will get entry in query_log for each remote query + initial query, so you can get information you need anyway (by using initial_query_id)

@azat
Copy link
Member Author

azat commented Jun 28, 2020

So to summarize I'm still not sure about PROXYing over user authorization (get user credential and authorize user with them on the remote side, w/o any reconnect)...

provide access if both initial user and cluster's user on that node have corresponding grants.

This is still not enough, since most of the time proxy user will have all grants, and since right now you can send another proxied_user it is not safe (ALLOW PROXYING VIA was a "hack" to address this). I think that better will be to do real user authorization with user:password (say by adding another type of message)

And in this case no need in any PROXY grants at all

in genral the idea is the following: server which are the part of cluster should be able to 'talk' each other. They should not need to use some external user creds to connect each other (they may need that connection out of the context of user queries). They should share the common secret. For the user only single login should happen after that he should have full access to resources he allowed to access.

Looks like two opposite things here:

  • from one side the server should talk to other nodes in the cluster without extra authentications
  • from another side extra permissions checks is required (to avoid hijacking)

I.e. SELECT * from foo settings as_user=Alice, proxying_signature = sha256(username, proxy_username, query_id, signing_server, target_server, Secret) so the target server can check if the query was issues by the apropriate server.

Something like this may help, but looks complex, and I don't see any real benefits over real user authorization...

@filimonov
Copy link
Contributor

real user authorization...

How? Storing user credentials in session is a very-very bad idea, not safe and is not acceptable. For authentication schemas like Kubernetes it require to ask for lot if tickets for executing single query, also not acceptable.

@filimonov
Copy link
Contributor

filimonov commented Jun 28, 2020

initial_user (it is the initiator of the Distributed queyr) is not the same as proxied_user (user who executes local query) and should not be.

It's mostly terminology.

1)We have logged in user.
2)That user wants to talk in the name of other user who was logged in remotely and initialized the query (you can call it initial_user or proxied user but that doesn't make big difference)

And even if we could use initial_user instead of proxied_user I'm pretty sure that this is a bad idea since this can introduce some bugs that will be hard to find.

I agree that it may lead to some issues due to existing initial_user usage patterns. But may be it will not be so hard to review / check them. And it will be better for the users to have less entities / simpler model compatible with existing.

@filimonov
Copy link
Contributor

filimonov commented Jun 28, 2020

Proposal:

  1. keep initial_user 'as is' . Don't introduce any changes in cluster config
  2. introduce effective_user - the one who is used for checking permissions, etc.
  3. if logged in user has a PROXY grant and initial_user passed, then use initial_user as the effective user.
  4. Otherwise currently logged in user is the effective user (just like now).
  5. (optionally) introduce a setting initial_user_without_proxy_grant = ignore/store/throw. I.e. it will never be used as effective but can be stored informationally (as is now, default), ignored silently, or send an exception

That sounds simple, should solve the issue, and is backward compatible. What do you think?

@vitlibar
Copy link
Member

I doubt there is a case where we should skip checking access rights for the initial user. Also it seems there is no reason to skip checking access rights for the remote users.

And I think we should prefer a solution which would be consistent with how other queries are processed.
When someone executes DDL on cluster (for example, CREATE TABLE ON CLUSTER)
ClickHouse checks whether the initial user has the grant CREATE TABLE on the initial node.

I suppose access rights' checking for distributed queries should be implemented in the similar way:
i.e.
Stage 1. StorageDistributed should check access rights for the initial user on the initial node before sending SELECT (or INSERT) queries to other nodes.
Stage 2. StorageDistributed also should get row policy's filter for the initial user
and send that filter to other nodes along with SELECT query.
Stage 3. Other nodes should check access rights for their remote users.
Stage 4. Other nodes should combine the filter from stage 2 with their own filters for
their remote users.

@filimonov
Copy link
Contributor

@vitlibar

And I think we should prefer a solution which would be consistent with how other queries are processed.
When someone executes DDL on cluster (for example, CREATE TABLE ON CLUSTER)
ClickHouse checks whether the initial user has the grant CREATE TABLE on the initial node.

That is not done good & safely. I can drop all your databases just having zookeeper access. You will not even know in that case who dropped your databases.

Stage 1. StorageDistributed should check access rights for the initial user on the initial node before sending SELECT (or INSERT) queries to other nodes.

That sounds like optimization to avoid sending queries that can't be executed on remote nodes due to a lack of permissions.

Stage 2. StorageDistributed also should get row policy's filter for the initial user and send that filter to other nodes along with SELECT query.

Interesting idea.
But
a) we can't blindly trust the filter arrived from the remote host (it can be modified or removed) - need smth like whitelist and/or signature confirming that it comes from the proper place (smth like PROXY grant)
b) if we are talking about the situation when cluster is homogenous - we expect that all permissions are the same across the nodes (no need to pass them conditions back and forward, we can just pick them by initial_user locally).
b) if we are talking about the situation when cluster is NOT homogenous - the initator node can have different databases and tables and it's quite strange to set up permissions for nonexistring objects.

Stage 3. Other nodes should check access rights for their remote users.
Stage 4. Other nodes should combine the filter from stage 2 with their own filters for
their remote users.

That leaves the issue described before (trackability):

I.e. If users Alice and Bob both work in the same department and have the same rights Alice could pretend that she is Bob and drop some tables, and it will be impossible (or hard) to track such situation in the logs (Bob could be fired by angry boss before Bob will able to prove that it was Alice).

@vitlibar
Copy link
Member

vitlibar commented Jun 29, 2020

@filimonov

I.e. If users Alice and Bob both work in the same department and have the same rights Alice could pretend that she is Bob and drop some tables, and it will be impossible (or hard) to track such situation in the logs (Bob could be fired by angry boss before Bob will able to prove that it was Alice).

Ok, I agree it's better to make sure on a node that a passed initial_user parameter is actually passed by that user. But how should we perform that check? As you said we must not simply pass initial user's password to other nodes. I think we can pass somehow salted password of initial_user. It can do the job however it won't work for all authentication types.

@vitlibar
Copy link
Member

vitlibar commented Jun 29, 2020

b) if we are talking about the situation when cluster is homogenous - we expect that all permissions are the same across the nodes (no need to pass them conditions back and forward, we can just pick them by initial_user locally).
b) if we are talking about the situation when cluster is NOT homogenous - the initator node can have different databases and tables and it's quite strange to set up permissions for nonexistring objects.

Good point.

@vitlibar
Copy link
Member

I can drop all your databases just having zookeeper access. You will not even know in that case who dropped your databases.

Yes, but it's unclear what to do about that. Don't give access to your zookeeper to bad people)

@filimonov
Copy link
Contributor

Yes, but it's unclear what to do about that. Don't give access to your zookeeper to bad people)

Some simple signature can be used to confirm that the data is created by proper person and was not changed, for example
alter_signature = sha256(initiator_username, query_id, query, signing_server, target_servers, secret_to_sign_alters, salt ), salt

@vitlibar
Copy link
Member

vitlibar commented Jun 29, 2020

Some simple signature can be used to confirm that the data is created by proper person and was not changed, for example
alter_signature = sha256(initiator_username, query_id, query, signing_server, target_servers, secret_to_sign_alters, salt ), salt

Ok.
For secret we can choose password's hash: if initial_user exists both on initial_node and target_node and the passwords and authentication types are same then password hashes (written in users.xml or on disk) should be same too and we can check signatures. We have nothing to choose for secret in case of the no_password authentication type, but it seems we can ignore that case. What do you think we can choose for secret in case of the ldap authentication type?

@filimonov
Copy link
Contributor

filimonov commented Jun 29, 2020

That secret should be inaccessible for users, should be configured between the servers (otherwise the user still can add bad queries into zookeeper w/o help of server).

I think it should be either smth completely isolated, or derived from interserver credentials (those listed in remote_servers)

@azat
Copy link
Member Author

azat commented Jun 30, 2020

Storing user credentials in session is a very-very bad idea, not safe and is not acceptable.

password/user already stored for the connection.

And we can use password hash for authorization (and it is already supported by the server but this does not allowed by the client but can be adjusted for this)

If some complicated authentication protocols will be used (like Kerberos) it will require asking for new tickets every time the user wants to go to further nodes

I guess that this kind of auth type will have some cache with TTL, that can address this (although not sure).

That sounds simple, should solve the issue, and is backward compatible. What do you think?

#11498 (comment) does not protects from user spoof, but this is addressed in other comments.

I doubt there is a case where we should skip checking access rights for the initial user. Also it seems there is no reason to skip checking access rights for the remote users.

AFAICS right now only one user is checked (+ #8926) ?

That secret should be inaccessible for users, should be configured between the servers (otherwise the user still can add bad queries into zookeeper w/o help of server).

Distributed queries (SELECT and INSERT) do not uses zookeeper, I guess you are talking about ON CLUSTER here?

alter_signature = sha256(initiator_username, query_id, query, signing_server, target_servers, secret_to_sign_alters, salt ), salt

So either user password should be used in the hash or some inter-server secret

@filimonov
Copy link
Contributor

filimonov commented Jun 30, 2020

password/user already stored for the connection.

That is a security hole/weakness, categorized as CVE-312 or more specific CVE-316

If some complicated authentication protocols will be used (like Kerberos) it will require asking for new tickets every time the user wants to go to further nodes
I guess that this kind of auth type will have some cache with TTL, that can address this (although not sure).

For Kerberos - you can't. Inside the ticket, there are the name of the target server (so you can't reuse a ticket for serverA to access serverB) and also timestamps (you can't reuse outdated tickets, and Kerberos requires clock sync, i.e. ntpd).

I guess you are talking about ON CLUSTER here?

Yes (but similar problem exists there)

So either user password should be used in the hash or some inter-server secret

It should be interserver secret. Using user password as a secret for making signatures will allow user to create valid signatures without server help.

@azat azat marked this pull request as draft July 2, 2020 21:49
@vitlibar vitlibar assigned vitlibar and unassigned abyss7 and vitlibar Jul 7, 2020
@azat azat closed this Aug 1, 2020
@azat azat deleted the dist-proxy-user branch October 2, 2020 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-improvement Pull request with some product improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Impersonate users in cluster Adding "use 'initial_user' option" to user specification in cluster definition

5 participants