Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/21728 )
Change subject: WIP IMPALA-13288: OAuth AuthN Support for Impala ...................................................................... Patch Set 30: (14 comments) This patch has been slimmed down quite a bit! Always good to see less code. The custom cluster JWT tests will also need to be ran for OAuth. You may be able to use pytest vectors (one for jwt and another for oauth) to make minimal changes to the tests. 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: pre-installed Nit: configured 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 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 support for trusting an authentication This wording is a bit awkward. Maybe something like "Impala will support OAuth generated JWTs"? 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."); Please add the same wording from the "oauth_verify_server_certificate" param -- This should only be set to false for development / testing. http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/rpc/authentication.cc@239 PS30, Line 239: retrieving the OAuth URL Nit: "retrieving the OAuth JWKS from the specified URL" http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/rpc/authentication.cc@821 PS30, Line 821: connection_context->return_headers.push_back( In this function, the "WWW-Authenticate" header is set when JWT verification fails, but in the JWTTokenAuth() function, its set when the username claim cannot be found. Is this difference intentional? If the differences is not intentional, the bulk of the logic from the JWTTokenAuth() and OAuthTokenAuth() functions should be moved into a separate function so the code is not repeated. http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/runtime/exec-env.h File be/src/runtime/exec-env.h: http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/runtime/exec-env.h@33 PS30, Line 33: #include "util/jwt-util-internal.h" Unneeded import http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/runtime/exec-env.cc@63 PS30, Line 63: #include "util/jwt-util-internal.h" Unneeded import 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 or JWT or : // OAuth Nit: of SAML SSO, JWT, or OAuth 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: if (metrics_enabled_) http_metrics_->total_oauth_token_auth_success_->Increment(1); Is JWT auth is also enabled, need to decrement http_metrics_->total_jwt_token_auth_failure or else that metric will increment each time an OAuth token auth succeeds. http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/transport/THttpServer.cpp@357 PS30, Line 357: } There could be some cleanup done by combining this if block with the if block that processes JWT auth, but combining them could also lead to confusion. Please consider combining the two blocks of code and whether or not that will make the code clearer or more muddled. 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); This line has a similar potential issue if both JWT and OAuth are enabled, then successful OAuth token auth needs to decrement impala.webserver.total-jwt-token-auth-failure. http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/util/webserver.cc@713 PS30, Line 713: if (use_oauth_) { There could be some cleanup done by combining this if block with the if block that processes JWT auth, but combining them could also lead to confusion. Please consider combining the two blocks of code and whether or not that will make the code clearer or more muddled. http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/util/webserver.cc@1108 PS30, Line 1108: bool Webserver::OAuthTokenAuth(const std::string& oauth_token, The bulk of the logic in OAuthTokenAuth() and JWTTokenAuth() is shared. Please consider pulling out the common logic into a separate function. -- 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: 30 Gerrit-Owner: gaurav singh <gsi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: gaurav singh <gsi...@cloudera.com> Gerrit-Comment-Date: Fri, 10 Jan 2025 21:42:50 +0000 Gerrit-HasComments: Yes
