gaurav singh has posted comments on this change. ( http://gerrit.cloudera.org:8080/21728 )
Change subject: WIP IMPALA-13288: OAuth AuthN Support for Impala ...................................................................... Patch Set 31: (12 comments) 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 > This wording is a bit awkward. Maybe something like "Impala will support O 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." > Please add the same wording from the "oauth_verify_server_certificate" para Done http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/rpc/authentication.cc@239 PS30, Line 239: ng(oauth_url, "", "URL o > Nit: "retrieving the OAuth JWKS from the specified URL" Done 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 verificatio That was incorrect. Fixed. 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 Removing this is causing build error. 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 Removing this is causing build error. 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 > Nit: of SAML SSO, JWT, or OAuth 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: // 1st SAML message in browser mode, redirect SSO. > Is JWT auth is also enabled, need to decrement http_metrics_->total_jwt_tok unfortunately there is no decrement method. http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/transport/THttpServer.cpp@357 PS30, Line 357: throw TTransportException("HTTP auth - SAML redirection."); > There could be some cleanup done by combining this if block with the if blo 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); > This line has a similar potential issue if both JWT and OAuth are enabled, Unfortunately there's no decrement method for the counter. http://gerrit.cloudera.org:8080/#/c/21728/30/be/src/util/webserver.cc@713 PS30, Line 713: check_csrf_protection = false; > There could be some cleanup done by combining this if block with the if blo 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 bulk of the logic in OAuthTokenAuth() and JWTTokenAuth() is shared. Pl The decode token part logic is common. Other parts depend on the respective flags. Also the error messages are different. -- 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: 31 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: Sat, 11 Jan 2025 02:18:41 +0000 Gerrit-HasComments: Yes
