Gergely Farkas has posted comments on this change. ( http://gerrit.cloudera.org:8080/19561 )
Change subject: IMPALA-11726: Allow LDAP user and group filter when Kerberos is enabled ...................................................................... Patch Set 4: (9 comments) http://gerrit.cloudera.org:8080/#/c/19561/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19561/4//COMMIT_MSG@10 PS4, Line 10: impala-shell > I think that this should affect any Impala client that uses Kerberos (e.g. Good point! http://gerrit.cloudera.org:8080/#/c/19561/4//COMMIT_MSG@12 PS4, Line 12: allow_custom_ldap_filters_with_kerberos_auth > Do the flags make sense in any combination, e.g. allow_custom_ldap_filters_ I've added some notes on edge cases. http://gerrit.cloudera.org:8080/#/c/19561/4//COMMIT_MSG@14 PS4, Line 14: search filters when Kerberos authentication is enabled. When the flag > Can you mention proxy users? My understanding is that LDAP filters will be I updated the commit message with some related thoughts and created two new tests: The first test validates that default LDAP search bind filters are applied for delegate users, even if the proxy user authenticated with Kerberos. This test should run green without the changes in this gerrit. The other test validates that the custom LDAP search filters are applied for delegate users even if the enable_group_filter_check_for_authenticated_kerberos_user is disabled and the proxy user authenticated with Kerberos. http://gerrit.cloudera.org:8080/#/c/19561/4/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/19561/4/be/src/rpc/authentication.cc@290 PS4, Line 290: if (!server->IsAuthorizedProxyUser(user)) { : return ldap->LdapCheckFilters(user); : } : return true; > optional: IMO reversing the order and returning early if IsAuthorizedProxyU Done http://gerrit.cloudera.org:8080/#/c/19561/4/be/src/rpc/authentication.cc@302 PS4, Line 302: if (success) { : return DoLdapCheckFilters(user); : } : : return false; > optional: IMO reversing the order and returning early if !success would be Done http://gerrit.cloudera.org:8080/#/c/19561/4/be/src/rpc/authentication.cc@523 PS4, Line 523: SaslAuthorizeExternal > maming: could be SaslKerberosAuthorizeExternal? Done http://gerrit.cloudera.org:8080/#/c/19561/4/be/src/rpc/authentication.cc@528 PS4, Line 528: IsKerberosEnabled > We should only get here with Kerberos, so this could be a DCHECK Done http://gerrit.cloudera.org:8080/#/c/19561/4/be/src/rpc/authentication.cc@569 PS4, Line 569: SaslLdapAuthorizeExternal > It is not clear to me why we check the LDAP filters for Kerberos during SAS At first I wanted to use the SASL_CB_SERVER_USERDB_CHECKPASS callback, the code would have been much simpler in that way, but it seems that SASL checkpass is called only in PLAIN authentication mode, and in Kerberos authentication, where no password is present on the server side, it is not used. http://gerrit.cloudera.org:8080/#/c/19561/4/be/src/rpc/authentication.cc@745 PS4, Line 745: IsKerberosEnabled > This doesn't seem necessary as NegotiateAuth is only used with Kerberos and Nice catch! -- To view, visit http://gerrit.cloudera.org:8080/19561 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If3ca9c4ff8a17167e5233afabdd14c948edb46de Gerrit-Change-Number: 19561 Gerrit-PatchSet: 4 Gerrit-Owner: Gergely Farkas <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Gergely Farkas <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Thu, 02 Mar 2023 14:17:28 +0000 Gerrit-HasComments: Yes
