Mihaly Szjatinya has posted comments on this change. ( http://gerrit.cloudera.org:8080/23237 )
Change subject: IMPALA-14285: Add SAML2 authentication support for Coordinator Web UI ...................................................................... Patch Set 18: (3 comments) Sorry for late response. Fixed the leftovers. http://gerrit.cloudera.org:8080/#/c/23237/15/be/src/rpc/authentication-test.cc File be/src/rpc/authentication-test.cc: http://gerrit.cloudera.org:8080/#/c/23237/15/be/src/rpc/authentication-test.cc@516 PS15, Line 516: ASSERT_FALSE(status.ok()); > This is a very good list of test cases. I think it would be good to add a This was left out. The test parser was allowing this, good catch! Fixed the function and added the testcase. http://gerrit.cloudera.org:8080/#/c/23237/13/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/23237/13/be/src/util/webserver.cc@373 PS13, Line 373: || use_saml_ > I was thinking the test would be for authenticating to the debug web ui usi Added test_webserver_auth_combined.py with testing of LDAP+SAML and SPNEGO+SAML scenarios. These test the intelligent fallthrough scenarios, which do not require successful LDAP/SPNEGO authentication, and thus no real LDAP/SPNEGO infrastructure (which we currently have only in java fe tests). For that I had to first fix the intelligent fallthrough itself for SPNEGO/LDAP -> SAML, which was not actually in place till now, despite the Csaba's explanation at https://gerrit.cloudera.org/c/23237/1/be/src/util/webserver.cc#930, my bad. Now, having these combinations tested, it obviously raises the question of testing all other possible combinations, for both Webserver and HS2. I'm sure a lot of subtle bugs would come up in the process. http://gerrit.cloudera.org:8080/#/c/23237/13/be/src/util/webserver.cc@851 PS13, Line 851: if (!authenticated && auth_mode_ == AuthMode::NONE && !use_saml_) { > The open_debug_webpage function (https://gerrit.cloudera.org/c/23237/16/tes This would theoretically work with test_saml2_sso.py::TestWebserverSaml, but not with test_webserver_auth_combined, since there we need LDAP/SPNEGO, which is mutually exclusive with basic auth. And even for the former, it would somehow make the test less precise. However, the same idea can be implemented with pre-generated cookie, which is preemptive to any auth method. The cookie would be deleted after bootstrap is finished. This would require some work with adding a flag, etc. For now I restricted the current approach with the X-Impala-EETest, limiting it only to localhost and to the few endpoints that are really needed. Besides, with adding test_webserver_auth_combined I had to expand this mechanism for LDAP/SPNEGO auth as well. Refactored the code around it. Let me know what you think. -- 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: 18 Gerrit-Owner: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Nandor Kollar <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Wed, 04 Mar 2026 13:41:54 +0000 Gerrit-HasComments: Yes
