aminghadersohi commented on code in PR #44723:
URL: https://github.com/apache/superset/pull/44723#discussion_r4139574830
##########
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:
Fixed in 485c4272bc. UPDATING and MySQLEngineSpec.metadata explicitly cover
saved connections at upgrade, verification failures, the unused Root
certificate/server_cert field, and ssl_ca files on every web and worker node.
They describe both CA configuration and opting into native settings without the
toggle. test_native_tls_without_toggle_is_unchanged covers the
native-dictionary recovery path.
##########
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:
Fixed in 485c4272bc. Restored the frozen 6.1.0 page to match master and
moved the TLS guidance into MySQLEngineSpec.metadata.notes. Ran
generate-database-docs.mjs and verified the generated Next MySQL page includes
the CA, Aurora and SSH guidance; no hand-edited generated page is needed.
##########
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:
Fixed in 485c4272bc. test_pymysql_old_version_rejected monkeypatches
pymysql.VERSION to (1, 1, 1) and asserts the PyMySQL >= 1.2 ValueError. The
MySQL and IAM unit suites pass (146 tests).
##########
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:
Fixed in 485c4272bc. The default and explicit REQUIRED modes use
MySQLdb.get_client_info(): recognized Oracle 5.7/8.x/9.x clients retain
REQUIRED, while MariaDB Connector/C and unrecognized versions use VERIFY_CA
conservatively. Explicit verification modes are preserved. Parameterized tests
cover client families, URI/toggle/connect_args requests, and explicit REQUIRED
from both option sources.
##########
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:
Fixed in 485c4272bc. UPDATING and the generated-docs metadata explain the
tunnel's local bind hostname and MariaDB Connector/C identity verification.
They document turning off the toggle/removing ssl=1 for SSH-only transport,
explicitly warning that the SSH endpoint-to-database leg then has no TLS
guarantee, and distinguish a separately validated native TLS setup for
end-to-end protection. Tests cover the 127.0.0.1 host retaining verification/CA
and native settings passing through with the toggle off.
--
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]
