Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/17435 )
Change subject: IMPALA-10489: Implement JWT support ...................................................................... Patch Set 9: (22 comments) http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/rpc/authentication.cc@156 PS9, Line 156: DEFINE_string(jwt_custom_claim_username > Do we have any code that would validate this value? (i.e. it should not be Add code to validate this value in AuthManager::Init(). http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/rpc/authentication.cc@156 PS9, Line 156: "Custom claim 'username'" > Nit: I would expand on this description to note that this specifies the cus Updated iy. http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/rpc/authentication.cc@1432 PS9, Line 1432: if (use_saml) { : SecureAuthProvider* sap = NULL; : external_http_auth_provider_.reset(sap = new SecureAuthProvider(false)); : sap->InitSaml(); : LOG(INFO) << : "External communication is authenticated for hs2-http protocol with SAML2 SSO"; : } else { : LOG(INFO) << "External communication is not authenticated for hs2-http protocol"; : } > I think we want to be able to have JWT as the only authentication method (w Added code as suggested. http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/transport/THttpServer.cpp File be/src/transport/THttpServer.cpp: http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/transport/THttpServer.cpp@278 PS9, Line 278: } else if (use_jwt_token_ : && stripped_bearer_auth_token.find('.') != string::npos) { : // Since Impala supports multiple auth mechanism in parallel, falls back to JWT. : if (callbacks_.jwt_token_auth_fn(stripped_bearer_auth_token)) { : authorized = true; : if (metrics_enabled_) { : http_metrics_->total_jwt_token_auth_success_->Increment(1); : } : } > The other way to do this is to move the below JWT auth section above this " Yes, this is more clean. Fixed as suggested. http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/util/jwt-util-test.cc File be/src/util/jwt-util-test.cc: http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/util/jwt-util-test.cc@145 PS9, Line 145: // Delete this temporary file : void Delete(); > If this isn't used directly, then you can make it private. Fixed. http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/util/jwt-util-test.cc@149 PS9, Line 149: const char* Filename() const { return name_.c_str(); } > Since JsonWebKeySet::Init() takes the filename in as a "const std::string&" fixed. http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/util/jwt-util-test.cc@169 PS9, Line 169: NULL > Nit: Use nullptr rather than NULL for new code. fixed. http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/util/jwt-util-test.cc@224 PS9, Line 224: TempTestDataFile* jwks_file = new TempTestDataFile( : "{" : " \"keys\": [" : " {" : " \"use\": \"sig\"," : " \"kty\": \"RSA\"," : " \"alg\": \"RS256\"," : " \"n\": \"sttddbg-_yjXzcFpbMJB1fIFam9lQBeXWbTqzJwbuFbspHMsRowa8FaPw\"," : " \"e\": \"AQAB\"" : " }" : " ]" : "}"); : JsonWebKeySet* jwks = new JsonWebKeySet(); > Here and elsewhere in this file, use smart pointers / std::unique_ptr where Fixed. http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/util/jwt-util-test.cc@348 PS9, Line 348: TEST(JwtUtilTest, VerifyJwtRS512) { : // Sign JWT token with RS512. : TempTestDataFile jwks_file(Substitute(jwks_file_format, kid_1, "RS512", rsa512_pub_key, : kid_2, "RS512", rsa512_pub_key_invalid)); : JsonWebKeySet* jwks = new JsonWebKeySet(); : Status status = jwks->Init(jwks_file.Filename()); : ASSERT_TRUE(status.ok()); : ASSERT_EQ(2, jwks->GetKeyNum()); : : const JsonWebKey* key1 = jwks->LookupKey(kid_1); : ASSERT_TRUE(key1 != nullptr); : ASSERT_EQ(ADD_PUB_KEY_HEADER_FOOTER(rsa512_pub_key), key1->key_value_); : : // Create a JWT token and sign it with RS512. : string priv_key = ADD_RSA_PRIV_KEY_HEADER_FOOTER(rsa512_priv_key); : string pub_key = ADD_PUB_KEY_HEADER_FOOTER(rsa512_pub_key); : auto token = jwt::create() : .set_issuer("auth0") : .set_type("JWS") : .set_algorithm("RS512") : .set_key_id(kid_1) : .set_payload_claim("username", picojson::value("impala")) : .sign(jwt::algorithm::rs512(pub_key, priv_key, "", "")); : : // Verify the JWT token with our wrapper API which use public key retrieved from JWKS, : // and read username from the token. : string username; : status = VerifyJWTTokenExtractUsername(token, jwks, "username", username); : ASSERT_TRUE(status.ok()); : ASSERT_EQ("impala", username); : : delete jwks; : } > For completeness, let's also add a similar case with RS384 Added test case for RS384. http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/util/jwt-util-test.cc@456 PS9, Line 456: ASSERT_TRUE(status.ok()); > When checking that status is ok, use ASSERT_OK/EXPECT_OK from testutil/gtes Fixed as suggested http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/util/jwt-util.cc File be/src/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/util/jwt-util.cc@103 PS9, Line 103: if (!keys.IsArray()) { : return Status(TErrorCode::JWKS_PARSE_ERROR, : Substitute( : "'keys' must be of type Array but is a '$0'", NameOfTypeOfJsonValue(keys))); : } > Should we add an additional check that the array has non-zero Size()? Done http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/util/jwt-util.cc@131 PS9, Line 131: for (Value::ConstMemberIterator member = json_key.MemberBegin(); : member != json_key.MemberEnd(); ++member) { > For this for loop, you might just insert all the values into an unordered_m Done http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/util/jwt-util.cc@133 PS9, Line 133: if (strcmp("use", member->name.GetString()) == 0) { : RETURN_IF_ERROR(ReadKeyProperty("use", json_key, &key_use, /*required*/ false)); > It sounds like this is optional, so maybe we don't need to require a specif Right, it's optional. We don't need to save it. http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/util/jwt-util.cc@133 PS9, Line 133: if (strcmp("use", member->name.GetString()) == 0) { : RETURN_IF_ERROR(ReadKeyProperty("use", json_key, &key_use, /*required*/ false)); > Do we care about what "use" is set to? Do we want to require any specific v No, remove it. http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/util/jwt-util.cc@146 PS9, Line 146: else if (strcmp("n", member->name.GetString()) == 0) { : found_kvalue = true; : RETURN_IF_ERROR(ReadKeyProperty("n", json_key, &key_value)); : if (key_value.empty()) { : return Status("'n' property must be a non-empty string"); : } > This works for RSA. Other encryption types like elliptic curve require othe Right, added comments for other algorithms. http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/util/jwt-util.cc@204 PS9, Line 204: NULL > Use "nullptr" in new code rather than NULL fixed http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/util/jwt-util.cc@249 PS9, Line 249: VerifyJWTTokenExtractUsername > From a style point of view, I think I would split up this function. I would Added a class with the suggested member functions. http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/util/jwt-util.cc@282 PS9, Line 282: if (strncasecmp(algorithm.c_str(), jwk->algorithm_.c_str(), algorithm.length()) : != 0) { > To be careful, compare the lengths as well to make sure this isn't just mat Done http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/util/jwt-util.cc@290 PS9, Line 290: if (strncasecmp(algorithm.c_str(), "rs256", algorithm.length()) == 0) { : jwt::verify() : .allow_algorithm(jwt::algorithm::rs256(jwk->key_value_, "", "", "")) : .with_issuer(issuer) : .verify(decoded_token); : } else if (strncasecmp(algorithm.c_str(), "rs384", algorithm.length()) == 0) { : jwt::verify() : .allow_algorithm(jwt::algorithm::rs384(jwk->key_value_, "", "", "")) : .with_issuer(issuer) : .verify(decoded_token); : } else if (strncasecmp(algorithm.c_str(), "rs512", algorithm.length()) == 0) { : jwt::verify() : .allow_algorithm(jwt::algorithm::rs512(jwk->key_value_, "", "", "")) : .with_issuer(issuer) : .verify(decoded_token); > One performance thing we may want to do is to construct the verifier once p Changed code to create one verifier. http://gerrit.cloudera.org:8080/#/c/17435/9/be/src/util/jwt-util.cc@312 PS9, Line 312: // Get value of custom claim 'username' from the token payload. : if (decoded_token.has_payload_claim(jwt_custom_claim_username)) { : // Assume the claim data type of 'username' is string. : username.assign( : decoded_token.get_payload_claim(jwt_custom_claim_username).to_json().to_str()); : } > What happens if the username claim is not present? Should we return an erro Changed code to return error. http://gerrit.cloudera.org:8080/#/c/17435/9/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/17435/9/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@421 PS9, Line 421: /** : * Tests if sessions are authenticated by verifying the JWT token for connections : * to the HTTP hiveserver2 endpoint. : * Since we don't have Java version of JWT library, we use pre-calculated JWT token : * and JWKS. The token and JWK set used in this test case were generated by using : * BE unit-test function JwtUtilTest::VerifyJwtRS256. : */ : @Test : public void testHiveserver2JwtAuth() throws Exception { : String jwksFilename = : new File(System.getenv("IMPALA_HOME"), "testdata/jwt/jwks_rs256.json").getPath(); : setUp(String.format( : "--jwt_token_auth=true --jwt_validate_signature=true --jwks_file_path=%s " : + "--jwt_allow_without_tls=true", : jwksFilename)); : verifyMetrics(0, 0); : THttpClient transport = new THttpClient("http://localhost:28000";); : Map<String, String> headers = new HashMap<String, String>(); : : // Case 1: Authenticate with valid JWT Token in HTTP header. : String jwtToken = : "eyJhbGciOiJSUzI1NiIsImtpZCI6InB1YmxpYzpjNDI0YjY3Yi1mZTI4LTQ1ZDctYjAxNS1m" : + "NzlkYTUwYjViMjEiLCJ0eXAiOiJKV1MifQ.eyJpc3MiOiJhdXRoMCIsInVzZXJuYW1lIjoia" : + "W1wYWxhIn0.OW5H2SClLlsotsCarTHYEbqlbRh43LFwOyo9WubpNTwE7hTuJDsnFoVrvHiWI" : + "02W69TZNat7DYcC86A_ogLMfNXagHjlMFJaRnvG5Ekag8NRuZNJmHVqfX-qr6x7_8mpOdU55" : + "4kc200pqbpYLhhuK4Qf7oT7y9mOrtNrUKGDCZ0Q2y_mizlbY6SMg4RWqSz0RQwJbRgXIWSgc" : + "bZd0GbD_MQQ8x7WRE4nluU-5Fl4N2Wo8T9fNTuxALPiuVeIczO25b5n4fryfKasSgaZfmk0C" : + "oOJzqbtmQxqiK9QNSJAiH2kaqMwLNgAdgn8fbd-lB1RAEGeyPH8Px8ipqcKsPk0bg"; : headers.put("Authorization", "Bearer " + jwtToken); : headers.put("X-Forwarded-For", "127.0.0.1"); : transport.setCustomHeaders(headers); : transport.open(); : TCLIService.Iface client = new TCLIService.Client(new TBinaryProtocol(transport)); : : // Open a session which will get username 'impala' from JWT token and use it as : // login user. : TOpenSessionReq openReq = new TOpenSessionReq(); : TOpenSessionResp openResp = client.OpenSession(openReq); : // One successful authentication. : verifyMetrics(0, 0); : verifyJwtAuthMetrics(1, 0); : // Running a query should succeed. : TOperationHandle operationHandle = execAndFetch( : client, openResp.getSessionHandle(), "select logged_in_user()", "impala"); : // Two more successful authentications - for the Exec() and the Fetch(). : verifyMetrics(0, 0); : verifyJwtAuthMetrics(3, 0); : : // case 2: Authenticate fails with invalid JWT token which does not have signature. : String invalidJwtToken = : "eyJhbGciOiJSUzI1NiIsImtpZCI6InB1YmxpYzpjNDI0YjY3Yi1mZTI4LTQ1ZDctYjAxNS1m" : + "NzlkYTUwYjViMjEiLCJ0eXAiOiJKV1MifQ.eyJpc3MiOiJhdXRoMCIsInVzZXJuYW1lIjoia" : + "W1wYWxhIn0."; : headers.put("Authorization", "Bearer " + invalidJwtToken); : headers.put("X-Forwarded-For", "127.0.0.1"); : transport.setCustomHeaders(headers); : try { : openResp = client.OpenSession(openReq); : fail("Exception exception."); : } catch (Exception e) { : verifyJwtAuthMetrics(3, 1); : assertEquals(e.getMessage(), "HTTP Response code: 401"); : } : } > These new test cases are testing LDAP + JWT. LdapHS2Test::setUp() includes Tried to add tests in Python custom cluster tests, but did not work. I will use LDAP library and move the JWT test cases as new Java file. Will try Python custom cluster later. http://gerrit.cloudera.org:8080/#/c/17435/9/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java File fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java: http://gerrit.cloudera.org:8080/#/c/17435/9/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java@276 PS9, Line 276: /** : * Tests if sessions are authenticated by verifying the JWT token for connections : * to the Web Server. : * Since we don't have Java version of JWT library, we use pre-calculated JWT token : * and JWKS. The token and JWK set used in this test case were generated by using : * BE unit-test function JwtUtilTest::VerifyJwtRS256. : */ : @Test : public void testWebserverJwtAuth() throws Exception { : String jwksFilename = : new File(System.getenv("IMPALA_HOME"), "testdata/jwt/jwks_rs256.json").getPath(); : setUp(String.format( : "--jwt_token_auth=true --jwt_validate_signature=true --jwks_file_path=%s " : + "--jwt_allow_without_tls=true", : jwksFilename), : ""); : : // Case 1: Authenticate with valid JWT Token in HTTP header. : String jwtToken = : "eyJhbGciOiJSUzI1NiIsImtpZCI6InB1YmxpYzpjNDI0YjY3Yi1mZTI4LTQ1ZDctYjAxNS1m" : + "NzlkYTUwYjViMjEiLCJ0eXAiOiJKV1MifQ.eyJpc3MiOiJhdXRoMCIsInVzZXJuYW1lIjoia" : + "W1wYWxhIn0.OW5H2SClLlsotsCarTHYEbqlbRh43LFwOyo9WubpNTwE7hTuJDsnFoVrvHiWI" : + "02W69TZNat7DYcC86A_ogLMfNXagHjlMFJaRnvG5Ekag8NRuZNJmHVqfX-qr6x7_8mpOdU55" : + "4kc200pqbpYLhhuK4Qf7oT7y9mOrtNrUKGDCZ0Q2y_mizlbY6SMg4RWqSz0RQwJbRgXIWSgc" : + "bZd0GbD_MQQ8x7WRE4nluU-5Fl4N2Wo8T9fNTuxALPiuVeIczO25b5n4fryfKasSgaZfmk0C" : + "oOJzqbtmQxqiK9QNSJAiH2kaqMwLNgAdgn8fbd-lB1RAEGeyPH8Px8ipqcKsPk0bg"; : attemptConnection("Bearer " + jwtToken, "127.0.0.1"); : verifyJwtAuthMetrics(Range.closed(1L, 1L), zero); : : // Case 2: Failed with invalid JWT Token. : String invalidJwtToken = : "eyJhbGciOiJSUzI1NiIsImtpZCI6InB1YmxpYzpjNDI0YjY3Yi1mZTI4LTQ1ZDctYjAxNS1m" : + "NzlkYTUwYjViMjEiLCJ0eXAiOiJKV1MifQ.eyJpc3MiOiJhdXRoMCIsInVzZXJuYW1lIjoia" : + "W1wYWxhIn0."; : try { : attemptConnection("Bearer " + invalidJwtToken, "127.0.0.1"); : } catch (IOException e) { : assertTrue(e.getMessage().contains("Server returned HTTP response code: 401")); : } : verifyJwtAuthMetrics(Range.closed(1L, 1L), Range.closed(1L, 1L)); : } > Same comment as on the other Java LDAP test file. It would be nice to split Tried to add tests in Python custom cluster tests, but did not work. I will use LDAP library and move the JWT test cases as new Java file. -- To view, visit http://gerrit.cloudera.org:8080/17435 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6b71fa854c9ddc8ca882878853395e1eb866143c Gerrit-Change-Number: 17435 Gerrit-PatchSet: 9 Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Wed, 16 Jun 2021 01:03:06 +0000 Gerrit-HasComments: Yes
