Skip to content

Conversation

@rhenium
Copy link
Member

@rhenium rhenium commented Jul 18, 2020

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


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.

rhenium added 2 commits July 18, 2020 17:09
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.
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.
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.
@rhenium rhenium force-pushed the ky/ssl-attr-default-values branch from 0983fbd to 87d8693 Compare August 12, 2020 09:40
@rhenium rhenium merged commit 0cdd377 into ruby:master Aug 12, 2020
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.

1 participant