gaurav singh has posted comments on this change. ( http://gerrit.cloudera.org:8080/21728 )
Change subject: IMPALA-13288: OAuth AuthN Support for Impala ...................................................................... Patch Set 43: (21 comments) http://gerrit.cloudera.org:8080/#/c/21728/39//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21728/39//COMMIT_MSG@16 PS39, Line 16: Else only one of jwt or oauth is supported. > Need brief explanation here as to why kerberos/ldap auth being enabled impa This has been a pre existing change for jwt. So OAuth will have the same policy. http://gerrit.cloudera.org:8080/#/c/21728/39/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/21728/39/be/src/rpc/authentication.cc@228 PS39, Line 228: a > Nit: an Done http://gerrit.cloudera.org:8080/#/c/21728/39/be/src/rpc/authentication.cc@240 PS39, Line 240: // Enables retrieving the OAuth JWKS from the specified URL without verifying the > This comment does not make sense. Need to add something like "without veri Done http://gerrit.cloudera.org:8080/#/c/21728/39/be/src/rpc/authentication.cc@257 PS39, Line 257: r."); > Nit: in the OAuth token Done http://gerrit.cloudera.org:8080/#/c/21728/39/be/src/rpc/authentication.cc@259 PS39, Line 259: > This help text copies from JWT, but that help text is unhelpful. Please ch Done http://gerrit.cloudera.org:8080/#/c/21728/40/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/21728/40/be/src/rpc/authentication.cc@231 PS40, Line 231: oauth_jwt_validate_signa > Should be named "oauth_jwt_validate_signature" Done http://gerrit.cloudera.org:8080/#/c/21728/40/be/src/rpc/authentication.cc@236 PS40, Line 236: oauth_jwks_file > Should be named "oauth_jwks_file_path" Done http://gerrit.cloudera.org:8080/#/c/21728/40/be/src/rpc/authentication.cc@239 PS40, Line 239: oauth_jwk > Should be named "oauth_jwks_url" Done http://gerrit.cloudera.org:8080/#/c/21728/40/be/src/rpc/authentication.cc@242 PS40, Line 242: oauth_jwks_verify_server_certif > Should be named "oauth_jwks_verify_server_certificate" Done http://gerrit.cloudera.org:8080/#/c/21728/40/be/src/rpc/authentication.cc@249 PS40, Line 249: oauth_jwks_ca_certif > Should be named "oauth_jwks_ca_certificate" Done http://gerrit.cloudera.org:8080/#/c/21728/40/be/src/rpc/authentication.cc@252 PS40, Line 252: oauth_jwks_update_freque > Should be named "oauth_jwks_update_frequency_s" Done http://gerrit.cloudera.org:8080/#/c/21728/40/be/src/rpc/authentication.cc@255 PS40, Line 255: oauth_jwks_pulling_time > Should be named "oauth_jwks_pulling_timeout_s" Done http://gerrit.cloudera.org:8080/#/c/21728/39/be/src/runtime/exec-env.h File be/src/runtime/exec-env.h: http://gerrit.cloudera.org:8080/#/c/21728/39/be/src/runtime/exec-env.h@33 PS39, Line 33: #include "util/jwt-util-internal.h" > Unused import. Removing this breaks the build http://gerrit.cloudera.org:8080/#/c/21728/39/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/21728/39/be/src/runtime/exec-env.cc@63 PS39, Line 63: #include "util/jwt-util-internal.h" > Unused import Removing this breaks the build http://gerrit.cloudera.org:8080/#/c/21728/40/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/21728/40/be/src/util/webserver.cc@716 PS40, Line 716: if (use_jwt_) { : LOG(INFO) << "Invalid JWT token provided: " << bearer_token; : total_jwt_token_auth_failure_->Increment(1); : } : if (use_oauth_) { : LOG(INFO) << "Invalid OAuth token provided: " << bearer_token; : total_oauth_token_auth_failure_->Increment(1); > indent spaces Done http://gerrit.cloudera.org:8080/#/c/21728/40/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java File fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java: http://gerrit.cloudera.org:8080/#/c/21728/40/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@165 PS40, Line 165: > combine this function with verifyJwtAuthMetrics by passing auth string type Done http://gerrit.cloudera.org:8080/#/c/21728/40/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@825 PS40, Line 825: * Assume that Impala JDBC driver has been downloaded and copied to : * ${FILESYSTEM_PREFIX}/test-warehouse/data-sources/jdbc-drivers/. : */ > not fixed Done http://gerrit.cloudera.org:8080/#/c/21728/40/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@825 PS40, Line 825: * Assume that Impala JDBC driver has been downloaded and copied to : * ${FILESYSTEM_PREFIX}/test-warehouse/data-sources/jdbc-drivers/. : */ > keep indent spaces consistent Done http://gerrit.cloudera.org:8080/#/c/21728/39/tests/custom_cluster/test_shell_oauth_auth.py File tests/custom_cluster/test_shell_oauth_auth.py: http://gerrit.cloudera.org:8080/#/c/21728/39/tests/custom_cluster/test_shell_oauth_auth.py@30 PS39, Line 30: s > Nit: remove extra space Done http://gerrit.cloudera.org:8080/#/c/21728/40/tests/custom_cluster/test_shell_oauth_auth.py File tests/custom_cluster/test_shell_oauth_auth.py: http://gerrit.cloudera.org:8080/#/c/21728/40/tests/custom_cluster/test_shell_oauth_auth.py@139 PS40, Line 139: JWT > OAuth ? This error message comes from jwt-cpp library. Since OAuth is using the same library function, it prints jwt. In the OAuthAuthToken() function, we add additional message "Error verifying OAuth token" to differentiate between oauth and jwt authentication. http://gerrit.cloudera.org:8080/#/c/21728/40/tests/custom_cluster/test_shell_oauth_auth.py@183 PS40, Line 183: JWT > OAuth ? This error message comes from jwt-cpp library. Since OAuth is using the same library function, it prints jwt. In the OAuthAuthToken() function, we add additional message "Error verifying OAuth token" to differentiate between oauth and jwt authentication. -- 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: 43 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: Tue, 14 Jan 2025 20:07:49 +0000 Gerrit-HasComments: Yes
