test/openssl/test_ssl.rb: ignore SSLError when the connection is closed

[mame]

"test_close_after_socket_close" checks if ssl.close is no-op even after
the wrapped socket is closed. The test itself is fair, but the other
endpoint that is reading the SSL connection may fail with SSLError:
"SSL_read: unexpected eof while reading" in some environments:

https://github.com/ruby/ruby/actions/runs/60085389 (MinGW)
https://rubyci.org/logs/rubyci.s3.amazonaws.com/android28-x86_64/ruby-master/log/20200321T034442Z.fail.html.gz

  1) Failure:
OpenSSL::TestSSL#test_close_after_socket_close [D:/a/ruby/ruby/src/test/openssl/utils.rb:299]:
exceptions on 1 threads:
SSL_read: unexpected eof while reading

This changeset rescues and ignores the SSLError in the test.

Note: I've already committed this change into ruby/ruby to suppress the CI failures.
ruby/ruby@be76e86

[MSP-Greg]

@mame

After checking the openssl changelog and the commit that changes the behavior, this is really a breaking change in OpenSSL 1.1.1e. The changelog entry:

 *) Properly detect EOF while reading in libssl. Previously if we hit an EOF
     while reading in libssl then we would report an error back to the
     application (SSL_ERROR_SYSCALL) but errno would be 0. We now add
     an error to the stack (which means we instead return SSL_ERROR_SSL) and
     therefore give a hint as to what went wrong.
     [Matt Caswell]

If you review the code in #356, you'll see that the current code in ossl_ssl.c where I added an additional case clause, the preceding (and existing) case clause catches the error and returns rb_eof_error(). The code I added takes the new/changed way of reporting the error and also returns rb_eof_error().

By changing the test, you're ignoring the breaking change, and hence, causing a breaking change here.

EDIT: The 2nd commit in PR #356 is the same as the commit in ruby/ruby#2971

[mame]

I just want to suppress the failure soon. If your PR is accepted and backported to ruby/ruby, I'm okay to revert my change.

In my opinion, ruby-openssl should be a simple wrapper of openssl. A wrapper should not hide a breaking change in the upstream. But I leave the decision up to @rhenium.

[MSP-Greg]

Some of the functionality in the Ruby OpenSSL stdlib is 'translating' OpenSSL errors to more Ruby friendly IO/Socket errors. With the release of 1.1.1e, OpenSSL changed how one of the 'translated' errors was raised. So, given that the error is being translated here, code should be adjusted to account for it.

JFYI, I'm not what I would consider to be a c coder, especially when compared to everyone in Core. I did verify that this was an OpenSSL 1.1.1e issue here (it isn't MinGW specific), and I am able to read the code well enough to determine the cause of this problem.

I just want to suppress the failure soon.

I think I'll post something in core about 'suppression'... The problem with suppression is that master builds are being used in external CI, and hiding errors affects that...

Thanks, Greg

[voxik]

Just FTR, the OpenSSL 1.1.1e was introduced into Fedora Rawhide [1], but the offending EOF patch was later reverted [2]. But it will be part of OpenSSL 3.x anyway.

[rhenium]

Closing the underlying TCP connection without sending close_notify is indeed a protocol error. It should be treated differently from a clean shutdown as it could be hiding data truncation.

However, ruby-openssl historically ignored that specific error probably because there were so many broken SSL/TLS implementations in the wild. This is one of the things I want to get rid of.

openssl/ext/openssl/ossl_ssl.c

Lines 1887 to 1896 in 2c43241

	else {
	/*
	* The underlying BIO returned 0. This is actually a
	* protocol error. But unfortunately, not all
	* implementations cleanly shutdown the TLS connection
	* but just shutdown/close the TCP connection. So report
	* EOF for now...
	*/
	if (no_exception_p(opts)) { return Qnil; }
	rb_eof_error();

Since OpenSSL 3.0 is expected to contain breaking changes anyway, I feel it's a good time to do so. In other words, no special handling for the new SSL_R_UNEXPECTED_EOF_WHILE_READING error.

[rhenium]

I'm merging this pull request since the diff does not affect the point of the test case (testing that #close will not raise an exception) and will be necessary when OpenSSL 3.0 is out.

[MSP-Greg]

probably because there were so many broken SSL/TLS implementations in the wild

And we can't control that. We might consider it a hint to some.

If the test will only pass by using getc or read(1), I think there may be a better solution...