Skip to content

ssl: separate SSLContext#min_version= and #max_version= #849

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 3, 2025
Merged

ssl: separate SSLContext#min_version= and #max_version= #849

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
146 changes: 89 additions & 57 deletions ext/openssl/ossl_ssl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,61 +96,6 @@ ossl_sslctx_s_alloc(VALUE klass)
return obj;
}

static int
parse_proto_version(VALUE str)
{
int i;
static const struct {
const char *name;
int version;
} map[] = {
{ "SSL2", SSL2_VERSION },
{ "SSL3", SSL3_VERSION },
{ "TLS1", TLS1_VERSION },
{ "TLS1_1", TLS1_1_VERSION },
{ "TLS1_2", TLS1_2_VERSION },
{ "TLS1_3", TLS1_3_VERSION },
};

if (NIL_P(str))
return 0;
if (RB_INTEGER_TYPE_P(str))
return NUM2INT(str);

if (SYMBOL_P(str))
str = rb_sym2str(str);
StringValue(str);
for (i = 0; i < numberof(map); i++)
if (!strncmp(map[i].name, RSTRING_PTR(str), RSTRING_LEN(str)))
return map[i].version;
rb_raise(rb_eArgError, "unrecognized version %+"PRIsVALUE, str);
}

/*
* call-seq:
* ctx.set_minmax_proto_version(min, max) -> nil
*
* Sets the minimum and maximum supported protocol versions. See #min_version=
* and #max_version=.
*/
static VALUE
ossl_sslctx_set_minmax_proto_version(VALUE self, VALUE min_v, VALUE max_v)
{
SSL_CTX *ctx;
int min, max;

GetSSLCTX(self, ctx);
min = parse_proto_version(min_v);
max = parse_proto_version(max_v);

if (!SSL_CTX_set_min_proto_version(ctx, min))
ossl_raise(eSSLError, "SSL_CTX_set_min_proto_version");
if (!SSL_CTX_set_max_proto_version(ctx, max))
ossl_raise(eSSLError, "SSL_CTX_set_max_proto_version");

return Qnil;
}

static VALUE
ossl_call_client_cert_cb(VALUE obj)
{
Expand Down Expand Up @@ -915,6 +860,93 @@ ossl_sslctx_setup(VALUE self)
return Qtrue;
}

static int
parse_proto_version(VALUE str)
{
int i;
static const struct {
const char *name;
int version;
} map[] = {
{ "SSL2", SSL2_VERSION },
{ "SSL3", SSL3_VERSION },
{ "TLS1", TLS1_VERSION },
{ "TLS1_1", TLS1_1_VERSION },
{ "TLS1_2", TLS1_2_VERSION },
{ "TLS1_3", TLS1_3_VERSION },
};

if (NIL_P(str))
return 0;
if (RB_INTEGER_TYPE_P(str))
return NUM2INT(str);

if (SYMBOL_P(str))
str = rb_sym2str(str);
StringValue(str);
for (i = 0; i < numberof(map); i++)
if (!strncmp(map[i].name, RSTRING_PTR(str), RSTRING_LEN(str)))
return map[i].version;
rb_raise(rb_eArgError, "unrecognized version %+"PRIsVALUE, str);
}

/*
* call-seq:
* ctx.min_version = OpenSSL::SSL::TLS1_2_VERSION
* ctx.min_version = :TLS1_2
* ctx.min_version = nil
*
* Sets the lower bound on the supported SSL/TLS protocol version. The
* version may be specified by an integer constant named
* OpenSSL::SSL::*_VERSION, a Symbol, or +nil+ which means "any version".
*
* === Example
* ctx = OpenSSL::SSL::SSLContext.new
* ctx.min_version = OpenSSL::SSL::TLS1_1_VERSION
* ctx.max_version = OpenSSL::SSL::TLS1_2_VERSION
*
* sock = OpenSSL::SSL::SSLSocket.new(tcp_sock, ctx)
* sock.connect # Initiates a connection using either TLS 1.1 or TLS 1.2
*/
static VALUE
ossl_sslctx_set_min_version(VALUE self, VALUE v)
{
SSL_CTX *ctx;
int version;

rb_check_frozen(self);
GetSSLCTX(self, ctx);
version = parse_proto_version(v);

if (!SSL_CTX_set_min_proto_version(ctx, version))
ossl_raise(eSSLError, "SSL_CTX_set_min_proto_version");
return v;
}

/*
* call-seq:
* ctx.max_version = OpenSSL::SSL::TLS1_2_VERSION
* ctx.max_version = :TLS1_2
* ctx.max_version = nil
*
* Sets the upper bound of the supported SSL/TLS protocol version. See
* #min_version= for the possible values.
*/
static VALUE
ossl_sslctx_set_max_version(VALUE self, VALUE v)
{
SSL_CTX *ctx;
int version;

rb_check_frozen(self);
GetSSLCTX(self, ctx);
version = parse_proto_version(v);

if (!SSL_CTX_set_max_proto_version(ctx, version))
ossl_raise(eSSLError, "SSL_CTX_set_max_proto_version");
return v;
}

static VALUE
ossl_ssl_cipher_to_ary(const SSL_CIPHER *cipher)
{
Expand Down Expand Up @@ -2846,8 +2878,8 @@ Init_ossl_ssl(void)

rb_define_alias(cSSLContext, "ssl_timeout", "timeout");
rb_define_alias(cSSLContext, "ssl_timeout=", "timeout=");
rb_define_private_method(cSSLContext, "set_minmax_proto_version",
ossl_sslctx_set_minmax_proto_version, 2);
rb_define_method(cSSLContext, "min_version=", ossl_sslctx_set_min_version, 1);
rb_define_method(cSSLContext, "max_version=", ossl_sslctx_set_max_version, 1);
rb_define_method(cSSLContext, "ciphers", ossl_sslctx_get_ciphers, 0);
rb_define_method(cSSLContext, "ciphers=", ossl_sslctx_set_ciphers, 1);
rb_define_method(cSSLContext, "ciphersuites=", ossl_sslctx_set_ciphersuites, 1);
Expand Down
40 changes: 1 addition & 39 deletions lib/openssl/ssl.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,43 +153,6 @@ def set_params(params={})
return params
end

# call-seq:
# ctx.min_version = OpenSSL::SSL::TLS1_2_VERSION
# ctx.min_version = :TLS1_2
# ctx.min_version = nil
#
# Sets the lower bound on the supported SSL/TLS protocol version. The
# version may be specified by an integer constant named
# OpenSSL::SSL::*_VERSION, a Symbol, or +nil+ which means "any version".
#
# Be careful that you don't overwrite OpenSSL::SSL::OP_NO_{SSL,TLS}v*
# options by #options= once you have called #min_version= or
# #max_version=.
#
# === Example
# ctx = OpenSSL::SSL::SSLContext.new
# ctx.min_version = OpenSSL::SSL::TLS1_1_VERSION
# ctx.max_version = OpenSSL::SSL::TLS1_2_VERSION
#
# sock = OpenSSL::SSL::SSLSocket.new(tcp_sock, ctx)
# sock.connect # Initiates a connection using either TLS 1.1 or TLS 1.2
def min_version=(version)
set_minmax_proto_version(version, @max_proto_version ||= nil)
@min_proto_version = version
end

# call-seq:
# ctx.max_version = OpenSSL::SSL::TLS1_2_VERSION
# ctx.max_version = :TLS1_2
# ctx.max_version = nil
#
# Sets the upper bound of the supported SSL/TLS protocol version. See
# #min_version= for the possible values.
def max_version=(version)
set_minmax_proto_version(@min_proto_version ||= nil, version)
@max_proto_version = version
end

# call-seq:
# ctx.ssl_version = :TLSv1
# ctx.ssl_version = "SSLv23"
Expand All @@ -214,8 +177,7 @@ def ssl_version=(meth)
end
version = METHODS_MAP[meth.intern] or
raise ArgumentError, "unknown SSL method `%s'" % meth
set_minmax_proto_version(version, version)
@min_proto_version = @max_proto_version = version
self.min_version = self.max_version = version
end

METHODS_MAP = {
Expand Down
44 changes: 44 additions & 0 deletions test/openssl/test_ssl.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1375,6 +1375,50 @@ def test_minmax_version
}
end

def test_minmax_version_system_default
omit "LibreSSL does not support OPENSSL_CONF" if libressl?

Tempfile.create("openssl.cnf") { |f|
f.puts(<<~EOF)
openssl_conf = default_conf
[default_conf]
ssl_conf = ssl_sect
[ssl_sect]
system_default = ssl_default_sect
[ssl_default_sect]
MaxProtocol = TLSv1.2
EOF
f.close

start_server(ignore_listener_error: true) do |port|
assert_separately([{ "OPENSSL_CONF" => f.path }, "-ropenssl", "-", port.to_s], <<~"end;")
sock = TCPSocket.new("127.0.0.1", ARGV[0].to_i)
ctx = OpenSSL::SSL::SSLContext.new
ctx.min_version = OpenSSL::SSL::TLS1_2_VERSION
ssl = OpenSSL::SSL::SSLSocket.new(sock, ctx)
ssl.sync_close = true
ssl.connect
assert_equal("TLSv1.2", ssl.ssl_version)
ssl.puts("abc"); assert_equal("abc\n", ssl.gets)
ssl.close
end;

assert_separately([{ "OPENSSL_CONF" => f.path }, "-ropenssl", "-", port.to_s], <<~"end;")
sock = TCPSocket.new("127.0.0.1", ARGV[0].to_i)
ctx = OpenSSL::SSL::SSLContext.new
ctx.min_version = OpenSSL::SSL::TLS1_2_VERSION
ctx.max_version = nil
ssl = OpenSSL::SSL::SSLSocket.new(sock, ctx)
ssl.sync_close = true
ssl.connect
assert_equal("TLSv1.3", ssl.ssl_version)
ssl.puts("abc"); assert_equal("abc\n", ssl.gets)
ssl.close
end;
end
}
end

Copy link
Member

@junaruga junaruga Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

@rhenium rhenium Jan 31, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@junaruga junaruga Jan 31, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Member

@junaruga junaruga Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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?

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).

Copy link
Member Author

@rhenium rhenium Jan 31, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@junaruga junaruga Jan 31, 2025

Choose a reason for hiding this comment

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

All right. The tests look good to me!

Copy link
Member Author

@rhenium rhenium Feb 3, 2025

Choose a reason for hiding this comment

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

Thank you for the review!

def test_options_disable_versions
# It's recommended to use SSLContext#{min,max}_version= instead in real
# applications. The purpose of this test case is to check that SSL options
Expand Down