ssl: separate SSLContext#min_version= and #max_version= #849
Conversation
| end | ||
| } | ||
| end | ||
|
|
There was a problem hiding this comment.
The test looks great!
I want to see you add one more test here with the following conditions to test ctx.min_version = nil (respecting MinProtocol = TLSv1.3) and ctx.max_version = OpenSSL::SSL::TLS1_3_VERSION. What do you think?
[ssl_default_sect]
MinProtocol = TLSv1.3
ctx.min_version = nil
ctx.max_version = OpenSSL::SSL::TLS1_3_VERSION
...
assert_equal("TLSv1.3", ssl.ssl_version)
There was a problem hiding this comment.
ctx.min_version = nil will not make it respect the MinProtocol value in the config file, but it will reset the minimum bound to "any version" by calling SSL_CTX_set_min_proto_version(ctx, 0). This PR doesn't change this.
I agree an additional test case would be useful.
There was a problem hiding this comment.
I see. Thanks for your explanation. So, I assume that if we execute the ctx.min_version = nil, the content of the setting MinProtocol = TLSv1.3 is also gone, right?
There was a problem hiding this comment.
I see. Thanks for your explanation. So, I assume that if we execute the
ctx.min_version = nil, the content of the settingMinProtocol = TLSv1.3is also gone, right?
All right. In the additional test, I can see the ctx.max_version = nil (or ctx.min_version = nil) resets the content set by the MaxProtocol = Foo (or MinProtocol = Bar).
There was a problem hiding this comment.
That is correct. Since protocol version negotiation chooses the highest common version, the difference isn't observable. It would negotiate TLS 1.3 whether you have MinProtocol or not.
I just pushed a new commit with updated tests. In the latter block, ctx.max_version = nil negates MaxProtocol = TLSv1.2 in the config to allow TLS 1.3 connection, as opposed to the first one.
There was a problem hiding this comment.
All right. The tests look good to me!
There was a problem hiding this comment.
Thank you for the review!
Make these methods simple wrappers around
SSL_CTX_set_{min,max}_proto_version().
When we introduced these methods in commit 1860394 [1], which went
to v2.1.0, we added a private method to SSLContext that set both the
minimum and maximum protocol versions at the same time. This was to
allow emulating the behavior using SSL options on older OpenSSL versions
that lack SSL_CTX_set_{min,max}_proto_version(). Since we no longer
support OpenSSL 1.0.2, the related code has already been removed.
In OpenSSL 1.1.1 or later, setting the minimum or maximum version to 0
is not equivalent to leaving it unset. Similar to SSL options, which we
avoid overwriting as of commit 00bec0d and commit 77c3db2 [2],
a system-wide configuration file may define a default protocol version
bounds. Setting the minimum version should not unset the maximum
version, and vice versa.
[1] ruby#142
[2] ruby#767
1fc427d to
5766386
Compare
`OpenSSL::SSL::SSLContext#set_minmax_proto_version` was removed in Ruby head/3.5. See: ruby/openssl#849 ruby/ruby@5a14f53
…al (#3615) `OpenSSL::SSL::SSLContext#set_minmax_proto_version` was removed in Ruby head/3.5. See: ruby/openssl#849 ruby/ruby@5a14f53
…al (puma#3615) `OpenSSL::SSL::SSLContext#set_minmax_proto_version` was removed in Ruby head/3.5. See: ruby/openssl#849 ruby/ruby@5a14f53
Make these methods simple wrappers around SSL_CTX_set_{min,max}_proto_version().
When we introduced these methods in commit 1860394 (#142), which went to v2.1.0, we added a private method to SSLContext that set both the minimum and maximum protocol versions at the same time. This was to allow emulating the behavior using SSL options on older OpenSSL versions that lack SSL_CTX_set_{min,max}_proto_version(). Since we no longer support OpenSSL 1.0.2, the related code has already been removed.
In OpenSSL 1.1.1 or later, setting the minimum or maximum version to 0 is not equivalent to leaving it unset. Similar to SSL options, which we avoid overwriting as of commit 00bec0d and commit 77c3db2 (#767), a system-wide configuration file may define a default protocol version bounds. Setting the minimum version should not unset the maximum version, and vice versa.