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.

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

openssl: if verifypeer is not requested, skip the CA loading #7892

Closed
wants to merge 1 commit into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Oct 22, 2021

It was previously done mostly to show a match/non-match in the verbose
output even when verification was not asked for. This change skips the
loading of the CA certs unless verifypeer is set to save memory and CPU.

It was previously done mostly to show a match/non-match in the verbose
output even when verification was not asked for. This change skips the
loading of the CA certs unless verifypeer is set to save memory and CPU.
@bagder bagder added the TLS label Oct 22, 2021
@bagder bagder closed this in 83393b1 Oct 22, 2021
@bagder bagder deleted the bagder/openssl-verify-skip branch Oct 22, 2021
@sjadhavar
Copy link

@sjadhavar sjadhavar commented Mar 16, 2022

This commit is causing the problem in our application that uses libcurl.

Please see the old comment for more details on this.
#2290 (comment)

IMO, we should revert this commit.

@bagder
Copy link
Member Author

@bagder bagder commented Mar 16, 2022

I think a start would be to file an issue explaining the problem.

@pschlan
Copy link
Contributor

@pschlan pschlan commented May 30, 2022

Interesting (and glad) to see this has been re-added after my previous commit has been reverted. After my PR in #2290, I've added a note in https://github.com/pschlan/curl/blob/master/docs/libcurl/opts/CURLOPT_SSL_VERIFYPEER.3#L61-L68 to document the expensive loading of the ca file despite VERIFYPEER being disabled. Should we remove this note now?

@bagder
Copy link
Member Author

@bagder bagder commented May 30, 2022

Oh nice catch @pschlan, yes we should. You wanna do it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants