Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/24448 )
Change subject: IMPALA-14799: Add oauth_servers support and tests ...................................................................... Patch Set 14: (23 comments) http://gerrit.cloudera.org:8080/#/c/24448/13//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24448/13//COMMIT_MSG@13 PS13, Line 13: Remove deprecated JWT/OAuth signature-toggl > Done Done http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/rpc/authentication.cc@190 PS13, Line 190: "(DEPRECATED) File path of the pre-installed JSON Web Key Set (JWKS) for JWT " > Done. I removed --jwt_validate_signature entirely and updated JwtHttpTest s Done http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/rpc/authentication.cc@237 PS13, Line 237: // Enables retrieving the OAuth JWKS from the specified URL without verifying the > Done. I removed --oauth_jwt_validate_signature entirely. Done http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/rpc/authentication.cc@802 PS13, Line 802: } > Done. The error log now appends username when one was extracted. Done http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/rpc/authentication.cc@790 PS14, Line 790: const string username_msg = username.empty() Nit: this variable declaration is not really necessary since its only used once. http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/jwt-util.h File be/src/util/jwt-util.h: http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/jwt-util.h@100 PS13, Line 100: std::unique_ptr<JWKSMgr> jwks_mgr_; > Done. jwks_mgr_ is now std::unique_ptr<JWKSMgr> again. Done http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/jwt-util.cc File be/src/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/jwt-util.cc@988 PS13, Line 988: Status JWTHelper::CanVerify( > Add DCHECK(jwks_mgr_ != nullptr) to ensure that Init() was already called. Done http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/jwt-util.cc@991 PS13, Line 991: DCHECK(decoded_token != nullptr); > I kept this as a non-error in CanVerify(). An empty JWKS is treated as “can Makes sense. let's leave it as-is. http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/jwt-util.cc@995 PS13, Line 995: > Done. CanVerify() now requires a non-empty alg, validates algorithm family Done http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/jwt-util.cc File be/src/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/jwt-util.cc@852 PS14, Line 852: JWTHelper::JWTHelper() = default; : JWTHelper::~JWTHelper() = default; : JWTHelper::JWTHelper(JWTHelper&&) noexcept = default; : JWTHelper& JWTHelper::operator=(JWTHelper&&) noexcept = default; Nit: since there is no code here, remove these lines and set the constructors to default in jwt-util.h. http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/jwt-util.cc@1006 PS14, Line 1006: string prefix = algorithm.substr(0, 2); Nit: add the `const` keyword to this variable declaration. http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/jwt-util.cc@1009 PS14, Line 1009: if (prefix == "hs") { : key_map = jwks->GetAllHSKeys(); : } else if (prefix == "rs" || prefix == "ps") { : key_map = jwks->GetAllRSAPublicKeys(); : } else if (prefix == "es") { Even though existing code uses magic strings for the algorithm prefixes, please avoid magic strings here. http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/oauth-server-config.h File be/src/util/oauth-server-config.h: http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/oauth-server-config.h@38 PS13, Line 38: int32_t jwks_pull_timeout_secs = DEFAULT_JWKS_PULL_TIMEOUT_SECS; > Done. Removed validate_token_signature from OAuthServerConfig and removed a Done http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/oauth-servers-manager.h File be/src/util/oauth-servers-manager.h: http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/oauth-servers-manager.h@53 PS14, Line 53: Status Verify(const JWTHelper::JWTDecodedToken* decoded_token, This function is only called from within OAuthServersManager. Move it to the private section. http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/oauth-servers-manager.h File be/src/util/oauth-servers-manager.h: http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/oauth-servers-manager.h@56 PS13, Line 56: private: : struct OAuthServerVerifier { : JWTHelper jwt_helper; > Done Done http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/oauth-servers-manager.cc File be/src/util/oauth-servers-manager.cc: http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/oauth-servers-manager.cc@59 PS13, Line 59: bool found_username_server = false; > Done Done http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/oauth-servers-manager.cc@69 PS13, Line 69: } > Done. Grouped DCHECK(jwt_helpers_) and DCHECK(!jwt_helpers_->empty()) toget Done http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/oauth-servers-manager.cc@75 PS13, Line 75: > Done. I removed that bypass path entirely by removing validate_token_signat Done http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/oauth-servers-manager.cc File be/src/util/oauth-servers-manager.cc: http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/oauth-servers-manager.cc@51 PS14, Line 51: Status OAuthServersManager::AuthenticateBearerToken(const string& token, There is duplicate code in this function and OAuthServersManager::Verify. http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/oauth-servers-manager.cc@55 PS14, Line 55: username_out->clear(); Is this line necessary? http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/webserver.cc@1160 PS13, Line 1160: } > Done. Removed the unused line and removed the sq_connection* parameter from Done http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/webserver.cc@1166 PS13, Line 1166: DCHECK(response_headers != nullptr); > Done. Added WWW-Authenticate: Bearer error="invalid_token", error_descripti Done http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/webserver.cc@753 PS14, Line 753: auto remove_bearer_auth_header = [&response_headers]() { : auto it = std::remove_if(response_headers.begin(), response_headers.end(), : [](const string& header) { : return header.rfind("WWW-Authenticate: Bearer", 0) == 0; : }); : response_headers.erase(it, response_headers.end()); : }; What is the reason for this code? The WWW-Authenticate header is only added if token auth fails, and this code is only called if token auth succeeds, thus it will never remove the WWW-Authenticate response header. -- To view, visit http://gerrit.cloudera.org:8080/24448 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib29ff36600406ba59c10f29d79cc632020f4a3f7 Gerrit-Change-Number: 24448 Gerrit-PatchSet: 14 Gerrit-Owner: Anubhav Jindal <[email protected]> Gerrit-Reviewer: Anubhav Jindal <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Comment-Date: Tue, 28 Jul 2026 17:08:30 +0000 Gerrit-HasComments: Yes
