SSL client handshake check not called #951
Conversation
SSL handshake must be done once connected
WalkthroughThe changes modify the SSL handshake process within the Changes
Sequence Diagram(s)sequenceDiagram
participant Stream as ACE_SSL_Asynch_Stream
participant SSL as ::SSL_connect
participant PostCheck as post_handshake_check
Stream->>SSL: Initiate SSL_connect
SSL-->>Stream: Return retval (1)
alt Handshake Successful
Stream->>Stream: Recursive call do_SSL_handshake()
Stream->>PostCheck: Execute post_handshake_check()
end
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ACE/ace/SSL/SSL_Asynch_Stream.cpp (1)
513-514: LGTM! The fix correctly ensures post_handshake_check is called.The recursive call to
do_SSL_handshake()after a successful SSL handshake completion is a good solution to ensure thatpost_handshake_checkis called at the right time, eliminating the race condition in the handshake process.Consider adding a comment to explain the timing issue and why the recursive call is necessary:
- if (retval == 1) // handshake was successfully completed - ensure that we call post_handshake_check + // When SSL_connect returns 1, the handshake is complete but post_handshake_check + // needs to be called immediately to avoid a race condition where it might be + // called too late during subsequent read/write operations. + if (retval == 1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ACE/ace/SSL/SSL_Asynch_Stream.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
With the former ACE 6.1.7, I have observed that a SSL client sometimes fails to establish connection. It looks like a chicken-and-egg problem where the SSL handshake sometimes is completed when SSL_connect returns and sometimes it is not. Resulting in client post_handshake_check first being called later on when client attempts to read/write some data.
This may be a result of how we use ACE to initiate a connection.
But I found that when a ACE SSL client calls SSL_connect as part of do_SSL_state_machine and this returns 1 meaning:
"The TLS/SSL handshake was successfully completed, a TLS/SSL connection has been established."
see https://www.openssl.org/docs/man1.0.2/man3/SSL_connect.html
The do_SSL_handshake will also return 1 which according to the code comments means"SSL handshake is finished already, success". But this is not entirely true from a ACE perspective, as it did not call post_handshake_check.
Ensuring that post_handshake_check is called within the function, resolves the issue I observe.
Summary by CodeRabbit