Skip to content

Fix upgrade sysvars pre-check for colon-delimited cipher lists#29

Open
tonychen2001 wants to merge 1 commit into
mysql:9.7from
tonychen2001:cipher-colon-delimiter-fix
Open

Fix upgrade sysvars pre-check for colon-delimited cipher lists#29
tonychen2001 wants to merge 1 commit into
mysql:9.7from
tonychen2001:cipher-colon-delimiter-fix

Conversation

@tonychen2001

Copy link
Copy Markdown

Description

The upgrade checker's system-variable check validates Set-typed variables by
splitting the configured value into individual elements and checking each
element against the variable's allowed-values list
(Sysvar_check::get_invalid_allowed_values / get_invalid_forbidden_values in
modules/util/upgrade_checker/sysvar_check.cc). The split was hard-coded to use
space and comma (" ,") as the only element separators.

The four cipher-list variables: ssl_cipher, admin_ssl_cipher,
tls_ciphersuites, admin_tls_ciphersuites are colon-delimited.
They are marked vartype: set in sysvars.json, so they went through the Set splitter, but colon was never a
recognized separator.
A value like ECDHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES128-GCM-SHA256 was therefore compared as
one opaque token against the list of individual ciphers, never matched, and the
whole value was reported as invalid resulting in a false positive even when every cipher in
the list is allowed.

Failing precheck example:

The MySQL server at repro-mysql84:3306, version 8.4.6 - MySQL Community Server
- GPL, will now be checked for compatibility issues for upgrade to MySQL 9.7.0.

1) System variable check for deprecation, removal, changes in defaults values
or invalid values. (sysVars)
   Following system variables that were detected as being used ware changed,
   deprecated or removed. Please update  your system accordingly before the
   upgrade.

   Error: The following system variables that were detected as being used are
   using values that are  invalid in the target version.
   - ssl_cipher: variable value includes
   'ECDHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES128-GCM-SHA256', allowed values
   include: ECDHE-ECDSA-AES128-GCM-SHA256, ECDHE-ECDSA-AES256-GCM-SHA384,
   ECDHE-RSA-AES128-GCM-SHA256, ECDHE-RSA-AES256-GCM-SHA384,
   ECDHE-ECDSA-CHACHA20-POLY1305, ECDHE-RSA-CHACHA20-POLY1305,
   ECDHE-ECDSA-AES256-CCM, ECDHE-ECDSA-AES128-CCM, DHE-RSA-AES128-GCM-SHA256,
   DHE-RSA-AES256-GCM-SHA384, DHE-RSA-AES256-CCM, DHE-RSA-AES128-CCM,
   DHE-RSA-CHACHA20-POLY1305, .
   - tls_ciphersuites: variable value includes
   'TLS_AES_128_GCM_SHA256:TLS_AES_256_GCM_SHA384', allowed values include:
   TLS_AES_128_GCM_SHA256, TLS_AES_256_GCM_SHA384,
   TLS_CHACHA20_POLY1305_SHA256, TLS_AES_128_CCM_SHA256,

A similar fix for the 8.4 branch was published in #24 but was ultimately not accepted as MySQL Shell 8.4 only receives critical updates. A partial fix in MySQL Shell 9.x was made to allow for comma delimited cipher lists but did not properly allow for colon delimited cipher lists.

Fix

We introduce a per-variable element separator property that can configure the allowed delimiters for a set typed variable.

Only the four cipher variables carry the colon-inclusive separator ( ,:). The other
Set-typed variables (optimizer_switch, sql-mode,
slave_type_conversions, slave_rows_search_algorithms) are comma-separated and
keep the default, so their behavior is untouched.

Release Notes

Fixed an upgrade-checker issue where a colon-separated list of TLS/SSL ciphers in
ssl_cipher, admin_ssl_cipher, tls_ciphersuites, or admin_tls_ciphersuites
was incorrectly reported as an invalid value during
util.checkForServerUpgrade(), even when every cipher in the list was allowed in
the target version. Each cipher in the list is now validated individually.

Testing

New/updated cases in
unittest/modules/util/upgrade_checker/sysvar_upgrade_check_t.cc:

  • Upgrade_checker_Sysvar_check.cipher_colon_list_allowed — a colon-separated
    list of individually-allowed ciphers produces no issue.
  • Upgrade_checker_Sysvar_check.cipher_colon_list_with_invalid — a colon list
    containing a disallowed cipher is still flagged.
  • Upgrade_checker_Sysvar_check.cipher_single_value_allowed — the single-value
    path still passes.
  • Upgrade_checker_sysvar_registry.load_configuration — extended to assert the
    four cipher variables load with separator == " ,:".

Copyright

This contribution is under the OCA signed by Amazon and covering submissions to
the MySQL project.

The upgrade checker validates Set-typed system variables by splitting the
configured value into individual elements and checking each against the
allowed list. The splitter only recognized space and comma as element
separators, but the cipher list variables -- ssl_cipher, admin_ssl_cipher,
tls_ciphersuites and admin_tls_ciphersuites -- are colon-separated.

As a result, a colon-separated cipher list was compared as a single opaque
string against the list of individual ciphers and never matched, so the
check always reported the whole value as invalid even when every cipher in
it was allowed.

Make the element separator a per-variable property. A new optional
"separator" field in sysvars.json overrides the default space/comma
separator; the four cipher variables set it to " ,:" so each cipher is
validated individually while remaining tolerant of space and comma. The
generator (update_sysvar_configuration.py) gains a matching FORCE_SEPARATORS
table so the field survives regeneration.

Adds unit tests for colon-separated allowed lists, a colon list containing a
disallowed cipher, and the single-value case, plus verification that the
cipher variables load the expected separator.

This contribution is under the OCA signed by Amazon and covering
submissions to the MySQL project.
@mysql-oca-bot

Copy link
Copy Markdown

Hi, thank you for your contribution. Please confirm this code is submitted under the terms of the OCA (Oracle's Contribution Agreement) you have previously signed by cutting and pasting the following text as a comment:
"I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it."
Thanks

@tonychen2001

Copy link
Copy Markdown
Author

I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it.

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.

2 participants