[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Impala Public Jenkins has submitted this change and it was merged. (
http://gerrit.cloudera.org:8080/17047 )
Change subject: IMPALA-10161: User LDAP Search bind support
..
IMPALA-10161: User LDAP Search bind support
This change adds user search bind support next to simple bind that can
be configured with LDAP filters. The group check was done with LDAP
search earlier, this change adds the possibility to configure it with
Hadoop library like options, which is the LDAP filter with optional
patterns. The '{0}' will be replaced with the user name while the
'{1}' pattern will be replaced with the user dn.
The following new flags have been added:
--ldap_search_bind_authentication: a flag to change between simple and
search bind
--ldap_user_search_basedn: the base dn for the LDAP subtree to search
--ldap_group_search_basedn: the base dn for the LDAP subtree to search
Tested:
- Custom cluster tests have been added
Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
Reviewed-on: http://gerrit.cloudera.org:8080/17047
Reviewed-by: Impala Public Jenkins
Tested-by: Impala Public Jenkins
---
M be/src/rpc/authentication.cc
M be/src/util/CMakeLists.txt
A be/src/util/ldap-search-bind.cc
A be/src/util/ldap-search-bind.h
A be/src/util/ldap-simple-bind.cc
A be/src/util/ldap-simple-bind.h
M be/src/util/ldap-util.cc
M be/src/util/ldap-util.h
M be/src/util/webserver.cc
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
A
fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java
A
fe/src/test/java/org/apache/impala/customcluster/LdapSimpleBindImpalaShellTest.java
M fe/src/test/java/org/apache/impala/testutil/LdapUtil.java
M fe/src/test/resources/users.ldif
14 files changed, 905 insertions(+), 336 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/17047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
Gerrit-Change-Number: 17047
Gerrit-PatchSet: 6
Gerrit-Owner: Tamas Mate
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tamas Mate
Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17047 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 5: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/17047 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 17047 Gerrit-PatchSet: 5 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Thu, 18 Feb 2021 23:12:46 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17047 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/8159/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/17047 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 17047 Gerrit-PatchSet: 4 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Thu, 18 Feb 2021 17:47:56 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17047 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/17047 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 17047 Gerrit-PatchSet: 5 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Thu, 18 Feb 2021 17:31:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17047 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 5: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6898/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/17047 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 17047 Gerrit-PatchSet: 5 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Thu, 18 Feb 2021 17:31:22 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/17047 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/17047 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 17047 Gerrit-PatchSet: 4 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Thu, 18 Feb 2021 17:30:04 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/17047 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 3: (1 comment) Thank you for the reviews Csaba, Thomas. Updated the change. http://gerrit.cloudera.org:8080/#/c/17047/3/be/src/util/ldap-search-bind.cc File be/src/util/ldap-search-bind.cc: http://gerrit.cloudera.org:8080/#/c/17047/3/be/src/util/ldap-search-bind.cc@112 PS3, Line 112: VLOG(2) << "LDAP bind successful" > We write successful even if success is false. Done -- To view, visit http://gerrit.cloudera.org:8080/17047 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 17047 Gerrit-PatchSet: 3 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Thu, 18 Feb 2021 17:29:17 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Tamas Mate has uploaded a new patch set (#4). (
http://gerrit.cloudera.org:8080/17047 )
Change subject: IMPALA-10161: User LDAP Search bind support
..
IMPALA-10161: User LDAP Search bind support
This change adds user search bind support next to simple bind that can
be configured with LDAP filters. The group check was done with LDAP
search earlier, this change adds the possibility to configure it with
Hadoop library like options, which is the LDAP filter with optional
patterns. The '{0}' will be replaced with the user name while the
'{1}' pattern will be replaced with the user dn.
The following new flags have been added:
--ldap_search_bind_authentication: a flag to change between simple and
search bind
--ldap_user_search_basedn: the base dn for the LDAP subtree to search
--ldap_group_search_basedn: the base dn for the LDAP subtree to search
Tested:
- Custom cluster tests have been added
Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
---
M be/src/rpc/authentication.cc
M be/src/util/CMakeLists.txt
A be/src/util/ldap-search-bind.cc
A be/src/util/ldap-search-bind.h
A be/src/util/ldap-simple-bind.cc
A be/src/util/ldap-simple-bind.h
M be/src/util/ldap-util.cc
M be/src/util/ldap-util.h
M be/src/util/webserver.cc
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
A
fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java
A
fe/src/test/java/org/apache/impala/customcluster/LdapSimpleBindImpalaShellTest.java
M fe/src/test/java/org/apache/impala/testutil/LdapUtil.java
M fe/src/test/resources/users.ldif
14 files changed, 905 insertions(+), 336 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/17047/4
--
To view, visit http://gerrit.cloudera.org:8080/17047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
Gerrit-Change-Number: 17047
Gerrit-PatchSet: 4
Gerrit-Owner: Tamas Mate
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tamas Mate
Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/17047 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 3: Code-Review+1 (1 comment) I only found a small issue, can upgrade to +2 once it is resolved. http://gerrit.cloudera.org:8080/#/c/17047/3/be/src/util/ldap-search-bind.cc File be/src/util/ldap-search-bind.cc: http://gerrit.cloudera.org:8080/#/c/17047/3/be/src/util/ldap-search-bind.cc@112 PS3, Line 112: VLOG(2) << "LDAP bind successful" We write successful even if success is false. -- To view, visit http://gerrit.cloudera.org:8080/17047 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 17047 Gerrit-PatchSet: 3 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Thu, 18 Feb 2021 17:09:31 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17047 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/8156/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/17047 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 17047 Gerrit-PatchSet: 3 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Thu, 18 Feb 2021 09:52:42 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/17047 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 3: Patch set 3 resolves the merge conflict. Also, added a small refactor, removed the use_ldap variable and changed it to the FLAGS_enable_ldap_auth in AuthManager::Init(). -- To view, visit http://gerrit.cloudera.org:8080/17047 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 17047 Gerrit-PatchSet: 3 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Thu, 18 Feb 2021 09:36:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Tamas Mate has uploaded a new patch set (#3). (
http://gerrit.cloudera.org:8080/17047 )
Change subject: IMPALA-10161: User LDAP Search bind support
..
IMPALA-10161: User LDAP Search bind support
This change adds user search bind support next to simple bind that can
be configured with LDAP filters. The group check was done with LDAP
search earlier, this change adds the possibility to configure it with
Hadoop library like options, which is the LDAP filter with optional
patterns. The '{0}' will be replaced with the user name while the
'{1}' pattern will be replaced with the user dn.
The following new flags have been added:
--ldap_search_bind_authentication: a flag to change between simple and
search bind
--ldap_user_search_basedn: the base dn for the LDAP subtree to search
--ldap_group_search_basedn: the base dn for the LDAP subtree to search
Tested:
- Custom cluster tests have been added
Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
---
M be/src/rpc/authentication.cc
M be/src/util/CMakeLists.txt
A be/src/util/ldap-search-bind.cc
A be/src/util/ldap-search-bind.h
A be/src/util/ldap-simple-bind.cc
A be/src/util/ldap-simple-bind.h
M be/src/util/ldap-util.cc
M be/src/util/ldap-util.h
M be/src/util/webserver.cc
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
A
fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java
A
fe/src/test/java/org/apache/impala/customcluster/LdapSimpleBindImpalaShellTest.java
M fe/src/test/java/org/apache/impala/testutil/LdapUtil.java
M fe/src/test/resources/users.ldif
14 files changed, 903 insertions(+), 336 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/17047/3
--
To view, visit http://gerrit.cloudera.org:8080/17047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
Gerrit-Change-Number: 17047
Gerrit-PatchSet: 3
Gerrit-Owner: Tamas Mate
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tamas Mate
Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/17047 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 2: Code-Review+1 Leaving at a +1 for now in case Csaba wants to take another look. If not, I'm happy to give it a +2 -- To view, visit http://gerrit.cloudera.org:8080/17047 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 17047 Gerrit-PatchSet: 2 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Wed, 17 Feb 2021 16:58:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17047 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/8119/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/17047 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 17047 Gerrit-PatchSet: 2 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Wed, 10 Feb 2021 16:37:59 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Tamas Mate has uploaded a new patch set (#2). (
http://gerrit.cloudera.org:8080/17047 )
Change subject: IMPALA-10161: User LDAP Search bind support
..
IMPALA-10161: User LDAP Search bind support
This change adds user search bind support next to simple bind that can
be configured with LDAP filters. The group check was done with LDAP
search earlier, this change adds the possibility to configure it with
Hadoop library like options, which is the LDAP filter with optional
patterns. The '{0}' will be replaced with the user name while the
'{1}' pattern will be replaced with the user dn.
The following new flags have been added:
--ldap_search_bind_authentication: a flag to change between simple and
search bind
--ldap_user_search_basedn: the base dn for the LDAP subtree to search
--ldap_group_search_basedn: the base dn for the LDAP subtree to search
Tested:
- Custom cluster tests have been added
Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
---
M be/src/rpc/authentication.cc
M be/src/util/CMakeLists.txt
A be/src/util/ldap-search-bind.cc
A be/src/util/ldap-search-bind.h
A be/src/util/ldap-simple-bind.cc
A be/src/util/ldap-simple-bind.h
M be/src/util/ldap-util.cc
M be/src/util/ldap-util.h
M be/src/util/webserver.cc
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
A
fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java
A
fe/src/test/java/org/apache/impala/customcluster/LdapSimpleBindImpalaShellTest.java
M fe/src/test/java/org/apache/impala/testutil/LdapUtil.java
M fe/src/test/resources/users.ldif
14 files changed, 901 insertions(+), 334 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/17047/2
--
To view, visit http://gerrit.cloudera.org:8080/17047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
Gerrit-Change-Number: 17047
Gerrit-PatchSet: 2
Gerrit-Owner: Tamas Mate
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tamas Mate
Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/17047 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 1: (3 comments) A few remaining nits, but otherwise it looks good to me. I'll give Csaba an opportunity to take another look if he wants before +2ing it http://gerrit.cloudera.org:8080/#/c/17047/1/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/17047/1/be/src/util/webserver.cc@128 PS1, Line 128: "Used as filter for both simple and " : "search nit: formatting (i.e. have the string start on its own line like it is here, but wrap it such that its as long as possible on that line), here and elsewhere unfortunately this is one of the things that clang-format gets wrong (since it won't automatically combine strings for you) http://gerrit.cloudera.org:8080/#/c/17047/1/fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java File fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java: http://gerrit.cloudera.org:8080/#/c/17047/1/fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java@104 PS1, Line 104: n typo http://gerrit.cloudera.org:8080/#/c/17047/1/fe/src/test/java/org/apache/impala/customcluster/LdapSimpleBindImpalaShellTest.java File fe/src/test/java/org/apache/impala/customcluster/LdapSimpleBindImpalaShellTest.java: http://gerrit.cloudera.org:8080/#/c/17047/1/fe/src/test/java/org/apache/impala/customcluster/LdapSimpleBindImpalaShellTest.java@42 PS1, Line 42: @Test I think you missed some instances of @Override here and below, though it might be more straightforward to just avoid overriding things by naming the functions in the base class something like test...Impl() or similar. Not a big deal, though. -- To view, visit http://gerrit.cloudera.org:8080/17047 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 17047 Gerrit-PatchSet: 1 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Tue, 09 Feb 2021 22:40:54 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17047 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/8103/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/17047 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 17047 Gerrit-PatchSet: 1 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Tue, 09 Feb 2021 12:59:32 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/17047 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 1: Hi Csaba, Thomas, thank you for the reviews, Apologies, I had to re-submit the change under a new change id. Compared to the previous review, this change contains: 1) A factory method that creates the LDAP instance based on the configuration 2) Updated the incorrect flag name in the commit message 3) Cleaned the headers in the webserver.h 4) Ran clang from cli, looks like my IDE was acting up -- To view, visit http://gerrit.cloudera.org:8080/17047 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 17047 Gerrit-PatchSet: 1 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Tue, 09 Feb 2021 12:39:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Tamas Mate has uploaded this change for review. (
http://gerrit.cloudera.org:8080/17047
Change subject: IMPALA-10161: User LDAP Search bind support
..
IMPALA-10161: User LDAP Search bind support
This change adds user search bind support next to simple bind that can
be configured with LDAP filters. The group check was done with LDAP
search earlier, this change adds the possibility to configure it with
Hadoop library like options, which is the LDAP filter with optional
patterns. The '{0}' will be replaced with the user name while the
'{1}' pattern will be replaced with the user dn.
The following new flags have been added:
--ldap_search_bind_authentication: a flag to change between simple and
search bind
--ldap_user_search_basedn: the base dn for the LDAP subtree to search
--ldap_group_search_basedn: the base dn for the LDAP subtree to search
Tested:
- Custom cluster tests have been added
Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
---
M be/src/rpc/authentication.cc
M be/src/util/CMakeLists.txt
A be/src/util/ldap-search-bind.cc
A be/src/util/ldap-search-bind.h
A be/src/util/ldap-simple-bind.cc
A be/src/util/ldap-simple-bind.h
M be/src/util/ldap-util.cc
M be/src/util/ldap-util.h
M be/src/util/webserver.cc
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
A
fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java
A
fe/src/test/java/org/apache/impala/customcluster/LdapSimpleBindImpalaShellTest.java
M fe/src/test/java/org/apache/impala/testutil/LdapUtil.java
M fe/src/test/resources/users.ldif
14 files changed, 904 insertions(+), 330 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/17047/1
--
To view, visit http://gerrit.cloudera.org:8080/17047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
Gerrit-Change-Number: 17047
Gerrit-PatchSet: 1
Gerrit-Owner: Tamas Mate
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Tamas Mate has abandoned this change. ( http://gerrit.cloudera.org:8080/16717 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Abandoned Abandoning this change for now. -- To view, visit http://gerrit.cloudera.org:8080/16717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 16717 Gerrit-PatchSet: 12 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16717 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 12: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/8100/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/16717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 16717 Gerrit-PatchSet: 12 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Tue, 09 Feb 2021 09:37:20 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Hello Thomas Tauber-Marshall, Attila Jeges, Csaba Ringhofer, Impala Public
Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16717
to look at the new patch set (#12).
Change subject: IMPALA-10161: User LDAP Search bind support
..
IMPALA-10161: User LDAP Search bind support
This change adds user search bind support next to simple bind that can
be configured with LDAP filters. The group check was done with LDAP
search earlier, this change adds the possibility to configure it with
Hadoop library like options, which is the LDAP filter with optional
patterns. The '{0}' will be replaced with the user name while the
'{1}' pattern will be replaced with the user dn.
The following new flags have been added:
--ldap_search_bind_authentication: a flag to change between simple and
search bind
--ldap_user_search_basedn: the base dn for the LDAP subtree to search
--ldap_group_search_basedn: the base dn for the LDAP subtree to search
Tested:
- Custom cluster tests have been added
Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
---
M be/src/rpc/authentication.cc
M be/src/util/CMakeLists.txt
A be/src/util/ldap-search-bind.cc
A be/src/util/ldap-search-bind.h
A be/src/util/ldap-simple-bind.cc
A be/src/util/ldap-simple-bind.h
M be/src/util/ldap-util.cc
M be/src/util/ldap-util.h
M be/src/util/webserver.cc
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
A
fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java
A
fe/src/test/java/org/apache/impala/customcluster/LdapSimpleBindImpalaShellTest.java
M fe/src/test/java/org/apache/impala/testutil/LdapUtil.java
M fe/src/test/resources/users.ldif
14 files changed, 904 insertions(+), 330 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/16717/12
--
To view, visit http://gerrit.cloudera.org:8080/16717
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
Gerrit-Change-Number: 16717
Gerrit-PatchSet: 12
Gerrit-Owner: Tamas Mate
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tamas Mate
Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16717 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 11: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/8099/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/16717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 16717 Gerrit-PatchSet: 11 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Tue, 09 Feb 2021 08:21:52 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Tamas Mate has uploaded a new patch set (#11). (
http://gerrit.cloudera.org:8080/16717 )
Change subject: IMPALA-10161: User LDAP Search bind support
..
IMPALA-10161: User LDAP Search bind support
This change adds user search bind support next to simple bind that can
be configured with LDAP filters. The group check was done with LDAP
search earlier, this change adds the possibility to configure it with
Hadoop library like options, which is the LDAP filter with optional
patterns. The '{0}' will be replaced with the user name while the
'{1}' pattern will be replaced with the user dn.
The following new flags have been added:
--ldap_search_bind_authentication: a flag to change between simple and
search bind
--ldap_user_search_basedn: the base dn for the LDAP subtree to search
--ldap_group_search_basedn: the base dn for the LDAP subtree to search
Tested:
- Custom cluster tests have been added
Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
---
M be/src/rpc/authentication.cc
M be/src/util/CMakeLists.txt
A be/src/util/ldap-search-bind.cc
A be/src/util/ldap-search-bind.h
A be/src/util/ldap-simple-bind.cc
A be/src/util/ldap-simple-bind.h
M be/src/util/ldap-util.cc
M be/src/util/ldap-util.h
M be/src/util/webserver.cc
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
A
fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java
A
fe/src/test/java/org/apache/impala/customcluster/LdapSimpleBindImpalaShellTest.java
M fe/src/test/java/org/apache/impala/testutil/LdapUtil.java
M fe/src/test/resources/users.ldif
A ldap_flags
15 files changed, 912 insertions(+), 330 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/16717/11
--
To view, visit http://gerrit.cloudera.org:8080/16717
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
Gerrit-Change-Number: 16717
Gerrit-PatchSet: 11
Gerrit-Owner: Tamas Mate
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tamas Mate
Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Tamas Mate has posted comments on this change. (
http://gerrit.cloudera.org:8080/16717 )
Change subject: IMPALA-10161: User LDAP Search bind support
..
Patch Set 10:
(4 comments)
Hi Thomas, thank you for the review, update the patch.
http://gerrit.cloudera.org:8080/#/c/16717/10//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16717/10//COMMIT_MSG@17
PS10, Line 17: ldap_search_bind
> ldap_search_bind_authentication?
Done
http://gerrit.cloudera.org:8080/#/c/16717/10/be/src/util/ldap-search-bind.cc
File be/src/util/ldap-search-bind.cc:
http://gerrit.cloudera.org:8080/#/c/16717/10/be/src/util/ldap-search-bind.cc@76
PS10, Line 76: << DEFAULT_USER_FILTER;
> There's a couple of formatting issues in the patch. It might be helpful if
Looks like my IDE is acting up. Ran it from CLI.
http://gerrit.cloudera.org:8080/#/c/16717/10/be/src/util/webserver.h
File be/src/util/webserver.h:
http://gerrit.cloudera.org:8080/#/c/16717/10/be/src/util/webserver.h@29
PS10, Line 29: #include "util/ldap-search-bind.h"
> I might be missing something, but not sure why this should go here, as oppo
Done, removed it because not needed with ImpalaLdap::CreateLdap.
http://gerrit.cloudera.org:8080/#/c/16717/10/be/src/util/webserver.cc
File be/src/util/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/16717/10/be/src/util/webserver.cc@451
PS10, Line 451: if (!FLAGS_ldap_search_bind_authentication) {
: ldap_.reset(new LdapSimpleBind());
: } else {
: ldap_.reset(new LdapSearchBind());
: }
> Might be cleaner to encapsulate this in something like a 'static void Impal
Done
--
To view, visit http://gerrit.cloudera.org:8080/16717
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
Gerrit-Change-Number: 16717
Gerrit-PatchSet: 10
Gerrit-Owner: Tamas Mate
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tamas Mate
Gerrit-Reviewer: Thomas Tauber-Marshall
Gerrit-Comment-Date: Mon, 08 Feb 2021 20:22:12 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Thomas Tauber-Marshall has posted comments on this change. (
http://gerrit.cloudera.org:8080/16717 )
Change subject: IMPALA-10161: User LDAP Search bind support
..
Patch Set 10:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16717/10//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16717/10//COMMIT_MSG@17
PS10, Line 17: ldap_search_bind
ldap_search_bind_authentication?
http://gerrit.cloudera.org:8080/#/c/16717/10/be/src/util/ldap-search-bind.cc
File be/src/util/ldap-search-bind.cc:
http://gerrit.cloudera.org:8080/#/c/16717/10/be/src/util/ldap-search-bind.cc@76
PS10, Line 76: << DEFAULT_USER_FILTER;
There's a couple of formatting issues in the patch. It might be helpful if you
ran clang-format-diff on it, per the instructions in
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=65868536
http://gerrit.cloudera.org:8080/#/c/16717/10/be/src/util/webserver.h
File be/src/util/webserver.h:
http://gerrit.cloudera.org:8080/#/c/16717/10/be/src/util/webserver.h@29
PS10, Line 29: #include "util/ldap-search-bind.h"
I might be missing something, but not sure why this should go here, as opposed
to in the .cc file.
http://gerrit.cloudera.org:8080/#/c/16717/10/be/src/util/webserver.cc
File be/src/util/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/16717/10/be/src/util/webserver.cc@451
PS10, Line 451: if (!FLAGS_ldap_search_bind_authentication) {
: ldap_.reset(new LdapSimpleBind());
: } else {
: ldap_.reset(new LdapSearchBind());
: }
Might be cleaner to encapsulate this in something like a 'static void
ImpalaLdap::CreateLdap(unique_ptr*)' or similar, and then use it in
authentication.cc as well
--
To view, visit http://gerrit.cloudera.org:8080/16717
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
Gerrit-Change-Number: 16717
Gerrit-PatchSet: 10
Gerrit-Owner: Tamas Mate
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tamas Mate
Gerrit-Reviewer: Thomas Tauber-Marshall
Gerrit-Comment-Date: Sat, 06 Feb 2021 00:51:22 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16717 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 10: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/8067/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/16717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 16717 Gerrit-PatchSet: 10 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Tue, 02 Feb 2021 22:18:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Tamas Mate has uploaded a new patch set (#10). (
http://gerrit.cloudera.org:8080/16717 )
Change subject: IMPALA-10161: User LDAP Search bind support
..
IMPALA-10161: User LDAP Search bind support
This change adds user search bind support next to simple bind that can
be configured with LDAP filters. The group check was done with LDAP
search earlier, this change adds the possibility to configure it with
Hadoop library like options, which is the LDAP filter with optional
patterns. The '{0}' will be replaced with the user name while the
'{1}' pattern will be replaced with the user dn.
The following new flags have been added:
--ldap_search_bind: a flag to change between simple and search bind
--ldap_user_search_basedn: the base dn for the LDAP subtree to search
--ldap_group_search_basedn: the base dn for the LDAP subtree to search
Tested:
- Custom cluster tests have been added
Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
---
M be/src/rpc/authentication.cc
M be/src/util/CMakeLists.txt
A be/src/util/ldap-search-bind.cc
A be/src/util/ldap-search-bind.h
A be/src/util/ldap-simple-bind.cc
A be/src/util/ldap-simple-bind.h
M be/src/util/ldap-util.cc
M be/src/util/ldap-util.h
M be/src/util/webserver.cc
M be/src/util/webserver.h
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
A
fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java
A
fe/src/test/java/org/apache/impala/customcluster/LdapSimpleBindImpalaShellTest.java
M fe/src/test/java/org/apache/impala/testutil/LdapUtil.java
M fe/src/test/resources/users.ldif
15 files changed, 900 insertions(+), 332 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/16717/10
--
To view, visit http://gerrit.cloudera.org:8080/16717
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
Gerrit-Change-Number: 16717
Gerrit-PatchSet: 10
Gerrit-Owner: Tamas Mate
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tamas Mate
Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/16717 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 7: (10 comments) Hi Csaba, thank you for the review! Updated the change. http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-search-bind.cc File be/src/util/ldap-search-bind.cc: http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-search-bind.cc@46 PS7, Line 46: std::string > Here and at other places: do you intentionally don't use "using namespace s I added it everywhere just in case. Revisited the includes/using directives and cleaned it up. The cpp files now use either the 'strings' namespace or the 'std::string'. 'strings' namespace uses the 'std::string'. http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-search-bind.cc@53 PS7, Line 53: Status ldapBaseValidateStatus = ImpalaLdap::ValidateFlags(); : if (!ldapBaseValidateStatus.ok()) return ldapBaseValidateStatus; > We generally use the RETURN_IF_ERROR macro for this pattern. Done http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-search-bind.cc@71 PS7, Line 71: Status ldapBaseInitStatus = ImpalaLdap::Init(user_filter, group_filter); : if (!ldapBaseInitStatus.ok()) return ldapBaseInitStatus; > RETURN_IF_ERROR Done http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-search-bind.cc@100 PS7, Line 100: std::string > not needed, user_filter_ is already a string Done http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-search-bind.cc@137 PS7, Line 137: group_filter_ > I think it would be a bit clearer to call find on group_filter instead of g Yes, agree, done. http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-search-bind.cc@142 PS7, Line 142: if (user_dn.empty()) return false; > ldap_unbind_ext is not called if we return here Done http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-simple-bind.cc File be/src/util/ldap-simple-bind.cc: http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-simple-bind.cc@56 PS7, Line 56: Status ldapBaseValidateStatus = ImpalaLdap::ValidateFlags(); : if (!ldapBaseValidateStatus.ok()) return ldapBaseValidateStatus; > RETURN_IF_ERROR Done http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-simple-bind.cc@80 PS7, Line 80: Status ldapBaseInitStatus = ImpalaLdap::Init(user_filter, group_filter); : if (!ldapBaseInitStatus.ok()) return ldapBaseInitStatus; > RETURN_IF_ERROR Done http://gerrit.cloudera.org:8080/#/c/16717/7/fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java File fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java: http://gerrit.cloudera.org:8080/#/c/16717/7/fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java@46 PS7, Line 46: LdapSearchBindImpalaShellTest > I think that a lot of duplication could be potentially avoided with LdapSim Refactored the tests, moved the common methods to a base class. http://gerrit.cloudera.org:8080/#/c/16717/7/fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java@291 PS7, Line 291: testLdapSearchBind > Can you make the name more descriptive? The whole file seems to be about te Done -- To view, visit http://gerrit.cloudera.org:8080/16717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 16717 Gerrit-PatchSet: 7 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Tue, 02 Feb 2021 21:55:11 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/16717 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 7: (10 comments) http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-search-bind.cc File be/src/util/ldap-search-bind.cc: http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-search-bind.cc@46 PS7, Line 46: std::string Here and at other places: do you intentionally don't use "using namespace std" or "using std::string"? Is there some kind of ambiguity in that case? http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-search-bind.cc@53 PS7, Line 53: Status ldapBaseValidateStatus = ImpalaLdap::ValidateFlags(); : if (!ldapBaseValidateStatus.ok()) return ldapBaseValidateStatus; We generally use the RETURN_IF_ERROR macro for this pattern. http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-search-bind.cc@71 PS7, Line 71: Status ldapBaseInitStatus = ImpalaLdap::Init(user_filter, group_filter); : if (!ldapBaseInitStatus.ok()) return ldapBaseInitStatus; RETURN_IF_ERROR http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-search-bind.cc@100 PS7, Line 100: std::string not needed, user_filter_ is already a string http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-search-bind.cc@137 PS7, Line 137: group_filter_ I think it would be a bit clearer to call find on group_filter instead of group_filter_. The result should be the same. http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-search-bind.cc@142 PS7, Line 142: if (user_dn.empty()) return false; ldap_unbind_ext is not called if we return here http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-simple-bind.cc File be/src/util/ldap-simple-bind.cc: http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-simple-bind.cc@56 PS7, Line 56: Status ldapBaseValidateStatus = ImpalaLdap::ValidateFlags(); : if (!ldapBaseValidateStatus.ok()) return ldapBaseValidateStatus; RETURN_IF_ERROR http://gerrit.cloudera.org:8080/#/c/16717/7/be/src/util/ldap-simple-bind.cc@80 PS7, Line 80: Status ldapBaseInitStatus = ImpalaLdap::Init(user_filter, group_filter); : if (!ldapBaseInitStatus.ok()) return ldapBaseInitStatus; RETURN_IF_ERROR http://gerrit.cloudera.org:8080/#/c/16717/7/fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java File fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java: http://gerrit.cloudera.org:8080/#/c/16717/7/fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java@46 PS7, Line 46: LdapSearchBindImpalaShellTest I think that a lot of duplication could be potentially avoided with LdapSimpleBindImpalaShellTest, e.g. by creating a common base class. If you agree, even if you don't want to deal with this in the current review a followup jira could be created. http://gerrit.cloudera.org:8080/#/c/16717/7/fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java@291 PS7, Line 291: testLdapSearchBind Can you make the name more descriptive? The whole file seems to be about testLdapSearchBind -- To view, visit http://gerrit.cloudera.org:8080/16717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 16717 Gerrit-PatchSet: 7 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Mon, 01 Feb 2021 23:21:55 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16717 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 7: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/8056/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/16717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 16717 Gerrit-PatchSet: 7 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Mon, 01 Feb 2021 11:48:58 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Tamas Mate has uploaded a new patch set (#7). (
http://gerrit.cloudera.org:8080/16717 )
Change subject: IMPALA-10161: User LDAP Search bind support
..
IMPALA-10161: User LDAP Search bind support
This change adds user search bind support next to simple bind that can
be configured with LDAP filters. The group check was done with LDAP
search earlier, this change adds the possibility to configure it with
Hadoop library like options, which is the LDAP filter with optional
patterns. The '{0}' will be replaced with the user name while the
'{1}' pattern will be replaced with the user dn.
The following new flags have been added:
--ldap_search_bind: a flag to change between simple and search bind
--ldap_user_search_basedn: the base dn for the LDAP subtree to search
--ldap_group_search_basedn: the base dn for the LDAP subtree to search
Tested:
- Custom cluster tests have been added
Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
---
M be/src/rpc/authentication.cc
M be/src/util/CMakeLists.txt
A be/src/util/ldap-search-bind.cc
A be/src/util/ldap-search-bind.h
A be/src/util/ldap-simple-bind.cc
A be/src/util/ldap-simple-bind.h
M be/src/util/ldap-util.cc
M be/src/util/ldap-util.h
M be/src/util/webserver.cc
M be/src/util/webserver.h
C
fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java
R
fe/src/test/java/org/apache/impala/customcluster/LdapSimpleBindImpalaShellTest.java
M fe/src/test/java/org/apache/impala/testutil/LdapUtil.java
M fe/src/test/resources/users.ldif
14 files changed, 702 insertions(+), 298 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/16717/7
--
To view, visit http://gerrit.cloudera.org:8080/16717
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
Gerrit-Change-Number: 16717
Gerrit-PatchSet: 7
Gerrit-Owner: Tamas Mate
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tamas Mate
Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16717 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 6: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/8049/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/16717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 16717 Gerrit-PatchSet: 6 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Fri, 29 Jan 2021 08:56:11 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Tamas Mate has uploaded a new patch set (#6). (
http://gerrit.cloudera.org:8080/16717 )
Change subject: IMPALA-10161: User LDAP Search bind support
..
IMPALA-10161: User LDAP Search bind support
This change adds user search bind support next to simple bind that can
be configured with LDAP filters. The group check was done with LDAP
search earlier, this change adds the possibility to configure it with
Hadoop library like options, which is the LDAP filter with optional
patterns. The '{0}' will be replaced with the user name while the
'{1}' pattern will be replaced with the user dn.
The following new flags have been added:
--ldap_search_bind: a flag to change between simple and search bind
--ldap_user_search_basedn: the base dn for the LDAP subtree to search
--ldap_group_search_basedn: the base dn for the LDAP subtree to search
Tested:
- Custom cluster tests have been added
Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
---
M be/src/rpc/authentication.cc
M be/src/util/CMakeLists.txt
A be/src/util/ldap-search-bind.cc
A be/src/util/ldap-search-bind.h
A be/src/util/ldap-simple-bind.cc
A be/src/util/ldap-simple-bind.h
M be/src/util/ldap-util.cc
M be/src/util/ldap-util.h
M be/src/util/webserver.cc
M be/src/util/webserver.h
C
fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java
R
fe/src/test/java/org/apache/impala/customcluster/LdapSimpleBindImpalaShellTest.java
M fe/src/test/java/org/apache/impala/testutil/LdapUtil.java
M fe/src/test/resources/users.ldif
14 files changed, 701 insertions(+), 299 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/16717/6
--
To view, visit http://gerrit.cloudera.org:8080/16717
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
Gerrit-Change-Number: 16717
Gerrit-PatchSet: 6
Gerrit-Owner: Tamas Mate
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tamas Mate
Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16717 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 5: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/8047/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/16717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 16717 Gerrit-PatchSet: 5 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Thu, 28 Jan 2021 22:15:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16717 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/16717/5/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/16717/5/be/src/util/webserver.cc@136 PS5, Line 136: "groups for authentication to succeed. For search bind it is an LDAP filter that will " line too long (91 > 90) -- To view, visit http://gerrit.cloudera.org:8080/16717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 16717 Gerrit-PatchSet: 5 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Thu, 28 Jan 2021 21:54:42 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Tamas Mate has uploaded a new patch set (#5). (
http://gerrit.cloudera.org:8080/16717 )
Change subject: IMPALA-10161: User LDAP Search bind support
..
IMPALA-10161: User LDAP Search bind support
This change adds user search bind support next to simple bind that can
be configured with LDAP filters. The group check was done with LDAP
search earlier, this change adds the possibility to configure it with
Hadoop library like options, which is the LDAP filter with optional
patterns. The '{0}' will be replaced with the user name while the
'{1}' pattern will be replaced with the user dn.
The following new flags have been added:
--ldap_search_bind: a flag to change between simple and search bind
--ldap_user_search_basedn: the base dn for the LDAP subtree to search
--ldap_group_search_basedn: the base dn for the LDAP subtree to search
Tested:
- Custom cluster tests have been added
Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
---
M be/src/rpc/authentication.cc
M be/src/util/CMakeLists.txt
A be/src/util/ldap-search-bind.cc
A be/src/util/ldap-search-bind.h
A be/src/util/ldap-simple-bind.cc
A be/src/util/ldap-simple-bind.h
M be/src/util/ldap-util.cc
M be/src/util/ldap-util.h
M be/src/util/webserver.cc
M be/src/util/webserver.h
C
fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java
R
fe/src/test/java/org/apache/impala/customcluster/LdapSimpleBindImpalaShellTest.java
M fe/src/test/java/org/apache/impala/testutil/LdapUtil.java
M fe/src/test/resources/users.ldif
14 files changed, 701 insertions(+), 299 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/16717/5
--
To view, visit http://gerrit.cloudera.org:8080/16717
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
Gerrit-Change-Number: 16717
Gerrit-PatchSet: 5
Gerrit-Owner: Tamas Mate
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tamas Mate
Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Tamas Mate has posted comments on this change. (
http://gerrit.cloudera.org:8080/16717 )
Change subject: IMPALA-10161: User LDAP Search bind support
..
Patch Set 4:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/rpc/authentication.cc@1120
PS4, Line 1120: // Tell Thrift not to initialize SSL for us, as we use Kudu's
SSL initializtion.
: TSSLSocketFactory::setManualOpenSSLInitialization(true);
: kudu::security::InitializeOpenSSL();
> It would have been nicer to separate the changes that come from rebasing.
Rebased, should be cleaner now.
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-search-bind.cc
File be/src/util/ldap-search-bind.cc:
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-search-bind.cc@27
PS4, Line 27: #include "ldap-search-bind.h"
> The header with the same is usually the one included first.
Done
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-search-bind.cc@49
PS4, Line 49: "(&(objectClass=user)(sAMAccountName={0}))";
> I don't get how this format and FLAGS_ldap_user_filter matches:
I tried to reuse existing flags, updated the description.
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-search-bind.cc@104
PS4, Line 104: VLOG_QUERY
> I don't think that "query" level logging is meaningful here, I would prefer
Moved to VLOG(1) here and in ldap-simple-bind.cc:114 as well.
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-search-bind.cc@135
PS4, Line 135: std::string
> this is not need, as group_filter_ is a string
Done
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-search-bind.cc@138
PS4, Line 138: std::string
> this is not need, as user_filter_ is a string
Done
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-search-bind.cc@139
PS4, Line 139: replace_all(user_filter, USER_NAME_PATTERN, username);
> Does this have to be inside the if block?
Not necessarily, but the user_filter is only needed in this block to retrieve
the user dn.
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-search-bind.cc@140
PS4, Line 140: std::string user_dn = LdapSearchObject(ld,
FLAGS_ldap_user_search_basedn.c_str(),
: user_filter.c_str());
: replace_all(group_filter, USER_DN_PATTERN, user_dn);
> What happens if LdapSearchObject was not successful?
I did not expect the user_dn to be empty at this stage normally, the filters
are checked after the 'LdapCheckPass' succeeds. However, proxy users are not
authenticated with 'LdapCheckPass', I added an extra guard condition here.
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-simple-bind.cc
File be/src/util/ldap-simple-bind.cc:
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-simple-bind.cc@28
PS4, Line 28: #include "ldap-simple-bind.h"
> The header with the same is usually the one included first. (This is useful
Done
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-simple-bind.cc@119
PS4, Line 119: VLOG(2) << "LDAP bind successful";
> This was the same in the old code, but I would prefer to log something diff
Moved this line into the successful block and revisited the logging of the
ImpalaLdap.Bind(...) to make sure that whenever it would return false a warning
is printed.
The only missing 'return false;' was the anonymous bind part in the beginning
ldap-util.cc:122, added a warning message there as well.
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-simple-bind.cc@139
PS4, Line 139: << "successful but user is not in the
authorized user list.";
> ldap_unbind_ext is missing here
Done
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-simple-bind.cc@180
PS4, Line 180: value[1]
> This comes from the old code, but I prefer to handle the case when the spli
Noticed that this method is only used in CheckGroupMembership and quite short,
earlier it was used in the middle of the switch of 'LdapSearchObject', with the
'LdapSearchObject' refactored I think we have room for the implementation of
this method. Let me know if it would look better outside.
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-util.cc
File be/src/util/ldap-util.cc:
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-util.cc@204
PS4, Line 204: result_dn = std::string(entry_dn);
> Is it legal for the for loop to actually have more than 1 elements? if ther
In general it is legal for an LDAP search to return more than one entry, for
authentication purposes it would be ambiguous. There is a check in line 188 to
make sure that one entry is returned.
However, this loop iterates over the returned messages, which could contain
references as well.
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Csaba Ringhofer has posted comments on this change. (
http://gerrit.cloudera.org:8080/16717 )
Change subject: IMPALA-10161: User LDAP Search bind support
..
Patch Set 4:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/rpc/authentication.cc@1120
PS4, Line 1120: // Tell Thrift not to initialize SSL for us, as we use Kudu's
SSL initializtion.
: TSSLSocketFactory::setManualOpenSSLInitialization(true);
: kudu::security::InitializeOpenSSL();
It would have been nicer to separate the changes that come from rebasing.
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-search-bind.h
File be/src/util/ldap-search-bind.h:
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-search-bind.h@56
PS4, Line 56: replace
nit: replaced
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-search-bind.cc
File be/src/util/ldap-search-bind.cc:
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-search-bind.cc@27
PS4, Line 27: #include "ldap-search-bind.h"
The header with the same is usually the one included first.
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-search-bind.cc@49
PS4, Line 49: "(&(objectClass=user)(sAMAccountName={0}))";
I don't get how this format and FLAGS_ldap_user_filter matches:
DEFINE_string(ldap_user_filter, "", "Comma separated list of usernames. If
specified, "
"users must be on this list for athentication to succeed."
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-search-bind.cc@104
PS4, Line 104: VLOG_QUERY
I don't think that "query" level logging is meaningful here, I would prefer to
use the numbers.
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-search-bind.cc@135
PS4, Line 135: std::string
this is not need, as group_filter_ is a string
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-search-bind.cc@138
PS4, Line 138: std::string
this is not need, as user_filter_ is a string
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-search-bind.cc@139
PS4, Line 139: replace_all(user_filter, USER_NAME_PATTERN, username);
Does this have to be inside the if block?
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-search-bind.cc@140
PS4, Line 140: std::string user_dn = LdapSearchObject(ld,
FLAGS_ldap_user_search_basedn.c_str(),
: user_filter.c_str());
: replace_all(group_filter, USER_DN_PATTERN, user_dn);
What happens if LdapSearchObject was not successful?
USER_DN_PATTERN will be replaced with an empty string - is this correct?
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-simple-bind.cc
File be/src/util/ldap-simple-bind.cc:
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-simple-bind.cc@28
PS4, Line 28: #include "ldap-simple-bind.h"
The header with the same is usually the one included first. (This is useful to
ensure that the header can compile with its own includes and doesn't rely on
headers included before it in .cc files.)
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-simple-bind.cc@119
PS4, Line 119: VLOG(2) << "LDAP bind successful";
This was the same in the old code, but I would prefer to log something
different when success == false
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-simple-bind.cc@139
PS4, Line 139: << "successful but user is not in the
authorized user list.";
ldap_unbind_ext is missing here
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-simple-bind.cc@180
PS4, Line 180: value[1]
This comes from the old code, but I prefer to handle the case when the split
was not successful, e.g. by returning an empty case if value.size() < 2, and
check for emptiness of the result at the call sites.
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-util.cc
File be/src/util/ldap-util.cc:
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/ldap-util.cc@204
PS4, Line 204: result_dn = std::string(entry_dn);
Is it legal for the for loop to actually have more than 1 elements? if there
can be more than one LDAP_RES_SEARCH_ENTRY, then the last one will overwrite
the previous results.
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/webserver.h
File be/src/util/webserver.h:
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/webserver.h@29
PS4, Line 29: #include "util/ldap-util.h"
: #include "util/ldap-simple-bind.h"
: #include "util/ldap-search-bind.h"
We try to keep headers alphabetically sorted.
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/webserver.cc
File be/src/util/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/webserver.cc@444
PS4, Line 444: LOG(INFO) << "Crash-";
Do we still crash here?
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16717 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 4: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/8031/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/16717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 16717 Gerrit-PatchSet: 4 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Tue, 26 Jan 2021 08:35:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16717 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/webserver.cc@453 PS4, Line 453: ldap_->Init(FLAGS_webserver_ldap_user_filter, FLAGS_webserver_ldap_group_filter)); line too long (92 > 90) -- To view, visit http://gerrit.cloudera.org:8080/16717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 16717 Gerrit-PatchSet: 4 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Tue, 26 Jan 2021 08:14:41 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Tamas Mate has uploaded a new patch set (#4). (
http://gerrit.cloudera.org:8080/16717 )
Change subject: IMPALA-10161: User LDAP Search bind support
..
IMPALA-10161: User LDAP Search bind support
This change adds user search bind support next to simple bind that can
be configured with LDAP filters. The group check was done with LDAP
search earlier, this change adds the possibility to configure it with
Hadoop library like options, which is the LDAP filter with optional
patterns. The '{0}' will be replaced with the user name while the
'{1}' pattern will be replaced with the user dn.
The following new flags have been added:
--ldap_search_bind: a flag to change between simple and search bind
--ldap_user_search_basedn: the base dn for the LDAP subtree to search
--ldap_group_search_basedn: the base dn for the LDAP subtree to search
Tested:
- Custom cluster tests have been added
Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
---
M be/src/rpc/authentication.cc
M be/src/util/CMakeLists.txt
A be/src/util/ldap-search-bind.cc
A be/src/util/ldap-search-bind.h
A be/src/util/ldap-simple-bind.cc
A be/src/util/ldap-simple-bind.h
M be/src/util/ldap-util.cc
M be/src/util/ldap-util.h
M be/src/util/webserver.cc
M be/src/util/webserver.h
C
fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java
R
fe/src/test/java/org/apache/impala/customcluster/LdapSimpleBindImpalaShellTest.java
M fe/src/test/java/org/apache/impala/testutil/LdapUtil.java
M fe/src/test/resources/users.ldif
14 files changed, 683 insertions(+), 289 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/16717/4
--
To view, visit http://gerrit.cloudera.org:8080/16717
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
Gerrit-Change-Number: 16717
Gerrit-PatchSet: 4
Gerrit-Owner: Tamas Mate
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tamas Mate
Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Csaba Ringhofer has posted comments on this change. (
http://gerrit.cloudera.org:8080/16717 )
Change subject: IMPALA-10161: User LDAP Search bind support
..
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16717/2/be/src/util/ldap-util.cc
File be/src/util/ldap-util.cc:
http://gerrit.cloudera.org:8080/#/c/16717/2/be/src/util/ldap-util.cc@318
PS2, Line 318: result
> Done
You still don't call ldap_msgfree(result); on all paths - it has to be done
even if rc != LDAP_SUCCESS or if nrOfEntries != 1
http://gerrit.cloudera.org:8080/#/c/16717/2/be/src/util/ldap-util.cc@339
PS2, Line 339:
> This should be legal, it is mentioned in the ldap_unbind_ext_s doc:
oops, somehow I didn't see that you ldap_unbind_ext it in the previous line. No
to 'ld's are necessary.
http://gerrit.cloudera.org:8080/#/c/16717/3/be/src/util/ldap-util.cc
File be/src/util/ldap-util.cc:
http://gerrit.cloudera.org:8080/#/c/16717/3/be/src/util/ldap-util.cc@323
PS3, Line 323: if (nrOfEntries != 1) {
: LOG(WARNING) << "LDAP Search returned " << nrOfEntries <<
" entries, authentication"
:<< "failed due to incorrect number of
results.";
: return false;
: }
Can you add a comment why do we require nrOfEntries to be 1?
Btw accepting only 1 means that the logic below could be simpler - no for loop
is necessary, as ldap_first_message() should be the only message, and any other
message than LDAP_RES_SEARCH_ENTRY should return false, as there can't be
another LDAP_RES_SEARCH_ENTRY to Bind to.
http://gerrit.cloudera.org:8080/#/c/16717/3/be/src/util/ldap-util.cc@379
PS3, Line 379: return success;
This can be incorrectly true in many paths, as 'success' starts as true after
line 304, and only line 338 can set it false.
--
To view, visit http://gerrit.cloudera.org:8080/16717
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7
Gerrit-Change-Number: 16717
Gerrit-PatchSet: 3
Gerrit-Owner: Tamas Mate
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tamas Mate
Gerrit-Reviewer: Thomas Tauber-Marshall
Gerrit-Comment-Date: Wed, 18 Nov 2020 08:32:25 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16717 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/7665/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/16717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 16717 Gerrit-PatchSet: 3 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Tue, 17 Nov 2020 19:48:33 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Tamas Mate has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/16717 ) Change subject: IMPALA-10161: User LDAP Search bind support .. IMPALA-10161: User LDAP Search bind support This change adds Search Bind support next to simple bind, this lets administrators to configure Impala to allow users to authenticate from multiple organizational units. As part of this change 3 new flags have been added: --ldap_search_bind: a flag to swap between simple and search bind --ldap_user_search_baseDN: the base DN for the LDAP Subtree to search --ldap_user_search_filter: LDAP filter to narrow down the search result Tested: - Manually tested the authentication with on OpenLDAP - Added unit test Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 --- M be/src/util/ldap-util.cc M be/src/util/ldap-util.h M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java M fe/src/test/java/org/apache/impala/testutil/LdapUtil.java M fe/src/test/resources/users.ldif 5 files changed, 161 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/16717/3 -- To view, visit http://gerrit.cloudera.org:8080/16717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 16717 Gerrit-PatchSet: 3 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/16717 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 2: (8 comments) Thank you Csaba for the review. While I was fixing noticed that the LdapCheckFilters uses the ConstructUserDN function to resolve the userDN, which can cause trouble if group filters are enabled, I will need some time to rework this part. http://gerrit.cloudera.org:8080/#/c/16717/2/be/src/util/ldap-util.cc File be/src/util/ldap-util.cc: http://gerrit.cloudera.org:8080/#/c/16717/2/be/src/util/ldap-util.cc@208 PS2, Line 208: VLOG_QUERY << "Trying simple LDAP bind for: " << user_dn; > simple ldap always logs here, but SearchBind doesn't. It would be good to a Done http://gerrit.cloudera.org:8080/#/c/16717/2/be/src/util/ldap-util.cc@299 PS2, Line 299: > nit: extra line Done http://gerrit.cloudera.org:8080/#/c/16717/2/be/src/util/ldap-util.cc@303 PS2, Line 303: ld > We should always unbind this if Bind was successful, similarly to LdapCheck I planned to do the LDAP Search with the admin user, in case the user does not have search privilege on the configured search base directory. http://gerrit.cloudera.org:8080/#/c/16717/2/be/src/util/ldap-util.cc@307 PS2, Line 307: replace > typo: replaced Done http://gerrit.cloudera.org:8080/#/c/16717/2/be/src/util/ldap-util.cc@318 PS2, Line 318: result > According to https://linux.die.net/man/3/ldap_search_ext_s "Note that res p Done http://gerrit.cloudera.org:8080/#/c/16717/2/be/src/util/ldap-util.cc@320 PS2, Line 320: ldap_count_entries > I am not sure whether calling this is legal if rc != LDAP_SUCCESS. I would Done, added an additional free after the for cycle. http://gerrit.cloudera.org:8080/#/c/16717/2/be/src/util/ldap-util.cc@339 PS2, Line 339: ld > Are you sure that it is legal to reuse ld? I didn't see it mentioned at lda This should be legal, it is mentioned in the ldap_unbind_ext_s doc: https://linux.die.net/man/3/ldap_unbind_ext_s Shall I create two lds instead to make this part more readable? http://gerrit.cloudera.org:8080/#/c/16717/2/be/src/util/ldap-util.cc@340 PS2, Line 340: ldap_unbind_ext(ld, nullptr, nullptr); > unbind should be only needed if success is true Done -- To view, visit http://gerrit.cloudera.org:8080/16717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 16717 Gerrit-PatchSet: 2 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Tue, 17 Nov 2020 19:24:52 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/16717 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/16717/2/be/src/util/ldap-util.cc File be/src/util/ldap-util.cc: http://gerrit.cloudera.org:8080/#/c/16717/2/be/src/util/ldap-util.cc@208 PS2, Line 208: VLOG_QUERY << "Trying simple LDAP bind for: " << user_dn; simple ldap always logs here, but SearchBind doesn't. It would be good to add some logging there too, even if we return at line 303 http://gerrit.cloudera.org:8080/#/c/16717/2/be/src/util/ldap-util.cc@299 PS2, Line 299: nit: extra line http://gerrit.cloudera.org:8080/#/c/16717/2/be/src/util/ldap-util.cc@303 PS2, Line 303: ld We should always unbind this if Bind was successful, similarly to LdapCheckFilters. http://gerrit.cloudera.org:8080/#/c/16717/2/be/src/util/ldap-util.cc@307 PS2, Line 307: replace typo: replaced http://gerrit.cloudera.org:8080/#/c/16717/2/be/src/util/ldap-util.cc@318 PS2, Line 318: result According to https://linux.die.net/man/3/ldap_search_ext_s "Note that res parameter of ldap_search_ext_s() and ldap_search_s() should be freed with ldap_msgfree() regardless of return value of these functions." This means that we could ldap_msgfree(result); before all return statements, not just line 342 http://gerrit.cloudera.org:8080/#/c/16717/2/be/src/util/ldap-util.cc@320 PS2, Line 320: ldap_count_entries I am not sure whether calling this is legal if rc != LDAP_SUCCESS. I would check rc earlier in a block and return if it is not success. http://gerrit.cloudera.org:8080/#/c/16717/2/be/src/util/ldap-util.cc@339 PS2, Line 339: ld Are you sure that it is legal to reuse ld? I didn't see it mentioned at ldap_initialize() how does it handle the old value of ld, so I assume that it allocates a new LDAP structure, leaking the old one. http://gerrit.cloudera.org:8080/#/c/16717/2/be/src/util/ldap-util.cc@340 PS2, Line 340: ldap_unbind_ext(ld, nullptr, nullptr); unbind should be only needed if success is true -- To view, visit http://gerrit.cloudera.org:8080/16717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 16717 Gerrit-PatchSet: 2 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Mon, 16 Nov 2020 15:41:14 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16717 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/7646/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/16717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 16717 Gerrit-PatchSet: 2 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Fri, 13 Nov 2020 14:32:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Tamas Mate has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/16717 ) Change subject: IMPALA-10161: User LDAP Search bind support .. IMPALA-10161: User LDAP Search bind support This change adds Search Bind support next to simple bind, this lets administrators to configure Impala to allow users to authenticate from multiple organizational units. As part of this change 3 new flags have been added: --ldap_search_bind: a flag to swap between simple and search bind --ldap_user_search_baseDN: the base DN for the LDAP Subtree to search --ldap_user_search_filter: LDAP filter to narrow down the search result Tested: - Manually tested the authentication with on OpenLDAP - Added unit test Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 --- M be/src/util/ldap-util.cc M be/src/util/ldap-util.h M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java M fe/src/test/java/org/apache/impala/testutil/LdapUtil.java M fe/src/test/resources/users.ldif 5 files changed, 160 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/16717/2 -- To view, visit http://gerrit.cloudera.org:8080/16717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 16717 Gerrit-PatchSet: 2 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16717 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/7645/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/16717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 16717 Gerrit-PatchSet: 1 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Fri, 13 Nov 2020 10:09:53 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16717 ) Change subject: IMPALA-10161: User LDAP Search bind support .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/16717/1/be/src/util/ldap-util.cc File be/src/util/ldap-util.cc: http://gerrit.cloudera.org:8080/#/c/16717/1/be/src/util/ldap-util.cc@302 PS1, Line 302: bool success = Bind(FLAGS_ldap_bind_dn, bind_password_.c_str(), bind_password_.size(), ); line too long (94 > 90) http://gerrit.cloudera.org:8080/#/c/16717/1/be/src/util/ldap-util.cc@315 PS1, Line 315: int rc = ldap_search_ext_s(ld, FLAGS_ldap_user_search_baseDN.c_str(), LDAP_SCOPE_SUBTREE, line too long (91 > 90) http://gerrit.cloudera.org:8080/#/c/16717/1/be/src/util/ldap-util.cc@348 PS1, Line 348: LOG(WARNING) << "LDAP search error for " << userFilter << " with DN=" << FLAGS_ldap_baseDN.c_str() line too long (108 > 90) -- To view, visit http://gerrit.cloudera.org:8080/16717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 16717 Gerrit-PatchSet: 1 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Fri, 13 Nov 2020 09:46:46 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Tamas Mate has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16717 Change subject: IMPALA-10161: User LDAP Search bind support .. IMPALA-10161: User LDAP Search bind support This change adds Search Bind support next to simple bind, this lets administrators to configure Impala to allow users to authenticate from multiple organizational units. As part of this change 3 new flags have been added: --ldap_search_bind: a flag to swap between simple and search bind --ldap_user_search_baseDN: the base DN for the LDAP Subtree to search --ldap_user_search_filter: LDAP filter to narrow down the search result Tested: - Manually tested the authentication with on OpenLDAP - Added unit test Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 --- M be/src/util/ldap-util.cc M be/src/util/ldap-util.h M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java M fe/src/test/java/org/apache/impala/testutil/LdapUtil.java M fe/src/test/resources/users.ldif 5 files changed, 158 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/16717/1 -- To view, visit http://gerrit.cloudera.org:8080/16717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I978744ad05d9ef408328d1e4dd2d18c329f4d3b7 Gerrit-Change-Number: 16717 Gerrit-PatchSet: 1 Gerrit-Owner: Tamas Mate
