Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15570 )
Change subject: IMPALA-2563: Support LDAP search bind operations ...................................................................... Patch Set 3: (9 comments) http://gerrit.cloudera.org:8080/#/c/15570/3/be/src/rpc/authentication.h File be/src/rpc/authentication.h: http://gerrit.cloudera.org:8080/#/c/15570/3/be/src/rpc/authentication.h@30 PS3, Line 30: #include "util/ldap-util.h" nit: we probably only need a forward declaration, so long as the constructor and destructor are declared in the .cc file. http://gerrit.cloudera.org:8080/#/c/15570/3/be/src/util/ldap-util.h File be/src/util/ldap-util.h: http://gerrit.cloudera.org:8080/#/c/15570/3/be/src/util/ldap-util.h@57 PS3, Line 57: set unordered_set? I didn't see a reason why ordering matters. http://gerrit.cloudera.org:8080/#/c/15570/3/be/src/util/ldap-util.cc File be/src/util/ldap-util.cc: http://gerrit.cloudera.org:8080/#/c/15570/3/be/src/util/ldap-util.cc@121 PS3, Line 121: user_filter_ = Split(user_filter, ","); I guess this wouldn't allow us to specify a user or group with a comma in it, since no escaping, but it seems like the hive syntax is the same: https://github.com/apache/hive/blob/037eacea46371015a7f9894c5a9ccfb9708d5c56/common/src/java/org/apache/hive/common/util/HiveStringUtils.java http://gerrit.cloudera.org:8080/#/c/15570/3/be/src/util/ldap-util.cc@133 PS3, Line 133: // DN patterns and replacing any '%s' with each group name. It looks like we're replacing only the first %s, is that intended? http://gerrit.cloudera.org:8080/#/c/15570/3/be/src/util/ldap-util.cc@241 PS3, Line 241: int rc = ldap_search_ext_s(ld, group_dn.c_str(), LDAP_SCOPE_SUBTREE, filter.c_str(), Can you add a brief comment about what this invocation is doing? http://gerrit.cloudera.org:8080/#/c/15570/3/be/src/util/ldap-util.cc@264 PS3, Line 264: LOG(WARNING) << "Was not able to get DN from search result."; Is there any more context that would be useful to debug this issue? E.g. the group_dn? http://gerrit.cloudera.org:8080/#/c/15570/3/be/src/util/ldap-util.cc@268 PS3, Line 268: LOG(WARNING) << "Following of references is not currently supported, ignoring."; Same comment http://gerrit.cloudera.org:8080/#/c/15570/3/be/src/util/ldap-util.cc@281 PS3, Line 281: vector<string> attributes = Split(rdn, delimiter::Limit(",", 1)); Do we need to do any unescaping here? It looks like the DN syntax uses \ for escaping - https://docs.microsoft.com/en-us/previous-versions/windows/desktop/ldap/distinguished-names FWIW it looks like other pre-existing string manipulation in this file doesn't do escaping, so it may be a general limitation, but maybe if there's an ldap library function to do the parsing that would be more robust http://gerrit.cloudera.org:8080/#/c/15570/3/fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java File fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java: http://gerrit.cloudera.org:8080/#/c/15570/3/fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java@205 PS3, Line 205: // Run with user that passes the group filter but not the user filter, should fail. Also check if it fails both filters? -- To view, visit http://gerrit.cloudera.org:8080/15570 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7502a96e9a3c16faa67c03ffac54df2bdebbca8c Gerrit-Change-Number: 15570 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Sat, 28 Mar 2020 01:11:12 +0000 Gerrit-HasComments: Yes
