From 040c67788e7a18fb599df68c4149412d75327a6d Mon Sep 17 00:00:00 2001 From: Tony Chen Date: Wed, 15 Jul 2026 21:21:41 +0000 Subject: [PATCH] Fix upgrade pre-check for colon-separated cipher lists 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. --- modules/util/upgrade_checker/sysvar_check.cc | 11 ++- modules/util/upgrade_checker/sysvar_check.h | 2 + res/upgrade_checker/sysvars.json | 6 +- .../update_sysvar_configuration.py | 15 +++ .../upgrade_checker/sysvar_upgrade_check_t.cc | 99 ++++++++++++++++++- 5 files changed, 129 insertions(+), 4 deletions(-) diff --git a/modules/util/upgrade_checker/sysvar_check.cc b/modules/util/upgrade_checker/sysvar_check.cc index 4df55e61c..492cd0d5e 100644 --- a/modules/util/upgrade_checker/sysvar_check.cc +++ b/modules/util/upgrade_checker/sysvar_check.cc @@ -378,6 +378,8 @@ constexpr const char *k_default = "default"; constexpr const char *k_allowed = "allowed"; constexpr const char *k_forbidden = "forbidden"; constexpr const char *k_vartype = "vartype"; +constexpr const char *k_separator = "separator"; +constexpr const char *k_default_set_separator = " ,"; std::vector parse_string_list(const rapidjson::Value &source) { std::vector result; @@ -511,6 +513,10 @@ void parse_sysvar(const rapidjson::Value &source, Sysvar_definition *sysvar) { sysvar->replacement = get_string(source["replacement"]); } + if (source.HasMember(k_separator)) { + sysvar->separator = get_string(source[k_separator]); + } + // Parses the initial values if (source.HasMember("initial")) { parse_configuration(source["initial"], &sysvar->initials); @@ -680,6 +686,7 @@ std::optional Sysvar_definition::get_check( } cnf.vartype = vartype; + cnf.separator = separator; return cnf; } @@ -755,7 +762,7 @@ std::vector Sysvar_check::get_invalid_allowed_values( } return true; }, - " ,"); + sysvar_check.separator.value_or(k_default_set_separator)); } else if (!cont_contains(sysvar_check.allowed_values, sysvar->value)) { invalid_values.push_back(sysvar->value); } @@ -780,7 +787,7 @@ std::vector Sysvar_check::get_invalid_forbidden_values( } return true; }, - " ,"); + sysvar_check.separator.value_or(k_default_set_separator)); } else if (cont_contains(sysvar_check.forbidden_values, sysvar->value)) { invalid_values.push_back(sysvar->value); } diff --git a/modules/util/upgrade_checker/sysvar_check.h b/modules/util/upgrade_checker/sysvar_check.h index f8e2fcbfa..53b6d6134 100644 --- a/modules/util/upgrade_checker/sysvar_check.h +++ b/modules/util/upgrade_checker/sysvar_check.h @@ -106,6 +106,7 @@ struct Sysvar_definition { Sysvar_configuration initials; std::map changes; std::optional replacement; + std::optional separator; void set_default(std::string value, const std::optional &version = {}, size_t bits = 0, @@ -136,6 +137,7 @@ struct Sysvar_version_check { std::optional removal_version; std::optional deprecation_version; std::optional vartype; + std::optional separator; }; class Sysvar_registry { diff --git a/res/upgrade_checker/sysvars.json b/res/upgrade_checker/sysvars.json index 6f90387a6..1f913e244 100644 --- a/res/upgrade_checker/sysvars.json +++ b/res/upgrade_checker/sysvars.json @@ -235,6 +235,7 @@ }, { "name": "admin_ssl_cipher", + "separator": " ,:", "version": "8.0.21", "initial": { "all": { @@ -273,6 +274,7 @@ }, { "name": "admin_tls_ciphersuites", + "separator": " ,:", "version": "8.0.21", "initial": { "all": { @@ -5400,6 +5402,7 @@ }, { "name": "ssl_cipher", + "separator": " ,:", "version": "5.0.23", "initial": { "all": { @@ -5803,6 +5806,7 @@ }, { "name": "tls_ciphersuites", + "separator": " ,:", "version": "8.0.16", "initial": { "all": { @@ -6165,4 +6169,4 @@ "name": "master_info_file", "removal": "8.3.0" } -] \ No newline at end of file +] diff --git a/scripts/upgrade_checker/update_sysvar_configuration.py b/scripts/upgrade_checker/update_sysvar_configuration.py index 2b28ce227..78ce2b39b 100644 --- a/scripts/upgrade_checker/update_sysvar_configuration.py +++ b/scripts/upgrade_checker/update_sysvar_configuration.py @@ -41,6 +41,17 @@ "admin_tls_ciphersuites": "set", } +# Set-typed variables whose elements are separated by characters other than the +# default space/comma. The upgrade checker splits the value on ANY of these +# characters before validating each element against the allowed list. Cipher +# lists are colon-separated but also accept comma/space, so they carry " ,:". +FORCE_SEPARATORS={ + "ssl_cipher": " ,:", + "admin_ssl_cipher": " ,:", + "tls_ciphersuites": " ,:", + "admin_tls_ciphersuites": " ,:", +} + class Sysvar_definition_error(Exception): def __init__(self, *args: object) -> None: super().__init__(*args) @@ -768,6 +779,10 @@ def shell_config(self): config = {} config["name"] = self.name + separator = FORCE_SEPARATORS.get(self.name) + if separator is not None: + config["separator"] = separator + if self.introduced: config["version"] = self.introduced diff --git a/unittest/modules/util/upgrade_checker/sysvar_upgrade_check_t.cc b/unittest/modules/util/upgrade_checker/sysvar_upgrade_check_t.cc index 96f003eaa..02f768152 100644 --- a/unittest/modules/util/upgrade_checker/sysvar_upgrade_check_t.cc +++ b/unittest/modules/util/upgrade_checker/sysvar_upgrade_check_t.cc @@ -32,6 +32,7 @@ #include "unittest/modules/util/upgrade_checker/test_utils.h" #include "unittest/test_utils.h" #include "unittest/test_utils/mocks/mysqlshdk/libs/db/mock_session.h" +#include "unittest/test_utils/mocks/mysqlshdk/libs/db/mock_session_pool.h" namespace mysqlsh { namespace upgrade_checker { @@ -1077,7 +1078,103 @@ TEST(Upgrade_checker_sysvar_registry, load_configuration) { EXPECT_STREQ("0", check->forbidden_values[0].c_str()); } + + // Verifies the cipher list variables carry a separator that includes the + // colon. These variables hold colon-separated cipher lists, so the Set-value + // splitter must split on ':' (in addition to space/comma) before validating + // each cipher against the allowed list. See sysvar_check.cc split helpers. + { + for (const auto &name : + {"ssl_cipher", "admin_ssl_cipher", "tls_ciphersuites", + "admin_tls_ciphersuites"}) { + auto sysvar = reg.get_sysvar(name); + ASSERT_TRUE(sysvar.separator.has_value()) + << "missing separator for " << name; + EXPECT_STREQ(" ,:", sysvar.separator.value().c_str()) + << "unexpected separator for " << name; + + auto ui = upgrade_info(Version(8, 4, 0), Version(9, 7, 0)); + auto check = sysvar.get_check(ui); + ASSERT_TRUE(check.has_value()) << "no check produced for " << name; + ASSERT_TRUE(check->separator.has_value()) + << "separator not carried into check for " << name; + EXPECT_STREQ(" ,:", check->separator.value().c_str()); + } + } +} + +namespace { +// Runs the sysvar upgrade check against a cache pre-populated with the given +// cipher values and returns the resulting issues. The cache is filled via +// override_sysvar before run(), so Checker_cache::cache_sysvars() short-circuits +// (it early-returns on a non-empty cache) and the mock session is never queried. +std::vector run_cipher_check( + const std::vector> &cipher_values) { + auto server_info = upgrade_info(Version(8, 4, 0), Version(9, 7, 0)); + + Sysvar_check check(server_info); + + auto msession = std::make_shared(); + auto mock_pool = std::make_shared(); + mock_pool->setup_repeated_session(msession); + + Upgrade_check_options options; + Checker_cache cache(options.filters); + for (const auto &[name, value] : cipher_values) { + override_sysvar(&cache, name, value); + } + + return check.run( + {msession, server_info, + std::dynamic_pointer_cast(mock_pool), + &cache}); +} + +// Counts issues whose schema matches the given sysvar name (issue.schema is set +// to the variable name by Sysvar_check::get_issue). +size_t count_issues_for(const std::vector &issues, + const std::string &name) { + size_t count = 0; + for (const auto &issue : issues) { + if (issue.schema == name) ++count; + } + return count; +} +} // namespace + +// Regression test for the colon-separated cipher list false positive: a +// ssl_cipher / tls_ciphersuites value that is a colon-separated list of +// individually-allowed ciphers must NOT be reported as invalid. Before the +// separator fix the whole colon-joined string was compared against the list of +// individual ciphers and always failed. +TEST(Upgrade_checker_Sysvar_check, cipher_colon_list_allowed) { + auto issues = run_cipher_check( + {{"ssl_cipher", + "ECDHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES128-GCM-SHA256"}, + {"tls_ciphersuites", + "TLS_AES_128_GCM_SHA256:TLS_AES_256_GCM_SHA384"}}); + + EXPECT_EQ(0, count_issues_for(issues, "ssl_cipher")); + EXPECT_EQ(0, count_issues_for(issues, "tls_ciphersuites")); +} + +// A colon-separated list that contains a cipher which is NOT in the allowed +// list must still be flagged +TEST(Upgrade_checker_Sysvar_check, cipher_colon_list_with_invalid) { + // RC4-MD5 is a valid OpenSSL cipher name but is not in the 8.4 allowed list. + auto issues = run_cipher_check( + {{"ssl_cipher", "ECDHE-RSA-AES128-GCM-SHA256:RC4-MD5"}}); + + EXPECT_EQ(1, count_issues_for(issues, "ssl_cipher")); +} + +// A single allowed cipher (no separator present in the value) must pass +TEST(Upgrade_checker_Sysvar_check, cipher_single_value_allowed) { + auto issues = + run_cipher_check({{"ssl_cipher", "ECDHE-RSA-AES128-GCM-SHA256"}}); + + EXPECT_EQ(0, count_issues_for(issues, "ssl_cipher")); } } // namespace upgrade_checker -} // namespace mysqlsh \ No newline at end of file +} // namespace mysqlsh