Windows - TLSv1.3 failure with 2.5, 2.6, & trunk

[MSP-Greg]

See PR #203, as above mentioned versions are set to 'allow failure'.

Not sure if the failure is due to Ruby, Ruby OpenSSL, or OpenSSL 1.1.1, which is used for all three Ruby versions. Older Windows Ruby versions use 1.0.2 & 1.0.1, which are passing.

[tarcieri]

RE: OpenSSL 1.1, #199 might potentially be related

[MSP-Greg]

I last was messing with OpenSSL 1.1.1 back in July thru Sept. Kind of rusty...

1. As I recall, there are some *nix OS's that restrict keys to 2k or more, but not anything in Travis or Appveyor. I keep an eye on EventMachine & Puma, as they both compile with OpenSSL...

2. I think Xenial MacOS is using 1.1.0. So, I'm guessing this is a 1.1.1 issue.

3. I have seen issues with testing (where a server & client are created), and the order of handshake callbacks changed in 1.1.1. I haven't jumped into the test code here.

[MSP-Greg]

I just disabled TLSv1.3, and all three failures (2,.5, 2.6, & trunk) passed.

I added:

ctx.max_version = :TLS1_2 if RUBY_VERSION >= '2.5'

to the below:

nio4r/spec/nio/selectables/ssl_socket_spec.rb

Lines 27 to 30 in 74fd1b1

	OpenSSL::SSL::SSLContext.new.tap do |ctx|
	ctx.cert = ssl_cert
	ctx.key = ssl_key
	end

[ioquatix]

Do you know why this works, is TLS v1.3 not supported? or buggy?

[MSP-Greg]

Do you know why this works

Honestly, right now, no. I was AFK, but I remembered that whether it was an issue specific to TLSv1.3 was something I hadn't checked earlier...

is TLS v1.3 not supported?

Well, 1.1.1 was released on 11-Sept, and current release is 1.1.1b. Ruby OpenSSL has been testing with it for quite a while, and I think trunk is tested with it on a macOS build. So, I'd consider it supported.

or buggy?

It's just a little different than earlier versions. I'll look at it more tomorrow.

Adding the above change would allow the builds to be removed from 'allow failure', but I assume fixing it is preferred...

[ioquatix]

I think I'm happy to defer to your judgement on how we should fix this. But I'm happy to take a closer look when I have time, which right now is very limited for me.

[tarcieri]

It's also entirely possible the current tests rely on an implementation detail present in previous TLS versions which differs when TLS 1.3 is enabled. The current tests are what I'd describe as "a little rickety"

[ioquatix]

@tarcieri Is that still the case? I did spend some time updating the tests, they were definitely as you describe them before. Can't remember how they are now... I think I fixed them a bit. Let's check.

[ioquatix]

Okay, so

350     1) OpenSSL::SSL::SSLSocket behaves like an NIO selectable stream selects readable when the other end closes
351     Failure/Error: expect(selector.select(1)).to be_nil
352
353       expected: nil
354            got: [#<NIO::Monitor:0x00000000052ce408 @io=#<TCPSocket:fd 11, AF_INET, 127.0.0.1, 1110>, @interests=:r, @...0000052c5d08>, @wakeup=#<IO:fd 8>, @waker=#<IO:fd 9>, @closed=false>, @closed=false, @readiness=:r>]
355     Shared Example Group: "an NIO selectable stream" called from ./spec/nio/selectables/ssl_socket_spec.rb:163
356     # ./spec/support/selectable_examples.rb:50:in `block (2 levels) in <top (required)>'

Failing at:

https://github.com/socketry/nio4r/blob/master/spec/support/selectable_examples.rb#L50

If it's being marked as readable, that might be due to a different buffering strategy.

The reason why that line exists I to assert a negative pre-condition before the actual test:

https://github.com/socketry/nio4r/blob/master/spec/support/selectable_examples.rb#L54

[ioquatix]

Otherwise, we wouldn't know if it was readable just due to some existing condition or because peer.close was invoked.

I wonder why it's being marked as readable, is there actually anything to read, or is it a spurious?

In theory there shouldn't be any data, but maybe the underlying socket has some TLS initialization data which is why it's marked as readable.

Maybe a simple way to work around this would be to actually do some communication, e.g.

let(:message) {"Hello World!"}
peer.write(message)
expect(stream.read(message.bytesize)).to be == message

Then, continue with the existing spec. That should ensure that the TLS session is established. The spec could still fail if the underlying stream has some kind of communication.

Another way to look at it, is does calling peer.close eventually cause stream.read to return nil, and does the selector indicate to the user that it was readable in that case. Maybe something like this:

    monitor = selector.register(stream, :r)
    peer.close

    expect(selector.select(0.1)).to include monitor
    expect(stream.read(1)).to be_nil

[MSP-Greg]

@tarcieri

It's also entirely possible

I might say very likely

current tests are what I'd describe as "a little rickety"

TLS 1.2 is 10+ years old, so that's ok. WebSocket (common use for TLS) is somewhat newer, and it and a lot of other things have probably resulted in changes to TLS 1.3.

I know from other work that, regarding servers & clients in the same test code, 'what happens when' changed with TLS 1.3. As I recall, careful conditionals allowed the code to work with both 1.2 & 1.3, but tests were added to check both.

I've always assumed it could be unstable, as OpenSSL isn't really concerned with what is an unlikely real world application (connected servers & clients in the same code).

@ioquatix

I'll try the things you've mentioned. I was thinking some of the same things.

So, I think I can load OpenSSL 1.1.1 with a macOS build on Travis. I'd like to see if this also occurs with 'compiled' nio4r. I think it also would be helpful to have tests for both 1.2 & 1.3 where needed. I'm happy to work on it this weekend...

[ioquatix]

If you can make a PR that fixes this issue that would be awesome :)

[MSP-Greg]

@ioquatix

See PR #205. I expect to revisit it when:

1. Travis has a reasonable way to use OpenSSL 1.1.1/TLS 1.3. It may be macOS, but I'd really like to check the code against the extension code, as I'm sure that's what the majority of users are using.

2. I'll probably try to change the 'echo server' example to use SSL and see how that's working with TLS 1.3.

PR's #206 & #207 followed from debugging, etc...

[MSP-Greg]

@ioquatix

Follow up to above. Re OpenSSL & Travis (see https://travis-ci.org/MSP-Greg/travis-ruby), the following is what's available:

1.0.1 Trusty
1.0.2 Xenial
1.1.1 XCode 10.2 (Ruby 2.4 thru master/trunk/head)

I'll have a look at it in a few days, I'd recommend testing on all three (not all Ruby versions.)

Puma was only testing against 1.0.2 & 1.1.1, and I broke 1.0.1, with a subsequent complaint about server's running Trusty...

Might be helpful for the log to show the OpenSSL version the tests were run with.

[ioquatix]

Agree on all points please make a PR.

[MSP-Greg]

See #212, which adds more OpenSSL testing to Travis.