Skip to content

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?


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