-
-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
Fix a few issues with the C generator (part 5) #20313
Conversation
@@ -447,8 +447,8 @@ void apiClient_invoke(apiClient_t *apiClient, | |||
curl_easy_setopt(handle, CURLOPT_SSL_VERIFYHOST, 2L); | |||
} | |||
} else { | |||
curl_easy_setopt(handle, CURLOPT_SSL_VERIFYPEER, 0L); | |||
curl_easy_setopt(handle, CURLOPT_SSL_VERIFYHOST, 0L); | |||
curl_easy_setopt(handle, CURLOPT_SSL_VERIFYPEER, 1L); |
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'm curious if apiClient->sslConfig == NULL
, where is the SSL configuraion for verification ?
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.
That's up to curl. According to their docs about the certificate authority here:
This option is by default set to the system path where libcurl's CA certificate bundle is assumed to be stored, as established at build time.
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.
You can see the value if you run curl-config --ca
. In my system it's /etc/ssl/certs/ca-certificates.crt
.
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 see. and agree with this change now.
User can set apiClient->sslConfig->insecureSkipTlsVerify
if needed.
Excellent, thank you both. I just sent a new pull request here: #20332. |
One more patch from the original pull request at #14379. I'm sending this one on its own because it was controversial the last time (@ityuhui). At my company we need the C api to verify ssl certificates by default. We already apply this patch ourselves, but I think a safer default like this should be better for everyone. Of course I'll appreciate any feedback on the matter.
@wing328 @ityuhui @zhemant @michelealbano