From 04bccec51e697d85eb27754503cbcf82228cdd80 Mon Sep 17 00:00:00 2001 From: Rene Cannao Date: Mon, 27 Apr 2026 03:15:21 +0000 Subject: [PATCH] chore(plugin-chassis): tighten gating, drop dead paths, gate forgery setters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five clean-up items from the independent review of PR #5651. None of these change behaviour on a normal run; they each fix a concrete way the current code is misleading or unnecessarily exposed. 1) lib/ProxySQL_Admin.cpp: gate the three plugin-DB-handle getters (proxysql_plugin_get_admindb / _configdb / _statsdb) under #ifdef PROXYSQL40. Previously these were defined unconditionally and emitted symbols into v3.0/v3.1 binaries. The chassis is a v4.0 feature; the user explicitly required that v3.x builds carry no plugin-aware code. Wrap in PROXYSQL40 so they are entirely absent from v3.x linkage. ProxySQL_PluginManager.cpp's extern declarations (lines 23-25) are already inside the file-wide PROXYSQL40 gate and so resolve only when the gate is active. 2) lib/ProxySQL_PluginManager.cpp + include/ProxySQL_PluginManager.h: delete the dead `#else /* !PROXYSQL40 */` branches. Both files are wrapped in a top-level `#ifdef PROXYSQL40` covering the entire body. Inner `#ifdef PROXYSQL40 ... #else ... #endif` blocks therefore had unreachable `#else` arms — 30+ lines of "pre-chassis two-phase loader" in the .cpp, plus a redundant declaration of proxysql_load_configured_plugins in the .h. The dead arms read as load-bearing alternative implementations on review and that is exactly the wrong signal. Drop them; the single PROXYSQL40 path is the only one. 3) lib/Admin_Bootstrap.cpp + include/proxysql_admin.h + src/main.cpp: remove ProxySQL_Admin::materialize_plugin_tables(). `Admin::init()` already merges plugin-declared schemas into tables_defs_{admin,config,stats} (~line 944) and runs the DDL via check_and_build_standard_tables() (~line 994), all on the same first-boot/reload code path the core tables use. The follow-up call to GloAdmin->materialize_plugin_tables() in main.cpp ran a second name-dedup pass that found everything already present and produced an empty new-rows set — i.e. the post-init helper was a no-op disguised as load-bearing infrastructure. Delete the helper, the header declaration, and the main.cpp call site, and update the comments in main.cpp + ProxySQL_PluginManager.cpp to point at Admin::init() as the single canonical materialization site. Leave a NOTE in Admin_Bootstrap.cpp at the old call site so anyone re-adding a similar helper sees why the prior one was removed. 4) plugins/mysqlx/include/mysqlx_session.h + plugins/mysqlx/src/mysqlx_session.cpp: gate the two genuine forgery vectors behind MYSQLX_TEST_BUILD. inject_identity_for_test() bypasses the full auth flow — no credential check, no capability negotiation, just sets identity_ to a caller-supplied MysqlxResolvedIdentity. resolve_backend_target_ for_test() drives a private routing helper without an authenticated identity. Both are necessary for unit tests but should not be reachable at all in shipped binaries; an in-process exploit reaching the session can call inject_identity_for_test() to forge an authenticated identity. Wrap them in #ifdef MYSQLX_TEST_BUILD; define -DMYSQLX_TEST_BUILD in the test Makefile only. The remaining target_*_for_test() getters are read-only state observers and are left unconditional — they cannot mutate the session and a debugger could observe the same state regardless. 5) test/tap/tests/unit/Makefile: define -DMYSQLX_TEST_BUILD on every unit test compile line via OPT. This is the test-only knob that re-enables the gated forgery methods so unit tests still compile. plugins/mysqlx/Makefile does NOT define this macro, so the production .so does not compile in the entry points. Verified locally: - plugins/mysqlx/ProxySQL_MySQLX_Plugin.so builds clean with PROXYSQL40=1 PROXYSQL31=1 PROXYSQLFFTO=1 PROXYSQLTSDB=1 PROXYSQLGENAI=1 (no MYSQLX_TEST_BUILD). - test/tap/tests/unit/mysqlx_robustness_unit-t builds and runs: 74/74 assertions pass, including the ones that exercise inject_identity_for_test (visible only because the test Makefile defines MYSQLX_TEST_BUILD). --- include/ProxySQL_PluginManager.h | 9 --- include/proxysql_admin.h | 3 - lib/Admin_Bootstrap.cpp | 73 ++++--------------------- lib/ProxySQL_Admin.cpp | 7 +++ lib/ProxySQL_PluginManager.cpp | 57 ++++--------------- plugins/mysqlx/include/mysqlx_session.h | 14 +++++ plugins/mysqlx/src/mysqlx_session.cpp | 2 + src/main.cpp | 8 +-- test/tap/tests/unit/Makefile | 11 +++- 9 files changed, 59 insertions(+), 125 deletions(-) diff --git a/include/ProxySQL_PluginManager.h b/include/ProxySQL_PluginManager.h index 51d48afe1..139e227cb 100644 --- a/include/ProxySQL_PluginManager.h +++ b/include/ProxySQL_PluginManager.h @@ -124,7 +124,6 @@ bool proxysql_dispatch_configured_plugin_admin_command( const std::string& sql, ProxySQL_PluginCommandResult& result ); -#ifdef PROXYSQL40 bool proxysql_dispatch_configured_plugin_query_hook( ProxySQL_PluginProtocol proto, const ProxySQL_PluginQueryHookPayload& payload, @@ -160,14 +159,6 @@ bool proxysql_init_configured_plugins( ProxySQL_PluginManager* manager, std::string& err ); -#else /* !PROXYSQL40 */ -// Pre-chassis two-phase load: dlopen + init_all in one call. -bool proxysql_load_configured_plugins( - std::unique_ptr& manager, - const std::vector& plugin_modules, - std::string& err -); -#endif /* PROXYSQL40 */ bool proxysql_start_configured_plugins( ProxySQL_PluginManager* manager, std::string& err diff --git a/include/proxysql_admin.h b/include/proxysql_admin.h index b296b733a..49928fb44 100644 --- a/include/proxysql_admin.h +++ b/include/proxysql_admin.h @@ -635,9 +635,6 @@ class ProxySQL_Admin { * @return Always true. */ bool init(const bootstrap_info_t& bootstrap_info); -#ifdef PROXYSQL40 - void materialize_plugin_tables(); -#endif /* PROXYSQL40 */ void init_ldap(); /** @brief Initializes the HTTP server. For safety should be called after 'phase3'. */ void init_http_server(); diff --git a/lib/Admin_Bootstrap.cpp b/lib/Admin_Bootstrap.cpp index 837351681..223571037 100644 --- a/lib/Admin_Bootstrap.cpp +++ b/lib/Admin_Bootstrap.cpp @@ -1347,64 +1347,15 @@ bool ProxySQL_Admin::init(const bootstrap_info_t& bootstrap_info) { return true; }; -#ifdef PROXYSQL40 -void ProxySQL_Admin::materialize_plugin_tables() { - ProxySQL_PluginManager* plugin_manager = proxysql_get_plugin_manager(); - if (!plugin_manager) return; - - // merge_new returns the subset of `defs` that were actually added - // (skipping entries whose table_name already exists in `target`). - // The caller runs DDL for only this subset so a repeat invocation - // would not re-execute CREATE TABLE statements for tables that - // already exist in the SQLite database. - auto merge_new = [this]( - std::vector* target, - const std::vector& defs - ) -> std::vector { - std::vector added; - added.reserve(defs.size()); - for (const auto& def : defs) { - bool exists = false; - for (const auto* existing : *target) { - if (strcasecmp(existing->table_name, def.table_name) == 0) { - exists = true; - break; - } - } - if (!exists) { - insert_into_tables_defs(target, def.table_name, def.table_def); - added.push_back(&def); - } - } - return added; - }; - - const auto new_admin = merge_new(tables_defs_admin, plugin_manager->tables(ProxySQL_PluginDBKind::admin_db)); - const auto new_config = merge_new(tables_defs_config, plugin_manager->tables(ProxySQL_PluginDBKind::config_db)); - const auto new_stats = merge_new(tables_defs_stats, plugin_manager->tables(ProxySQL_PluginDBKind::stats_db)); - - // Plugin-declared DDL failures are startup-fatal: a partially - // materialized schema would only surface later as opaque runtime - // errors from the plugin itself. Log the plugin, table name, and - // SQLite message, then abort. - auto run_ddl = [](SQLite3DB* db, const std::vector& defs, const char* kind) { - for (const auto* def : defs) { - if (!db->execute(def->table_def)) { - proxy_error("Plugin %s-db DDL failed for table %s: %s\n", - kind, - def->table_name ? def->table_name : "(null)", - def->table_def ? def->table_def : "(null)"); - exit(EXIT_FAILURE); - } - } - }; - run_ddl(admindb, new_admin, "admin"); - run_ddl(configdb, new_config, "config"); - run_ddl(statsdb, new_stats, "stats"); - - // admindb<->configdb("disk") and admindb<->statsdb("stats") are - // already attached by init_sqlite3db() above this function in the - // startup flow; duplicate ATTACH errors out in SQLite. Do NOT - // re-attach them here. -} -#endif /* PROXYSQL40 */ +// NOTE: materialize_plugin_tables() used to live here. It was removed +// because plugin tables are already merged into tables_defs_{admin, +// config,stats} by ProxySQL_Admin::init() (~line 944) and the DDL is +// already executed by check_and_build_standard_tables() (~line 994) on +// the same path. Calling materialize_plugin_tables() afterwards from +// src/main.cpp would re-run a name-dedup pass that found everything +// already present and produced an empty new-rows set, then run no DDL — +// dead code that read as load-bearing. The merge in Admin::init() is +// the single canonical site for plugin-table materialization. If a +// future change wants Admin to expose plugin-tables for live reload +// (admindb merge after init returns), reintroduce a function with a +// clear contract; do not resurrect the previous post-init no-op. diff --git a/lib/ProxySQL_Admin.cpp b/lib/ProxySQL_Admin.cpp index 284aaf6a5..7d2bec1f6 100644 --- a/lib/ProxySQL_Admin.cpp +++ b/lib/ProxySQL_Admin.cpp @@ -339,6 +339,12 @@ extern AI_Features_Manager *GloAI; extern void (*flush_logs_function)(); +#ifdef PROXYSQL40 +// Plugin DB-handle getters consumed by ProxySQL_PluginManager when it +// builds the Phase-B services struct. Gated under PROXYSQL40 so v3.x +// builds export no plugin-aware symbols at all (the chassis is a v4.0 +// feature; in v3.0/v3.1 these would be unused-but-reachable symbols +// that contradict the "chassis fully invisible to v3.x" requirement). SQLite3DB* proxysql_plugin_get_admindb() { return GloAdmin ? GloAdmin->admindb : nullptr; } @@ -350,6 +356,7 @@ SQLite3DB* proxysql_plugin_get_configdb() { SQLite3DB* proxysql_plugin_get_statsdb() { return GloAdmin ? GloAdmin->statsdb : nullptr; } +#endif /* PROXYSQL40 */ extern Web_Interface *GloWebInterface; diff --git a/lib/ProxySQL_PluginManager.cpp b/lib/ProxySQL_PluginManager.cpp index c8b3fab05..d96046bf2 100644 --- a/lib/ProxySQL_PluginManager.cpp +++ b/lib/ProxySQL_PluginManager.cpp @@ -609,14 +609,10 @@ bool ProxySQL_PluginManager::dispatch_admin_command(const ProxySQL_PluginCommand } proxy_debug(PROXY_DEBUG_ADMIN, 4, "Dispatching plugin command: %s (via %s)\n", command.sql.c_str(), normalized_sql.c_str()); -#ifdef PROXYSQL40 // Pass the CANONICAL form to the callback so plugins can ignore // which alias the user typed — they match on their own canonical // strings only. result = command.cb(ctx, command.sql.c_str()); -#else - result = command.cb(ctx, normalized_sql.c_str()); -#endif /* PROXYSQL40 */ return true; } @@ -886,14 +882,14 @@ bool proxysql_load_configured_plugins( const std::vector& plugin_modules, std::string& err ) { -#ifdef PROXYSQL40 // Phase A + Phase B of the four-phase lifecycle. Executed BEFORE // ProxySQL_Main_init_Admin_module so that plugin-declared schemas are - // available when merge_plugin_tables materializes the SQLite schema. + // available when Admin::init() merges them into tables_defs_* and + // runs the DDL via check_and_build_standard_tables. // // On return, `manager` is populated and installed as the active - // manager — GloAdmin->materialize_plugin_tables() can then query it - // for the tables to CREATE. Phase D (init) runs later, via + // manager — Admin::init() reads it via proxysql_get_plugin_manager() + // to find the tables to merge. Phase D (init) runs later, via // proxysql_init_configured_plugins, once admin is up. std::lock_guard lifecycle_lock(g_plugin_lifecycle_mutex); err.clear(); @@ -917,17 +913,19 @@ bool proxysql_load_configured_plugins( // Phase B: register_schemas runs here. Plugins declare their // admin-schema tables into the manager's pending-tables list. - // GloAdmin->materialize_plugin_tables() (called after + // ProxySQL_Admin::init() (called next, via // ProxySQL_Main_init_Admin_module in src/main.cpp) drains that list - // into SQLite via the merge_plugin_tables code path. Plugins that - // left register_schemas null are no-ops here. + // by merging into tables_defs_{admin,config,stats} and then running + // the DDL via check_and_build_standard_tables — same code path as + // the core tables. Plugins that left register_schemas null are + // no-ops here. if (!next_manager->invoke_register_schemas_phase(err)) { return false; } // Install as active manager BEFORE admin init, so that - // proxysql_get_plugin_manager() — used by - // ProxySQL_Admin::materialize_plugin_tables — can find the + // proxysql_get_plugin_manager() — used by ProxySQL_Admin::init() to + // merge plugin-declared schemas into tables_defs_* — can find the // registered tables. // // INVARIANT (publish-before-Phase-D): after this point Phase D @@ -946,39 +944,6 @@ bool proxysql_load_configured_plugins( g_active_plugin_manager.store(manager.get(), std::memory_order_release); } return true; -#else /* !PROXYSQL40 */ - // Pre-chassis two-phase: load + init_all in one call, installed as - // active manager only on full success. - err.clear(); - { - std::lock_guard lock(g_active_plugin_manager_mutex); - g_active_plugin_manager.store(nullptr, std::memory_order_release); - } - manager.reset(); - - if (plugin_modules.empty()) { - return true; - } - - auto next_manager = std::make_unique(); - for (const auto& path : plugin_modules) { - if (!next_manager->load(path, err)) { - err = path + ": " + err; - return false; - } - } - - if (!next_manager->init_all(err)) { - return false; - } - - { - std::lock_guard lock(g_active_plugin_manager_mutex); - manager = std::move(next_manager); - g_active_plugin_manager.store(manager.get(), std::memory_order_release); - } - return true; -#endif /* PROXYSQL40 */ } #ifdef PROXYSQL40 diff --git a/plugins/mysqlx/include/mysqlx_session.h b/plugins/mysqlx/include/mysqlx_session.h index 6e531e9ae..493507bbc 100644 --- a/plugins/mysqlx/include/mysqlx_session.h +++ b/plugins/mysqlx/include/mysqlx_session.h @@ -119,9 +119,23 @@ public: // string overload is a convenience wrapper that fetches the identity // from the thread's configured MysqlxConfigStore, mimicking what the // auth handler does when a real client connects. + // + // inject_identity_for_test and resolve_backend_target_for_test are + // gated behind MYSQLX_TEST_BUILD because they are forgery vectors: + // inject_identity_for_test bypasses the full auth flow (no credential + // check, no cap negotiation), and resolve_backend_target_for_test + // drives a private routing helper without an authenticated identity. + // The test Makefile defines MYSQLX_TEST_BUILD; the production .so + // build does not, so these methods do not exist in shipped binaries. + // The remaining target_*_for_test getters are read-only state + // observers and are left available unconditionally (they leak no + // state a debugger could not also observe and cannot mutate the + // session). +#ifdef MYSQLX_TEST_BUILD void inject_identity_for_test(const MysqlxResolvedIdentity& id) { identity_ = id; } void inject_identity_for_test(const std::string& username); int resolve_backend_target_for_test() { return resolve_backend_target(); } +#endif /* MYSQLX_TEST_BUILD */ int target_hostgroup_for_test() const { return target_hostgroup_; } const std::string& target_address_for_test() const { return target_address_; } int target_port_for_test() const { return target_port_; } diff --git a/plugins/mysqlx/src/mysqlx_session.cpp b/plugins/mysqlx/src/mysqlx_session.cpp index acd63331b..8d4705d3b 100644 --- a/plugins/mysqlx/src/mysqlx_session.cpp +++ b/plugins/mysqlx/src/mysqlx_session.cpp @@ -1016,6 +1016,7 @@ int MysqlxSession::resolve_backend_target() { return 0; } +#ifdef MYSQLX_TEST_BUILD // Test-only convenience overload. Mirrors what the auth handler does on a // real client connection: look up the identity via the thread's config // store, caching the result in identity_. Silently no-ops if the thread @@ -1028,6 +1029,7 @@ void MysqlxSession::inject_identity_for_test(const std::string& username) { auto id = cs->resolve_identity(username); if (id) identity_ = *id; } +#endif /* MYSQLX_TEST_BUILD */ void MysqlxSession::handler_connecting_server() { if (!backend_conn_) { diff --git a/src/main.cpp b/src/main.cpp index 633fa6f61..a73985405 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1561,15 +1561,15 @@ void ProxySQL_Main_init_phase2___not_started(const bootstrap_info_t& boostrap_in // loader at all): // Phase A+B: dlopen + register_schemas (plugin-declared schemas // populate the pending-tables list). - // Phase C: admin module init + materialize_plugin_tables (creates - // plugin-owned SQLite tables through the - // merge_plugin_tables path — first-boot == reload). + // Phase C: admin module init merges plugin-declared schemas into + // tables_defs_{admin,config,stats} and runs the DDL via + // check_and_build_standard_tables, all on the same + // first-boot/reload code path as the core tables. // Phase D: init() with full services (live DB handles pointing at // a schema that already contains the plugin's own tables). // Phase E: start() launches the plugin's threads / accept loops. LoadConfiguredPlugins(); ProxySQL_Main_init_Admin_module(boostrap_info); - GloAdmin->materialize_plugin_tables(); InitConfiguredPlugins(); StartConfiguredPlugins(); #else /* !PROXYSQL40 */ diff --git a/test/tap/tests/unit/Makefile b/test/tap/tests/unit/Makefile index 9a4b4eb05..2c23a2193 100644 --- a/test/tap/tests/unit/Makefile +++ b/test/tap/tests/unit/Makefile @@ -232,13 +232,20 @@ ifneq ($(shell nm $(LIBPROXYSQLAR) 2>/dev/null | grep -cw 'init_debug_struct'),0 PSQLDEBUG := -DDEBUG endif +# MYSQLX_TEST_BUILD enables the test-only methods (`*_for_test`) on the +# mysqlx plugin's public classes (e.g. MysqlxSession::inject_identity_for_test). +# Defined ONLY here, in the unit-test Makefile — never in plugins/mysqlx/Makefile. +# That way the production .so does not compile in entry points that could be +# misused (most importantly identity-forgery setters), while tests that include +# the plugin headers and link against the plugin sources still see the helpers +# they need. OPT := $(STDCPP) -O0 -ggdb $(PSQLCH) $(PSQLGA) $(PSQL40) $(PSQL31) $(PSQLFFTO) $(PSQLTSDB) $(PSQLDEBUG) \ - -DGITVERSION=\"$(GIT_VERSION)\" $(NOJEM) $(WGCOV) $(WASAN) \ + -DGITVERSION=\"$(GIT_VERSION)\" -DMYSQLX_TEST_BUILD $(NOJEM) $(WGCOV) $(WASAN) \ -Wl,--no-as-needed -Wl,-rpath,$(TAP_LDIR) ifeq ($(UNAME_S),Darwin) OPT := $(STDCPP) -O0 -ggdb $(PSQLCH) $(PSQLGA) $(PSQL40) $(PSQL31) $(PSQLFFTO) $(PSQLTSDB) $(PSQLDEBUG) \ - -DGITVERSION=\"$(GIT_VERSION)\" $(NOJEM) $(WGCOV) $(WASAN) + -DGITVERSION=\"$(GIT_VERSION)\" -DMYSQLX_TEST_BUILD $(NOJEM) $(WGCOV) $(WASAN) endif