Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/21728 )
Change subject: IMPALA-13288: OAuth AuthN Support for Impala ...................................................................... Patch Set 35: (25 comments) This change keeps getting better! http://gerrit.cloudera.org:8080/#/c/21728/30//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21728/30//COMMIT_MSG@10 PS30, Line 10: configured JS > Nit: configured Done http://gerrit.cloudera.org:8080/#/c/21728/30//COMMIT_MSG@11 PS30, Line 11: * Read the OAuth Access token from the HTTP Header > Please explain the OAuth access token is a bearer JWT Done http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/rpc/authentication.cc@223 PS30, Line 223: Impala will support OAuth based authentication > Done Done http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/rpc/authentication.cc@232 PS30, Line 232: "When true, validate the signature of OAuth token with pre-installed JWKS." > Done Done http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/rpc/authentication.cc@239 PS30, Line 239: ng(oauth_url, "", "URL o > Done Done http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/rpc/authentication.cc@821 PS30, Line 821: connection_context->return_headers.push_back( > That was incorrect. Fixed. Done http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/transport/THttpServer.h File be/src/transport/THttpServer.h: http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/transport/THttpServer.h@201 PS30, Line 201: of SAML SSO, JWT or : // OAuth > Done Done http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/transport/THttpServer.cpp File be/src/transport/THttpServer.cpp: http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/transport/THttpServer.cpp@352 PS30, Line 352: fallback_to_other_auths = false; > unfortunately there is no decrement method. Oh, good point. There would be a decrement method if the metric type was switched from a counter to a gauge, but this metric doesn't make sense as a gauge. Please consider incrementing failure metrics at the very end (after both JWT and OAuth tokens have been checked. http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/transport/THttpServer.cpp@357 PS30, Line 357: resetAuthState(); > Done Done http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/util/webserver.cc@367 PS30, Line 367: metrics->AddCounter("impala.webserver.total-oauth-token-auth-success", 0); > Unfortunately there's no decrement method for the counter. Please see comment on THttpServer.cpp http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/util/webserver.cc@713 PS30, Line 713: check_csrf_protection = false; > Done Done http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/util/webserver.cc@1108 PS30, Line 1108: LOG(ERROR) << "Error verifying OAuth token in Authorization header, " > The decode token part logic is common. Other parts depend on the respective The two functions are still almost exactly the same. It would be better to have a helper function called by OAuthTokenAuth() and JWTTokenAuth() and the correct flags/words passed in. http://gerrit.cloudera.org:8080/#/c/21728/35/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/21728/35/be/src/util/webserver.cc@696 PS35, Line 696: jwt_token Nit: rename to something more generic (such as `bearer_token`) to avoid confusion when this token is used in OAuth. http://gerrit.cloudera.org:8080/#/c/21728/35/be/src/util/webserver.cc@706 PS35, Line 706: LOG(INFO) << "Invalid JWT token provided: " << jwt_token; I don't like logging a jwt auth failure automatically if there is a potential for OAuth to succeed. These invalid errors will cause confusion. Please do not log any failure messages until both JWT and OAuth have been attempted (if applicable). http://gerrit.cloudera.org:8080/#/c/21728/35/fe/src/test/java/org/apache/impala/customcluster/JwtWebserverTest.java File fe/src/test/java/org/apache/impala/customcluster/JwtWebserverTest.java: http://gerrit.cloudera.org:8080/#/c/21728/35/fe/src/test/java/org/apache/impala/customcluster/JwtWebserverTest.java@71 PS35, Line 71: private void verifyOAuthAuthMetrics( This function can be combined with the previous function since the only differences are parts of the final section of the metric names. http://gerrit.cloudera.org:8080/#/c/21728/35/tests/custom_cluster/test_shell_jwt_auth.py File tests/custom_cluster/test_shell_jwt_auth.py: http://gerrit.cloudera.org:8080/#/c/21728/35/tests/custom_cluster/test_shell_jwt_auth.py@26 PS35, Line 26: class TestImpalaShellJWTAuth(CustomClusterTestSuite): Please create a class hierarchy with a single base class (such as TestImpalaShellBearerAuth) and then three subclasses -- TestImpalaShellJWTAuth, TestImpalaShellOAuth, and TestImpalaShellJWTOAuth (where both auth methods are enabled at the same time on the backend server). The common logic will go in the base class with the specific tests for each auth method in the subclasses. Having this class hierarchy makes it much clearer what each test is doing which makes it easier to debug failing tests. http://gerrit.cloudera.org:8080/#/c/21728/35/tests/custom_cluster/test_shell_jwt_auth.py@47 PS35, Line 47: .format(JWKS_JSON_PATH)) Nit: The above two lines need to be indented so they line up with line 45. http://gerrit.cloudera.org:8080/#/c/21728/35/tests/custom_cluster/test_shell_jwt_auth.py@51 PS35, Line 51: .format(JWKS_JSON_PATH)) Nit: The above two lines need to be indented so they line up with line 45. http://gerrit.cloudera.org:8080/#/c/21728/35/tests/custom_cluster/test_shell_jwt_auth.py@68 PS35, Line 68: impalad_args=IMPALAD_ARGS_JWT, If setting up the test class hierarchy, these existing tests should not have to change. http://gerrit.cloudera.org:8080/#/c/21728/35/tests/custom_cluster/test_shell_jwt_auth.py@246 PS35, Line 246: a Nit: an http://gerrit.cloudera.org:8080/#/c/21728/35/tests/custom_cluster/test_shell_jwt_auth.py@255 PS35, Line 255: # Ensure the Impala coordinator is correctly reporting the oauth auth metrics Nit: OAuth http://gerrit.cloudera.org:8080/#/c/21728/35/tests/custom_cluster/test_shell_jwt_auth.py@265 PS35, Line 265: OAUth Nit: OAuth http://gerrit.cloudera.org:8080/#/c/21728/35/tests/custom_cluster/test_shell_jwt_auth.py@291 PS35, Line 291: a Nit: an http://gerrit.cloudera.org:8080/#/c/21728/35/tests/custom_cluster/test_shell_jwt_auth.py@300 PS35, Line 300: oauth Nit: OAuth http://gerrit.cloudera.org:8080/#/c/21728/35/tests/custom_cluster/test_shell_jwt_auth.py@309 PS35, Line 309: OAUth Nit: OAuth -- To view, visit http://gerrit.cloudera.org:8080/21728 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65dc8db917476b0f0d29b659b9fa51ebaf45b7a6 Gerrit-Change-Number: 21728 Gerrit-PatchSet: 35 Gerrit-Owner: gaurav singh <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: gaurav singh <[email protected]> Gerrit-Comment-Date: Mon, 13 Jan 2025 20:57:00 +0000 Gerrit-HasComments: Yes
