Jason Fehr 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: (8 comments) 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 was left out. Done 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_ > Added test_webserver_auth_combined.py with testing of LDAP+SAML and SPNEGO+ The tests in `test_webserver_auth_combined.py` look great. Thanks for adding them! 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_) { > This would theoretically work with test_saml2_sso.py::TestWebserverSaml, bu I left some comments on the code implementing this new approach. 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: strcmp(google::ProgramInvocationShortName(), "impalad") == 0 The program invocation short name depends on the name of the file containing the Impala binary. We should not rely on this file to always be named "impalad". Relying on FLAGS_is_coordinator should be sufficient. http://gerrit.cloudera.org:8080/#/c/23237/18/be/src/util/webserver.cc@945 PS18, Line 945: strncmp(uri, "/jsonmetrics", 12) == 0 || : strncmp(uri, "/backends", 9) == 0 || : strncmp(uri, "/varz", 5) == 0; Nit: please use constants instead of raw strings and numbers. http://gerrit.cloudera.org:8080/#/c/23237/18/be/src/util/webserver.cc@952 PS18, Line 952: } I like this overall approach much better. My preference is to require the `saml2_ee_test_mode` flag to be `true` for enabling support of the "X-Impala-EETest" header. It's a simple check that shuts down an attack vector in production systems. 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: # Send GET request WITHOUT Authorization header (simulates browser) : opener = build_opener(NoRedirection) : req = Request("http://localhost:25000/";) : response = opener.open(req) : : # Should get 302 redirect to SAML2 IdP, NOT 401 : assert response.getcode() == 302, \ : "Browser without auth header should redirect to SAML2, not return 401" : : location = response.info()["location"] : assert TestSamlBase.IDP_URL in location, \ : "Should redirect to SAML2 IdP, got: {}".format(location) : : # Parse the SAML request to ensure it's valid : relay_state, saml_req_xml = self._parse_redirection_response_common(response) : request_id = self._parse_authn_request(saml_req_xml) : assert request_id is not None, "Should get valid SAML AuthnRequest" : : # Verify SPNEGO was NOT attempted (no Authorization header = no attempt) : impalad = self.cluster.impalads[0] : spnego_failures = impalad.service.get_metric_value( : "impala.webserver.total-negotiate-auth-failure") : # May be 0 or None depending on when metric was initialized : assert (spnego_failures is None or spnego_failures == 0), \ : "SPNEGO should not be attempted when no Authorization header present, " \ : "got {} failures".format(spnego_failures) Nit: this code is very similar to the code in 'test_browser_fallthrough_to_saml'. Please look into refactoring this code into a generic function that can be called by both tests. http://gerrit.cloudera.org:8080/#/c/23237/18/tests/custom_cluster/test_webserver_auth_combined.py@256 PS18, Line 256: # Send GET request WITH Authorization: Negotiate header (explicit choice) : opener = build_opener(NoRedirection) : req = Request("http://localhost:25000/";) : : # Add Authorization: Negotiate header with invalid token : # Using a dummy base64 token that will fail SPNEGO validation : invalid_token = base64.b64encode(b"invalid_spnego_token").decode('ascii') : req.add_header("Authorization", "Negotiate {}".format(invalid_token)) : : # Expect 401, not 302 redirect : try: : response = opener.open(req) : status_code = response.getcode() : response_body = response.read().decode('utf-8') : except HTTPError as e: : status_code = e.code : response_body = e.read().decode('utf-8') : : # Should get 401, NOT 302 redirect to SAML2 : assert status_code == 401, \ : "Explicit Negotiate auth failure should return 401, got {}".format(status_code) : : # Response should NOT be a redirect to SAML2 : # SPNEGO returns "Not authorized: Invalid token was supplied" for invalid tokens : assert "Not authorized" in response_body, \ : "Should get SPNEGO auth failure message, not SAML2 redirect" : : # Verify SPNEGO failure metric incremented : impalad = self.cluster.impalads[0] : spnego_failures = impalad.service.get_metric_value( : "impala.webserver.total-negotiate-auth-failure") : assert spnego_failures >= 1, \ : "SPNEGO failure metric should increment for Negotiate auth, got {}".format( : spnego_failures) Nit: this code is very similar to the code in 'test_explicit_basic_auth_no_fallthrough'. Please look into refactoring this code into a generic function that can be called by both tests. -- 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: Thu, 05 Mar 2026 19:16:48 +0000 Gerrit-HasComments: Yes
