reef: librbd/migration/HttpClient: avoid reusing ssl_stream after shut down#61094
Merged
sunilangadi2 merged 10 commits intoceph:reeffrom Jan 17, 2025
Merged
reef: librbd/migration/HttpClient: avoid reusing ssl_stream after shut down#61094sunilangadi2 merged 10 commits intoceph:reeffrom
sunilangadi2 merged 10 commits intoceph:reeffrom
Conversation
Logging just the (negated) value makes tracking down the error category and message unnecessarily hard. Signed-off-by: Ilya Dryomov <idryomov@gmail.com> (cherry picked from commit eb77349)
Signed-off-by: Ilya Dryomov <idryomov@gmail.com> (cherry picked from commit d1a83f2)
resolve_host() is called from init() and issue() when transitioning out of STATE_UNINITIALIZED and from advance_state() right after the call to shutdown_socket(). In all three cases the socket should get closed, so drop the redundant call and place asserts in connect() implementations instead. Signed-off-by: Ilya Dryomov <idryomov@gmail.com> (cherry picked from commit 5699e4b)
ssl_stream objects can't be reused after shut down: despite a successful reconnect and handshake, any attempt to read or write fails with "end of stream" (beast.http:1) or "protocol is shutdown" (asio.ssl:337690831) error respectively. This doesn't appear to be documented, but Beast and ASIO authors both mention that the stream must be destroyed and recreated [1][2]. This was missed because the only integration test with a big enough image used http instead of https. [1] boostorg/beast#821 (comment) [2] chriskohlhoff/asio#804 (comment) Fixes: https://tracker.ceph.com/issues/69178 Signed-off-by: Ilya Dryomov <idryomov@gmail.com> (cherry picked from commit 20885b1)
…ake() With m_ssl_enabled set to false, disconnect() is a no-op. Since m_ssl_enabled is flipped to true only when the handshake succeeds, calling disconnect() on "failed to complete handshake" error is bogus (as would be attempting to shut down SSL there). Signed-off-by: Ilya Dryomov <idryomov@gmail.com> (cherry picked from commit 331b5ea)
The remaining callers of disconnect() call it only when m_ssl_enabled is set to true (i.e. after the handshake is completed): - shut_down(), in STATE_READY - maybe_finalize_reset(), very shortly after transitioning out of STATE_READY as part of performing a reset - advance_state(), on a transition to STATE_READY that is intercepted by a previously delayed shut down m_ssl_enabled isn't used outside of disconnect() and on top of that is never cleared. Signed-off-by: Ilya Dryomov <idryomov@gmail.com> (cherry picked from commit 8566224)
Get rid of get_callback_adapter() which only obfuscates the error: handle_handshake: failed to complete SSL handshake: (337047686) Unknown error 337047686 vs handle_handshake: failed to complete SSL handshake: certificate verify failed (SSL routines, tls_process_server_certificate) Signed-off-by: Ilya Dryomov <idryomov@gmail.com> (cherry picked from commit e305a59)
…wn SSL
Propagate ec to handle_disconnect() and use it to suppress
stream_truncated errors. Here is a quote from Beast documentation [1]:
// Gracefully shutdown the SSL/TLS connection
error_code ec;
stream.shutdown(ec);
// Non-compliant servers don't participate in the SSL/TLS shutdown process and
// close the underlying transport layer. This causes the shutdown operation to
// complete with a `stream_truncated` error. One might decide not to log such
// errors as there are many non-compliant servers in the wild.
if(ec != net::ssl::error::stream_truncated)
log(ec);
... and a commit that made ignoring stream_truncated safe [2]:
// ssl::error::stream_truncated, also known as an SSL "short read",
// indicates the peer closed the connection without performing the
// required closing handshake
// [...]
// When a short read would cut off the end of an HTTP message,
// Beast returns the error beast::http::error::partial_message.
// Therefore, if we see a short read here, it has occurred
// after the message has been completed, so it is safe to ignore it.
[1] https://www.boost.org/doc/libs/develop/libs/beast/doc/html/beast/using_io/ssl_tls_shutdown.html
[2] boostorg/beast@094f5ec
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
(cherry picked from commit 9fa0bcc)
If the shutdown gets delayed until the state transition from STATE_RESET_CONNECTING completes and the reconnect is successful (i.e. next_state is STATE_READY), we eventually hit "unexpected state transition" assert in advance_state(). The reason is that advance_state() would update m_state and call disconnect() under STATE_READY instead of STATE_SHUTTING_DOWN. After the disconnect maybe_finalize_shutdown() would enter advance_state() again with STATE_SHUTDOWN as next_state, but the transition to that from STATE_READY is invalid. Plug this by not transitioning to next_state if current_state is STATE_SHUTTING_DOWN. Signed-off-by: Ilya Dryomov <idryomov@gmail.com> (cherry picked from commit 1046d61)
…nsitions If shut_down() gets delayed until a) the state transition from STATE_RESET_CONNECTING completes and the reconnect is unsuccessful or b) the state transition from STATE_RESET_DISCONNECTING completes (i.e. next_state is STATE_UNINITIALIZED or STATE_RESET_CONNECTING), the socket needs to be shut down before m_on_shutdown is invoked. The line of thought here is the same as for the corresponding state transitions that don't involve STATE_SHUTTING_DOWN. Signed-off-by: Ilya Dryomov <idryomov@gmail.com> (cherry picked from commit 88557df)
ajarr
approved these changes
Dec 16, 2024
|
jenkins test make check |
1 similar comment
Contributor
|
jenkins test make check |
Contributor
|
This is ready for merge as soon as |
Contributor
|
jenkins test make check |
1 similar comment
Contributor
|
jenkins test make check |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
backport tracker: https://tracker.ceph.com/issues/69242
backport of #61079
parent tracker: https://tracker.ceph.com/issues/69178