Azure / azure-storage-cpp

Microsoft Azure Storage Client Library for C++
http://azure.github.io/azure-storage-cpp
Apache License 2.0
131 stars 147 forks source link

Re-using http connection with custom ssl callback / custom certificate validation #384

Open plougue opened 3 years ago

plougue commented 3 years ago

Hi !

I'm trying to work within a context in which I need to customize certificate validation. For this I use operation_context::set_ssl_context_callback to work directly with the boost::asio::ssl::context object and it works.

However it seems like it's impacting performances by preventing existing connections from being reused. Specifically, it looks like it's because the key used to retrieve a connection is using an std::function object's address : https://github.com/Azure/azure-storage-cpp/blob/5b1c1597e6a637bc205d91342054814c8d901777/Microsoft.WindowsAzure.Storage/src/util.cpp#L487 This std::function has been copied from my context into the http::config object right here https://github.com/Azure/azure-storage-cpp/blob/5b1c1597e6a637bc205d91342054814c8d901777/Microsoft.WindowsAzure.Storage/src/executor.cpp#L149 so the address shouldn't be the same over 2 calls. I've also noticed that in some cases the std::function object inside the http::config object is allocated at the same address over and over again so it is actually the same but there is no real guarantee for it.

Would it be possible to allow consistently reusing the same connection when setting an ssl_context ?

Thanks !

Jinming-Hu commented 3 years ago

Hi @plougue , how did you copy the operation_context among multiple API calls? If you use the same operation_context, the address of the callback object should be the same.

plougue commented 3 years ago

Hello ! My operation_context is created once and held by the object in which the various API calls are made. It is not copied.

However on API calls, the callback object seems to be copied from the operation_context object to a new cpprest http::config object in the sdk's execute_async() function here : https://github.com/Azure/azure-storage-cpp/blob/master/Microsoft.WindowsAzure.Storage/src/executor.cpp#L149

Because cpprest's set_ssl_context_callback actually copies the std::function object into its m_ssl_context_callback member : https://github.com/microsoft/cpprestsdk/blob/master/Release/include/cpprest/http_client.h#L371

The key for retrieving the existing connection is made using the address of the std::function inside the http::config object but from what I understand it has been internally copied at this point

Jinming-Hu commented 3 years ago

Hello ! My operation_context is created once and held by the object in which the various API calls are made. It is not copied.

However on API calls, the callback object seems to be copied from the operation_context object to a new cpprest http::config object in the sdk's execute_async() function here : https://github.com/Azure/azure-storage-cpp/blob/master/Microsoft.WindowsAzure.Storage/src/executor.cpp#L149

Because cpprest's set_ssl_context_callback actually copies the std::function object into its m_ssl_context_callback member : https://github.com/microsoft/cpprestsdk/blob/master/Release/include/cpprest/http_client.h#L371

The key for retrieving the existing connection is made using the address of the std::function inside the http::config object but from what I understand it has been internally copied at this point

Yeah, I got it.

Jinming-Hu commented 3 years ago

But how can we fix this issue? I'm thinking maybe adding another parameter callback_address to get_http_client() function, and passing the address of the function object in operation_context instead of the copied one.

plougue commented 3 years ago

Hello again. I think the solution you suggests works. An alternative could also be to reuse the same connection without testing that the ssl_context_callback is the same since I believe there are other callbacks that are not compared anyway (ssl_native_session_handle_options_callback and such). But if you want to keep the test I'd say what you suggest is the way to go :)