You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tamas Mate (Code Review)" <ge...@cloudera.org> on 2020/11/13 09:45:55 UTC
[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 <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Nov 2020 14:32:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 16 Nov 2020 15:41:14 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 26 Jan 2021 08:14:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Feb 2021 08:21:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Nov 2020 08:32:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
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<ImpalaLdap>*)' 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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Sat, 06 Feb 2021 00:51:22 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 26 Jan 2021 08:35:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
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.
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.
Done
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?
Not anymore :)
It did not happen here actually.
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/webserver.cc@446
PS4, Line 446: FLAGS_enable_ldap_auth
> Is this needed? Line 434 already seems to check that we use LDAP. If auth_m
Right, this check is not needed.
I could not find where the Webserver::Start() is called, but I think if it returns with a status message impala aborts.
Added ValidateFlags() for this object as well.
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)
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: 4
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Jan 2021 18:10:16 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Feb 2021 09:37:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Nov 2020 19:48:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Nov 2020 10:09:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Feb 2021 23:21:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 Jan 2021 08:56:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Feb 2021 20:22:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Jan 2021 21:54:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 02 Feb 2021 21:55:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Jan 2021 22:15:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Feb 2021 11:48:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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(), &ld);
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 <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Nov 2020 09:46:46 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Nov 2020 19:24:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 02 Feb 2021 22:18:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10161: User LDAP Search bind support
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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?
http://gerrit.cloudera.org:8080/#/c/16717/4/be/src/util/webserver.cc@446
PS4, Line 446: FLAGS_enable_ldap_auth
Is this needed? Line 434 already seems to check that we use LDAP. If auth_mode_ == AuthMode::LDAP but !FLAGS_enable_ldap_auth can be true, then this may result in a crash, as ldap_ will be uninitialized.
--
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 <tm...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 26 Jan 2021 21:27:19 +0000
Gerrit-HasComments: Yes