[MDEV-34009] Client-side implementation of instant failover mechanism (and TLS fixes) #245
Conversation
This file contains 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
4 tasks
dlenski
force-pushed
the
MDEV-15935_instant_failover
branch
from
041b095
to
2bed28f
Compare
dlenski
changed the title
dlenski
changed the title
…s requested The 2015 commit mariadb-corporation@23895fbd4#diff-4339ae6506ef1fb201f6f836085257e72c191d2b4498df507d499fc30d891005 was described as "Fixed gnutls support", which is an extremely incomplete description of what it actually does. In that commit, the Connector/C client authentication plugin was modified to abort following the initial server greeting packet if the client has requested a secure transport (`mariadb --ssl`), but the server does not advertise support for SSL/TLS. However, there's a crucial caveat here: If the client has requested secure transport (`mariadb --ssl`) but has not requested verification of the server's TLS certificate (`mariadb --ssl --ssl-verify-server-cert`), then the client will silently accept an unencrypted connection, without even printing a diagnostic message. Thus, any such client is susceptible to a trivial downgrade to an unencrypted connection by an on-path attacker who simply flips the TLS/SSL capability bit in the advertised server capabilities in the greeting packet. The entire design of MariaDB's TLS support is severely flawed in terms of user expectations surrounding secure-by-default connections: the `--ssl` option SHOULD imply `--ssl-verify-server-cert` BY DEFAULT; if a client actually wants a TLS connection that's susceptible to a trivial MITM'ed by any pervasive or on-path attacker, that should be an exceptional case (e.g. `--insecure-ssl-without-server-cert-verification`) rather than the default. Even without resolving the issue of no default verification of server certificates, the issue of silent downgrade to unencrypted connections can and should be resolved. Backwards compatibility: This is an INTENTIONAL backwards-incompatible change to prevent clients from being silently downgraded to unencrypted connections, when they've specified `--ssl` and thus clearly indicated that they do not want unencrypted connections. All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.
…_connect Two reasons: 1. Reduction of attack surface As soon as the client receives the server's capability flags, it knows whether the server supports TLS/SSL. If the server does not support TLS/SSL, but the client expects and requires it, the client should immediately abort at this point in order to truncate any code paths by which it could inadvertently continue to communicate without TLS/SSL. 2. Separation of concerns Whether or not the server supports TLS/SSL encryption at the transport layer (TLS stands for TRANSPORT-layer security) is a logically separate issue from what APPLICATION-layer authentication modes the client and server support or should use. All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.
All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.
… "as secure as TLS" in terms of encryption All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.
If the server responds to our initial connection with an error packet having error number `ER_INSTANT_FAILOVER`, and an error string formatted as `|Human-readable message|host[:port]`, then redirect accordingly. This redirection is performed in `mthd_my_real_connect`, which appears to be the most appropriate location given that it already contains code for retrying/repeating a new server connection. All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.
This adds a new boolean option, `follow-instant-failovers` (ENABLED by default), to allow the client to disable server-initiated instant failover and simply return an error message. All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.
Set the limit to 8 for now. This will prevent the client from getting stuck in redirect loops. All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.
dlenski
force-pushed
the
MDEV-15935_instant_failover
branch
from
2bed28f
to
fe2dd63
Compare
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.
This PR is a replacement for #226, which proposed a very similar feature under the name of "server redirect.
Upstream developers chose a different approach (see MariaDB/server#1472 and #137) for that feature. See MDEV-15935 ("connection redirection") for further discussion.
I maintain that a simple and obligatory server-directed failover mechanism will be a very useful feature for many users, so I'm reintroducing it here under the alternative name of "instant failover."
This is the client counterpart of MariaDB/server#3224, which provides an initial server-initiated mechanism for instantly redirecting new client connections to a failover server.
It modifies the Connector/C library, so that if the server responds to our initial connection with an error packet having
error number
ER_INSTANT_FAILOVER), and an error string formatted as|Human-readable message|host[:port], the client will redirect its connection accordingly.This redirection is performed in
mthd_my_real_connect, which appears to be the most appropriate location given that it already contains code for retrying/repeating a new server connection.