-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add handling of cainfo property for cURL streams #81
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #81 +/- ##
==========================================
- Coverage 65.11% 65.11% -0.01%
==========================================
Files 81 81
Lines 10470 10474 +4
==========================================
+ Hits 6818 6820 +2
- Misses 3652 3654 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Ja, something like that, LGTM. I wonder
But - this can wait for a later PR I guess. Could you add a short note here -> https://github.com/ZEISS/libczi/blob/main/Src/libCZI/Doc/version-history.markdown , then it would be "fine for now for me". |
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.
Please consider renaming the enum to 'kCurlHttp_CaInfo' and adding a note to the version-history.
@@ -228,6 +228,9 @@ namespace libCZI | |||
kCurlHttp_FollowLocation = 108, ///< For CurlHttpInputStream, type bool: a boolean indicating whether redirects are to be followed, c.f. https://curl.se/libcurl/c/CURLOPT_FOLLOWLOCATION.html for more information. | |||
|
|||
kCurlHttp_MaxRedirs = 109, ///< For CurlHttpInputStream, type int32: gives the maximum number of redirects to follow, c.f. https://curl.se/libcurl/c/CURLOPT_MAXREDIRS.html for more information. | |||
|
|||
kCurlCaInfo = 110, ///< For CurlHttpInputStream, type string: gives the directory to check for ca certificate bundle , c.f. https://curl.se/libcurl/c/CURLOPT_CAINFO.html for more information. |
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.
I'd guess this should be named "kCurlHttp_CaInfo" to follow the convention used by the other enum-values.
I think this would fit well into the scope of the PR, sometimes I may have a whole bundle based on the IT of my organization rather than a specific file.
A pass-through here may make sense, but I think we will only ever use a subset.
Yes, absolutely! Perhaps not within the scope of this PR but I think that it would be reasonable to add the option of setting |
Please check #82 - where I tried to extend on this PR. This is supposed to supersede this PR. |
STOP - Read this First!
Reporting a security vulnerability?
Check out the project's security policy.
Fill out and Adjust this Template
Description
Add a flag to the StreamProperties that allows setting the CURLOPT_CAINFO option for cURL based streams to a custom location.
The reason for this is that libczi is being consumed by pylibCZIrw and built for multiple different platforms that have their certificates storead at different locations. As a result, it's necessary to determine the location of the certificate bundle at runtime instead of build time.
Fixes # (issue)
Type of change
How Has This Been Tested?
This functionality is going to be used in an upcoming release of pylibCZIrw and will be tested there (either automatically or manually)
Checklist: