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

bmleite commented 5 months ago

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

[X] Bugfix (non-breaking change which fixes issue #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

[X] I have read the CONTRIBUTING.md document
[X] I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
[X] 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 commented 5 months ago

Thank you for your analysis and contribution!

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

michaelklishin commented 5 months ago

@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 commented 5 months ago

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

bmleite commented 5 months ago

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 commented 5 months ago

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.

rabbitmq / rabbitmq-java-client