-
Notifications
You must be signed in to change notification settings - Fork 135
Add SSL caching share for curl #6537
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for sharing SSL session caching with libcurl by introducing a new SSL share handle and an option to disable built-in SSL caching.
- Adds a new member (m_sslShareHandle) in CurlConnection to manage the SSL share context.
- Introduces a new helper function SetLibcurlShareOption in curl.cpp and updates the CurlConnection constructor to initialize and apply the SSL share handle.
- Updates the transport options to include a Boolean flag (DisableCurlSslCaching) to let consumers disable built-in SSL caching.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
sdk/core/azure-core/src/http/curl/curl_connection_private.hpp | Added m_sslShareHandle member to hold the SSL share handle for curl. |
sdk/core/azure-core/src/http/curl/curl.cpp | Introduced SetLibcurlShareOption and integrated SSL share initialization and configuration in the CurlConnection constructor. |
sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp | Added DisableCurlSslCaching flag to the transport options to control SSL caching behavior. |
Thank you for your contribution @sushshring! We will review the pull request and get back to you soon. |
API change check APIView has identified API level changes in this PR and created following API reviews. |
Ready for review. Currently only seeing the CheckFailedCrlValidation, which is also failing on master so doesn't seem related to this change. |
|
I will rebase main and run CI again |
This enables a new CURL feature by default, however I'm not seeing any evidence of why we need to enable this feature by default. Could you please explain in more detail why we need to enable this feature? In particular, why are we not seeing this with Azure services. Remember that the Azure CURL transport is not a general-purpose HTTP transport but instead is intended to be used to communicate with Azure services. |
I would recommend to also add CHANGELOG.md entry in this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me, the test coverage you add in this PR looks adequate, so I think it is ok to drop the expected coverage 1% (it is hard and probably to excessive to simulate errors being returned from the curl library itself).
The coverage precentages are defined in ci.yml file in the corresponding directory.
@LarryOsterman We noticed the error while making a call to the Azure Attestation SDK. Since Azure SDK for CPP uses Curl version 8.10.1, the bug isn't noticed. Switching to a newer version of curl make this issue start to repro. For reference, CURL used to have SSL caching enabled by default already, but starting with version 8.12.0, they seem to be requiring the SSL cache location to be set by the caller using curl_options. The implementation in this PR keeps the behavior by default to what it was before version 8.12. For the purposes of this issue and PR, I wanted to narrow down the scope to the root-cause of the bug, which is why I just mentioned that making a request to any HTTPS URL reproes the bug. Let me know if you'd like me to include a broader scope in either the PR description or the issue and I can update it that way. |
@antkmsft For the changelog it seems it needs a new version number. Should I just add it under 1.16.0-beta.1? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sushshring, yes, please add under the latest unreleased, i.e. 1.16.0-beta.1.
This probably fits more under a new feature and not a bug fix?
So, something like this, I think:
### Features Added
- [[#6535]](https://github.com/Azure/azure-sdk-for-cpp/issues/6535) Enable SSL caching for libcurl transport by default, which is backwards compatible behavior with older libcurl versions, so using the default settings won't result in transport error when using libcurl >= 8.12. The option is controlled by `CurlTransportOptions::EnableCurlSslCaching`, and is on by default.
I think there are reasons to look at it as a bugfix for curl >= 8.12 as well, I'd leave it up to you, but I think of it as a new feature also because we're adding new API (new option).
Just think of that section as our users will read it to get a sense of what is new, and whether they want to upgrade, and also if they have been planning to use newer libcurl version, or have tried to use it in the past, and failed, so they rolled back without filing a bug, and this section will give them an idea that the thing they've seen in the past is fexed now.
I am approving the PR, but I expect my new feedback to be addressed (fixed/replied to if it is not needed), as well as the changelog section added, but in general what I see looks good to me. Thank you!
… validating non-default codepath
This change fixes #6535. In CURL version 8.12, they removed the option to use the default ssl cache, instead the cache share needs to be set on the easy/multi handle. See commits: disable ssl->state and add magic checks. While I couldn't find any docs for the breaking change, adding a call to
curl_easy_setopt
with a created ssl_share fixes the exception.Pull Request Checklist
Please leverage this checklist as a reminder to address commonly occurring feedback when submitting a pull request to make sure your PR can be reviewed quickly:
See the detailed list in the contributing guide.