-
Notifications
You must be signed in to change notification settings - Fork 550
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
Update README and improve handling of ssl_mode settings #1306
Conversation
d951a03
to
a4d1568
Compare
Thanks for the PR! I am happy to review! |
|
||
MariaDB does not support the `:preferred` or `:verify_ca` options. For more information SSL/TLS in MariaDB, see | ||
[https://mariadb.com/kb/en/securing-connections-for-client-and-server/](https://mariadb.com/kb/en/securing-connections-for-client-and-server/) | ||
and [https://mariadb.com/kb/en/mysql_optionsv/#tls-options](https://mariadb.com/kb/en/mysql_optionsv/#tls-options) |
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 are printing the values of the ssl_mode in the warning message below. Maybe it's helpful to have a document about the mapping table of ssl_mode and the value or column for that on this table, such as :disabled
| 0
| MYSQL_OPT_SSL_ENFORCE = 0
rb_warn("Your mysql client library does not support ssl_mode %d", val);
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.
I think the awkward message is really because the translation from Ruby atom to C constant is in Ruby code while the mode setting action is in C code. The C code has the error but doesn't know what the original Ruby atom was.
A deeper cleanup might be moving the whole method into Ruby space, but I think it would be better get in the way of good enough for now.
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.
OK. I agree with it!
I reviewed and commented! |
* Workaround code path for MySQL 5.7.3 - 5.7.10 and MySQL Connector/C 6.1.3 - 6.1.x | ||
*/ | ||
if (version >= 100000 // MariaDB (all versions numbered 10.x) | ||
|| (version >= 30000 && version < 40000) // MariaDB Connector/C (all versions numbered 3.x) |
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.
I just double-checked the following definitions are available in the MariaDB connector C case.
- https://mariadb.com/kb/en/configuring-mariadb-connectorc-with-option-files/#ssl-verify-server-cert - MYSQL_OPT_SSL_VERIFY_SERVER_CERT - Introduced: MariaDB Connector/C 3.0.0
- https://mariadb.com/kb/en/configuring-mariadb-connectorc-with-option-files/#ssl-enforce - MYSQL_OPT_SSL_ENFORCE - Introduced: MariaDB Connector/C 3.1.1
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.
I think the ifdefs will handle this as is, at the risk of someone compiling against 3.0.x but running against 3.1.x or vice versa.
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.
I think so. My comment above is not about the issue or not about asking to change the code. I just checked and noted here that the 2 definitions are available for the MariaDB Connector C version range: version >= 30000 && version < 40000
.
978e16e
to
f1ef9db
Compare
… MariaDB versions Print the client version number in SSL warning. Add comments to explain the ssl mode setting function. Add MariaDB Connector/C 3.x to the relevant SSL mode code path. With thanks to Jun Aruga for identifying this issue.
f1ef9db
to
dc1d644
Compare
Print the client version number in SSL warning.
Add comments to explain the ssl mode setting function.
Add MariaDB Connector/C 3.x to the relevant SSL mode code path.
Supersedes #1294
Resolves #1305
@junaruga please review!