From 785b5569fc5630e7bdfdd071c23dfea52db421b7 Mon Sep 17 00:00:00 2001 From: Kazuki Yamaguchi Date: Sat, 18 Jul 2020 16:45:01 +0900 Subject: [PATCH 1/3] test/openssl/test_ssl: revise a test case for client_cert_cb The current test_client_auth_public_key test case checks that supplying a PKey containing only public components through client_cert_cb will cause handshake to fail. While this is a correct behavior as a whole, the assertions are misleading in the sense that giving a public key is causing the failure. Actually, the handshake fails because a client certificate is not supplied at all, as a result of ArgumentError that is silently ignored. Rename the test case to test_client_cert_cb_ignore_error and simplify it to clarify what it is testing. --- test/openssl/test_ssl.rb | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/test/openssl/test_ssl.rb b/test/openssl/test_ssl.rb index b4619de25..1d3cdf90d 100644 --- a/test/openssl/test_ssl.rb +++ b/test/openssl/test_ssl.rb @@ -282,20 +282,16 @@ def test_client_auth_success } end - def test_client_auth_public_key + def test_client_cert_cb_ignore_error vflag = OpenSSL::SSL::VERIFY_PEER|OpenSSL::SSL::VERIFY_FAIL_IF_NO_PEER_CERT start_server(verify_mode: vflag, ignore_listener_error: true) do |port| - assert_raise(ArgumentError) { - ctx = OpenSSL::SSL::SSLContext.new - ctx.key = @cli_key.public_key - ctx.cert = @cli_cert - server_connect(port, ctx) { |ssl| ssl.puts("abc"); ssl.gets } - } - ctx = OpenSSL::SSL::SSLContext.new - ctx.client_cert_cb = Proc.new{ |ssl| - [@cli_cert, @cli_key.public_key] + ctx.client_cert_cb = -> ssl { + raise "exception in client_cert_cb must be suppressed" } + # 1. Exception in client_cert_cb is suppressed + # 2. No client certificate will be sent to the server + # 3. SSL_VERIFY_FAIL_IF_NO_PEER_CERT causes the handshake to fail assert_handshake_error { server_connect(port, ctx) { |ssl| ssl.puts("abc"); ssl.gets } } From 1ccdc05662a7817e8fe7a73ed589a8b092b527ac Mon Sep 17 00:00:00 2001 From: Kazuki Yamaguchi Date: Sat, 18 Jul 2020 17:09:37 +0900 Subject: [PATCH 2/3] test/openssl/test_ssl: revise verify_mode test cases Add explicit test cases for the behaviors with different verify_mode. If we made a bug in verify_mode, we would notice it by failures of other test cases, but there were no dedicated test cases for verify_mode. --- test/openssl/test_ssl.rb | 46 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/test/openssl/test_ssl.rb b/test/openssl/test_ssl.rb index 1d3cdf90d..4015b050d 100644 --- a/test/openssl/test_ssl.rb +++ b/test/openssl/test_ssl.rb @@ -246,7 +246,51 @@ def test_copy_stream end end - def test_client_auth_failure + def test_verify_mode_server_cert + start_server(ignore_listener_error: true) { |port| + populated_store = OpenSSL::X509::Store.new + populated_store.add_cert(@ca_cert) + empty_store = OpenSSL::X509::Store.new + + # Valid certificate, SSL_VERIFY_PEER + assert_nothing_raised { + ctx = OpenSSL::SSL::SSLContext.new + ctx.verify_mode = OpenSSL::SSL::VERIFY_PEER + ctx.cert_store = populated_store + server_connect(port, ctx) { |ssl| ssl.puts("abc"); ssl.gets } + } + + # Invalid certificate, SSL_VERIFY_NONE + assert_nothing_raised { + ctx = OpenSSL::SSL::SSLContext.new + ctx.verify_mode = OpenSSL::SSL::VERIFY_NONE + ctx.cert_store = empty_store + server_connect(port, ctx) { |ssl| ssl.puts("abc"); ssl.gets } + } + + # Invalid certificate, SSL_VERIFY_PEER + assert_handshake_error { + ctx = OpenSSL::SSL::SSLContext.new + ctx.verify_mode = OpenSSL::SSL::VERIFY_PEER + ctx.cert_store = empty_store + server_connect(port, ctx) { |ssl| ssl.puts("abc"); ssl.gets } + } + } + end + + def test_verify_mode_client_cert_required + # Optional, client certificate not supplied + vflag = OpenSSL::SSL::VERIFY_PEER + accept_proc = -> ssl { + assert_equal nil, ssl.peer_cert + } + start_server(verify_mode: vflag, accept_proc: accept_proc) { |port| + assert_nothing_raised { + server_connect(port) { |ssl| ssl.puts("abc"); ssl.gets } + } + } + + # Required, client certificate not supplied vflag = OpenSSL::SSL::VERIFY_PEER|OpenSSL::SSL::VERIFY_FAIL_IF_NO_PEER_CERT start_server(verify_mode: vflag, ignore_listener_error: true) { |port| assert_handshake_error { From 87d869352c34f678e15dc728880756a5a4448a3c Mon Sep 17 00:00:00 2001 From: Kazuki Yamaguchi Date: Sat, 18 Jul 2020 17:14:55 +0900 Subject: [PATCH 3/3] ssl: initialize verify_mode and verify_hostname with default values SSLContext's verify_mode expects an SSL_VERIFY_* constant (an integer) and verify_hostname expects either true or false. However, they are set to nil after calling OpenSSL::SSL::SSLContext.new, which is surprising. Set a proper value to them by default: verify_mode is set to OpenSSL::SSL::VERIFY_NONE and verify_hostname is set to false by default. Note that this does not change the default behavior. The certificate verification was never performed unless verify_mode is set to OpenSSL::SSL::VERIFY_PEER by a user. The same applies to verify_hostname. --- lib/openssl/ssl.rb | 2 ++ test/openssl/test_ssl.rb | 6 ++++++ 2 files changed, 8 insertions(+) diff --git a/lib/openssl/ssl.rb b/lib/openssl/ssl.rb index 8554ada0b..438daab01 100644 --- a/lib/openssl/ssl.rb +++ b/lib/openssl/ssl.rb @@ -122,6 +122,8 @@ class SSLContext def initialize(version = nil) self.options |= OpenSSL::SSL::OP_ALL self.ssl_version = version if version + self.verify_mode = OpenSSL::SSL::VERIFY_NONE + self.verify_hostname = false end ## diff --git a/test/openssl/test_ssl.rb b/test/openssl/test_ssl.rb index 4015b050d..59c94932c 100644 --- a/test/openssl/test_ssl.rb +++ b/test/openssl/test_ssl.rb @@ -246,6 +246,11 @@ def test_copy_stream end end + def test_verify_mode_default + ctx = OpenSSL::SSL::SSLContext.new + assert_equal OpenSSL::SSL::VERIFY_NONE, ctx.verify_mode + end + def test_verify_mode_server_cert start_server(ignore_listener_error: true) { |port| populated_store = OpenSSL::X509::Store.new @@ -919,6 +924,7 @@ def test_verify_hostname_on_connect start_server(ctx_proc: ctx_proc, ignore_listener_error: true) do |port| ctx = OpenSSL::SSL::SSLContext.new + assert_equal false, ctx.verify_hostname ctx.verify_hostname = true ctx.cert_store = OpenSSL::X509::Store.new ctx.cert_store.add_cert(@ca_cert)