Skip to content

Conversation

@nishantactive
Copy link
Contributor

Current implementation do not give option to select a specific SSL version to use during communication. Sometime, servers have requirement of a specific SSL version to use and it rejects connection initated by open-uri.

Added an optional option called ssl_version when correctly specified as one of the values returned by this method OpenSSL::SSL::SSLContext::METHODS, open method uses this ssl version to establish connection.

…cation. Allow versions are OpenSSL::SSL::SSLContext::METHODS
@nishantactive nishantactive changed the title feat: allow option to pass user specified SSL / TLS to use during communication feat: allow option to pass user specified SSL version to use during communication Feb 5, 2022
@nishantactive nishantactive deleted the ssl-version-option branch February 8, 2022 14:36
@nishantactive nishantactive restored the ssl-version-option branch February 8, 2022 14:36
@nishantactive nishantactive reopened this Feb 8, 2022
@olleolleolle
Copy link
Contributor

Hi, thanks for making a PR! Is it possible to add a test which exercises the setting? (E.g. the "silent no action when given bad data" behaviour, could be covered by a test.)

@nishantactive nishantactive deleted the ssl-version-option branch February 16, 2022 06:05
@nishantactive nishantactive restored the ssl-version-option branch February 16, 2022 06:06
@nishantactive nishantactive reopened this Feb 16, 2022
@nishantactive
Copy link
Contributor Author

Thank you. Have added suggested test case.

@hsbt hsbt merged commit e01becf into ruby:master Oct 6, 2022
@hsbt
Copy link
Member

hsbt commented Oct 6, 2022

👍 Sorry to late response. It seems reasonable.

@rhenium
Copy link
Member

rhenium commented Oct 7, 2022

I'm sorry I wasn't following this PR, but I'm against this change. It should be accessors for OpenSSL::SSL::SSLContext#{min,max}_version= and not for #ssl_version= because

  • In typical situations one wants to restrict SSL/TLS protocol versions to a specific range (e.g., TLS 1.2 or later) rather than to a single version.
  • OpenSSL::SSL::SSLContext#ssl_version= is obsolete. Actually, running the test suite would show a warning because OpenSSL::SSL::SSLContext::METHODS is a deprecated constant.

I will come up with a PR to update it to use OpenSSL::SSL::SSLContext#{min,max}_version=.

http.use_ssl = true
http.verify_mode = options[:ssl_verify_mode] || OpenSSL::SSL::VERIFY_PEER
http.ssl_version = options[:ssl_version] if options[:ssl_version] &&
OpenSSL::SSL::SSLContext::METHODS.include?(options[:ssl_version])
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason why it ignores an invalid value?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants