-
Notifications
You must be signed in to change notification settings - Fork 172
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
Only set min_version on OpenSSL < 1.1.0 #710
Conversation
I don't know if the Windows test failure is related. For the record, this is how I verified this. I first created some self signed certificates and an Apache config on CentOS Stream 8 that provided a vhost with only a single TLS version enabled:
Then I used a minimal script to retrieve the content: require 'net/http'
domains = [
'tls-10.example.com',
'tls-11.example.com',
'tls-12.example.com',
'tls-13.example.com',
]
ca_file = '/etc/ssl/certs/cacert.crt'
domains.each do |domain|
uri = URI("https://#{domain}")
begin
Net::HTTP.start(domain, 443, use_ssl: true, ca_file: ca_file) do |http|
http.get(uri)
end
rescue StandardError => e
puts "Failed to load from #{domain}: #{e}"
else
puts "Loaded from #{domain}"
end
end By default this loads from all 4 domains. When I manually patched # grep '^TLS.Min' /etc/crypto-policies/back-ends/opensslcnf.config
TLS.MinProtocol = TLSv1.2
# ruby test.rb
Failed to load from tls-10.example.com: SSL_connect returned=1 errno=0 state=error: tlsv1 alert protocol version
Failed to load from tls-11.example.com: SSL_connect returned=1 errno=0 state=error: tlsv1 alert protocol version
Loaded from tls-12.example.com
Loaded from tls-13.example.com
# vi /etc/crypto-policies/back-ends/opensslcnf.config
# grep '^TLS.Min' /etc/crypto-policies/back-ends/opensslcnf.config
TLS.MinProtocol = TLSv1.0
# ruby test.rb
Loaded from tls-10.example.com
Loaded from tls-11.example.com
Loaded from tls-12.example.com
Loaded from tls-13.example.com
# vi /etc/crypto-policies/back-ends/opensslcnf.config
# ruby test.rb
Failed to load from tls-10.example.com: SSL_connect returned=1 errno=0 state=error: tlsv1 alert protocol version
Failed to load from tls-11.example.com: SSL_connect returned=1 errno=0 state=error: tlsv1 alert protocol version
Failed to load from tls-12.example.com: SSL_connect returned=1 errno=0 state=error: tlsv1 alert protocol version
Loaded from tls-13.example.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good to me. This shouldn't re-enable SSL 3.0 since
- OpenSSL 1.1.0 is compiled without SSL 3.0 support by default (openssl/openssl@9829b5a).
- LibreSSL 2.3.0 (2015-09-23) removed SSL 3.0 support (https://ftp.openbsd.org/pub/OpenBSD/LibreSSL/libressl-2.3.0-relnotes.txt).
Is it better for us to write a unit test for this change? |
@rhenium thanks for that context. I hadn't considered that, but glad to see my implementation works with those in mind. @junaruga how would you write such a unit test? Test out diff --git a/test/openssl/test_ssl.rb b/test/openssl/test_ssl.rb
index 07dc9a343cef..8efc48ad6dc9 100644
--- a/test/openssl/test_ssl.rb
+++ b/test/openssl/test_ssl.rb
@@ -554,6 +554,15 @@ class OpenSSL::TestSSL < OpenSSL::SSLTestCase
assert_equal 0, ctx.options & OpenSSL::SSL::OP_DONT_INSERT_EMPTY_FRAGMENTS
assert_equal OpenSSL::SSL::OP_NO_COMPRESSION,
ctx.options & OpenSSL::SSL::OP_NO_COMPRESSION
+
+ if (OpenSSL::OPENSSL_VERSION.start_with?("OpenSSL") &&
+ OpenSSL::OPENSSL_VERSION_NUMBER >= 0x10100000)
+ # The cached value should not exist
+ assert_raise(NoMethodError) { ctx.send(:@min_proto_version) }
+ else
+ # This is not publicly exposed, but read the cached version
+ assert_equal OpenSSL::SSL::TLS1_VERSION, ctx.send(:@min_proto_version)
+ end
end
def test_post_connect_check_with_anon_ciphers Would that address your concerns? I always question reusing the same version detection logic, because it's so fragile. |
@ekohl good question! Nowadays we tend to separate the tests to small bits rater than adding the conditional assertions in one test. I wished that we could find a better way rater than checking the Below is my suggestion. @rhenium is the decision maker. What do you think? diff --git a/test/openssl/test_ssl.rb b/test/openssl/test_ssl.rb
index 07dc9a3..0f0b557 100644
--- a/test/openssl/test_ssl.rb
+++ b/test/openssl/test_ssl.rb
@@ -556,6 +556,29 @@ class OpenSSL::TestSSL < OpenSSL::SSLTestCase
ctx.options & OpenSSL::SSL::OP_NO_COMPRESSION
end
+ def test_sslctx_set_params_no_min_version_on_greater_than_equal_openssl_11
+ omit 'Omit in OpenSSL 1.0 or eariler versions' unless openssl?(1, 1, 0)
+ omit 'Omit in LibreSSL' if libressl?
+
+ ctx = OpenSSL::SSL::SSLContext.new
+ ctx.set_params
+
+ # The cached value should not exist.
+ assert_raise(NoMethodError) { ctx.send(:@min_proto_version) }
+ end
+
+ def test_sslctx_set_params_min_version_on_less_than_openssl_11_or_libressl
+ if openssl?(1, 1, 0)
+ omit 'Omit in OpenSSL 1.1 or later versions'
+ end
+
+ ctx = OpenSSL::SSL::SSLContext.new
+ ctx.set_params
+
+ # This is not publicly exposed, but read the cached version.
+ assert_equal OpenSSL::SSL::TLS1_VERSION, ctx.send(:@min_proto_version)
+ end
+
def test_post_connect_check_with_anon_ciphers
ctx_proc = -> ctx {
ctx.ssl_version = :TLSv1_2 |
Actually a very good practice and I completely understand you don't immediately refactor your entire test suite. I've pushed your tests as an additional commit. |
@ekohl Sorry. Seeing the CI result: https://github.com/ruby/openssl/actions/runs/7464356851?pr=710, I forget considering the LibreSSL cases. I updated my proposed patch above adding the logic. I don't know why the OpenSSL 1.0.2u case is failing with NoMethodError. And windows-latest (ruby) 3.3 case, it seems the |
I filed the windows-latest (ruby) 3.3 case's issue on #711. We can ignore the issue on this PR as it is not related to this PR. |
I'm looking at the code and test logic. I assume that |
We are printing some constant values before running the tests. It seems the
|
Sorry I updated my proposal again. The CI result is below as a refernece. |
@ekohl Could you check this issue by installing OpenSSL 1.0 or LibreSSL on your environment? A document to build the Ruby OpenSSL with different version's OpenSSL or LibreSSL is here. If you want to compile and install OpenSSL 1.0 on Linux, it can be like this.
|
You can rebase this PR on the latest master branch. The CI windows-latest 3.3 case's failures were fixed by a workaround. |
Both Red Hat and Debian-like systems configure the minimum TLS version to be 1.2 by default, but allow users to change this via configs. On Red Hat and derivatives this happens via crypto-policies[1], which in writes settings in /etc/crypto-policies/back-ends/opensslcnf.config. Most notably, it sets TLS.MinProtocol there. For Debian there's MinProtocol in /etc/ssl/openssl.cnf. Both default to TLSv1.2, which is considered a secure default. In constrast, the SSLContext has a hard coded OpenSSL::SSL::TLS1_VERSION for min_version. TLS 1.0 and 1.1 are considered insecure. By always setting this in the default parameters, the system wide default can't be respected, even if a developer wants to. This takes the approach that's also done for ciphers: it's only set for OpenSSL < 1.1.0. [1]: https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/security_hardening/using-the-system-wide-cryptographic-policies_security-hardening
9836dbc
to
104d68f
Compare
I've only applied the typo fix and |
We already have a test case for this, that #set_params won't allow connecting to SSL 3.0-only server: openssl/test/openssl/test_ssl.rb Lines 1159 to 1175 in 69384ac
Honestly I think this is enough. I could have pushed the merge button already, but I wanted to see what was causing the failure on Windows (you have figured it out - #711). |
Since this is now causing test failures, should I drop that commit then? |
Yes, I think that you can drop the 2nd commit! As I don't understand the logic of testing this PR, I may check it later with OpenSSL 3 and 1.0 on my local. But it doesn't block the PR! |
104d68f
to
ae215a4
Compare
Done |
Merged, thank you for the PR! |
I have investigated a bit about this PR because I may need to backport this PR's commit to downstream OpenSSL packages. Below is the summary. I am sure you guys already know about the information. But just in case for someone who is interested in the topic. Fixed openssl gem versionsFirst, the actual merged commit on the master branch is ae215a4. This commit is not included in any released openssl gem versions yet. while the openssl gem current latest released version is 3.2.0. So, there are no fixed openssl gem versions yet. Affected casesThis issue is that calling the
Reproducing the issueIf we confirm the value of the set min_version, we need to run a HTTPS (SSL) server as this example. Because Ruby OpenSSL doesn't provide the readable attributes to check the actual values. I assume that this is due to a better security. However, if we just check if a Ruby OpenSSL includes this commit in the code level, the reproducing script is as follows. the script refers to the cached variable
If a Ruby OpenSSL doesn't include this commit, the result is below. The
If a Ruby OpenSSL includes this commit, the result is below. The
Note that openssl/ext/openssl/ossl_ssl.c Lines 3111 to 3128 in d3d857c
And the macros are defined in the OpenSSL's code below. |
Hello @ekohl, I am trying to reproduce this issue with your HTTPD virtual host config and ruby script. I prepared the script below. When running the ruby script for this repository's master branch, the "tls-12" and "tls-13" cases that should pass, failed with "certificate verify failed (hostname mismatch)" instead of "Loaded from ...". Could you tell me how you did setup the SSL cert keys and etc?
As a reference, the cert config file used to generate the SSL cert keys is below.
|
It's been a while since I did that, but pretty sure I generated a certificate with the |
@ekohl thanks for your quick reply! I tried to set the As a reference, the commit for the change is the junaruga/ruby-openssl-tls-min-version-test@27524f1 . diff --git a/assets/cert.conf.tmpl b/assets/cert.conf.tmpl
index 49e255c..0410cf4 100644
--- a/assets/cert.conf.tmpl
+++ b/assets/cert.conf.tmpl
@@ -7,7 +7,8 @@ stateOrProvinceName_default = Brno
# The openssl command fails with "error, no objects specified in config file"
# without this item.
commonName = Common Name (hostname, IP, or your name)
-commonName_default = @DOMAIN@
+# Enable all the sub-domains of the domain.
+commonName_default = *.@DOMAIN@
localityName_default = Brno
organizationName_default = Test Organization
emailAddress_default = test@@DOMAIN@ Here is the generated SSL cert config file by my script.
Here is the local environment (Fedora 39) security policy setting.
And the ruby script only refused the first 2 servers, the server with the protocol
|
@ekohl I am seeing an unexpected behavior on my environment with the Ruby OpenSSL the latest master branch 818aa9f including this fix. I am using Fedora 39, and the following HTTPD and mod_ssl RPM packages. Do you have any idea about why this unexpected behavior happened?
I tested with the legacy security policy including
But the result showed the
I edited to change the
But I still saw the same unexpected result that the tls-10 and tls-11 servers rejected the access.
I am using the same httpd config like you.
And the
|
Maybe I was able to reproduce the issue the The Bash script is below.
The executing result is below. You can see the messages about the failing for "tls-10.example.com" and "tls-11.example.com", and the messages about passing for "tls-12.example.com" and "tls-13.example.com".
As a reference, you can check the stdout and stderr logs by the |
Just quick update. I asked the issue above which I faced to my colleagues, and it seems that they found the cause and how to fix. I will share it here later next week. |
I appreciate writing proper tests for this. I'm back in the office on Wednesday, I'll try to find time for a proper review then. |
All right!
|
@ekohl Let me explain my working status to reproduce the fix with your script. I was able to reproduce the fix on my Fedora Linux with the OpenSSL RPM referring to the OS's crypto policy config files. You can see the test results in the There were 3 issues for my testings. And the junaruga/ruby-openssl-tls-min-version-test@35d9bd4 fixed the 2 issues. The 1st issue was the The 2nd issue was the Fixing the 2 issues made the testing with the And the 3rd issue was I was building the Ruby OpenSSL with the upstream OpenSSL Ruby not with the downstream OpenSSL RPM. And the behavior showed the SSL servers only allowed TLS 1.2 and 1.3 connection, and rejected TLS 1.0 and 1.1 connection. Perhaps the upstream OpenSSL may have the own limitation about the supported TLS versions. I plan to ask this question to the upstream OpenSSL project. I was also told by my colleague that there was |
The Additionally, loading from crypto-policies is also patched in RPM packaging: https://src.fedoraproject.org/rpms/openssl/blob/rawhide/f/0004-Override-default-paths-for-the-CA-directory-tree.patch I was also looking at Debian. In bullseye (Debian 11) there https://sources.debian.org/patches/openssl/1.1.1w-0%2Bdeb11u1/Set-systemwide-default-settings-for-libssl-users.patch/ but I can't see a similar patch for bookworm (Debian 12). Though https://www.openssl.org/news/openssl-3.1-notes.html mentions that SSL 3, TLS 1.0 and TLS 1.1 only work at security level 0 starting OpenSSL 3.1 (included in Debian 12) while I think the default security level is 2 (could also be 1). |
Sorry for my late response. Yes, I learned the difference between the upstream OpenSSL and downstream OpenSSL (Fedora OpenSSL RPM, Debian OpenSSL deb package). It seems to me that the upstream OpenSSL doesn't have a logic like Fedora's crypto-policies which overrides TLS minimal version setting. We need to think both upstream and downstream OpenSSL cases separately.
I updated my reproducing script: https://github.com/junaruga/ruby-openssl-tls-min-version-test, implementing the Initially I wanted to contribute the tests into this Ruby OpenSSL project based on my reproducing script's tests. However, I think that this project's scope is only the cases with the upstream OpenSSL versions. See the CI file: |
Both Red Hat and Debian-like systems configure the minimum TLS version to be 1.2 by default, but allow users to change this via configs.
On Red Hat and derivatives this happens via crypto-policies, which in writes settings in /etc/crypto-policies/back-ends/opensslcnf.config. Most notably, it sets TLS.MinProtocol there. For Debian there's MinProtocol in /etc/ssl/openssl.cnf. Both default to TLSv1.2, which is considered a secure default.
In constrast, the SSLContext has a hard coded
OpenSSL::SSL::TLS1_VERSION
formin_version
. TLS 1.0 and 1.1 are considered insecure. By always setting this in the default parameters, the system wide default can't be respected, even if a developer wants to.This takes the approach that's also done for ciphers: it's only set for OpenSSL < 1.1.0.
Fixes #709