Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/23237 )
Change subject: IMPALA-14285: Add SAML2 authentication support for Impala Web UI ...................................................................... Patch Set 1: (4 comments) Looks great (and simpler than I expected)! Added a few comments, mainly high level ones. I think that it would be useful to unify more code with the hs2-http implementation to make the reviewers' work easier. http://gerrit.cloudera.org:8080/#/c/23237/1/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/23237/1/be/src/util/webserver.cc@930 PS1, Line 930: if (!authenticated && use_saml) IMO this block should be done at the very end, after LDAP and Kerberos authentication - currently if both saml and Kerberos are used, we'll use SAML even if Kerberos would work based on the auth header. This would also need the LDAP/Kerberos block to be modified, as it currently expects that authentication must be either LDAP or Kerberos. The hs2-http implementation is different in this regard as the initial request uses SAML based on the existence of header X-Hive-Token-Response-Port. http://gerrit.cloudera.org:8080/#/c/23237/1/be/src/util/webserver.cc@1163 PS1, Line 1163: std::string Webserver::getTimeRFC1123() { maybe reuse this somehow? https://github.com/apache/impala/blob/a7efa7665fc113ec13a53a77efcc2d9138d44330/be/src/transport/THttpServer.cpp#L547 http://gerrit.cloudera.org:8080/#/c/23237/1/tests/common/impala_service.py File tests/common/impala_service.py: http://gerrit.cloudera.org:8080/#/c/23237/1/tests/common/impala_service.py@82 PS1, Line 82: X-Init I would prefer to have a more descriptive name, e.g. X-Impala-EETest Another alternative would be to also setup JWT authentication for the SAML test and use a JWT token when we are just looking for a metric. An even more complex alternative would be to actually use SAML2 by detecting in the test that relevant flags are set - it would be enough to do one request that gets redirected and after that we could just reuse the cookie. The benefit of this that it could increase coverage of SAML code. http://gerrit.cloudera.org:8080/#/c/23237/1/tests/custom_cluster/test_saml2_sso_webserver.py File tests/custom_cluster/test_saml2_sso_webserver.py: http://gerrit.cloudera.org:8080/#/c/23237/1/tests/custom_cluster/test_saml2_sso_webserver.py@18 PS1, Line 18: I would highly prefer to merge this class with test_saml2_sso.py and share as many things as possible (e.g. AUTHN_RESPONSE_SCHEMA by adding a few placeholders). Besides long term maintainability it would make reviewing easier by highlighting what is new/what is different. It would also make it easier to test both having SAML2 in hs2-http and webui at the same time which is probably the most realistic scenario. -- To view, visit http://gerrit.cloudera.org:8080/23237 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I12540300529f9c240abf7196141ecb0ae6e37995 Gerrit-Change-Number: 23237 Gerrit-PatchSet: 1 Gerrit-Owner: Mihaly Szjatinya <msz...@pm.me> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Mon, 11 Aug 2025 08:43:30 +0000 Gerrit-HasComments: Yes
