EnxDev commented on code in PR #44723:
URL: https://github.com/apache/superset/pull/44723#discussion_r4137536140
##########
UPDATING.md:
##########
@@ -24,6 +24,27 @@ assists people when migrating to a new version.
## Next
+### MySQL SSL requests require TLS
+
+The MySQL SSL toggle and legacy `ssl=1` URLs require an encrypted connection.
+For mysqlclient (`mysql://` and `mysql+mysqldb://`), Superset translates this
+request to `ssl_mode=VERIFY_CA`, retaining explicit `VERIFY_CA` or
+`VERIFY_IDENTITY` modes. Contradictory options such as `ssl_mode=DISABLED`
+or `ssl_disabled=True` fail rather than cancelling the SSL request.
+
+Certificate verification is needed because mysqlclient built with MariaDB
+Connector/C can fall back to cleartext even with `ssl_mode=REQUIRED`; the
toggle
+upgrades that mode to `VERIFY_CA`. Configure a trusted CA and a server
certificate
+valid for the connection hostname (MariaDB Connector/C also checks identity).
Review Comment:
+1 to rebenitez1802 on spelling out that saved connections with the toggle
on and no CA stop connecting after upgrade. Worth adding that MySQL ignores the
Root certificate field in the connection form (only Postgres, Trino and Druid
read `server_cert`), so the CA has to be a file path present on every web and
worker node.
Either wiring `server_cert` through `create_ssl_cert_file` the way Postgres
does, or saying this plainly here, would save people some head scratching.
##########
docs/user_docs_versioned_docs/version-6.1.0/databases/supported/mysql.mdx:
##########
@@ -191,3 +191,19 @@ export const databaseInfo = {
};
<DatabasePage name="MySQL" database={databaseInfo} />
+
+## TLS connections
Review Comment:
This lands in the frozen 6.1.0 snapshot, so the 6.1 docs will describe
behavior 6.1 doesn't have. The Next page won't show it either, since
`docs/databases/supported/mysql.mdx` is regenerated from the spec's `metadata`
by `generate-database-docs.mjs`.
Could the text move into `MySQLEngineSpec.metadata` (a `notes` entry, or an
SSL item in `connection_examples` like Postgres has) and this file stay
untouched?
##########
superset/db_engine_specs/mysql.py:
##########
@@ -78,6 +80,106 @@
)
+def _mysql_bool_option(options: dict[str, Any], key: str) -> Optional[bool]:
+ """Parse a boolean driver option, treating a blank value as unset."""
+ value = options.get(key)
+ if value is None or value == "":
+ return None
+ return asbool(value) if isinstance(value, str) else bool(value)
+
+
+def _require_pymysql_tls(query: dict[str, Any], args: dict[str, Any]) -> None:
+ """Keep PyMySQL TLS options native so verification applies to them."""
+ pymysql = import_module("pymysql")
+
+ # Older releases silently fall back even with explicit SSL options.
+ if pymysql.VERSION[:2] < (1, 2):
Review Comment:
This gate is the only thing keeping PyMySQL < 1.2 off the silent cleartext
path, and nothing exercises it (it's in the codecov misses).
A test that monkeypatches `pymysql.VERSION` to `(1, 1, 1)` and expects the
ValueError would pin it.
##########
superset/db_engine_specs/mysql.py:
##########
@@ -78,6 +80,106 @@
)
+def _mysql_bool_option(options: dict[str, Any], key: str) -> Optional[bool]:
+ """Parse a boolean driver option, treating a blank value as unset."""
+ value = options.get(key)
+ if value is None or value == "":
+ return None
+ return asbool(value) if isinstance(value, str) else bool(value)
+
+
+def _require_pymysql_tls(query: dict[str, Any], args: dict[str, Any]) -> None:
+ """Keep PyMySQL TLS options native so verification applies to them."""
+ pymysql = import_module("pymysql")
+
+ # Older releases silently fall back even with explicit SSL options.
+ if pymysql.VERSION[:2] < (1, 2):
+ raise ValueError("The MySQL SSL toggle requires PyMySQL >= 1.2")
+ # SQLAlchemy folds URL ssl_ca/cert/key into an ssl dictionary,
+ # but PyMySQL ignores that dictionary when ssl_verify_cert is set.
+ # Keep these as native connect_args so the CA is not discarded.
+ for key in ("ssl_ca", "ssl_cert", "ssl_key"):
+ if key in query:
+ args.setdefault(key, query.pop(key))
+ check_hostname = _mysql_bool_option(query, "ssl_check_hostname")
+ query.pop("ssl_check_hostname", None)
+ if check_hostname is not None:
+ verify_identity = _mysql_bool_option(args, "ssl_verify_identity")
+ if verify_identity not in (None, check_hostname):
+ raise ValueError("MySQL SSL request conflicts with
ssl_verify_identity")
+ args["ssl_verify_identity"] = check_hostname
+ if query.keys() & {"ssl_capath", "ssl_cipher"}:
+ raise ValueError("Unsupported PyMySQL SSL option with the SSL toggle")
+ if "ssl" in args:
+ raise ValueError(
+ "Use individual ssl_ca/ssl_cert/ssl_key options with the SSL
toggle"
+ )
+
+
+def _require_mysql_verified_tls(
+ driver: str, query: dict[str, Any], args: dict[str, Any]
+) -> None:
+ """Use required verification on drivers without an encryption-only mode."""
+ # The drivers test ssl_disabled for truthiness, so a URL string such as
+ # "false" would disable TLS. Parse it here and drop non-disabling values.
+ for source in (query, args):
+ if _mysql_bool_option(source, "ssl_disabled"):
+ raise ValueError("MySQL SSL request conflicts with ssl_disabled")
+ source.pop("ssl_disabled", None)
+ # Connector/Python has no REQUIRED mode: certificate verification is
+ # necessary to prevent its opportunistic fallback to cleartext.
+ if _mysql_bool_option({**query, **args}, "ssl_verify_cert") is False:
+ raise ValueError("MySQL SSL request requires ssl_verify_cert")
+ if driver == "pymysql":
+ _require_pymysql_tls(query, args)
+ args["ssl_verify_cert"] = True
+
+
+def _mysql_ssl_requested(value: Any) -> bool:
+ """Parse scalar requests; leave native SSL dictionaries to the driver."""
+ if isinstance(value, (str, bool, int)):
+ return asbool(value)
+ if value is not None and not isinstance(value, dict):
+ raise ValueError("Invalid MySQL ssl option")
+ return False
+
+
+def require_mysql_tls(
+ uri: URL, connect_args: dict[str, Any]
+) -> tuple[URL, dict[str, Any]]:
+ """Consume scalar ``ssl`` requests without weakening native TLS
settings."""
+ if uri.get_backend_name() != "mysql":
+ return uri, connect_args
+ query = dict(uri.query)
+ args = dict(connect_args)
+ # A true URL request cannot be cancelled by an advanced connect argument.
+ requested = _mysql_ssl_requested(query.get("ssl"))
+ requested = _mysql_ssl_requested(args.get("ssl")) or requested
+ if not requested:
+ return uri, connect_args
+
+ for options in (query, args):
+ if isinstance(options.get("ssl"), (str, bool, int)):
+ options.pop("ssl")
+ driver = uri.get_driver_name()
+ options = {**query, **args}
+ if driver == "mysqldb":
+ # mysqlclient maps REQUIRED to opportunistic TLS with MariaDB
+ # Connector/C. Verification modes fail closed on both client libraries.
+ mode = options.get("ssl_mode", "VERIFY_CA")
Review Comment:
The toggle alone lands on VERIFY_CA regardless of client library, but
REQUIRED is only leaky on MariaDB Connector/C. On Oracle's libmysqlclient
REQUIRED already fails closed, so installs linking it break at upgrade without
a CA for no security gain, and the MySQL toggle ends up stricter than the
Postgres one (`sslmode=require`).
Would it work to pick the mode off `MySQLdb.get_client_info()` (Connector/C
reports 3.x) and only force VERIFY_CA there?
##########
superset/db_engine_specs/mysql.py:
##########
@@ -78,6 +80,106 @@
)
+def _mysql_bool_option(options: dict[str, Any], key: str) -> Optional[bool]:
+ """Parse a boolean driver option, treating a blank value as unset."""
+ value = options.get(key)
+ if value is None or value == "":
+ return None
+ return asbool(value) if isinstance(value, str) else bool(value)
+
+
+def _require_pymysql_tls(query: dict[str, Any], args: dict[str, Any]) -> None:
+ """Keep PyMySQL TLS options native so verification applies to them."""
+ pymysql = import_module("pymysql")
+
+ # Older releases silently fall back even with explicit SSL options.
+ if pymysql.VERSION[:2] < (1, 2):
+ raise ValueError("The MySQL SSL toggle requires PyMySQL >= 1.2")
+ # SQLAlchemy folds URL ssl_ca/cert/key into an ssl dictionary,
+ # but PyMySQL ignores that dictionary when ssl_verify_cert is set.
+ # Keep these as native connect_args so the CA is not discarded.
+ for key in ("ssl_ca", "ssl_cert", "ssl_key"):
+ if key in query:
+ args.setdefault(key, query.pop(key))
+ check_hostname = _mysql_bool_option(query, "ssl_check_hostname")
+ query.pop("ssl_check_hostname", None)
+ if check_hostname is not None:
+ verify_identity = _mysql_bool_option(args, "ssl_verify_identity")
+ if verify_identity not in (None, check_hostname):
+ raise ValueError("MySQL SSL request conflicts with
ssl_verify_identity")
+ args["ssl_verify_identity"] = check_hostname
+ if query.keys() & {"ssl_capath", "ssl_cipher"}:
+ raise ValueError("Unsupported PyMySQL SSL option with the SSL toggle")
+ if "ssl" in args:
+ raise ValueError(
+ "Use individual ssl_ca/ssl_cert/ssl_key options with the SSL
toggle"
+ )
+
+
+def _require_mysql_verified_tls(
+ driver: str, query: dict[str, Any], args: dict[str, Any]
+) -> None:
+ """Use required verification on drivers without an encryption-only mode."""
+ # The drivers test ssl_disabled for truthiness, so a URL string such as
+ # "false" would disable TLS. Parse it here and drop non-disabling values.
+ for source in (query, args):
+ if _mysql_bool_option(source, "ssl_disabled"):
+ raise ValueError("MySQL SSL request conflicts with ssl_disabled")
+ source.pop("ssl_disabled", None)
+ # Connector/Python has no REQUIRED mode: certificate verification is
+ # necessary to prevent its opportunistic fallback to cleartext.
+ if _mysql_bool_option({**query, **args}, "ssl_verify_cert") is False:
+ raise ValueError("MySQL SSL request requires ssl_verify_cert")
+ if driver == "pymysql":
+ _require_pymysql_tls(query, args)
+ args["ssl_verify_cert"] = True
+
+
+def _mysql_ssl_requested(value: Any) -> bool:
+ """Parse scalar requests; leave native SSL dictionaries to the driver."""
+ if isinstance(value, (str, bool, int)):
+ return asbool(value)
+ if value is not None and not isinstance(value, dict):
+ raise ValueError("Invalid MySQL ssl option")
+ return False
+
+
+def require_mysql_tls(
+ uri: URL, connect_args: dict[str, Any]
+) -> tuple[URL, dict[str, Any]]:
+ """Consume scalar ``ssl`` requests without weakening native TLS
settings."""
+ if uri.get_backend_name() != "mysql":
+ return uri, connect_args
+ query = dict(uri.query)
+ args = dict(connect_args)
+ # A true URL request cannot be cancelled by an advanced connect argument.
+ requested = _mysql_ssl_requested(query.get("ssl"))
+ requested = _mysql_ssl_requested(args.get("ssl")) or requested
+ if not requested:
+ return uri, connect_args
+
+ for options in (query, args):
+ if isinstance(options.get("ssl"), (str, bool, int)):
+ options.pop("ssl")
+ driver = uri.get_driver_name()
+ options = {**query, **args}
+ if driver == "mysqldb":
+ # mysqlclient maps REQUIRED to opportunistic TLS with MariaDB
+ # Connector/C. Verification modes fail closed on both client libraries.
+ mode = options.get("ssl_mode", "VERIFY_CA")
+ if mode not in ("REQUIRED", "VERIFY_CA", "VERIFY_IDENTITY"):
+ raise ValueError("MySQL SSL request conflicts with ssl_mode")
+ args["ssl_mode"] = "VERIFY_CA" if mode == "REQUIRED" else mode
Review Comment:
With an SSH tunnel, `get_sqla_engine` has already rewritten the host to the
local bind address before this runs. The official image builds mysqlclient
against MariaDB Connector/C (`default-libmysqlclient-dev` on trixie), where
VERIFY_CA also checks the hostname, so tunneled connections with the toggle on
will fail the cert check against 127.0.0.1 after upgrade.
Is the intended answer "turn the toggle off when tunneling"? If so it
belongs in UPDATING and the docs, since people won't find it from the error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
