-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extend TLS/SSL functionality with SNI and using system roots #358
Conversation
This changes enables sending along the SNI extension to the server when connecting over TLS with a client. It's best practice these days to send SNI as a TLS client. Usage of SNI would also enable uses cases like routing of MySQL connections to different backends behind a single IP. This change only implements the client side parts. Server side there is also value in providing different certificates on different SNI names, for example for multi homed MySQL servers with different DNS entries to connect combined with verify_identity semantics. At first though, clients should start sending the extension before it's useful to implement anything on the server side.
include/violite.h
Outdated
@@ -264,7 +265,7 @@ struct st_VioSSLFd *new_VioSSLConnectorFd( | |||
const char *key_file, const char *cert_file, const char *ca_file, | |||
const char *ca_path, const char *cipher, const char *ciphersuites, | |||
enum enum_ssl_init_error *error, const char *crl_file, const char *crl_path, | |||
const long ssl_ctx_flags, const char *server_host); | |||
const long ssl_ctx_flags, const char *server_host, bool verify_identity); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is split out into a separate argument since before, setting the host implied verify_identity
. The goal here is though to always send the SNI extension independent of the verify mode, so that's why it's now a separate flag here.
Hi, thank you for your contribution. Please confirm this code is submitted under the terms of the OCA (Oracle's Contribution Agreement) you have previously signed by cutting and pasting the following text as a comment: |
@mysql-oca-bot I have changed jobs since I've signed the OCA and I think it was also under the company name there. This means I might need a new OCA? |
Right now a CA chain is required to be provided in verify_ca or verify_identity mode. This makes it much more cumbersome when writing a client that connects to a server where the certificate in the system roots and when it provides a proper certificate signed by such an authority. By falling back to the system roots, deployments are greatly simplified because it removes the need to manually specify the root chain. The root chain is highly operating system and deployment dependent, so it requires a different configuration flag on each platform. By setting up the system roots through OpenSSL, deploying a MySQL server with a root signed installed on a system is greatly simplified for client configuration. Many clients in other languages / on other platforms already follow a similar behavior where system roots are used if no specific CA chain is configured. This applies to for example Java, Go, Node.js, Rust & .NET. This change also makes it simpler for drivers that use libmysqlclient such as the ones often used in Ruby and Python to simplify configuration for developers there as well.
* This is set up in new_VioSSLConnectorFd and adding here it here | ||
* again is superfluous. | ||
*/ | ||
#if OPENSSL_VERSION_NUMBER < 0x10002000L |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On OpenSSL 1.0.2 and newer, the additional identity check here is not needed since we already set it up inside new_VioSSLConnectorFd
using the OpenSSL provided validation function. This means the check here was superfluous and would result in double checking the certificate.
If anything, it makes it more brittle because there could be implementation differences between these validation functions (such as honoring DNS SANs, using the deprecated CN or not etc).
Turn off verification of servers certificate if both | ||
ca_file and ca_path is set to NULL | ||
*/ | ||
if (ca_file == nullptr && ca_path == nullptr) verify = SSL_VERIFY_NONE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We no longer infer this value, but it gets passed in explicitly by the caller based on the SSL mode that is selected. This means the SSL mode flag is leading and we fall back already automatically to the system roots.
"is VERIFY_CA or VERIFY_IDENTITY"); | ||
goto error; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code itself already supports fallback to the system roots if nothing is given. So this check can be removed on its own. The reason for all the other changes is that providing the ca path or file was also used to infer the SSL verification mode, which is now made all explicit.
The fallback logic exists here:
mysql-server/vio/viosslfactories.cc
Lines 696 to 700 in beb865a
/* otherwise go use the defaults */ | |
if (SSL_CTX_set_default_verify_paths(ssl_fd->ssl_context) == 0) { | |
*error = SSL_INITERR_BAD_PATHS; | |
goto error; | |
} |
Hi, the OCA associated with the owner of the this pull request has been cancled. As such this request will be closed. Thanks |
Uhm, what? Things were still in progress afaik? Can this please be reopened? |
@dbussink Sorry about that. The bot is not as clever as us humans and was not aware of the change in OCA being worked on. |
Hi, thank you for submitting this pull request. In order to consider your code we need you to sign the Oracle Contribution Agreement (OCA). Please review the details and follow the instructions at https://oca.opensource.oracle.com/ |
I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it. |
Hi, thank you for your contribution. Your code has been assigned to an internal queue. Please follow |
This adds setup of a default CA path if there's no path provided by the user. This enables easier configuration of system level CA validation if the MySQL server has a certificate signed by a system root. On more and more cloud based MySQL platforms system signed CA certificates are used and this hides the issue of selecting the appropriate path from the user. The real longer term answer here is that this is a default that changes in libmysqlclient itself. The current situation here is mixed. When using MariaDB (including the changes in #1205), the default system roots are already loaded and used if no CA is provided. On MySQL itself on the other hand, a CA path is required today. I have also opened a PR to improve that, see mysql/mysql-server#358 & https://bugs.mysql.com/bug.php?id=104649.
This adds setup of a default CA path if there's no path provided by the user. This enables easier configuration of system level CA validation if the MySQL server has a certificate signed by a system root. On more and more cloud based MySQL platforms system signed CA certificates are used and this hides the issue of selecting the appropriate path from the user. The real longer term answer here is that this is a default that changes in libmysqlclient itself. The current situation here is mixed. When using MariaDB (including the changes in #1205), the default system roots are already loaded and used if no CA is provided. On MySQL itself on the other hand, a CA path is required today. I have also opened a PR to improve that, see mysql/mysql-server#358 & https://bugs.mysql.com/bug.php?id=104649.
This adds setup of a default CA path if there's no path provided by the user. This enables easier configuration of system level CA validation if the MySQL server has a certificate signed by a system root. On more and more cloud based MySQL platforms system signed CA certificates are used and this hides the issue of selecting the appropriate path from the user. The real longer term answer here is that this is a default that changes in libmysqlclient itself. The current situation here is mixed. When using MariaDB (including the changes in #1205), the default system roots are already loaded and used if no CA is provided. On MySQL itself on the other hand, a CA path is required today. I have also opened a PR to improve that, see mysql/mysql-server#358 & https://bugs.mysql.com/bug.php?id=104649.
This adds setup of a default CA path if there's no path provided by the user. This enables easier configuration of system level CA validation if the MySQL server has a certificate signed by a system root. On more and more cloud based MySQL platforms system signed CA certificates are used and this hides the issue of selecting the appropriate path from the user. The real longer term answer here is that this is a default that changes in libmysqlclient itself. The current situation here is mixed. When using MariaDB (including the changes in #1205), the default system roots are already loaded and used if no CA is provided. On MySQL itself on the other hand, a CA path is required today. I have also opened a PR to improve that, see mysql/mysql-server#358 & https://bugs.mysql.com/bug.php?id=104649.
We're interested in responding to any feedback on this PR and continue to work with mysql community to get this merged, Can this please be reopened? |
@Phanatic I think things were moved to https://bugs.mysql.com/bug.php?id=104649 but sadly there's not much activity there yet. |
This changes enables sending along the SNI extension to the server when connecting over TLS with a client. It's best practice these days to send SNI as a TLS client.
Usage of SNI would also enable uses cases like routing of MySQL connections to different backends behind a single IP.
This change only implements the client side parts. Server side there is also value in providing different certificates on different SNI names, for example for multi homed MySQL servers with different DNS entries to connect combined with verify_identity semantics.
At first though, clients should start sending the extension before it's useful to implement anything on the server side.
Additionally, the second commit here enables usage of the system roots when no CA file or path is configured. Right now a CA chain is required to be provided in verify_ca or verify_identity mode. This makes it much more cumbersome when writing a client that connects to a server where the certificate in the system roots and when it provides a proper certificate signed by such an authority.
By falling back to the system roots, deployments are greatly simplified because it removes the need to manually specify the root chain. The root chain is highly operating system and deployment dependent, so it requires a different configuration flag on each platform.
By setting up the system roots through OpenSSL, deploying a MySQL server with a root signed installed on a system is greatly simplified for client configuration.
Many clients in other languages / on other platforms already follow a similar behavior where system roots are used if no specific CA chain is configured. This applies to for example Java, Go, Node.js, Rust & .NET.
This change also makes it simpler for drivers that use libmysqlclient such as the ones often used in Ruby and Python to simplify configuration for developers there as well.