Fix unwrapping loop in case reading bytebuffer has exactly 1 handshake message

[bmleite]

Fixes #1280

Proposed Changes

In the scenario where the reading ByteBuffer only has enough bytes to unwrap one handshake message, the flow may enter a loop due to the call to ByteBuffer.clear(). That method does not actually erase any data, instead, it sets the position back to 0, the limit to the capacity, and the mark is discarded. Since we are doing another unwrap attempt using the same reading ByteBuffer, the same handshake message will be read.

Updating the reading ByteBuffer position instead of trying to clear it will result in a BUFFER_UNDERFLOW, which will then trigger another read from the channel, as expected.

Logs

Debug logs from successful NIO handshake attempt using AWS NLB with TLSv1.2:

Starting TLS handshake
Initial handshake status is NEED_WRAP
Handshake status is NEED_WRAP
Wrapping...
Handshake status is NEED_WRAP before wrapping
SSL engine result is Status = OK HandshakeStatus = NEED_UNWRAP
bytesConsumed = 0 bytesProduced = 362 sequenceNumber = 0 after wrapping
Wrote 362 byte(s)
Handshake status is NEED_UNWRAP
Unwrapping...
Handshake status is NEED_UNWRAP before unwrapping
Cipher in position 0
Reading from channel
Read 100 byte(s) from channel
Before unwrapping cipherIn is java.nio.HeapByteBuffer[pos=0 lim=100 cap=16709], with 100 remaining byte(s)
SSL engine result is Status = OK HandshakeStatus = NEED_TASK
bytesConsumed = 100 bytesProduced = 0 after unwrapping
Running delegated task
Before unwrapping cipherIn is java.nio.HeapByteBuffer[pos=100 lim=100 cap=16709], with 0 remaining byte(s)
SSL engine result is Status = BUFFER_UNDERFLOW HandshakeStatus = NEED_UNWRAP
bytesConsumed = 0 bytesProduced = 0 after unwrapping
Buffer underflow
Reading from channel...
Done reading from channel...
Before unwrapping cipherIn is java.nio.HeapByteBuffer[pos=0 lim=5298 cap=16709], with 5298 remaining byte(s)
SSL engine result is Status = OK HandshakeStatus = NEED_TASK
bytesConsumed = 4984 bytesProduced = 0 after unwrapping
Running delegated task
Before unwrapping cipherIn is java.nio.HeapByteBuffer[pos=4984 lim=5298 cap=16709], with 314 remaining byte(s)
SSL engine result is Status = OK HandshakeStatus = NEED_TASK
bytesConsumed = 305 bytesProduced = 0 after unwrapping
Running delegated task
Before unwrapping cipherIn is java.nio.HeapByteBuffer[pos=5289 lim=5298 cap=16709], with 9 remaining byte(s)
SSL engine result is Status = OK HandshakeStatus = NEED_TASK
bytesConsumed = 9 bytesProduced = 0 after unwrapping
Running delegated task
Before unwrapping cipherIn is java.nio.HeapByteBuffer[pos=5298 lim=5298 cap=16709], with 0 remaining byte(s)
SSL engine result is Status = OK HandshakeStatus = NEED_WRAP
bytesConsumed = 0 bytesProduced = 0 after unwrapping
cipherIn position after unwrap 5298
Handshake status is NEED_WRAP
Wrapping...
Handshake status is NEED_WRAP before wrapping
SSL engine result is Status = OK HandshakeStatus = NEED_WRAP
bytesConsumed = 0 bytesProduced = 42 sequenceNumber = 1 after wrapping
Wrote 42 byte(s)
Handshake status is NEED_WRAP
Wrapping...
Handshake status is NEED_WRAP before wrapping
SSL engine result is Status = OK HandshakeStatus = NEED_WRAP
bytesConsumed = 0 bytesProduced = 6 sequenceNumber = 2 after wrapping
Wrote 6 byte(s)
Handshake status is NEED_WRAP
Wrapping...
Handshake status is NEED_WRAP before wrapping
SSL engine result is Status = OK HandshakeStatus = NEED_UNWRAP
bytesConsumed = 0 bytesProduced = 45 sequenceNumber = 0 after wrapping
Wrote 45 byte(s)
Handshake status is NEED_UNWRAP
Unwrapping...
Handshake status is NEED_UNWRAP before unwrapping
Cipher in position 5298
Not reading
Before unwrapping cipherIn is java.nio.HeapByteBuffer[pos=5298 lim=5298 cap=16709], with 0 remaining byte(s)
SSL engine result is Status = BUFFER_UNDERFLOW HandshakeStatus = NEED_UNWRAP
bytesConsumed = 0 bytesProduced = 0 after unwrapping
Buffer underflow
Reading from channel...
Done reading from channel...
Before unwrapping cipherIn is java.nio.HeapByteBuffer[pos=0 lim=51 cap=16709], with 51 remaining byte(s)
SSL engine result is Status = OK HandshakeStatus = NEED_UNWRAP
bytesConsumed = 6 bytesProduced = 0 after unwrapping
Before unwrapping cipherIn is java.nio.HeapByteBuffer[pos=6 lim=51 cap=16709], with 45 remaining byte(s)
SSL engine result is Status = OK HandshakeStatus = FINISHED
bytesConsumed = 45 bytesProduced = 0 after unwrapping
cipherIn position after unwrap 51
TLS handshake completed

Types of Changes

• Bugfix (non-breaking change which fixes issue Handshake error when connecting to AWS NLB using TLS 1.2 and NIO #1280)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation (correction or otherwise)
• Cosmetics (whitespace, appearance)

Checklist

• I have read the CONTRIBUTING.md document
• I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
• All tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)
• Any dependent changes have been merged and published in related repositories

[lukebakken]

Thank you for your analysis and contribution!

Can we ensure that this change does not re-introduce these issues?

• TLS handshake hangs with NIO and TLS 1.3 #715
• Enabling NIO on a working SSL example causes newConnection to hang forever #712

[michaelklishin]

@lukebakken TLS 1.3 with NIO is tested in com.rabbitmq.client.test.ssl (well, when NIO is enabled for all tests), as long as the JRE provides a TLSv1.3 implementation.

The tests explicitly require JDK 11+, which does support TLS 1.3.

[acogoluegnes]

Is there a way to reproduce the issue mentioned in #1280 in a test to make sure the PR fixes it?

[bmleite]

I was able to successfully test the fix in one of our development environments but I wasn't able to set up a similar scenario for an integration test.

We could eventually replicate the issue if there was a way to control the ingest of handshake messages, but I do not see an easy way to do that. Any ideas?

[michaelklishin]

Since this is an IaaS-provider specific networking/TLS behavior (configuration), it can be extremely painful to test.

If we trust our existing suites for #712 #715, then I find it acceptable to merge this PR as is, since @bmleite has identified the problem and tested a fix exactly in the environment where it was manifesting itself.

We fairly often accept these env-specific testability limitations for RabbitMQ itself.