Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/15570 )
Change subject: IMPALA-2563: Support LDAP search bind operations ...................................................................... Patch Set 4: (8 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 "common/status.h" > nit: we probably only need a forward declaration, so long as the constructo Done 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: uno > unordered_set? I didn't see a reason why ordering matters. Done 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 Its already the case that we can't handle usernames with commas (or any other character that needs to be escaped) since we don't escape usernames before passing them to LDAP. I think that's probably something worth fixing, and I can do it if you want, but its somewhat outside the scope of this patch and I would prefer to leave it as follow up work. http://gerrit.cloudera.org:8080/#/c/15570/3/be/src/util/ldap-util.cc@133 PS3, Line 133: // DN patterns and replacing the optional '%s' with each group name, if present. > It looks like we're replacing only the first %s, is that intended? Yes. I was intending to follow Hive's example as closely as possible, and the documentation of this for Hive seems to say that they'll only replace a single '%s'. This comment is just wrong. >From looking through their code, I think that Hive doesn't even actually do >this but instead just cuts off the first attribute of any pattern containing a >'%s', which in theory should work assuming that the '%s' is in fact the first >attribute since you would just be starting the search at a higher point in the >tree. (see >https://github.com/apache/hive/blob/037eacea46371015a7f9894c5a9ccfb9708d5c56/service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java#L175) I think the best choice here though is to go ahead and do what Hive's docs claim they do, i.e. replace a single '%s', and I'll update this comment to reflect that. http://gerrit.cloudera.org:8080/#/c/15570/3/be/src/util/ldap-util.cc@241 PS3, Line 241: > Can you add a brief comment about what this invocation is doing? Done http://gerrit.cloudera.org:8080/#/c/15570/3/be/src/util/ldap-util.cc@264 PS3, Line 264: if (group_filter_.count(short_name) == 1) { > Is there any more context that would be useful to debug this issue? E.g. th Done http://gerrit.cloudera.org:8080/#/c/15570/3/be/src/util/ldap-util.cc@268 PS3, Line 268: } > Same comment Done http://gerrit.cloudera.org:8080/#/c/15570/3/be/src/util/ldap-util.cc@281 PS3, Line 281: LOG(WARNING) << "Was unable to parse LDAP search reference result: " > Do we need to do any unescaping here? It looks like the DN syntax uses \ fo Addressed above -- 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: 4 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: Tue, 31 Mar 2020 17:40:14 +0000 Gerrit-HasComments: Yes
