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 15: (25 comments) http://gerrit.cloudera.org:8080/#/c/23237/13//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23237/13//COMMIT_MSG@31 PS13, Line 31: > Ack Done 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 url such as "http:/localhost:25000/SAML2/SSO" http://gerrit.cloudera.org:8080/#/c/23237/13/be/src/rpc/authentication-util.cc File be/src/rpc/authentication-util.cc: http://gerrit.cloudera.org:8080/#/c/23237/13/be/src/rpc/authentication-util.cc@263 PS13, Line 263: vector<string> split = Split(url, delimiter::Limit("/", 3)); > Added. Done Very comprehensive test suite! http://gerrit.cloudera.org:8080/#/c/23237/13/be/src/rpc/authentication-util.cc@277 PS13, Line 277: bool ParseParams(std::map<string, string*>& params_to_check, const string& original, > Added. Done Nice list of test cases! http://gerrit.cloudera.org:8080/#/c/23237/13/be/src/rpc/authentication-util.cc@280 PS13, Line 280: vector<string> key_value = Split(pair, delimiter::Limit("=", 1)); > It means the parameter doesn't have the standard key=value format. The code Done. Thanks for the explanation. http://gerrit.cloudera.org:8080/#/c/23237/13/be/src/rpc/authentication-util.cc@326 PS13, Line 326: > Thanks Csaba. I can only add that without load balancer multiple coordinato Oh, yes, those two statements are rather ambiguous. My intention was to test the server rejecting the SAML assertion, the client requesting a new one, and the client sending it back to the server. In hindsight, that is a test case for clients to implement and should not be tested as part of this patch. http://gerrit.cloudera.org:8080/#/c/23237/13/be/src/util/backend-gflag-util.cc File be/src/util/backend-gflag-util.cc: http://gerrit.cloudera.org:8080/#/c/23237/13/be/src/util/backend-gflag-util.cc@174 PS13, Line 174: for client conn > Ack Done http://gerrit.cloudera.org:8080/#/c/23237/13/be/src/util/backend-gflag-util.cc@180 PS13, Line 180: debug web > ack, maybe 'debug web UI' Done http://gerrit.cloudera.org:8080/#/c/23237/13/be/src/util/backend-gflag-util.cc@190 PS13, Line 190: > ack Done http://gerrit.cloudera.org:8080/#/c/23237/13/be/src/util/backend-gflag-util.cc@194 PS13, Line 194: > ack Done http://gerrit.cloudera.org:8080/#/c/23237/13/be/src/util/webserver.h File be/src/util/webserver.h: http://gerrit.cloudera.org:8080/#/c/23237/13/be/src/util/webserver.h@202 PS13, Line 202: : // Returns a '401 - Unauthorized' / 'AuthenticationRequired' to the client. : void returnUnauthorized( : struct sq_connection* connection); : : // Helper method to handle SAML authentication failure with metrics and return code : sq_callback_result_t returnSamlAuthFailure(struct sq_connection* connection); : : // Returns a response based on a TWrappedHttpResponse that can come from Frontend code. : void returnWrappedResponse( : struct sq_connection* connection, const impala::TWrappedHttpResponse& response); : > Ack Done http://gerrit.cloudera.org:8080/#/c/23237/13/be/src/util/webserver.h@329 PS13, Line 329: a before s > Ack 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_ > @Csaba, thanks for details! I was thinking the test would be for authenticating to the debug web ui using multiple auth methods. http://gerrit.cloudera.org:8080/#/c/23237/13/be/src/util/webserver.cc@851 PS13, Line 851: bool use_saml = use_saml_ && !sq_get_header(connection, "X-Impala-EETest"); > Actually the comment above is wrong here. I confused 'FLAGS_saml2_ee_test_m Ah, that is helpful. It still appears to me that any client specifying the `X-Impala-EETest` http header will be automatically authenticated if SAML auth is the only auth method available (since that case results in auth_mode_ being set to AuthMode::NONE). http://gerrit.cloudera.org:8080/#/c/23237/13/fe/src/main/java/org/apache/impala/authentication/saml/ImpalaSamlClientBase.java File fe/src/main/java/org/apache/impala/authentication/saml/ImpalaSamlClientBase.java: http://gerrit.cloudera.org:8080/#/c/23237/13/fe/src/main/java/org/apache/impala/authentication/saml/ImpalaSamlClientBase.java@87 PS13, Line 87: // 1. Validate that InResponseTo matches a request ID we actually sent > This method is not even needed. Done http://gerrit.cloudera.org:8080/#/c/23237/13/fe/src/main/java/org/apache/impala/authentication/saml/ImpalaSamlClientBase.java@93 PS13, Line 93: > Ack Done http://gerrit.cloudera.org:8080/#/c/23237/13/fe/src/main/java/org/apache/impala/authentication/saml/ImpalaSamlClientBase.java@108 PS13, Line 108: WithLocationAction locationAction = (WithLocationAction) redirect.get(); > Ack Done http://gerrit.cloudera.org:8080/#/c/23237/13/fe/src/main/java/org/apache/impala/authentication/saml/ImpalaSamlClientBase.java@137 PS13, Line 137: return nameId; > Ack Done http://gerrit.cloudera.org:8080/#/c/23237/15/fe/src/main/java/org/apache/impala/authentication/saml/ImpalaSamlClientBase.java File fe/src/main/java/org/apache/impala/authentication/saml/ImpalaSamlClientBase.java: http://gerrit.cloudera.org:8080/#/c/23237/15/fe/src/main/java/org/apache/impala/authentication/saml/ImpalaSamlClientBase.java@61 PS15, Line 61: // ReplayCache is now configured via ImpalaSAMLMessageStoreFactory in getSamlConfig(). Nice! http://gerrit.cloudera.org:8080/#/c/23237/13/fe/src/main/java/org/apache/impala/authentication/saml/ImpalaSamlClientHS2.java File fe/src/main/java/org/apache/impala/authentication/saml/ImpalaSamlClientHS2.java: http://gerrit.cloudera.org:8080/#/c/23237/13/fe/src/main/java/org/apache/impala/authentication/saml/ImpalaSamlClientHS2.java@46 PS13, Line 46: ImpalaSamlClientHS2.class.getSimpleName(), : HiveSamlRelayStateStoreHS2.get(), > Good point, fixed Done http://gerrit.cloudera.org:8080/#/c/23237/13/fe/src/main/java/org/apache/impala/authentication/saml/ImpalaSamlClientHS2.java@53 PS13, Line 53: throws InternalException { > Fixed Done http://gerrit.cloudera.org:8080/#/c/23237/13/fe/src/main/java/org/apache/impala/service/JniFrontend.java File fe/src/main/java/org/apache/impala/service/JniFrontend.java: http://gerrit.cloudera.org:8080/#/c/23237/13/fe/src/main/java/org/apache/impala/service/JniFrontend.java@825 PS13, Line 825: Preconditions.checkNotNull(client, "SAML client not configured. " > Ack Done http://gerrit.cloudera.org:8080/#/c/23237/13/fe/src/main/java/org/apache/impala/service/JniFrontend.java@844 PS13, Line 844: > Ack Done http://gerrit.cloudera.org:8080/#/c/23237/13/tests/custom_cluster/test_saml2_sso.py File tests/custom_cluster/test_saml2_sso.py: http://gerrit.cloudera.org:8080/#/c/23237/13/tests/custom_cluster/test_saml2_sso.py@145 PS13, Line 145: assertation_id=str(uuid.uuid4()), > Done. The first three are easy. Nice tests! Thanks for adding. For Note 2, it's still good to verify replay attacks are not possible to make sure code/configurations are not accidentally changed in a way to re-enables replay attacks. http://gerrit.cloudera.org:8080/#/c/23237/13/tests/custom_cluster/test_saml2_sso.py@324 PS13, Line 324: self._test_common_protocol_checks() > Good point. Done -- 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: 15 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 Feb 2026 20:20:07 +0000 Gerrit-HasComments: Yes
