Skip to content
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

Make sure ssl is enabled if only :sslverify is set #889

Merged
merged 1 commit into from
Nov 14, 2017

Conversation

rhafer
Copy link
Contributor

@rhafer rhafer commented Sep 20, 2017

Previously, when "sslverify: false/true" was the only ssl related
options passed to the constructor, the module skipped the call to
"mysql_ssl_set". It seems however that for some variants for the mysql
client libraries calling "mysql_ssl_set" is the only way to enable SSL
for the client connections. (E.g. the libraries shipped as part of
mariadb 10.1 still lack support for MYSQL_OPT_SSL_ENFORCE and
MYSQL_OPT_SSL_MODE)

This change allows enabling ssl with default values for all other
options by just passing "sslverify: true" or "sslverify: false" to the
constructor. (Depending on whether server certificate verification is
wanted or not)

Previously, when "sslverify: false/true" was the only ssl related
options passed to the constructor, the module skipped the call to
"mysql_ssl_set". It seems however that for some variants for the mysql
client libraries calling "mysql_ssl_set" is the only way to enable SSL
for the client connections. (E.g. the libraries shipped as part of
mariadb 10.1 still lack support for MYSQL_OPT_SSL_ENFORCE and
MYSQL_OPT_SSL_MODE)

This change allows enabling ssl with default values for all other
options by just passing "sslverify: true" or "sslverify: false" to the
constructor. (Depending on whether server certificate verification is
wanted or not)
@sodabrew
Copy link
Collaborator

Based on my reading of https://dev.mysql.com/doc/refman/5.7/en/mysql-ssl-set.html if all of the arguments to mysql_ssl_set are NULL, then it is a no-op. I haven't confirmed if the actual client code matches the documentation on this point.

Based on https://dev.mysql.com/doc/refman/5.7/en/c-api-encrypted-connections.html the way to enforce SSL is to call mysql_options(... MYSQL_OPT_SSL_MODE ... SSL_MODE_REQUIRED).

@rhafer
Copy link
Contributor Author

rhafer commented Sep 21, 2017

Based on my reading of https://dev.mysql.com/doc/refman/5.7/en/mysql-ssl-set.html if all of the arguments to mysql_ssl_set are NULL, then it is a no-op. I haven't confirmed if the actual client code matches the documentation on this point.

This doesn't seem to be true for mariadb 10.1:
https://github.com/MariaDB/server/blob/10.1/sql-common/client.c#L1683

Even if it's called with all arguments NULL it still sets mysql->options.use_ssl= TRUE;

Based on https://dev.mysql.com/doc/refman/5.7/en/c-api-encrypted-connections.html the way to enforce SSL is to call mysql_options(... MYSQL_OPT_SSL_MODE ... SSL_MODE_REQUIRED).

Unfortunately mariadb 10.1 (which AFAIK is still officially maintained) doesn't have those options.

@sodabrew
Copy link
Collaborator

Makes sense now! Thanks for linking to the relevant line of code in the driver.

Copy link

@jhesketh jhesketh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm;

@rhafer
Copy link
Contributor Author

rhafer commented Oct 16, 2017

@sodabrew Hi. Is there any chance to get this merged?

@@ -47,7 +47,7 @@ def initialize(opts = {})
self.charset_name = opts[:encoding] || 'utf8'

ssl_options = opts.values_at(:sslkey, :sslcert, :sslca, :sslcapath, :sslcipher)
ssl_set(*ssl_options) if ssl_options.any?
ssl_set(*ssl_options) if ssl_options.any? || opts.key?(:sslverify)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will turn on ssl even if the only flag is sslverify: false which doesn't quite make sense to me.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think given #879 that ssl_set should be called if any value of :ssl_mode is set, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants