From 6df5ecc311c248e52cdcd38fd10fa63488e85a46 Mon Sep 17 00:00:00 2001 From: Yunze Xu Date: Mon, 11 Oct 2021 22:46:10 +0800 Subject: [PATCH 1/9] Set all fields of Oauth2TokenResult --- pulsar-client-cpp/lib/auth/AuthOauth2.cc | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/pulsar-client-cpp/lib/auth/AuthOauth2.cc b/pulsar-client-cpp/lib/auth/AuthOauth2.cc index 953dd8eff156f..580ef6a521c5d 100644 --- a/pulsar-client-cpp/lib/auth/AuthOauth2.cc +++ b/pulsar-client-cpp/lib/auth/AuthOauth2.cc @@ -303,11 +303,18 @@ Oauth2TokenResultPtr ClientCredentialFlow::authenticate() { break; } - resultPtr->setAccessToken(root.get("access_token")); - resultPtr->setExpiresIn(root.get("expires_in")); - - LOG_DEBUG("access_token: " << resultPtr->getAccessToken() - << " expires_in: " << resultPtr->getExpiresIn()); + resultPtr->setAccessToken(root.get("access_token", "")); + resultPtr->setExpiresIn( + root.get("expires_in", Oauth2TokenResult::undefined_expiration)); + resultPtr->setIdToken(root.get("refresh_token", "")); + resultPtr->setIdToken(root.get("id_token", "")); + + if (!resultPtr->getAccessToken().empty()) { + LOG_DEBUG("access_token: " << resultPtr->getAccessToken() + << " expires_in: " << resultPtr->getExpiresIn()); + } else { + LOG_ERROR("Response doesn't contain access_token, the response is: " << responseData); + } } else { LOG_ERROR("Response failed for issuerurl " << issuerUrl_ << ". response Code " << response_code << " passedin: " << jsonBody); From a007adf72ddfa30f2e7b64d8ac8a15d961b34c26 Mon Sep 17 00:00:00 2001 From: Yunze Xu Date: Tue, 12 Oct 2021 13:20:20 +0800 Subject: [PATCH 2/9] Expose AuthenticationError to client APIs --- .../include/pulsar/Authentication.h | 6 +++--- .../lib/BinaryProtoLookupService.cc | 2 +- pulsar-client-cpp/lib/ClientConnection.cc | 21 +++++++++++++++---- pulsar-client-cpp/lib/ClientConnection.h | 2 +- pulsar-client-cpp/lib/Commands.cc | 18 ++++++++++++---- pulsar-client-cpp/lib/Commands.h | 4 ++-- pulsar-client-cpp/lib/HTTPLookupService.cc | 5 +---- pulsar-client-cpp/lib/auth/AuthOauth2.cc | 7 ++++++- 8 files changed, 45 insertions(+), 20 deletions(-) diff --git a/pulsar-client-cpp/include/pulsar/Authentication.h b/pulsar-client-cpp/include/pulsar/Authentication.h index 991e35b68b068..185ac335c8001 100644 --- a/pulsar-client-cpp/include/pulsar/Authentication.h +++ b/pulsar-client-cpp/include/pulsar/Authentication.h @@ -98,7 +98,7 @@ class PULSAR_PUBLIC Authentication { * * @param[out] authDataContent the shared pointer of AuthenticationData. The content of AuthenticationData * is changed to the internal data of the current instance. - * @return ResultOk + * @return ResultOk or ResultAuthenticationError if authentication failed */ virtual Result getAuthData(AuthenticationDataPtr& authDataContent) { authDataContent = authData_; @@ -450,7 +450,7 @@ class CachedToken { /** * Get AuthenticationData from the current instance * - * @return ResultOk + * @return ResultOk or ResultAuthenticationError if authentication failed */ virtual AuthenticationDataPtr getAuthData() = 0; @@ -504,7 +504,7 @@ class PULSAR_PUBLIC AuthOauth2 : public Authentication { * * @param[out] authDataOauth2 the shared pointer of AuthenticationData. The content of AuthenticationData * is changed to the internal data of the current instance. - * @return ResultOk + * @return ResultOk or ResultAuthenticationError if authentication failed */ Result getAuthData(AuthenticationDataPtr& authDataOauth2); diff --git a/pulsar-client-cpp/lib/BinaryProtoLookupService.cc b/pulsar-client-cpp/lib/BinaryProtoLookupService.cc index 6391ece3d5f92..716ff91c58a27 100644 --- a/pulsar-client-cpp/lib/BinaryProtoLookupService.cc +++ b/pulsar-client-cpp/lib/BinaryProtoLookupService.cc @@ -126,7 +126,7 @@ void BinaryProtoLookupService::sendPartitionMetadataLookupRequest(const std::str const ClientConnectionWeakPtr& clientCnx, LookupDataResultPromisePtr promise) { if (result != ResultOk) { - promise->setFailed(ResultConnectError); + promise->setFailed(result); Future future = promise->getFuture(); return; } diff --git a/pulsar-client-cpp/lib/ClientConnection.cc b/pulsar-client-cpp/lib/ClientConnection.cc index 8781979c3aaaa..6f947312f01e6 100644 --- a/pulsar-client-cpp/lib/ClientConnection.cc +++ b/pulsar-client-cpp/lib/ClientConnection.cc @@ -458,7 +458,14 @@ void ClientConnection::handleHandshake(const boost::system::error_code& err) { } bool connectingThroughProxy = logicalAddress_ != physicalAddress_; - SharedBuffer buffer = Commands::newConnect(authentication_, logicalAddress_, connectingThroughProxy); + Result result = ResultOk; + SharedBuffer buffer = + Commands::newConnect(authentication_, logicalAddress_, connectingThroughProxy, result); + if (result != ResultOk) { + LOG_ERROR(cnxString_ << "Failed to establish connection: " << result); + close(result); + return; + } // Send CONNECT command to broker asyncWrite(buffer.const_asio_buffer(), std::bind(&ClientConnection::handleSentPulsarConnect, shared_from_this(), std::placeholders::_1, buffer)); @@ -1144,7 +1151,13 @@ void ClientConnection::handleIncomingCommand() { case BaseCommand::AUTH_CHALLENGE: { LOG_DEBUG(cnxString_ << "Received auth challenge from broker"); - SharedBuffer buffer = Commands::newAuthResponse(authentication_); + Result result; + SharedBuffer buffer = Commands::newAuthResponse(authentication_, result); + if (result != ResultOk) { + LOG_ERROR(cnxString_ << "Failed to send auth response: " << result); + close(result); + break; + } asyncWrite(buffer.const_asio_buffer(), std::bind(&ClientConnection::handleSentAuthResponse, shared_from_this(), std::placeholders::_1, buffer)); @@ -1472,7 +1485,7 @@ void ClientConnection::handleConsumerStatsTimeout(const boost::system::error_cod startConsumerStatsTimer(consumerStatsRequests); } -void ClientConnection::close() { +void ClientConnection::close(Result result) { Lock lock(mutex_); if (isClosed()) { return; @@ -1529,7 +1542,7 @@ void ClientConnection::close() { HandlerBase::handleDisconnection(ResultConnectError, shared_from_this(), it->second); } - connectPromise_.setFailed(ResultConnectError); + connectPromise_.setFailed(result); // Fail all pending requests, all these type are map whose value type contains the Promise object for (auto& kv : pendingRequests) { diff --git a/pulsar-client-cpp/lib/ClientConnection.h b/pulsar-client-cpp/lib/ClientConnection.h index 25fb6a10e028a..48e6d57a0a23c 100644 --- a/pulsar-client-cpp/lib/ClientConnection.h +++ b/pulsar-client-cpp/lib/ClientConnection.h @@ -113,7 +113,7 @@ class PULSAR_PUBLIC ClientConnection : public std::enable_shared_from_thisgetAuthData(authDataContent) == ResultOk && authDataContent->hasDataFromCommand()) { + result = authentication->getAuthData(authDataContent); + if (result != ResultOk) { + return SharedBuffer::allocate(0); + } + + if (authDataContent->hasDataFromCommand()) { connect->set_auth_data(authDataContent->getCommandData()); } return writeMessageWithSize(cmd); } -SharedBuffer Commands::newAuthResponse(const AuthenticationPtr& authentication) { +SharedBuffer Commands::newAuthResponse(const AuthenticationPtr& authentication, Result& result) { BaseCommand cmd; cmd.set_type(BaseCommand::AUTH_RESPONSE); CommandAuthResponse* authResponse = cmd.mutable_authresponse(); @@ -244,7 +249,12 @@ SharedBuffer Commands::newAuthResponse(const AuthenticationPtr& authentication) authData->set_auth_method_name(authentication->getAuthMethodName()); AuthenticationDataPtr authDataContent; - if (authentication->getAuthData(authDataContent) == ResultOk && authDataContent->hasDataFromCommand()) { + result = authentication->getAuthData(authDataContent); + if (result != ResultOk) { + return SharedBuffer::allocate(0); + } + + if (authDataContent->hasDataFromCommand()) { authData->set_auth_data(authDataContent->getCommandData()); } diff --git a/pulsar-client-cpp/lib/Commands.h b/pulsar-client-cpp/lib/Commands.h index 91c218d8f1210..e72057182ff2c 100644 --- a/pulsar-client-cpp/lib/Commands.h +++ b/pulsar-client-cpp/lib/Commands.h @@ -70,9 +70,9 @@ class Commands { const static int checksumSize = 4; static SharedBuffer newConnect(const AuthenticationPtr& authentication, const std::string& logicalAddress, - bool connectingThroughProxy); + bool connectingThroughProxy, Result& result); - static SharedBuffer newAuthResponse(const AuthenticationPtr& authentication); + static SharedBuffer newAuthResponse(const AuthenticationPtr& authentication, Result& result); static SharedBuffer newPartitionMetadataRequest(const std::string& topic, uint64_t requestId); diff --git a/pulsar-client-cpp/lib/HTTPLookupService.cc b/pulsar-client-cpp/lib/HTTPLookupService.cc index 5fb6a6d094fc5..a54a4c1f4f770 100644 --- a/pulsar-client-cpp/lib/HTTPLookupService.cc +++ b/pulsar-client-cpp/lib/HTTPLookupService.cc @@ -190,10 +190,7 @@ Result HTTPLookupService::sendHTTPRequest(const std::string completeUrl, std::st AuthenticationDataPtr authDataContent; Result authResult = authenticationPtr_->getAuthData(authDataContent); if (authResult != ResultOk) { - LOG_ERROR( - "All Authentication methods should have AuthenticationData and return true on getAuthData for " - "url " - << completeUrl); + LOG_ERROR("Failed to getAuthData: " << authResult); curl_easy_cleanup(handle); return authResult; } diff --git a/pulsar-client-cpp/lib/auth/AuthOauth2.cc b/pulsar-client-cpp/lib/auth/AuthOauth2.cc index 580ef6a521c5d..e16b0d3e1f9ee 100644 --- a/pulsar-client-cpp/lib/auth/AuthOauth2.cc +++ b/pulsar-client-cpp/lib/auth/AuthOauth2.cc @@ -370,7 +370,12 @@ const std::string AuthOauth2::getAuthMethodName() const { return "token"; } Result AuthOauth2::getAuthData(AuthenticationDataPtr& authDataContent) { if (cachedTokenPtr_ == nullptr || cachedTokenPtr_->isExpired()) { - cachedTokenPtr_ = CachedTokenPtr(new Oauth2CachedToken(flowPtr_->authenticate())); + try { + cachedTokenPtr_ = CachedTokenPtr(new Oauth2CachedToken(flowPtr_->authenticate())); + } catch (const std::runtime_error& e) { + // The real error logs have already been printed in authenticate() + return ResultAuthenticationError; + } } authDataContent = cachedTokenPtr_->getAuthData(); From 1fc98fe8cb0145c6d9a4040ef543b2d344db63d2 Mon Sep 17 00:00:00 2001 From: Yunze Xu Date: Tue, 12 Oct 2021 13:34:02 +0800 Subject: [PATCH 3/9] Print human readable error message instead of error code --- pulsar-client-cpp/lib/auth/AuthOauth2.cc | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/pulsar-client-cpp/lib/auth/AuthOauth2.cc b/pulsar-client-cpp/lib/auth/AuthOauth2.cc index e16b0d3e1f9ee..58e38c46e61d4 100644 --- a/pulsar-client-cpp/lib/auth/AuthOauth2.cc +++ b/pulsar-client-cpp/lib/auth/AuthOauth2.cc @@ -189,6 +189,9 @@ void ClientCredentialFlow::initialize() { curl_easy_setopt(handle, CURLOPT_SSL_VERIFYPEER, 0L); curl_easy_setopt(handle, CURLOPT_SSL_VERIFYHOST, 0L); + char errorBuffer[CURL_ERROR_SIZE]; + curl_easy_setopt(handle, CURLOPT_ERRORBUFFER, errorBuffer); + // Make get call to server res = curl_easy_perform(handle); @@ -218,8 +221,8 @@ void ClientCredentialFlow::initialize() { } break; default: - LOG_ERROR("Response failed for getting the well-known configuration " << issuerUrl_ - << ". Error Code " << res); + LOG_ERROR("Response failed for getting the well-known configuration " + << issuerUrl_ << ". Error Code " << res << ": " << errorBuffer); break; } // Free header list @@ -254,7 +257,7 @@ Oauth2TokenResultPtr ClientCredentialFlow::authenticate() { if (jsonBody.empty() || tokenEndPoint_.empty()) { return resultPtr; } - LOG_DEBUG("Generate JSON body for ClientCredentialFlow: " << jsonBody); + LOG_INFO("Generate JSON body for ClientCredentialFlow: " << jsonBody); CURL* handle = curl_easy_init(); CURLcode res; @@ -283,6 +286,9 @@ Oauth2TokenResultPtr ClientCredentialFlow::authenticate() { curl_easy_setopt(handle, CURLOPT_POSTFIELDS, jsonBody.c_str()); + char errorBuffer[CURL_ERROR_SIZE]; + curl_easy_setopt(handle, CURLOPT_ERRORBUFFER, errorBuffer); + // Make get call to server res = curl_easy_perform(handle); @@ -321,8 +327,8 @@ Oauth2TokenResultPtr ClientCredentialFlow::authenticate() { } break; default: - LOG_ERROR("Response failed for issuerurl " << issuerUrl_ << ". Error Code " << res - << " passedin: " << jsonBody); + LOG_ERROR("Response failed for issuerurl " << issuerUrl_ << ". ErrorCode " << res << ": " + << errorBuffer << " passedin: " << jsonBody); break; } // Free header list From 8327c1e07fac29a24af358625f0c9b0c580a58f2 Mon Sep 17 00:00:00 2001 From: Yunze Xu Date: Tue, 12 Oct 2021 13:34:40 +0800 Subject: [PATCH 4/9] Revert log level change --- pulsar-client-cpp/lib/auth/AuthOauth2.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pulsar-client-cpp/lib/auth/AuthOauth2.cc b/pulsar-client-cpp/lib/auth/AuthOauth2.cc index 58e38c46e61d4..f91355cb8b030 100644 --- a/pulsar-client-cpp/lib/auth/AuthOauth2.cc +++ b/pulsar-client-cpp/lib/auth/AuthOauth2.cc @@ -257,7 +257,7 @@ Oauth2TokenResultPtr ClientCredentialFlow::authenticate() { if (jsonBody.empty() || tokenEndPoint_.empty()) { return resultPtr; } - LOG_INFO("Generate JSON body for ClientCredentialFlow: " << jsonBody); + LOG_DEBUG("Generate JSON body for ClientCredentialFlow: " << jsonBody); CURL* handle = curl_easy_init(); CURLcode res; From 4a17522c933de04b10a83cfee15b007234890744 Mon Sep 17 00:00:00 2001 From: Yunze Xu Date: Tue, 12 Oct 2021 13:42:08 +0800 Subject: [PATCH 5/9] Handle the case when issuer_url is not set --- pulsar-client-cpp/lib/auth/AuthOauth2.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pulsar-client-cpp/lib/auth/AuthOauth2.cc b/pulsar-client-cpp/lib/auth/AuthOauth2.cc index f91355cb8b030..c5f42f3f86edb 100644 --- a/pulsar-client-cpp/lib/auth/AuthOauth2.cc +++ b/pulsar-client-cpp/lib/auth/AuthOauth2.cc @@ -160,6 +160,10 @@ static size_t curlWriteCallback(void* contents, size_t size, size_t nmemb, void* } void ClientCredentialFlow::initialize() { + if (issuerUrl_.empty()) { + LOG_ERROR("Failed to initialize ClientCredentialFlow: issuer_url is not set"); + return; + } if (!keyFile_.isValid()) { return; } From 52de0656783d981e7bff0d99425fd0125576733a Mon Sep 17 00:00:00 2001 From: Yunze Xu Date: Tue, 12 Oct 2021 16:02:11 +0800 Subject: [PATCH 6/9] Add tests for OAuth2 failure --- pulsar-client-cpp/tests/AuthPluginTest.cc | 46 +++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/pulsar-client-cpp/tests/AuthPluginTest.cc b/pulsar-client-cpp/tests/AuthPluginTest.cc index db448627ccc89..bfe2d5d9f6813 100644 --- a/pulsar-client-cpp/tests/AuthPluginTest.cc +++ b/pulsar-client-cpp/tests/AuthPluginTest.cc @@ -420,3 +420,49 @@ TEST(AuthPluginTest, testOauth2RequestBody) { ClientCredentialFlow flow2(params); ASSERT_EQ(flow2.generateJsonBody(), expectedJson); } + +TEST(AuthPluginTest, testOauth2Failure) { + ParamMap params; + auto addKeyValue = [&](const std::string& key, const std::string& value) { + params[key] = value; + LOG_INFO("Configure \"" << key << "\" to \"" << value << "\""); + }; + + auto createClient = [&]() -> Client { + ClientConfiguration conf; + conf.setAuth(AuthOauth2::create(params)); + return {"pulsar://localhost:6650", conf}; + }; + + const std::string topic = "AuthPluginTest-testOauth2Failure"; + Producer producer; + + // No issuer_url + auto client1 = createClient(); + ASSERT_EQ(client1.createProducer(topic, producer), ResultAuthenticationError); + client1.close(); + + // Invalid issuer_url + addKeyValue("issuer_url", "hello"); + auto client2 = createClient(); + ASSERT_EQ(client2.createProducer(topic, producer), ResultAuthenticationError); + client2.close(); + + addKeyValue("issuer_url", "https://google.com"); + auto client3 = createClient(); + ASSERT_EQ(client3.createProducer(topic, producer), ResultAuthenticationError); + client3.close(); + + // No client id and secret + addKeyValue("issuer_url", "https://dev-kt-aa9ne.us.auth0.com"); + auto client4 = createClient(); + ASSERT_EQ(client4.createProducer(topic, producer), ResultAuthenticationError); + client4.close(); + + // Invalid client_id and client_secret + addKeyValue("client_id", "my_id"); + addKeyValue("client_secret", "my-secret"); + auto client5 = createClient(); + ASSERT_EQ(client5.createProducer(topic, producer), ResultAuthenticationError); + client5.close(); +} From 150b44443136256e5f6beeabe2cd77abb4048142 Mon Sep 17 00:00:00 2001 From: Yunze Xu Date: Tue, 12 Oct 2021 17:09:45 +0800 Subject: [PATCH 7/9] Fix wrong setIdToken --- pulsar-client-cpp/lib/auth/AuthOauth2.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pulsar-client-cpp/lib/auth/AuthOauth2.cc b/pulsar-client-cpp/lib/auth/AuthOauth2.cc index c5f42f3f86edb..1f72becc54257 100644 --- a/pulsar-client-cpp/lib/auth/AuthOauth2.cc +++ b/pulsar-client-cpp/lib/auth/AuthOauth2.cc @@ -316,7 +316,7 @@ Oauth2TokenResultPtr ClientCredentialFlow::authenticate() { resultPtr->setAccessToken(root.get("access_token", "")); resultPtr->setExpiresIn( root.get("expires_in", Oauth2TokenResult::undefined_expiration)); - resultPtr->setIdToken(root.get("refresh_token", "")); + resultPtr->setRefreshToken(root.get("refresh_token", "")); resultPtr->setIdToken(root.get("id_token", "")); if (!resultPtr->getAccessToken().empty()) { From ae2ea9c4bb1ad2d90b8e66c5d8e907a8234312a5 Mon Sep 17 00:00:00 2001 From: Yunze Xu Date: Tue, 12 Oct 2021 18:08:41 +0800 Subject: [PATCH 8/9] Fix testOauth2WrongSecret --- pulsar-client-cpp/tests/AuthPluginTest.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pulsar-client-cpp/tests/AuthPluginTest.cc b/pulsar-client-cpp/tests/AuthPluginTest.cc index bfe2d5d9f6813..fa59dcd1d1165 100644 --- a/pulsar-client-cpp/tests/AuthPluginTest.cc +++ b/pulsar-client-cpp/tests/AuthPluginTest.cc @@ -366,9 +366,7 @@ TEST(AuthPluginTest, testOauth2WrongSecret) { LOG_INFO("PARAMS: " << params); pulsar::AuthenticationPtr auth = pulsar::AuthOauth2::create(params); ASSERT_EQ(auth->getAuthMethodName(), "token"); - - EXPECT_THROW(auth->getAuthData(data), std::runtime_error) - << "Expected fail for wrong secret when to get token from server"; + ASSERT_EQ(auth->getAuthData(data), ResultAuthenticationError); } TEST(AuthPluginTest, testOauth2CredentialFile) { From ee450963239d1be53a1df74b4bf3b31789d54cd9 Mon Sep 17 00:00:00 2001 From: Yunze Xu Date: Wed, 13 Oct 2021 09:26:49 +0800 Subject: [PATCH 9/9] Return default constructed SharedBuffer --- pulsar-client-cpp/lib/Commands.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pulsar-client-cpp/lib/Commands.cc b/pulsar-client-cpp/lib/Commands.cc index e3f5e75fda4ac..54c8c65f0c71b 100644 --- a/pulsar-client-cpp/lib/Commands.cc +++ b/pulsar-client-cpp/lib/Commands.cc @@ -230,7 +230,7 @@ SharedBuffer Commands::newConnect(const AuthenticationPtr& authentication, const AuthenticationDataPtr authDataContent; result = authentication->getAuthData(authDataContent); if (result != ResultOk) { - return SharedBuffer::allocate(0); + return SharedBuffer{}; } if (authDataContent->hasDataFromCommand()) { @@ -251,7 +251,7 @@ SharedBuffer Commands::newAuthResponse(const AuthenticationPtr& authentication, AuthenticationDataPtr authDataContent; result = authentication->getAuthData(authDataContent); if (result != ResultOk) { - return SharedBuffer::allocate(0); + return SharedBuffer{}; } if (authDataContent->hasDataFromCommand()) {