Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LDAP group to local role mapping support #17211

Merged
merged 22 commits into from
Jan 25, 2021

Conversation

traceon
Copy link
Contributor

@traceon traceon commented Nov 19, 2020

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category:

  • New Feature

Changelog entry:

  • Added support of mapping LDAP group names, and attribute values in general, to local roles for users from ldap user directories

Detailed description:

@robot-clickhouse robot-clickhouse added doc-alert pr-feature Pull request with new product feature labels Nov 19, 2020
Add role_mapping documentation (comments)
Add serialization of new params in getStorageParamsJSON()
* master: (159 commits)
  Review fix.
  Update version_date.tsv after release 20.8.7.15
  wrong translation
  Update version_date.tsv after release 20.9.6.14
  Update version_date.tsv after release 20.10.5.10
  Update version_date.tsv after release 20.11.4.13
  Improvements in coverage images
  Fixed a problem with the translation of the document
  final_parallel
  final_parallel
  DOCSUP-4162: Document the system.replicated_fetches system table (ClickHouse#16900)
  Update settings.md
  Update settings.md
  Less verbose logging when fetch is impossible
  Don't add tons of client coverage files in stateful tests with coverage
  More compatible watches in TestKeeper
  Trying to make read_in_order_many_parts more stable
  trigger CI
  Update version_date.tsv after release 20.6.10.2
  Update visibleWidth.cpp
  ...
* master: (50 commits)
  Update documentation-issue.md
  Add an option to use existing tables to perf.py
  DOCSUP-4280: Update the SELECT query (ClickHouse#17231)
  DOCSUP-3584 edit and translate (ClickHouse#17176)
  Fixed flaky test_storage_s3::test_custom_auth_headers
  Update 01560_merge_distributed_join.sql
  Minor improvements
  Slightly more correct
  Auto version update to [20.13.1.1] [54444]
  Auto version update to [20.12.1.5236] [54443]
  Update roadmap
  Add favicon; add loading indicator
  Fix race condition; history and sharing capabilities
  Update bitmap-functions.md
  Fix exception message
  Use default value for read-only flag in metadata for Disk3.
  ISSUES-16605 try fix review comment
  trigger CI
  ISSUES-16605 try fix integration failure
  ISSUES-16605 try fix integration test failure
  ...
* master: (207 commits)
  Update RadixSort.h
  rerun tests to be sure
  Update date_time_short perf test for toUnixTimestamp(Date())
  update test
  remove comments
  better
  fix tests
  style
  update copy pasted test
  better
  comments
  better merge
  new interface for the function
  better
  Fix comments
  Add missing file
  Make the code less bad
  initial
  test added
  style
  ...
@traceon traceon marked this pull request as ready for review November 30, 2020 08:13
@vitlibar vitlibar self-assigned this Dec 2, 2020
Copy link
Member

@vitlibar vitlibar left a comment

Choose a reason for hiding this comment

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

programs/server/config.xml Show resolved Hide resolved
src/Access/Authentication.h Outdated Show resolved Hide resolved
will fail as if the provided password was incorrect.
If no roles are specified here or assigned during role mapping (below), user will not be able to perform any
actions after authentication.
role_mapping - section with LDAP search parameters and mapping rules.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's better to rename role_mapping into roles_search? Just to represent the fact that all those parameters are about searching in LDAP and to make the difference between this section and the roles section more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

imported_roles maybe?

The resulting filter will be constructed by replacing all '{username}', '{bind_dn}', and '{base_dn}'
substrings of the template with the actual user name, bind DN, and base DN during each LDAP search.
Note, that the special characters must be escaped properly in XML.
prefix - prefix, that will be expected to be in front of each string in the original list of strings returned by
Copy link
Member

@vitlibar vitlibar Dec 30, 2020

Choose a reason for hiding this comment

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

What if the attribute doesn't have a specified prefix in its value? It won't be used as a role name then? I'm not sure but maybe it's better to rename prefix into remove_prefix to emphasize the fact we remove the prefix (otherwise someone could possibly think that we're going to insert the prefix).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we learned from the feedback from people who routinely configure such things for enterprise, it's a common (and effectively the only) way of mapping the role names, so I think it's better to keep it prefix since the underlying functionality is quite familiar to everyone.

scope - scope of the LDAP search.
Accepted values are: 'base', 'one_level', 'children', 'subtree' (the default).
search_filter - template used to construct the search filter for the LDAP search.
The resulting filter will be constructed by replacing all '{username}', '{bind_dn}', and '{base_dn}'
Copy link
Member

@vitlibar vitlibar Dec 30, 2020

Choose a reason for hiding this comment

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

About the replacement of {bind_dn} and {base_dn} in search_filter, they look quite unnecessary and a bit misleading. Are you sure there is a real case when they're useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, some use cases may require search filter to contain those.

* master: (620 commits)
  Add test for some possible ambiguities in syntax
  Update PushingToViewsBlockOutputStream.h
  [For ClickHouse#18707] MySQL compatibility: support DIV and MOD operators
  Mark another flaky test
  Remove some headers
  Mark some TestFlows as flaky
  Fix error
  Fix errors
  One more test
  Arcadia does not support distributed queries
  Add a test for ClickHouse#14974
  Added a test from ClickHouse#15641
  More robust stateful test
  Update tests
  Remove bad code in HashJoin
  Update test
  Don't allow conversion between UUID and numeric types
  Remove pink screen with confusing questions about Kerberos
  Do not throw from Parser
  Fix the unexpected behaviour of show tables when antlr parser enabled (ClickHouse#18431)
  ...

# Conflicts:
#	programs/server/config.xml
#	src/Access/Authentication.cpp
#	src/Access/Authentication.h
Add caching/checking of search_params
Adjust comments/doc
Use special authentication logic from ExternalAuthenticators::checkLDAPCredentials
@traceon traceon requested a review from vitlibar January 8, 2021 19:59
traceon and others added 2 commits January 18, 2021 22:55
* master: (605 commits)
  DOCSUP-4710: Added support numeric parameters in number and string data types (ClickHouse#18696)
  DOCSUP-5604: Edit and translate to Russian (ClickHouse#18929)
  Update version_date.tsv after release 21.1.2.15
  Usability improvement of clickhouse-test
  Update jit_large_requests.xml
  Update README.md
  Update images.json
  Make symbolizers available in fuzzer Docker image
  Update Dragonbox
  Speed up aggregate function sum
  Fix MSan report in Kerberos library
  Fix MSan error in rocksdb ClickHouse#19213
  Add more Fuzzer tasks
  Fixes
  Update comment for curl dependency for aws
  Disable curl for mariadb-connector-c (it is not required)
  Fix TSan
  Skip test for ANTLR
  DistributedBlockOutputStream: add more comments
  DistributedBlockOutputStream: Remove superfluous brackets for string construction
  ...
Increasing clickhouse container health check timeouts.
@vitlibar
Copy link
Member

The problem with AST fuzzer has been already fixed (#19287)

@vitlibar vitlibar merged commit 813b2bc into ClickHouse:master Jan 25, 2021
@traceon traceon deleted the ldap-role-mapping branch January 25, 2021 13:09
@olgarev
Copy link
Contributor

olgarev commented Jul 19, 2021

@vitlibar Hi! It seems that this functionality was already documented here #20208. Am I right?

@traceon
Copy link
Contributor Author

traceon commented Jul 20, 2021

@olgarev Yes, that's correct. Since then, the doc has undergone some adjustments, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-feature Pull request with new product feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

system.user_directories Can not convert to std::string with ldap server.
6 participants