Csaba Ringhofer 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) haven't looked at the tests yet, look great in general! 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. odbc, jdbc, Impyla), not just impala-shell. 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_with_kerberos_auth == false && enable_group_filter_check_for_authenticated_kerberos_user == true? 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 applied for delegated users, even if the the connection comes from 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 IsAuthorizedProxyUser() returns true would be cleaner 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 cleaner. http://gerrit.cloudera.org:8080/#/c/19561/4/be/src/rpc/authentication.cc@523 PS4, Line 523: SaslAuthorizeExternal maming: could be SaslKerberosAuthorizeExternal? 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 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 SASL_CB_PROXY_POLICY while in LDAP at SASL_CB_SERVER_USERDB_CHECKPASS. 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 other parts of the code assume that we are using Kerberos. -- 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 <gfar...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Wed, 01 Mar 2023 17:35:30 +0000 Gerrit-HasComments: Yes
