diff --git a/.changelog/bugfix-crt-http-client-tls-init-failure.json b/.changelog/bugfix-crt-http-client-tls-init-failure.json new file mode 100644 index 000000000000..00ea85974013 --- /dev/null +++ b/.changelog/bugfix-crt-http-client-tls-init-failure.json @@ -0,0 +1,6 @@ +{ + "type": "bugfix", + "category": "aws-cpp-sdk-core", + "contributor": "", + "description": "CRTHttpClient now refuses every request with a non-retryable error, instead of sending it, when its TLS context or proxy TLS context failed to initialize (for example an unreadable caFile, caPath or proxyCaFile). This also applies when verifySSL is false and to plain http requests." +} diff --git a/src/aws-cpp-sdk-core/source/http/HttpClientFactory.cpp b/src/aws-cpp-sdk-core/source/http/HttpClientFactory.cpp index 615d0de0a3ff..51b7f3bff681 100644 --- a/src/aws-cpp-sdk-core/source/http/HttpClientFactory.cpp +++ b/src/aws-cpp-sdk-core/source/http/HttpClientFactory.cpp @@ -199,7 +199,7 @@ namespace Aws assert(GetHttpClientFactory()); auto client = GetHttpClientFactory()->CreateHttpClient(clientConfiguration); - if (!client) + if (!client || !*client) { AWS_LOGSTREAM_FATAL(HTTP_CLIENT_FACTORY_ALLOCATION_TAG, "Initializing Http Client failed!"); // assert just in case this is a misconfiguration at development time to make the dev's job easier. diff --git a/src/aws-cpp-sdk-core/source/http/crt/CRTHttpClient.cpp b/src/aws-cpp-sdk-core/source/http/crt/CRTHttpClient.cpp index 057b9f5ea807..31fbd0ee3903 100644 --- a/src/aws-cpp-sdk-core/source/http/crt/CRTHttpClient.cpp +++ b/src/aws-cpp-sdk-core/source/http/crt/CRTHttpClient.cpp @@ -25,6 +25,15 @@ static const char *const CRT_HTTP_CLIENT_TAG = "CRTHttpClient"; namespace { +Aws::Client::AWSError MakeCrtClientInitializationError(const Aws::Http::HttpRequest& request) { + AWS_LOGSTREAM_ERROR(CRT_HTTP_CLIENT_TAG, "Refusing to send request to " << request.GetUri().GetAuthority() + << ", the client failed to initialize its TLS context"); + return Aws::Client::AWSError{Aws::Client::CoreErrors::INVALID_PARAMETER_COMBINATION, + "InvalidParameterCombination", + "CRTHttpClient failed to initialize its TLS context, check caFile, caPath and the proxy TLS settings", + false}; +} + void AddRequestMetadataToCrtRequest(const std::shared_ptr& request, const std::shared_ptr& crtRequest) { const char* methodStr = Aws::Http::HttpMethodMapper::GetNameForHttpMethod(request->GetMethod()); @@ -429,6 +438,8 @@ namespace Aws const char* caFile = m_configuration.caFile.empty() ? nullptr : m_configuration.caFile.c_str(); if (!tlsContextOptions.OverrideDefaultTrustStore(caPath, caFile)) { + AWS_LOGSTREAM_ERROR(CRT_HTTP_CLIENT_TAG, "Failed to load the trust store, caPath: \"" << m_configuration.caPath + << "\", caFile: \"" << m_configuration.caFile << "\": " << aws_error_debug_str(aws_last_error())); m_bad = true; return; } @@ -442,6 +453,7 @@ namespace Aws // this may need to be pulled from the client configuration.... if (!tlsContextOptions.SetAlpnList("h2;http/1.1")) { + AWS_LOGSTREAM_ERROR(CRT_HTTP_CLIENT_TAG, "Failed to set the TLS ALPN list: " << aws_error_debug_str(aws_last_error())); m_bad = true; return; } @@ -451,6 +463,8 @@ namespace Aws if (!newContext) { + AWS_LOGSTREAM_ERROR(CRT_HTTP_CLIENT_TAG, "Failed to create the TLS context: " + << aws_error_debug_str(newContext.GetInitializationError())); m_bad = true; return; } @@ -482,6 +496,11 @@ namespace Aws const std::shared_ptr& request, std::function)> onResponseComplete) const { + if (m_bad) + { + return MakeCrtClientInitializationError(*request); + } + auto state = Aws::MakeShared(CRT_HTTP_CLIENT_TAG, request, std::move(onResponseComplete)); auto requestConnOptions = CreateConnectionOptionsForRequest(request); @@ -647,6 +666,8 @@ namespace Aws { auto response = Aws::MakeShared(CRT_HTTP_CLIENT_TAG, request); response->SetResponseCode(Aws::Http::HttpResponseCode::REQUEST_NOT_MADE); + response->SetClientErrorType(error->GetErrorType()); + response->SetClientErrorMessage(error->GetMessage()); onResponse(std::move(response)); } }; @@ -835,18 +856,34 @@ namespace Aws { const char* caPath = clientConfig.proxyCaPath.empty() ? nullptr : clientConfig.proxyCaPath.c_str(); const char* caFile = clientConfig.proxyCaFile.empty() ? nullptr : clientConfig.proxyCaFile.c_str(); - contextOptions.OverrideDefaultTrustStore(caPath, caFile); - } + if (!contextOptions.OverrideDefaultTrustStore(caPath, caFile)) + { + AWS_LOGSTREAM_ERROR(CRT_HTTP_CLIENT_TAG, "Failed to load the proxy trust store, proxyCaPath: \"" << clientConfig.proxyCaPath + << "\", proxyCaFile: \"" << clientConfig.proxyCaFile << "\": " << aws_error_debug_str(aws_last_error())); + m_bad = true; + } + } else if (!m_configuration.caFile.empty() || !m_configuration.caPath.empty()) { const char* caPath = clientConfig.caPath.empty() ? nullptr : clientConfig.caPath.c_str(); const char* caFile = clientConfig.caFile.empty() ? nullptr : clientConfig.caFile.c_str(); - contextOptions.OverrideDefaultTrustStore(caPath, caFile); + if (!contextOptions.OverrideDefaultTrustStore(caPath, caFile)) + { + AWS_LOGSTREAM_ERROR(CRT_HTTP_CLIENT_TAG, "Failed to load the proxy trust store, caPath: \"" << clientConfig.caPath + << "\", caFile: \"" << clientConfig.caFile << "\": " << aws_error_debug_str(aws_last_error())); + m_bad = true; + } } contextOptions.SetVerifyPeer(clientConfig.verifySSL); Crt::Io::TlsContext context = Crt::Io::TlsContext(contextOptions, Crt::Io::TlsMode::CLIENT); - proxyOptions.TlsOptions = context.NewConnectionOptions(); + if (!context) + { + AWS_LOGSTREAM_ERROR(CRT_HTTP_CLIENT_TAG, "Failed to create the proxy TLS context: " + << aws_error_debug_str(context.GetInitializationError())); + m_bad = true; + } + proxyOptions.TlsOptions = context.NewConnectionOptions(); } m_proxyOptions = std::move(proxyOptions); @@ -856,6 +893,11 @@ namespace Aws Aws::Crt::Optional> CRTHttpClient::AcquireConnection( const std::shared_ptr& request, const std::function, int)>& onClientConnectionAvailable) { + if (m_bad) + { + return MakeCrtClientInitializationError(*request); + } + auto requestConnOptions = CreateConnectionOptionsForRequest(request); auto connectionManager = GetWithCreateConnectionManagerForRequest(request, requestConnOptions); diff --git a/src/aws-cpp-sdk-core/source/utils/stream/HttpWriteDataStreamBuf.cpp b/src/aws-cpp-sdk-core/source/utils/stream/HttpWriteDataStreamBuf.cpp index 6833ac214b19..aa7ca372f5ba 100644 --- a/src/aws-cpp-sdk-core/source/utils/stream/HttpWriteDataStreamBuf.cpp +++ b/src/aws-cpp-sdk-core/source/utils/stream/HttpWriteDataStreamBuf.cpp @@ -39,20 +39,24 @@ Aws::Crt::Optional> Aws::Utils::S // Callback captures stack locals by reference. This is safe because // we block on acquireCondition.wait() below, keeping the stack frame // alive until the callback has fired. - m_client->AcquireConnection(request, - [&acquireMutex, &acquireCondition, &connectionAcquired, &error, this]( - std::shared_ptr connection, int errorCode) -> void { - { - std::unique_lock const lock{acquireMutex}; - if (errorCode != AWS_ERROR_SUCCESS) { - error = - Aws::Client::AWSError(Aws::Client::CoreErrors::NETWORK_CONNECTION, false); - } - m_connection = std::move(connection); - connectionAcquired = true; - } - acquireCondition.notify_all(); - }); + auto acquireError = m_client->AcquireConnection(request, + [&acquireMutex, &acquireCondition, &connectionAcquired, &error, this]( + std::shared_ptr connection, int errorCode) -> void { + { + std::unique_lock const lock{acquireMutex}; + if (errorCode != AWS_ERROR_SUCCESS) { + error = Aws::Client::AWSError( + Aws::Client::CoreErrors::NETWORK_CONNECTION, false); + } + m_connection = std::move(connection); + connectionAcquired = true; + } + acquireCondition.notify_all(); + }); + + if (acquireError.has_value()) { + return acquireError; + } std::unique_lock lock{acquireMutex}; acquireCondition.wait(lock, [&connectionAcquired]() -> bool { return connectionAcquired; }); diff --git a/tests/aws-cpp-sdk-core-tests/http/CRTHttpClientTlsInitTest.cpp b/tests/aws-cpp-sdk-core-tests/http/CRTHttpClientTlsInitTest.cpp new file mode 100644 index 000000000000..e1556593df19 --- /dev/null +++ b/tests/aws-cpp-sdk-core-tests/http/CRTHttpClientTlsInitTest.cpp @@ -0,0 +1,143 @@ +/** + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0. + */ + +#if AWS_SDK_USE_CRT_HTTP +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +using namespace Aws::Http; +using namespace Aws::Client; + +namespace { +const char* CRT_TLS_INIT_TEST_TAG = "CRTHttpClientTlsInitTest"; + +class CRTHttpClientTlsInitTest : public Aws::Testing::AwsCppSdkGTestSuite { + protected: + static ClientConfiguration MakeConfig() { + ClientConfiguration config; + config.verifySSL = true; + config.connectTimeoutMs = 1000; + config.requestTimeoutMs = 2000; + return config; + } + + static std::shared_ptr MakeClient(const ClientConfiguration& config) { + return Aws::MakeShared(CRT_TLS_INIT_TEST_TAG, config, *Aws::GetDefaultClientBootstrap()); + } + + static std::shared_ptr MakeClientWithMissingCaFile() { + auto config = MakeConfig(); + config.caFile = "/nonexistent/ca.pem"; + return MakeClient(config); + } + + static std::shared_ptr MakeTestRequest(const char* scheme) { + return CreateHttpRequest(Aws::String(scheme) + "://127.0.0.1:1/x", HttpMethod::HTTP_GET, + Aws::Utils::Stream::DefaultResponseStreamFactoryMethod); + } +}; + +TEST_F(CRTHttpClientTlsInitTest, BadCaFileRefusesRequests) { + auto client = MakeClientWithMissingCaFile(); + EXPECT_FALSE(static_cast(*client)); + + for (const char* scheme : {"https", "http"}) { + auto response = client->MakeRequest(MakeTestRequest(scheme), nullptr, nullptr); + ASSERT_NE(nullptr, response); + EXPECT_TRUE(response->HasClientError()); + EXPECT_EQ(CoreErrors::INVALID_PARAMETER_COMBINATION, response->GetClientErrorType()) << scheme; + } +} + +TEST_F(CRTHttpClientTlsInitTest, BadProxyCaFileRefusesRequests) { + auto config = MakeConfig(); + config.proxyHost = "127.0.0.1"; + config.proxyPort = 1; + config.proxyScheme = Scheme::HTTPS; + config.proxyCaFile = "/nonexistent/proxy-ca.pem"; + auto client = MakeClient(config); + EXPECT_FALSE(static_cast(*client)); + + auto response = client->MakeRequest(MakeTestRequest("https"), nullptr, nullptr); + ASSERT_NE(nullptr, response); + EXPECT_TRUE(response->HasClientError()); + EXPECT_EQ(CoreErrors::INVALID_PARAMETER_COMBINATION, response->GetClientErrorType()); +} + +TEST_F(CRTHttpClientTlsInitTest, BadCaFileAcquireConnectionFails) { + auto client = MakeClientWithMissingCaFile(); + + std::atomic callbackInvoked{false}; + auto error = client->AcquireConnection(MakeTestRequest("https"), + [&callbackInvoked](std::shared_ptr, int) { callbackInvoked = true; }); + ASSERT_TRUE(error.has_value()); + EXPECT_EQ(CoreErrors::INVALID_PARAMETER_COMBINATION, error->GetErrorType()); + EXPECT_FALSE(callbackInvoked.load()); +} + +TEST_F(CRTHttpClientTlsInitTest, BadCaFileWriteStreamInitializeFails) { + std::shared_ptr client = MakeClientWithMissingCaFile(); + + Aws::Utils::Stream::HttpWriteDataStreamBuf streamBuf(client, 1024, 1000); + auto error = streamBuf.Initialize(MakeTestRequest("https")); + ASSERT_TRUE(error.has_value()); + EXPECT_EQ(CoreErrors::INVALID_PARAMETER_COMBINATION, error->GetErrorType()); +} + +TEST_F(CRTHttpClientTlsInitTest, BadCaFileMakeRequestAsyncCompletesWithClientError) { + auto client = MakeClientWithMissingCaFile(); + auto executor = Aws::MakeShared(CRT_TLS_INIT_TEST_TAG, 2); + auto request = MakeTestRequest("https"); + + std::shared_ptr recorded; + int attempts = 0; + Aws::Utils::Threading::Semaphore done(0, 1); + auto error = client->MakeRequestAsync( + [&request, &attempts]() -> std::shared_ptr { + ++attempts; + return request; + }, + [&recorded](std::shared_ptr response) -> HttpClient::AttemptOutcome { + recorded = std::move(response); + return HttpClient::AttemptOutcome{false, std::chrono::milliseconds(0)}; + }, + [&done]() { done.Release(); }, executor); + ASSERT_FALSE(error.has_value()); + done.WaitOne(); + + EXPECT_EQ(1, attempts); + ASSERT_NE(nullptr, recorded); + EXPECT_TRUE(recorded->HasClientError()); + EXPECT_EQ(CoreErrors::INVALID_PARAMETER_COMBINATION, recorded->GetClientErrorType()); + EXPECT_EQ(HttpResponseCode::REQUEST_NOT_MADE, recorded->GetResponseCode()); +} + +TEST_F(CRTHttpClientTlsInitTest, GoodConfigIsNotBad) { + auto client = MakeClient(MakeConfig()); + EXPECT_TRUE(static_cast(*client)); +} + +TEST_F(CRTHttpClientTlsInitTest, VerifySslOffIsNotBad) { + auto config = MakeConfig(); + config.verifySSL = false; + auto client = MakeClient(config); + EXPECT_TRUE(static_cast(*client)); + + auto response = client->MakeRequest(MakeTestRequest("https"), nullptr, nullptr); + ASSERT_NE(nullptr, response); + EXPECT_NE(CoreErrors::INVALID_PARAMETER_COMBINATION, response->GetClientErrorType()); +} +} // namespace +#endif