Skip to content

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

Merged
michaelklishin merged 1 commit intorabbitmq:mainfrom
bmleite:nio-tls-v1.2-handshake-error
Apr 8, 2024
Merged

Fix unwrapping loop in case reading bytebuffer has exactly 1 handshake message#1281
michaelklishin merged 1 commit intorabbitmq:mainfrom
bmleite:nio-tls-v1.2-handshake-error

Conversation

@bmleite
Copy link
Contributor

@bmleite bmleite commented Apr 3, 2024 •
edited by lukebakken
Loading

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

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

@bmleite
Fix unwrapping loop in case bytebuffer has exactly 1 handshake message
7382a33 
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).
@lukebakken
Copy link
Collaborator

lukebakken commented Apr 3, 2024

Thank you for your analysis and contribution!

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

@michaelklishin
Copy link
Contributor

michaelklishin commented Apr 3, 2024 •
edited
Loading

@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
Copy link
Contributor

acogoluegnes commented Apr 3, 2024

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

@bmleite
Copy link
Contributor Author

bmleite commented Apr 4, 2024

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
Copy link
Contributor

michaelklishin commented Apr 4, 2024

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.

@michaelklishin michaelklishin merged commit dbd5788 into rabbitmq:main Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Handshake error when connecting to AWS NLB using TLS 1.2 and NIO

4 participants

@bmleite @lukebakken @michaelklishin @acogoluegnes