Details
-
Bug
-
Status: Closed (View Workflow)
-
Major
-
Resolution: Cannot Reproduce
-
11.4
-
None
Description
In 585c096aa53c ("cleanup: unify client's setting of ssl options"), serg introduced a bug that causes the USE_SSL_FORBIDDEN case to be treated identically to the USE_SSL_REQUIRED case.
These means that any and all tests which use connect(..., NOSSL) to try to establish a non-SSL connection are actually establish an SSL-required connection.
Completely breaks and/or inverts the logic of all these tests.
See https://github.com/mariadb/server/commit/585c096aa53c#r140576270 for the details.
The short summary is that there's a critical missing case in the SET_SSL_OPTS macro:
#define SET_SSL_OPTS(M) \
|
do { \ |
if (opt_use_ssl) \ |
/* if (opt_use_ssl == -1) {} else */ /* SSL forbidden THIS IS THE MISSING CASE */ \ |
if (opt_use_ssl) /* SSL required */ \ |
{ \
|
mysql_ssl_set((M), opt_ssl_key, opt_ssl_cert, opt_ssl_ca, \
|
opt_ssl_capath, opt_ssl_cipher); \
|
mysql_options((M), MYSQL_OPT_SSL_CRL, opt_ssl_crl); \
|
mysql_options((M), MYSQL_OPT_SSL_CRLPATH, opt_ssl_crlpath); \
|
mysql_options((M), MARIADB_OPT_TLS_VERSION, opt_tls_version); \
|
mysql_options((M), MARIADB_OPT_TLS_PEER_FP, opt_ssl_fp); \
|
mysql_options((M), MARIADB_OPT_TLS_PEER_FP_LIST, opt_ssl_fplist); \
|
} \
|
else /* SSL if available */ \ |
opt_ssl_verify_server_cert= 0; \
|
mysql_options((M),MYSQL_OPT_SSL_VERIFY_SERVER_CERT, \
|
&opt_ssl_verify_server_cert); \
|
} while(0) |
Attachments
Activity
The PR https://github.com/MariaDB/server/pull/3224 includes a commit to fix this
I don't understand the issue here.
NOSSL is used in the test as:
connect without_ssl,localhost,root,,,,,TCP NOSSL;
|
SELECT (VARIABLE_VALUE <> '') AS have_ssl FROM INFORMATION_SCHEMA.SESSION_STATUS WHERE VARIABLE_NAME='Ssl_cipher';
|
have_ssl
|
0
|
and the value of "have_ssl" is zero here, meaning SSL is not used.
opt_use_ssl doesn't seem to be set to -1 anywhere, it's simply not a valid¹⁾ value for a variable of the my_bool type, it can only be TRUE or FALSE.
—
¹⁾ the value is logically invalid, but from the compiler point of view it's allowed
I am definitely seeing failures in all the tests with NOSSL, but may have jumped to a premature conclusion about exactly which commit broke this.
Will update tomorrow.