💥 Don't set verify_callback to VerifyCallbackProc
#534
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `VerifyCallbackProc` constant will no longer be automatically
assigned to `context.verify_callback`.
The callback can still be set explicitly:
```
imap = Net::IMAP.new(host, ssl: {verify_callback: VerifyCallbackProc})
imap.starttls(verify_callback: VerifyCallbackProc)
```
Prior to this commit, if `VerifyCallbackProc` were set (it could be any
of `Net::IMAP::VerifyCallbackProc`, `OpenSSL::SSL::VerifyCallbackProc`,
`OpenSSL::VerifyCallbackProc`, `Net::VerifyCallbackProc`, or
`::VerifyCallbackProc`), it would automatically be assigned to
`context.verify_callback`. I can't find any evidence that this constant
has ever been set for `net-imap`, `openssl`, or ruby. But it _is_ also
used by `net-ftp`.
This functionality existed from the beginning of `net-imap`'s TLS
support, but was never documented. I don't know the original motivation
for this. (Perhaps it's still useful for inserting a debug logger?) But
I'm guessing that past motivations for this code are less significant
now. Earlier versions of ruby's `openssl` did not have very secure
defaults, but the modern `openssl` gem already handles the verify
callback just fine.
Collaborator
Author
|
@shugo Any objections to this? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
VerifyCallbackProcconstant will no longer be automatically assigned tocontext.verify_callback.The callback can still be set explicitly:
Prior to this commit, if
VerifyCallbackProcwere set (it could be any ofNet::IMAP::VerifyCallbackProc,OpenSSL::SSL::VerifyCallbackProc,OpenSSL::VerifyCallbackProc,Net::VerifyCallbackProc, or::VerifyCallbackProc), it would automatically be assigned tocontext.verify_callback. I can't find any evidence that this constant has ever been set fornet-imap,openssl, or ruby. But it is also used bynet-ftp.This functionality existed from the beginning of
net-imap's TLS support, but was never documented. I don't know the original motivation for this. (Perhaps it's still useful for inserting a debug logger?) But I'm guessing that past motivations for this code are less significant now. Earlier versions of ruby'sopenssldid not have very secure defaults, but the modernopensslgem already handles the verify callback just fine.