Passing user roles from query originator to other nodes by Enmk · Pull Request #42537 · ClickHouse/ClickHouse

Enmk · 2022-10-20T14:59:45Z

Changelog category (leave one):

New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Push user roles from query originator to other nodes in cluster. Helpful when only originator has access to the external authenticator (like LDAP).
...

closes: #34130

alexey-milovidov · 2022-10-22T02:24:12Z

Missing tests.

vitlibar · 2022-12-28T00:22:40Z

Is this PR still in work?

vitlibar · 2022-12-28T00:23:42Z

src/Access/AccessControl.h

Why can't we combine those additional_roles with current_roles and use one parameter instead of two ones?

Because there is also use_default_roles, that controls which roles are applied to the ContextAccess.

I'm not entirely sure why use_default_roles should be false or true, hence wasn't sure if the same logic should be applied to external_roles.

vitlibar · 2022-12-28T00:25:33Z

src/Access/AccessControl.h

What are those additional_roles used for?

vitlibar · 2022-12-28T00:27:26Z

src/Access/AccessControl.cpp

Isn't it enough logging for authentication already?

Those are leftovers from debugging session and should be removed

vitlibar · 2022-12-28T00:28:22Z

src/Access/ContextAccess.cpp

What was wrong with just toString?

Leftovers from debugging, will be removed.

vitlibar · 2022-12-28T00:29:51Z

src/Access/IAccessStorage.cpp

Leftovers from debugging, will be removed.

vitlibar · 2022-12-28T00:37:54Z

src/Interpreters/Session.h

If I understood correctly, additional roles == extra_roles == extra_granted_roles? Is it correct? So many names...

Agree, renaming all that to just external_roles

vitlibar · 2022-12-28T00:40:18Z

src/QueryPipeline/RemoteQueryExecutor.cpp

This part looks strange. What is it for?

Roles UUIDs might be different on cluster nodes, while names 100% match, so it makes sense to send roles as list of names rather than list of UUIDs.

vitlibar · 2022-12-28T00:42:54Z

src/Interpreters/Session.cpp

Probably it's better to check this setting before sending those external roles (and don't send them if it's false).

yep, please see https://github.com/ClickHouse/ClickHouse/pull/42537/files#diff-a05838cce3aacc32a5b2fc017af198fb3086277d8912159aa7a3dcac7aeb7478R260

This is to address case when originator have access to remote auth provider (like LDAP server), but other nodes in cluster don't.

Sending roles only if `allow_extenral_roles_in_interserver_queries` setting is ON renamed various naming variants of 'additional', 'extra', 'externally granted' to 'external_roles' removed various debugging leftovers minor cleanup

alexey-milovidov · 2023-02-13T07:47:17Z

Missing tests.

Enmk · 2023-02-14T09:55:54Z

Missing tests.

Could you please mark this as "ready for tests", so we can execute produced binaries against LDAP suite from here: https://github.com/Altinity/clickhouse-regression/tree/main/ldap ?

alexey-milovidov · 2023-02-14T10:45:23Z

The tests should run in CI.

Since it is not meant to be rebuilt by the CI/CD

robot-clickhouse · 2023-05-27T07:48:48Z

This is an automated comment for commit 558c806 with description of existing statuses. It's updated for the latest CI running
The full report is available here
The overall status of the commit is 🔴 failure

Check name	Description	Status
CI running	A meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR	🟡 pending
Docs Check	Builds and tests the documentation	🟡 pending
Fast test	Normally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here	🔴 failure
Mergeable Check	Checks if all other necessary checks are successful	🔴 failure
Push to Dockerhub	The check for building and pushing the CI related docker images to docker hub	🟢 success
Style Check	Runs a set of checks to keep the code style clean. If some of tests failed, see the related log from the report	🟢 success

Tests for PR 42537

Co-authored-by: Enmk <[email protected]>

Co-authored-by: Enmk <[email protected]> fix memory access

robot-ch-test-poll added the pr-feature Pull request with new product feature label Oct 20, 2022

vitlibar self-assigned this Oct 20, 2022

alexey-milovidov marked this pull request as draft October 22, 2022 02:24

vitlibar reviewed Dec 28, 2022

View reviewed changes

src/Access/AccessControl.h Outdated

Copy link

Member

vitlibar Dec 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are those additional_roles used for?

vitlibar reviewed Dec 28, 2022

View reviewed changes

Enmk added 2 commits February 13, 2023 11:16

Passing user roles from query originator to other nodes

42153a8

This is to address case when originator have access to remote auth provider (like LDAP server), but other nodes in cluster don't.

Fixes according to PR review

fe1a932

Sending roles only if `allow_extenral_roles_in_interserver_queries` setting is ON renamed various naming variants of 'additional', 'extra', 'externally granted' to 'external_roles' removed various debugging leftovers minor cleanup

Enmk added 4 commits February 13, 2023 13:49

Merge branch 'master' into ldap_remote_roles

9eb6207

Fixed typo

c9e0cc6

Removed extra include

872f073

Minor cleanup

2cf7aeb

Enmk marked this pull request as ready for review February 13, 2023 14:45

vitlibar added the can be tested Allows running workflows for external contributors label Feb 14, 2023

zvonand and others added 4 commits April 27, 2023 00:20

upload not working

63e4e30

fixed, shortened

0458879

removed ldap image from list

cb9dd71

Since it is not meant to be rebuilt by the CI/CD

Merge remote-tracking branch 'upstream/master' into ldap_remote_roles

558c806

Merge pull request #5 from zvonand/zvonand-ldap-tests

48005b8

Tests for PR 42537

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Oct 9, 2024

Revive ClickHouse#42537

0350d19

Co-authored-by: Enmk <[email protected]>

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Oct 20, 2024

Revive ClickHouse#42537

83f67be

Co-authored-by: Enmk <[email protected]>

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Oct 20, 2024

Revive ClickHouse#42537

8c53aa5

Co-authored-by: Enmk <[email protected]>

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Oct 20, 2024

Revive ClickHouse#42537

5227993

Co-authored-by: Enmk <[email protected]>

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Oct 23, 2024

Revive ClickHouse#42537

708aa0b

Co-authored-by: Enmk <[email protected]>

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Oct 23, 2024

Revive ClickHouse#42537

2030b12

Co-authored-by: Enmk <[email protected]>

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Oct 23, 2024

Revive ClickHouse#42537

a95fd80

Co-authored-by: Enmk <[email protected]>

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Oct 26, 2024

Revive ClickHouse#42537

d2184d5

Co-authored-by: Enmk <[email protected]>

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Oct 28, 2024

Revive ClickHouse#42537

7df9ccd

Co-authored-by: Enmk <[email protected]>

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Oct 28, 2024

Revive ClickHouse#42537

d4cd857

Co-authored-by: Enmk <[email protected]>

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Oct 28, 2024

Revive ClickHouse#42537

9a25715

Co-authored-by: Enmk <[email protected]>

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Oct 28, 2024

Revive ClickHouse#42537

dedb46c

Co-authored-by: Enmk <[email protected]>

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Oct 28, 2024

Revive ClickHouse#42537

8310403

Co-authored-by: Enmk <[email protected]>

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Oct 28, 2024

Revive ClickHouse#42537

e610b59

Co-authored-by: Enmk <[email protected]>

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Oct 28, 2024

Revive ClickHouse#42537

6404dc3

Co-authored-by: Enmk <[email protected]>

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Oct 28, 2024

Revive ClickHouse#42537

db68941

Co-authored-by: Enmk <[email protected]>

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Oct 29, 2024

Revive ClickHouse#42537

af35ecf

Co-authored-by: Enmk <[email protected]>

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Nov 1, 2024

Revive ClickHouse#42537

67db844

Co-authored-by: Enmk <[email protected]>

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Nov 1, 2024

Revive ClickHouse#42537

1c85463

Co-authored-by: Enmk <[email protected]>

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Nov 1, 2024

Revive ClickHouse#42537

caf3584

Co-authored-by: Enmk <[email protected]>

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Nov 4, 2024

Revive ClickHouse#42537

c1a197d

Co-authored-by: Enmk <[email protected]>

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Nov 5, 2024

Revive ClickHouse#42537

39131d0

Co-authored-by: Enmk <[email protected]> fix memory access

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Nov 5, 2024

Revive ClickHouse#42537

71d3e4f

Co-authored-by: Enmk <[email protected]> fix memory access

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Nov 9, 2024

Revive ClickHouse#42537

d17dbbe

Co-authored-by: Enmk <[email protected]> fix memory access

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Nov 10, 2024

Revive ClickHouse#42537

ea1e49f

Co-authored-by: Enmk <[email protected]> fix memory access

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Nov 11, 2024

Revive ClickHouse#42537

d2c87f6

Co-authored-by: Enmk <[email protected]> fix memory access

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Nov 11, 2024

Revive ClickHouse#42537

5e1f560

Co-authored-by: Enmk <[email protected]> fix memory access

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Nov 18, 2024

Revive ClickHouse#42537

2b46c0a

Co-authored-by: Enmk <[email protected]> fix memory access

zvonand added a commit to zvonand/ClickHouse that referenced this pull request Nov 19, 2024

Revive ClickHouse#42537

86eb3d6

Co-authored-by: Enmk <[email protected]> fix memory access

Conversation

Enmk commented Oct 20, 2022

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Uh oh!

alexey-milovidov commented Oct 22, 2022

Uh oh!

vitlibar commented Dec 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitlibar Dec 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitlibar Dec 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov commented Feb 13, 2023

Uh oh!

Enmk commented Feb 14, 2023

Uh oh!

alexey-milovidov commented Feb 14, 2023

Uh oh!

robot-clickhouse commented May 27, 2023 • edited by robot-ch-test-poll1 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vitlibar Dec 28, 2022 •

edited

Loading

vitlibar Dec 28, 2022 •

edited

Loading

robot-clickhouse commented May 27, 2023 •

edited by robot-ch-test-poll1

Loading