Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support of setting CAPath and relevant test #4982

Merged
merged 2 commits into from
Sep 29, 2023
Merged

Add support of setting CAPath and relevant test #4982

merged 2 commits into from
Sep 29, 2023

Conversation

phoebusm
Copy link
Contributor

@phoebusm phoebusm commented Sep 22, 2023 •
edited
Loading

#4983

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.

  • C++ Guidelines
  • Doxygen docs
  • Unit tests
  • No unwanted commits/changes
  • Descriptive title/description
    • PR is single purpose
    • Related issue listed
  • Comments in source
  • No typos
  • Update changelog
  • Not work-in-progress
  • External references or docs updated
  • Self review of PR done
  • Any breaking changes?

@github-actions github-actions bot added Azure.Core Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Sep 22, 2023
@github-actions
Copy link

Thank you for your contribution @phoebusm! We will review the pull request and get back to you soon.

@phoebusm
Copy link
Contributor Author

@microsoft-github-policy-service agree company="Man Group"

LarryOsterman
LarryOsterman requested changes Sep 26, 2023
View reviewed changes
Copy link
Member

@LarryOsterman LarryOsterman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, the changes look good to me, I still have a few nits though.

Thank you so much for the hard work you've put into this PR.

sdk/core/azure-core/src/http/curl/curl.cpp Outdated Show resolved Hide resolved
sdk/core/azure-core/test/ut/curl_options_test.cpp Show resolved Hide resolved
@phoebusm phoebusm force-pushed the feaure/add_capath_setting_support branch from cd41297 to 89b9b3b Compare September 26, 2023 17:23
@antkmsft
Copy link
Member

antkmsft commented Sep 26, 2023 •
edited
Loading

Documentation (https://curl.se/libcurl/c/CURLOPT_CAPATH.html) says it is not supported on Windows.
But we compile it as if it is only supported on Linux, is that the intent?
I.e. it is not supported on macOS as well, correct?
Otherwise, the conditions should be #if !defined(AZ_PLATFORM_WINDOWS) instead of #if defined(AZ_PLATFORM_LINUX).

@phoebusm
Copy link
Contributor Author

phoebusm commented Sep 28, 2023 •
edited
Loading

@antkmsft Yes only Linux is support.
libcurl on Mac is backed by nss, which does not support CAPATH. CURLE_NOT_BUILT_IN will be returned runtime if trying to do so.

@phoebusm phoebusm force-pushed the feaure/add_capath_setting_support branch from c27f923 to a206ea2 Compare September 28, 2023 14:53
@phoebusm phoebusm force-pushed the feaure/add_capath_setting_support branch from a206ea2 to 236232e Compare September 28, 2023 15:52
@phoebusm phoebusm mentioned this pull request Sep 28, 2023
Merged
5 tasks
@phoebusm
Copy link
Contributor Author

It seems discussions are settled and comments are addressed as well.
I will appreciate an approval if it looks good to the team. :)

@LarryOsterman
Copy link
Member

@phoebusm Thank you so very much for your contributions. I've signed off on the PR, it looks good to me.

@LarryOsterman LarryOsterman merged commit bf652dc into Azure:main Sep 29, 2023
39 checks passed
@phoebusm
Copy link
Contributor Author

Thank you everyone for the review and the merge!

@LarryOsterman
Copy link
Member

FYI, this change should be available on vcpkg at some time next week with the October release of the Azure SDK.

@antkmsft antkmsft mentioned this pull request Sep 29, 2023
Merged
jjerphan added a commit to jjerphan/azure-core-cpp-feedstock that referenced this pull request Oct 2, 2023
@jjerphan @phoebusm
Backport Azure/azure-sdk-for-cpp#4982
8bd6725 
Signed-off-by: Julien Jerphanion <[email protected]>
Co-authored-by: Phoebus Mak <[email protected]>
@jjerphan jjerphan mentioned this pull request Oct 2, 2023
Closed
5 tasks
@ahsonkhan
Copy link
Member

@phoebusm could you please share some more details on the scenario where you need this capability? What are you trying to do in your application, and which service are you trying to talk to?
That context will be quite helpful in understanding the broader requirements, which motivated adding this setting.

@antkmsft
Copy link
Member

antkmsft commented Oct 2, 2023

To hopefully clarify, we are planning to release this change as 1.11.0-beta.1 on Thursday, October 5th.
Please note, since this goes to a beta release, and the beta release is for the library already had stable releases, the further Betas do not go to the official vcpkg repo. Usually, the beta changes are baked for a month until they are released in stable release. If you are looking for this change to appear in the vcpkg official repo, you'll probably need to wait until, I'd say, November 10th.
Otherwise, this change will be available in our Beta vcpkg registry - https://github.com/Azure/azure-sdk-vcpkg-betas - it should happen on October 5th.

ahsonkhan
ahsonkhan reviewed Oct 3, 2023
View reviewed changes
sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp
Comment on lines +19 to +21
#if OPENSSL_VERSION_NUMBER >= 0x00905100L
#define _azure_SUPPORT_SETTING_CAPATH
#endif // OPENSSL_VERSION_NUMBER >= 0x00905100L
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure we need this openssl version check.

0x00905100L seems to correspond to 0.9.5 which is 20+ years old. We can assume the version of openssl is 1.1.1 or higher, since we don't support older versions anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which means we can remove the _azure_SUPPORT_SETTING_CAPATH and assume this option is available wherever (AZ_PLATFORM_LINUX) is defined.

@ahsonkhan ahsonkhan mentioned this pull request Oct 5, 2023
Merged
@phoebusm
Copy link
Contributor Author

Hi I have noticed that azure-core_1.11.0, which has my patch, doesn't get released on 10th Nov.
Would you kindly advise when will it be done? Thank you!

@gforsyth gforsyth mentioned this pull request Nov 13, 2023
Closed
@phoebusm
Copy link
Contributor Author

Hi @ahsonkhan @antkmsft @LarryOsterman would you please kindly advise when will this patch be released?

@LarryOsterman
Copy link
Member

We are hoping to release it in a GA release today, January 11, 2024.

@teo-tsirpanis teo-tsirpanis mentioned this pull request Feb 5, 2024
Closed
3 tasks
@daviewales daviewales mentioned this pull request Feb 15, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahsonkhan ahsonkhan ahsonkhan left review comments

@LarryOsterman LarryOsterman LarryOsterman approved these changes

@antkmsft antkmsft antkmsft approved these changes

@RickWinter RickWinter Awaiting requested review from RickWinter RickWinter is a code owner

@vhvb1989 vhvb1989 Awaiting requested review from vhvb1989

@gearama gearama Awaiting requested review from gearama gearama is a code owner

Assignees
No one assigned
Labels
Azure.Core Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@phoebusm @antkmsft @LarryOsterman @ahsonkhan