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 <atti...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Thu, 28 Jan 2021 18:10:16 +0000 Gerrit-HasComments: Yes
