MDEV-39218: Master_ssl_verify_server_cert value is not set correctly#4880
MDEV-39218: Master_ssl_verify_server_cert value is not set correctly#4880gengtianuiowa wants to merge 1 commit intoMariaDB:12.3from
Conversation
ParadoxV5
left a comment
ParadoxV5
left a comment
There was a problem hiding this comment.
Good catch!
How did this slip past both CI and QA 🤦?
I’m not sure about the new tests, complete with new certificate files.
It’d be quite a disappointment if the repo really not have any SSL replication test.
| */ | ||
| mysql_options(mysql, MYSQL_OPT_SSL_VERIFY_SERVER_CERT, | ||
| &mi->master_ssl_verify_server_cert); | ||
| &mi->master_ssl_verify_server_cert.value); |
There was a problem hiding this comment.
I recall MYSQL_OPT_SSL_VERIFY_SERVER_CERT (i.e., client flag --ssl-verify-server-cert) expects a boolean, so it should be
| */ | |
| mysql_options(mysql, MYSQL_OPT_SSL_VERIFY_SERVER_CERT, | |
| &mi->master_ssl_verify_server_cert); | |
| &mi->master_ssl_verify_server_cert.value); | |
| */ | |
| bool ssl_verify_server_cert= mi->master_ssl_verify_server_cert; | |
| mysql_options(mysql, MYSQL_OPT_SSL_VERIFY_SERVER_CERT, | |
| &ssl_verify_server_cert); |
as MDEV-28302 intended.
There was a problem hiding this comment.
How did this slip past both CI and QA 🤦?
I read through part of the existing tests. It's easy for this issue to slip through the existing tests because the current tests only verify the response after running show replica status;. In the result, the Master_SSL_Verify_Server_Cert still shows the right (YES/NO) value because the logic for parsing the YES/NO value seems different than the value retrieval mentioned here.
The new implementation should be the same as the current implementation if there are only two possible values. Even though the default value (-1) is picked, !*(my_bool*) arg will convert it to false, which is the same as passing YES. This is consistent with the current default of turning on Master_SSL_Verify_Server_Cert by default.
ParadoxV5
added
External Contribution
gkodinov
changed the title
gkodinov
changed the title
gkodinov
changed the title
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
LGTM. Please work with the current reviewers towards the final one.
bnestere
left a comment
bnestere
left a comment
There was a problem hiding this comment.
Hi @gengtianuiowa !
Thank you for the contribution! I wasn't able to get your test to run locally (see my comment). Once that is addressed, I will provide another round of review.
| @@ -0,0 +1,3 @@ | |||
| --ssl-ca=$MYSQL_TEST_DIR/std_data/expired-ca-cert.pem | |||
There was a problem hiding this comment.
This isn't guaranteed to work across all environments. Many SSL libraries validate the certificate at server startup time (in mysqld_main() -> init_ssl()), and if the cert is expired, the server will fail to start.
Perhaps instead we can use the DEBUG_DBUG mechanism to simulate an expired certificate during checking. You can see existing examples of DEBUG_DBUG like this by grepping for:
- grep -ri "DEBUG_DBUG" ./sql
- grep -ri "debug_dbug=\"+d" ./mysql-test/suite/rpl/t
There was a problem hiding this comment.
Made the change based on this suggestion.
gengtianuiowa
force-pushed
the
fix_master_ssl_cert
branch
from
a066e54 to
21bef81
Compare
There was a problem hiding this comment.
How about we reword the filename and description a bit, so this test verifies master_ssl_verify_server_cert=0 in general, not just a specific bug?
There was a problem hiding this comment.
Reworded the file name and description
…y on replica In the major release of MariaDB 12.3.1, MariaDB this change to refactor ssl_verify_server_cert to master_ssl_verify_server_cert in commit in 89bd6b0. However, these values are different types. While ssl_verify_server_cert is my_bool type, the master_ssl_verify_server_cert is trilean type which accepts three possible values (Yes/No/Default). When reading the value of the argument using !*(my_bool*)arg in mysql_options, trilean type value can't be retrieved because it accesses the wrong memory address by just passing the master_ssl_verify_server_cert, causing the master_ssl_verify_server_cert still behaves as "ON" when it's set to "OFF". This commit changes the implementation to `master_ssl_verify_server_cert.value` retrieve the correct value. All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.
gengtianuiowa
force-pushed
the
fix_master_ssl_cert
branch
from
21bef81 to
c252733
Compare
4 participants
Description
In MariaDB 12.3.1, MariaDB made this change to refactor ssl_verify_server_cert to master_ssl_verify_server_cert in commit 89bd6b0.
However, these values are different types. While ssl_verify_server_cert is my_bool type, the master_ssl_verify_server_cert is trilean type which accepts three possible values (Yes/No/Default). When reading the value of the argument using !(my_bool)arg in mysql_options, trilean type value can't be retrieved because it accesses the wrong memory address by just passing the master_ssl_verify_server_cert, causing the master_ssl_verify_server_cert still behaves as "ON" when it's set to "OFF".
This commit changes the implementation to
master_ssl_verify_server_cert.valueto retrieve the correct value.All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.
How can this PR be tested?
Execute MTR test
rpl_ssl_verify_server_cert_trilean_bugin therplsuite.Basing the PR against the correct MariaDB version
Backward compatibility
This is a fix for the bug which only exists in 12.3 version. It's shouldn't be back-ported because they don't have the similar issue.
Copyright
All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.