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 <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: Tue, 26 Jan 2021 21:27:19 +0000 Gerrit-HasComments: Yes
