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 <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Tue, 17 Nov 2020 19:24:52 +0000 Gerrit-HasComments: Yes
