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 19: (5 comments) http://gerrit.cloudera.org:8080/#/c/23237/18/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/23237/18/be/src/util/webserver.cc@356 PS18, Line 356: FLAGS_is_coordinator && !FLAGS_webserver_saml2_sp_callback_u > The program invocation short name depends on the name of the file containin Yeah, makes sense, since only impalad can be coordinator anyway. http://gerrit.cloudera.org:8080/#/c/23237/18/be/src/util/webserver.cc@945 PS18, Line 945: const char* uri = request_info->uri; : const bool is_monitoring_endpoint = : strncmp(uri, kJsonMetricsPath, > Nit: please use constants instead of raw strings and numbers. Ack http://gerrit.cloudera.org:8080/#/c/23237/18/be/src/util/webserver.cc@952 PS18, Line 952: check_csrf_protection = false; > I like this overall approach much better. My preference is to require the I may be missing something, but the problem is that this flag also skips the real SAML verification in Java code. So if we require this flag for 'start-impala-cluster' to work, the script is not going to work on production without it. And I would assume the 'start-impala-cluster' script is used also on production. In other words, post-startup checks is not only a development-time precaution but is needed on production as well. Therefore this cannot be fixed with an impalad flag. What we need is a way for impalad's Webserver to respond differenlty to script's requests as opposed to all others. http://gerrit.cloudera.org:8080/#/c/23237/18/tests/custom_cluster/test_webserver_auth_combined.py File tests/custom_cluster/test_webserver_auth_combined.py: http://gerrit.cloudera.org:8080/#/c/23237/18/tests/custom_cluster/test_webserver_auth_combined.py@220 PS18, Line 220: @classmethod : def setup_class(cls): : # Create the dummy keytab file - SPNEGO startup requires the file to exist : # The file doesn't need to be a valid keytab; authentication will fail as expected : with open(cls.KEYTAB_FILE, 'wb') as f: : f.write(b'') # Empty file is sufficient : super(TestWebserverSpnegoSaml2, cls).setup_class() : : @classmethod : def teardown_class(cls): : super(TestWebserverSpnegoSaml2, cls).teardown_class() : # Clean up the dummy keytab file : if os.path.exists(cls.KEYTAB_FILE): : os.remove(cls.KEYTAB_FILE) : : def test_browser_fallthrough_to_saml(self): : """ : Test that a browser without Authorization header falls through from SPNEGO to SAML2. : : When a browser accesses the webserver without credentials: : - SPNEGO auth is skipped (no Authorization header = no explicit client choice) : - Request falls through to SAML2 : - Client gets 302 redirect to IdP : """ : self._assert_browser_fallthrough_to_saml( : "impala.webserver.total-negotiate-auth-fa > Nit: this code is very similar to the code in 'test_browser_fallthrough_to_ Ack http://gerrit.cloudera.org:8080/#/c/23237/18/tests/custom_cluster/test_webserver_auth_combined.py@256 PS18, Line 256: self._assert_explicit_auth_no_fallthrough( : "Negotiate", b"invalid_spnego_token", : "Not authorized", : "impala.webserver.total-negotiate-auth-failure") : : def test_complete_saml_flow_after_fallthrough(self): : """ : Test complete SAML2 workflow after SPNEGO fallthrough. : : This validates that after falling through from SPNEGO: : 1. Client gets redirect to IdP : 2. Client can complete SAML2 authentication : 3. Client receives authentication cookie : """ : # Step 1: Initial request without auth header -> redirect to IdP : relay_state, request_id = self._ws_request_resource(25000) : : # Step 2: Complete SAML2 authentication flow : initial_success = self.cluster.impalads[0].service.get_metric_value( : TestSamlBase.WS_SAML_SUCCESS_METRIC) : initial_success = 0 if initial_success is None else initial_success : : attributes_xml = TestSamlBase.ATTRIBUTE_STATEMENT.format(group_name="group1") : self._ws_send_authn_response( : "http://localhost:25000/SAML2/SSO/POST";, : request_id, relay_state, attributes_xml, expect_success=True) : : # Step 3: Verify SAML2 authentication succeeded : final_success = self.cluster.impalads[0].service.get_metric_value( : TestSamlBase.WS_SAML_SUCCESS_METRIC) : assert final_success == initial_success + 1, \ : "SAML success metric should increment after successful auth" : : > Nit: this code is very similar to the code in 'test_explicit_basic_auth_no_ Ack -- 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: 19 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: Sun, 08 Mar 2026 22:13:43 +0000 Gerrit-HasComments: Yes
