From ef9d2a5e1858ede21708babdf8091e5bc6c0ac00 Mon Sep 17 00:00:00 2001 From: Anton Kolesnyk <41349689+antkmsft@users.noreply.github.com> Date: Fri, 7 Jun 2024 03:07:10 -0700 Subject: [PATCH] Remove 'include windows.h' from inc/* (#5691) * Remove 'include windows.h' from inc/* * Clang format * Qualify namespace * Clean up includes a bit further * Fix errors * Clang format * move out _detail definitions --------- Co-authored-by: Anton Kolesnyk --- .../inc/azure/core/http/curl_transport.hpp | 8 +- .../inc/azure/core/http/transport.hpp | 3 + .../azure/core/http/win_http_transport.hpp | 290 ++++++++---------- .../inc/azure/core/internal/unique_handle.hpp | 18 ++ .../src/http/winhttp/win_http_request.hpp | 10 +- .../src/http/winhttp/win_http_transport.cpp | 109 ++++--- 6 files changed, 210 insertions(+), 228 deletions(-) diff --git a/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp b/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp index e02bd6491..d1780a5e1 100644 --- a/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp +++ b/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp @@ -8,11 +8,13 @@ #pragma once -#include "azure/core/context.hpp" -#include "azure/core/http/http.hpp" #include "azure/core/http/policies/policy.hpp" #include "azure/core/http/transport.hpp" -#include "azure/core/platform.hpp" +#include "azure/core/nullable.hpp" + +#include +#include +#include namespace Azure { namespace Core { namespace Http { class CurlNetworkConnection; diff --git a/sdk/core/azure-core/inc/azure/core/http/transport.hpp b/sdk/core/azure-core/inc/azure/core/http/transport.hpp index d738aba01..95c01906b 100644 --- a/sdk/core/azure-core/inc/azure/core/http/transport.hpp +++ b/sdk/core/azure-core/inc/azure/core/http/transport.hpp @@ -10,6 +10,9 @@ #include "azure/core/context.hpp" #include "azure/core/http/http.hpp" +#include "azure/core/http/raw_response.hpp" + +#include namespace Azure { namespace Core { namespace Http { diff --git a/sdk/core/azure-core/inc/azure/core/http/win_http_transport.hpp b/sdk/core/azure-core/inc/azure/core/http/win_http_transport.hpp index f57962c08..8f2e3e75f 100644 --- a/sdk/core/azure-core/inc/azure/core/http/win_http_transport.hpp +++ b/sdk/core/azure-core/inc/azure/core/http/win_http_transport.hpp @@ -13,183 +13,141 @@ #include "azure/core/http/policies/policy.hpp" #include "azure/core/http/transport.hpp" #include "azure/core/internal/unique_handle.hpp" -#include "azure/core/platform.hpp" - -#if defined(AZ_PLATFORM_WINDOWS) -#if !defined(WIN32_LEAN_AND_MEAN) -#define WIN32_LEAN_AND_MEAN -#endif -#if !defined(NOMINMAX) -#define NOMINMAX -#endif -#include -#endif +#include "azure/core/nullable.hpp" +#include "azure/core/url.hpp" #include -#include +#include #include -#include -#include -namespace Azure { namespace Core { +namespace Azure { namespace Core { namespace Http { namespace _detail { + class WinHttpRequest; + } + + /** + * @brief Sets the WinHTTP session and connection options used to customize the behavior of the + * transport. + */ + struct WinHttpTransportOptions final + { /** - * @brief Unique handle for WinHTTP HINTERNET handles. + * @brief When `true`, allows an invalid certificate authority. + */ + bool IgnoreUnknownCertificateAuthority{false}; + + /** + * @brief When `true`, allows an invalid common name in a certificate. + */ + bool IgnoreInvalidCertificateCommonName{false}; + + /** + * Proxy information. + */ + + /** + * @brief If True, enables the use of the system default proxy. * - * @note HINTERNET is declared as a "void *". This means that this definition subsumes all other - * `void *` types when used with Azure::Core::_internal::UniqueHandle. + * @remarks Set this to "true" if you would like to use a local HTTP proxy like "Fiddler" to + * capture and analyze HTTP traffic. + * + * Set to "false" by default because it is not recommended to use a proxy for production and + * Fiddler's proxy interferes with the HTTP functional tests. + */ + bool EnableSystemDefaultProxy{false}; + + /** + * @brief If True, enables checks for certificate revocation. + */ + bool EnableCertificateRevocationListCheck{false}; + + /** + * @brief Proxy information. + * + * @remark The Proxy Information string is composed of a set of elements + * formatted as follows: + * + * @code + * (\[\=\]\[\"://"\]\\[":"\\]) + * @endcode + * + * Each element should be separated with semicolons or whitespace. + */ + std::string ProxyInformation; + + /** + * @brief User name for proxy authentication. + */ + Azure::Nullable ProxyUserName; + + /** + * @brief Password for proxy authentication. + */ + Azure::Nullable ProxyPassword; + + /** + * @brief Array of Base64 encoded DER encoded X.509 certificate. These certificates should + * form a chain of certificates which will be used to validate the server certificate sent by + * the server. + */ + std::vector ExpectedTlsRootCertificates; + }; + + /** + * @brief Concrete implementation of an HTTP transport that uses WinHTTP when sending and + * receiving requests and responses over the wire. + */ + class WinHttpTransport : public HttpTransport { + private: + WinHttpTransportOptions m_options; + // m_sessionhandle is const to ensure immutability. + const Azure::Core::_internal::UniqueHandle m_sessionHandle; + + Azure::Core::_internal::UniqueHandle CreateSessionHandle(); + Azure::Core::_internal::UniqueHandle CreateConnectionHandle( + Azure::Core::Url const& url, + Azure::Core::Context const& context); + + std::unique_ptr<_detail::WinHttpRequest> CreateRequestHandle( + Azure::Core::_internal::UniqueHandle const& connectionHandle, + Azure::Core::Url const& url, + Azure::Core::Http::HttpMethod const& method); + + // Callback to allow a derived transport to extract the request handle. Used for WebSocket + // transports. + virtual void OnUpgradedConnection(std::unique_ptr<_detail::WinHttpRequest> const&){}; + + public: + /** + * @brief Constructs `%WinHttpTransport`. + * + * @param options Optional parameter to override the default settings. + */ + WinHttpTransport(WinHttpTransportOptions const& options = WinHttpTransportOptions()); + + /** + * @brief Constructs `%WinHttpTransport`. + * + * @param options Optional parameter to override the default settings. + */ + /** + * @brief Constructs `%WinHttpTransport` object based on common Azure HTTP Transport Options * */ - template <> struct UniqueHandleHelper - { - static void FreeHandle(HINTERNET obj) { WinHttpCloseHandle(obj); } - - using type = _internal::BasicUniqueHandle; - }; - } // namespace _detail - - namespace Http { - - namespace _detail { - - constexpr static size_t DefaultUploadChunkSize = 1024 * 64; - constexpr static size_t MaximumUploadChunkSize = 1024 * 1024; - - // Forward declaration for WinHttpRequest. - class WinHttpRequest; - } // namespace _detail + WinHttpTransport(Azure::Core::Http::Policies::TransportOptions const& options); /** - * @brief Sets the WinHTTP session and connection options used to customize the behavior of the - * transport. + * @brief Implements the HTTP transport interface to send an HTTP Request and produce an + * HTTP RawResponse. + * */ - struct WinHttpTransportOptions final - { - /** - * @brief When `true`, allows an invalid certificate authority. - */ - bool IgnoreUnknownCertificateAuthority{false}; + virtual std::unique_ptr Send(Request& request, Context const& context) override; - /** - * @brief When `true`, allows an invalid common name in a certificate. - */ - bool IgnoreInvalidCertificateCommonName{false}; + // See also: + // [Core Guidelines C.35: "A base class destructor should be either public + // and virtual or protected and + // non-virtual"](http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c35-a-base-class-destructor-should-be-either-public-and-virtual-or-protected-and-non-virtual) + virtual ~WinHttpTransport(); + }; - /** - * Proxy information. - */ - - /** - * @brief If True, enables the use of the system default proxy. - * - * @remarks Set this to "true" if you would like to use a local HTTP proxy like "Fiddler" to - * capture and analyze HTTP traffic. - * - * Set to "false" by default because it is not recommended to use a proxy for production and - * Fiddler's proxy interferes with the HTTP functional tests. - */ - bool EnableSystemDefaultProxy{false}; - - /** - * @brief If True, enables checks for certificate revocation. - */ - bool EnableCertificateRevocationListCheck{false}; - - /** - * @brief Proxy information. - * - * @remark The Proxy Information string is composed of a set of elements - * formatted as follows: - * - * @code - * (\[\=\]\[\"://"\]\\[":"\\]) - * @endcode - * - * Each element should be separated with semicolons or whitespace. - */ - std::string ProxyInformation; - - /** - * @brief User name for proxy authentication. - */ - Azure::Nullable ProxyUserName; - - /** - * @brief Password for proxy authentication. - */ - Azure::Nullable ProxyPassword; - - /** - * @brief Array of Base64 encoded DER encoded X.509 certificate. These certificates should - * form a chain of certificates which will be used to validate the server certificate sent by - * the server. - */ - std::vector ExpectedTlsRootCertificates; - }; - - /** - * @brief Concrete implementation of an HTTP transport that uses WinHTTP when sending and - * receiving requests and responses over the wire. - */ - class WinHttpTransport : public HttpTransport { - private: - WinHttpTransportOptions m_options; - // m_sessionhandle is const to ensure immutability. - const Azure::Core::_internal::UniqueHandle m_sessionHandle; - - Azure::Core::_internal::UniqueHandle CreateSessionHandle(); - Azure::Core::_internal::UniqueHandle CreateConnectionHandle( - Azure::Core::Url const& url, - Azure::Core::Context const& context); - std::unique_ptr<_detail::WinHttpRequest> CreateRequestHandle( - Azure::Core::_internal::UniqueHandle const& connectionHandle, - Azure::Core::Url const& url, - Azure::Core::Http::HttpMethod const& method); - - // Callback to allow a derived transport to extract the request handle. Used for WebSocket - // transports. - virtual void OnUpgradedConnection(std::unique_ptr<_detail::WinHttpRequest> const&){}; - - /** - * @brief Throw an exception based on the Win32 Error code - * - * @param exceptionMessage Message describing error. - * @param error Win32 Error code. - */ - void GetErrorAndThrow(const std::string& exceptionMessage, DWORD error = GetLastError()); - - public: - /** - * @brief Constructs `%WinHttpTransport`. - * - * @param options Optional parameter to override the default settings. - */ - WinHttpTransport(WinHttpTransportOptions const& options = WinHttpTransportOptions()); - - /** - * @brief Constructs `%WinHttpTransport`. - * - * @param options Optional parameter to override the default settings. - */ - /** - * @brief Constructs `%WinHttpTransport` object based on common Azure HTTP Transport Options - * - */ - WinHttpTransport(Azure::Core::Http::Policies::TransportOptions const& options); - - /** - * @brief Implements the HTTP transport interface to send an HTTP Request and produce an - * HTTP RawResponse. - * - */ - virtual std::unique_ptr Send(Request& request, Context const& context) override; - - // See also: - // [Core Guidelines C.35: "A base class destructor should be either public - // and virtual or protected and - // non-virtual"](http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c35-a-base-class-destructor-should-be-either-public-and-virtual-or-protected-and-non-virtual) - virtual ~WinHttpTransport(); - }; - - } // namespace Http -}} // namespace Azure::Core +}}} // namespace Azure::Core::Http diff --git a/sdk/core/azure-core/inc/azure/core/internal/unique_handle.hpp b/sdk/core/azure-core/inc/azure/core/internal/unique_handle.hpp index 27543fcff..941012254 100644 --- a/sdk/core/azure-core/inc/azure/core/internal/unique_handle.hpp +++ b/sdk/core/azure-core/inc/azure/core/internal/unique_handle.hpp @@ -67,4 +67,22 @@ namespace Azure { namespace Core { template class U = _detail::UniqueHandleHelper> using UniqueHandle = typename U::type; } // namespace _internal + + namespace _detail { + void FreeWinHttpHandleImpl(void* obj); + + /** + * @brief Unique handle for WinHTTP HINTERNET handles. + * + * @note HINTERNET is declared as a "void *". This means that this definition subsumes all other + * `void *` types when used with Azure::Core::_internal::UniqueHandle. + * + */ + template <> struct UniqueHandleHelper + { + static void FreeWinHttpHandle(void* obj) { FreeWinHttpHandleImpl(obj); } + + using type = _internal::BasicUniqueHandle; + }; + } // namespace _detail }} // namespace Azure::Core diff --git a/sdk/core/azure-core/src/http/winhttp/win_http_request.hpp b/sdk/core/azure-core/src/http/winhttp/win_http_request.hpp index a9386ead7..e84fb1083 100644 --- a/sdk/core/azure-core/src/http/winhttp/win_http_request.hpp +++ b/sdk/core/azure-core/src/http/winhttp/win_http_request.hpp @@ -20,7 +20,7 @@ #define NOMINMAX #endif -#include +#include #include #include @@ -30,6 +30,7 @@ // 'GetProcAddress'. #include #pragma warning(pop) +#include #include namespace Azure { namespace Core { namespace Http { namespace _detail { @@ -169,13 +170,6 @@ namespace Azure { namespace Core { namespace Http { namespace _detail { bool VerifyCertificatesInChain( std::vector const& trustedCertificates, PCCERT_CONTEXT serverCertificate) const; - /** - * @brief Throw an exception based on the Win32 Error code - * - * @param exceptionMessage Message describing error. - * @param error Win32 Error code. - */ - void GetErrorAndThrow(const std::string& exceptionMessage, DWORD error = GetLastError()) const; public: WinHttpRequest( diff --git a/sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp b/sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp index 154bb46f3..7172786d0 100644 --- a/sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp +++ b/sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #pragma warning(push) #pragma warning(disable : 6553) #pragma warning(disable : 6387) // An argument in result_macros.h may be '0', for the function @@ -45,6 +46,49 @@ using namespace Azure::Core::Diagnostics::_internal; namespace { +constexpr static const size_t DefaultUploadChunkSize = 1024 * 64; +constexpr static const size_t MaximumUploadChunkSize = 1024 * 1024; + +std::string GetErrorMessage(DWORD error) +{ + std::string errorMessage = " Error Code: " + std::to_string(error); + + char* errorMsg = nullptr; + if (FormatMessageA( + FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_FROM_HMODULE | FORMAT_MESSAGE_ALLOCATE_BUFFER, + GetModuleHandleA("winhttp.dll"), + error, + MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), + reinterpret_cast(&errorMsg), + 0, + nullptr) + != 0) + { + // Use a unique_ptr to manage the lifetime of errorMsg. + std::unique_ptr errorString(errorMsg, &LocalFree); + errorMsg = nullptr; + + errorMessage += ": "; + errorMessage += errorString.get(); + // If the end of the error message is a CRLF, remove it. + if (errorMessage.back() == '\n') + { + errorMessage.erase(errorMessage.size() - 1); + if (errorMessage.back() == '\r') + { + errorMessage.erase(errorMessage.size() - 1); + } + } + } + errorMessage += '.'; + return errorMessage; +} + +void GetErrorAndThrow(const std::string& exceptionMessage, DWORD error = GetLastError()) +{ + throw Azure::Core::Http::TransportException(exceptionMessage + GetErrorMessage(error)); +} + const std::string HttpScheme = "http"; const std::string WebSocketScheme = "ws"; @@ -243,6 +287,18 @@ std::string GetHeadersAsString(Azure::Core::Http::Request const& request) } } // namespace +void Azure::Core::_detail::FreeWinHttpHandleImpl(void* obj) +{ + // If definitions from windows.h are only being used as private members and not a public API, we + // don't want to include windows.h in inc/ headers, so that it does not end up being included in + // customer code. + // Formally, WinHttpCloseHandle() takes HINTERNET, which is LPVOID, which is void*. That is why we + // defined it that way in the header, and here, we are going to static_assert that it is the same + // type and safely cast it to HINTERNET. + static_assert(std::is_same::value, "HINTERNET == void*"); + WinHttpCloseHandle(static_cast(obj)); +} + // For each certificate specified in trustedCertificate, add to certificateStore. bool _detail::WinHttpRequest::AddCertificatesToStore( std::vector const& trustedCertificates, @@ -406,41 +462,6 @@ std::string InternetStatusToString(DWORD internetStatus) return rv; } #undef APPEND_ENUM_STRING - -std::string GetErrorMessage(DWORD error) -{ - std::string errorMessage = " Error Code: " + std::to_string(error); - - char* errorMsg = nullptr; - if (FormatMessage( - FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_FROM_HMODULE | FORMAT_MESSAGE_ALLOCATE_BUFFER, - GetModuleHandle("winhttp.dll"), - error, - MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), - reinterpret_cast(&errorMsg), - 0, - nullptr) - != 0) - { - // Use a unique_ptr to manage the lifetime of errorMsg. - std::unique_ptr errorString(errorMsg, &LocalFree); - errorMsg = nullptr; - - errorMessage += ": "; - errorMessage += errorString.get(); - // If the end of the error message is a CRLF, remove it. - if (errorMessage.back() == '\n') - { - errorMessage.erase(errorMessage.size() - 1); - if (errorMessage.back() == '\r') - { - errorMessage.erase(errorMessage.size() - 1); - } - } - } - errorMessage += '.'; - return errorMessage; -} } // namespace namespace Azure { namespace Core { namespace Http { namespace _detail { @@ -737,22 +758,8 @@ namespace Azure { namespace Core { namespace Http { namespace _detail { } } } - - void WinHttpRequest::GetErrorAndThrow(const std::string& exceptionMessage, DWORD error) const - { - std::string errorMessage = exceptionMessage + GetErrorMessage(error); - - throw Azure::Core::Http::TransportException(errorMessage); - } }}}} // namespace Azure::Core::Http::_detail -void WinHttpTransport::GetErrorAndThrow(const std::string& exceptionMessage, DWORD error) -{ - std::string errorMessage = exceptionMessage + GetErrorMessage(error); - - throw Azure::Core::Http::TransportException(errorMessage); -} - Azure::Core::_internal::UniqueHandle WinHttpTransport::CreateSessionHandle() { // Use WinHttpOpen to obtain a session handle. @@ -1101,8 +1108,8 @@ void _detail::WinHttpRequest::Upload( int64_t streamLength = streamBody->Length(); // Consider using `MaximumUploadChunkSize` here, after some perf measurements - size_t uploadChunkSize = _detail::DefaultUploadChunkSize; - if (streamLength < _detail::MaximumUploadChunkSize) + size_t uploadChunkSize = DefaultUploadChunkSize; + if (streamLength < MaximumUploadChunkSize) { uploadChunkSize = static_cast(streamLength); }