-
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
Can't enable SSL with MariaDB driver library #1182
Comments
@vakuum thank you for the detailed report and the investigation! When the We have CI cases on GitHub Actions. Could you sent the PR for your patch, adding your case to the GitHub Actions as one case in mysql2/.github/workflows/build.yml Lines 30 to 39 in 346b4a4
You can use |
Maybe we can modify the comment in
|
I guess the
|
@vakuum Do you know how can we enable |
This one? https://dev.mysql.com/doc/c-api/5.7/en/mysql-options.html
|
I got a feedback at #1186 . Maybe we should not add the new CI case: DB server: MySQL, and DB client: MariaDB for now. Instead of that, we need to add a new unit test that can detect this issue. Because on the current unit tests on the current CI cases, this issue (the warning) is not detected. |
I captured the compiler's flags used for GitHub - Build Ruby 3.0 case.
GitHub - Container CentOS 7 Ruby 2.0.0 case.
Travis Ruby 2.4 DB=mariadb10.3 case.
|
@junaruga The macro HAVE_CONST_MYSQL_OPT_SSL_ENFORCE is defined in the generated Makefile when using the MariaDB driver:
The following spec fails without the change of https://github.com/brianmario/mysql2/blob/0.5.3/ext/mysql2/client.c#L117 and the warning "Your mysql client library does not support ssl_mode as expected" is also printed:
After adding the new version range to https://github.com/brianmario/mysql2/blob/0.5.3/ext/mysql2/client.c#L117 the spec turns green. |
@vakuum Thanks for the info. OK. You see the macro definition. I also can see the macro on my local. The defined macro The macro in the
I found the reason. How the macro is defined, and used. First the maccos are checked from the MySQL/MariaDB client library's header. Line 112 in 346b4a4
Then, Lines 15 to 21 in 346b4a4
When For the above GitHub - Build Ruby 3.0 case, I found the And seeing the logic in the |
@vakuum Have you tried to install and start the daemon of the Ubuntu mariadb-server-10.3 on your Ubuntu 20.04 (focal) environment? Because we can not start the MariaDB 10.3 on GitHub Actions Ubuntu focal (#1184). If you succeed to start the daemon, we can add the MariaDB 10.3 case with the client library |
@vakuum Can you paste the output of |
As a reference, here is a log on my local. |
I could found the |
@vakuum On the #1184 (comment) , I was able to run MariaDB server on GitHub Actions using MariaDB's official container. So, I will prepare to verify your patch. |
@junaruga I get the same results with mariadb-server-10.3 as with mysql-server-8.0. The Makefile contains And after adding the new version range to https://github.com/brianmario/mysql2/blob/0.5.3/ext/mysql2/client.c#L117 the spec turns green. It was a surprise to me that mariadb-server-10.3 doesn't have a working SSL configuration out of the box. :-)
Adding the new version range:
|
@vakuum Nice! Right now the mariadb10.3 case using It's great if you can fix it, and enable the case. .github/workflows/build.yml
As a note, I noticed today that on the GitHub focal, the So, I am using this script to remove and to install the package
|
If you want to check the used DB client library, the following command might be useful, as you may know it.
|
@vakuum I was able to run MariaDB 10.0, 10.1, 10.2, 10.3, 10.4, 10.5 with SSL mode by MariaDB official container on my testing repository. I will prepare to contribute the cases to this repository's GitHub Actions. So, we can reproduce this issue on GitHub Actions. Your working log for MariaDB helped me to do it. Thanks! If you are working on fixing the current |
@junaruga I added a commit to #1183 for fixing the MariaDB 10.3 installation under Ubuntu 20.04. The following issues caused the installation to fail: After installing the mariadb-server-10.3 package with ci/setup.sh the /var/log/mysql/error.log contained some additional errors:
The /var/lib/mysql directory already contained some database files from the preinstalled MySQL 8.0 installation. And these database files were not compatible with MariaDB 10.3. So I modified ci/setup.sh to remove the MySQL packages and directories before installing the MariaDB packages:
With the above change, the installation of MariaDB hanged forever at this step:
The /var/log/mysql/error.log looked good:
But the output of journalctl contained an apparmor error for /usr/sbin/mysqld:
So the apparmor for /usr/sbin/mysqld had to be disabled before the preinstalled MySQL packages are removed:
|
This reverts commit 22fce00.
This reverts commit 22fce00.
@vakuum Fantastic! I have 2 things for your PR. First, seeing the MariaDB 10.3 case you fixed, there are 3 failures. https://github.com/brianmario/mysql2/pull/1183/checks?check_run_id=2354587767#step:9:520
1), 2) is a known issue #965. I think it's better to update the comment mysql2/.github/workflows/build.yml Line 34 in 346b4a4
to
Could you update your PR? Second, I noticed there is no test to test your warning case currently.
And simply there is not test filling the "3.". That's why we could not find the issue on the current CI. I add the unit tests on the following commit. Could you pick up the commit and add it to your PR? Here is the result we can observe the warning as a test failure by the unit tests. I added every ssl_mode values to the unit tests. Line 134 in 346b4a4
I suspect the condition of this warning is outdated. We need to update the logic on another PR. As this is beyond your PR and your case, you can just remove ssl_mode values except disabled and required .
As I let @sodabrew know this, I keep the logic to the commit. That's it! Amazing work! |
Or I think it might be better to keep every ssl_mode value's test case, printing failures, because the MariaDB 10.3 has "allow_failure". |
I think it's better to avoid including |
@junaruga I changed the comment in .github/workflows/build.yml and added your commit junaruga/mysql2@3026da1. |
@vakuum Thank you for that. Sorry my patch causes additional tests in other CI cases such as in macOS cases in GitHub and MariaDB cases in Travis. |
@junaruga I updated the PR. |
Can't enable SSL with MariaDB driver library. (#1182)
PR #1183 was merged. |
#1205 also further improves MariaDB handling by allowing |
It is currently not possible to enable SSL through the ssl_mode parameter when using the MariaDB driver library.
This is critical because you can't connect to a MySQL server with a user that requires SSL.
How to reproduce?
Versions:
Create a MySQL database user with "REQUIRE SSL":
Create a test script:
🔴 Test with the MariaDB driver library
Install the driver library and the mysql2 gem:
Run the test script:
🟢 Test with the MySQL driver library
Install the driver library and the mysql2 gem:
Run the test script:
🟢 Solution?
With the MariaDB driver, the "mysql_get_client_version" at https://github.com/brianmario/mysql2/blob/0.5.3/ext/mysql2/client.c#L107 returns 100325 and HAVE_CONST_MYSQL_OPT_SSL_ENFORCE at https://github.com/brianmario/mysql2/blob/0.5.3/ext/mysql2/client.c#L113 is defined. The version that "mysql_get_client_version" returns is calculated at https://github.com/mariadb-corporation/mariadb-connector-c/blob/v3.1.12/CMakeLists.txt#L173.
With the MySQL driver, the "mysql_get_client_version" at https://github.com/brianmario/mysql2/blob/0.5.3/ext/mysql2/client.c#L107 returns 80023 and FULL_SSL_MODE_SUPPORT at https://github.com/brianmario/mysql2/blob/0.5.3/ext/mysql2/client.c#L131 is defined.
After adding a new version range for the MariaDB driver to https://github.com/brianmario/mysql2/blob/0.5.3/ext/mysql2/client.c#L117 the ssl_mode is correctly set and SSL is used for the database connection.
1. Create a patched mysql2 gem
2. Install the MariaDB driver library and the patched mysql2 gem
3. Run the test script
The text was updated successfully, but these errors were encountered: