You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "aminghadersohi (via GitHub)" <gi...@apache.org> on 2026/09/27 01:46:16 UTC
[PR] fix(mysql): require TLS when SSL is requested [superset]
aminghadersohi opened a new pull request, #44723:
URL: https://github.com/apache/superset/pull/44723
### SUMMARY
Make MySQL's SSL toggle and saved `ssl=1` URLs require TLS through driver-native options. Preserve schema selection, local-infile restrictions, and stronger verification settings; do not change MySQL-compatible engine subclasses.
A local MySQL 8.4.11 run through database creation and SQL Lab reproduced an empty `Ssl_cipher` with mysqlclient 2.2.8 (`mysql://` and `mysql+mysqldb://`). Connector/Python 9.3.0 rejected the `ssl` keyword and PyMySQL 1.2.0 rejected its string value.
Importantly, using `ssl_mode=REQUIRED` alone is insufficient: mysqlclient linked to MariaDB Connector/C 3.3.19 still falls back to cleartext when the server does not advertise TLS. Use `VERIFY_CA` (retaining `VERIFY_IDENTITY`) for mysqlclient, and `ssl_verify_cert=True` for Connector/Python/PyMySQL. A private CA and a valid server certificate may therefore be required. Reject incompatible overrides and older PyMySQL versions that permit fallback. Preserve PyMySQL CA options through SQLAlchemy's SSL-dictionary conversion.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable (connection transport behavior).
### TESTING INSTRUCTIONS
`pytest tests/unit_tests/db_engine_specs/test_mysql.py`: 89 passed. Initial new regressions failed before the change. Changed-file pre-commit passed, including mypy and pylint.
Live: run local MySQL 8 with TLS, then enable `require_secure_transport=ON`; test each of the four URI spellings with `ssl=1` and with the toggle-produced URI. Supply the test CA and execute `SHOW STATUS LIKE 'Ssl_cipher'` in SQL Lab. All 16 final cases negotiated `TLS_AES_256_GCM_SHA384`. Repeat against a server started with `--tls-version=`: all 8 connection attempts must fail with native TLS errors, not produce an empty cipher. The verification-mode behavior was checked with a locally signed server certificate valid for the loopback hostname/address.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mysql): require TLS when SSL is requested [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mysql): require TLS when SSL is requested [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mysql): require TLS when SSL is requested [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44723:
URL: https://github.com/apache/superset/pull/44723#issuecomment-5851739181
### <span aria-hidden="true">β
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">π¨</span> Latest commit | 73f25c17a5ec099ad63aa73ae7fbb9aac601f80c |
|<span aria-hidden="true">π</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6ab875693b51d5000878c851 |
|<span aria-hidden="true">π</span> Deploy Preview | [https://deploy-preview-44723--superset-docs-preview.netlify.app](https://deploy-preview-44723--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">π±</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">π€</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#q1YqKMrPLShRslJS0lFKKkrMS84AstMyK3RzK4sLc3RLcop1i1ILS1OLS4AKUlILcvIrPVOASswSkyzMTc0sjZNMDVNMDQwMLMwtki1MDZVqAQ) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44723--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mysql): require TLS when SSL is requested [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44723:
URL: https://github.com/apache/superset/pull/44723#issuecomment-5851727539
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mysql): require TLS when SSL is requested [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #44723:
URL: https://github.com/apache/superset/pull/44723#issuecomment-5884262595
Addressed the additional suggestion from Bito run #6870e8 (set-intersection idiom) in 42404b91a6: the check now reads `query.keys() & {"ssl_capath", "ssl_cipher"}`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mysql): require TLS when SSL is requested [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44723:
URL: https://github.com/apache/superset/pull/44723#discussion_r4129944076
##########
superset/db_engine_specs/mysql.py:
##########
@@ -78,6 +80,78 @@
)
+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."""
+ options = {**query, **args}
+ # Connector/Python has no REQUIRED mode: certificate verification is
+ # necessary to prevent its opportunistic fallback to cleartext.
+ if options.get("ssl_disabled"):
+ raise ValueError("MySQL SSL request conflicts with ssl_disabled")
Review Comment:
Fixed in 42404b91a6: `ssl_disabled` is now parsed with `asbool`, so `false`/`0` is accepted and dropped (never reaching the driver as a truthy string), while a true value still fails closed. Covered by `test_ssl_request_drops_non_disabling_ssl_disabled` and `test_ssl_request_rejects_url_ssl_disabled`.
##########
superset/db_engine_specs/mysql.py:
##########
@@ -445,6 +519,8 @@ def adjust_engine_params(
if schema:
uri = uri.set(database=parse.quote(schema, safe=""))
+ if cls.engine == "mysql":
+ return require_mysql_tls(uri, new_connect_args)
Review Comment:
Fixed in 42404b91a6: the `auroradataapi` driver now accepts the SSL toggle, consuming the scalar `ssl` request without passing it to the HTTPS-only Data API driver. Covered by `test_ssl_request_aurora_data_api_uses_https`.
##########
superset/db_engine_specs/mysql.py:
##########
@@ -78,6 +80,84 @@
)
+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."""
+ options = {**query, **args}
+ # Connector/Python has no REQUIRED mode: certificate verification is
+ # necessary to prevent its opportunistic fallback to cleartext.
+ if options.get("ssl_disabled"):
+ raise ValueError("MySQL SSL request conflicts with ssl_disabled")
+ if "ssl_verify_cert" in options and not asbool(options["ssl_verify_cert"]):
+ raise ValueError("MySQL SSL request requires ssl_verify_cert")
+ if driver == "pymysql":
+ 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))
+ if "ssl_check_hostname" in query:
+ args.setdefault(
+ "ssl_verify_identity", asbool(query.pop("ssl_check_hostname"))
+ )
Review Comment:
Fixed both in 42404b91a6: blank boolean options are treated as unset before `asbool` (`test_pymysql_blank_hostname_check_is_unset`), and an `ssl_verify_identity` connect_arg that contradicts URL `ssl_check_hostname` is now rejected instead of silently winning (`test_pymysql_hostname_check_conflict_fails_closed`).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mysql): require TLS when SSL is requested [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44723:
URL: https://github.com/apache/superset/pull/44723#issuecomment-5852760785
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mysql): require TLS when SSL is requested [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44723:
URL: https://github.com/apache/superset/pull/44723#discussion_r4114162106
##########
superset/db_engine_specs/mysql.py:
##########
@@ -78,6 +80,84 @@
)
+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."""
+ options = {**query, **args}
+ # Connector/Python has no REQUIRED mode: certificate verification is
+ # necessary to prevent its opportunistic fallback to cleartext.
+ if options.get("ssl_disabled"):
+ raise ValueError("MySQL SSL request conflicts with ssl_disabled")
+ if "ssl_verify_cert" in options and not asbool(options["ssl_verify_cert"]):
+ raise ValueError("MySQL SSL request requires ssl_verify_cert")
+ if driver == "pymysql":
+ 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))
+ if "ssl_check_hostname" in query:
+ args.setdefault(
+ "ssl_verify_identity", asbool(query.pop("ssl_check_hostname"))
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-20: asbool Crashes On Bare Param</b></div>
<div id="fix">
`asbool` raises `ValueError` on empty strings (verified in SQLAlchemy 2.0.52 `sqlalchemy/util/langhelpers.py`), and SQLAlchemy 2.0 URL parsing yields `""` for a valueless query param like `mysql://host/db?ssl_check_hostname`. The guard on line 106 passes, then `asbool(query.pop("ssl_check_hostname"))` on line 108 raises, so `update_params`/`get_sqla_engine` fails for a valid RFC 3986 URI. Handle the empty form explicitly before calling `asbool`. ([CWE-20](https://cwe.mitre.org/data/definitions/20.html))
</div>
</div>
<div id="suggestion">
<div id="issue"><b>Silent TLS Override Conflict</b></div>
<div id="fix">
`args.setdefault("ssl_verify_identity", ...)` silently keeps a pre-existing `ssl_verify_identity=False` connect_arg, so the toggle reports enforced TLS while certificate/identity verification is actually disabled. Sibling guards in `_require_mysql_verified_tls` fail closed on such conflicts (`ssl_disabled` line 90, `ssl_verify_cert` line 92, `ssl` line 112). Detect and reject the conflicting key instead of silently ignoring it.
</div>
</div>
<small><i>Code Review Run #6870e8</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mysql): require TLS when SSL is requested [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44723:
URL: https://github.com/apache/superset/pull/44723#discussion_r4120023216
##########
superset/db_engine_specs/mysql.py:
##########
@@ -78,6 +80,78 @@
)
+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."""
+ options = {**query, **args}
+ # Connector/Python has no REQUIRED mode: certificate verification is
+ # necessary to prevent its opportunistic fallback to cleartext.
+ if options.get("ssl_disabled"):
+ raise ValueError("MySQL SSL request conflicts with ssl_disabled")
Review Comment:
Not applicable: SQLAlchemy does not coerce URL `ssl_disabled`, and both mysql-connector (`if self._ssl_disabled:`) and PyMySQL test the raw value for truthiness, so the string `"false"`/`"0"` actually disables TLS in the driver β rejecting it is the correct fail-closed behavior.
##########
superset/db_engine_specs/mysql.py:
##########
@@ -445,6 +519,8 @@ def adjust_engine_params(
if schema:
uri = uri.set(database=parse.quote(schema, safe=""))
+ if cls.engine == "mysql":
+ return require_mysql_tls(uri, new_connect_args)
Review Comment:
Not a regression: the pinned `preset-aurora-data-api` `connect()` has no `ssl` parameter, so `ssl=1` already failed with a TypeError before this PR (the Data API is HTTPS-only), and an explicit unsupported-driver error is clearer.
##########
superset/db_engine_specs/mysql.py:
##########
@@ -78,6 +80,84 @@
)
+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."""
+ options = {**query, **args}
+ # Connector/Python has no REQUIRED mode: certificate verification is
+ # necessary to prevent its opportunistic fallback to cleartext.
+ if options.get("ssl_disabled"):
+ raise ValueError("MySQL SSL request conflicts with ssl_disabled")
+ if "ssl_verify_cert" in options and not asbool(options["ssl_verify_cert"]):
+ raise ValueError("MySQL SSL request requires ssl_verify_cert")
+ if driver == "pymysql":
+ 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))
+ if "ssl_check_hostname" in query:
+ args.setdefault(
+ "ssl_verify_identity", asbool(query.pop("ssl_check_hostname"))
+ )
Review Comment:
Not applicable: SQLAlchemy 2.0 URL parsing drops valueless/empty query params (`make_url("mysql+pymysql://h/db?ssl_check_hostname").query == {}`), so `asbool("")` is unreachable here; and an explicit `ssl_verify_identity=False` connect_arg only relaxes hostname matching while TLS stays required with `ssl_verify_cert=True`, matching SQLAlchemy's normal connect_args-over-URL precedence.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mysql): require TLS when SSL is requested [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #44723:
URL: https://github.com/apache/superset/pull/44723#issuecomment-5901463956
Addressed both migration notes in review 5356356322 in 485c4272bc: UPDATING and the MySQL metadata explain the immediate effect on saved SSL-enabled connections, CA files on all web/worker nodes, and recovery using native settings without the toggle. Standard Aurora MySQL enforcement is intentional, including IAM; the documentation calls out the Amazon RDS CA bundle when verification is used and distinguishes the HTTPS Data API. Added standard-Aurora CA preservation and native-settings regression tests. MySQL and IAM unit suites: 146 passed; pre-commit: all applicable hooks passed, including mypy. Also merged Apache master in 9a0e549926 to clear the conflict.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mysql): require TLS when SSL is requested [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44723:
URL: https://github.com/apache/superset/pull/44723#issuecomment-5903330727
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mysql): require TLS when SSL is requested [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi merged PR #44723:
URL: https://github.com/apache/superset/pull/44723
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mysql): require TLS when SSL is requested [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44723:
URL: https://github.com/apache/superset/pull/44723#discussion_r4120024913
##########
superset/db_engine_specs/mysql.py:
##########
@@ -78,6 +80,84 @@
)
+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."""
+ options = {**query, **args}
+ # Connector/Python has no REQUIRED mode: certificate verification is
+ # necessary to prevent its opportunistic fallback to cleartext.
+ if options.get("ssl_disabled"):
+ raise ValueError("MySQL SSL request conflicts with ssl_disabled")
+ if "ssl_verify_cert" in options and not asbool(options["ssl_verify_cert"]):
+ raise ValueError("MySQL SSL request requires ssl_verify_cert")
+ if driver == "pymysql":
+ 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))
+ if "ssl_check_hostname" in query:
+ args.setdefault(
+ "ssl_verify_identity", asbool(query.pop("ssl_check_hostname"))
+ )
Review Comment:
<!-- Bito Reply -->
The suggestion to handle empty strings before calling `asbool` is appropriate, as `asbool` raises a `ValueError` when encountering an empty string, which can occur with valueless query parameters in SQLAlchemy 2.0. Additionally, the suggestion to detect and reject conflicting `ssl_verify_identity` arguments is valid, as it prevents the silent override of security settings. Both suggestions improve the robustness and security of the TLS verification logic.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mysql): require TLS when SSL is requested [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44723:
URL: https://github.com/apache/superset/pull/44723#discussion_r4113724208
##########
superset/db_engine_specs/mysql.py:
##########
@@ -78,6 +80,78 @@
)
+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."""
+ options = {**query, **args}
+ # Connector/Python has no REQUIRED mode: certificate verification is
+ # necessary to prevent its opportunistic fallback to cleartext.
+ if options.get("ssl_disabled"):
+ raise ValueError("MySQL SSL request conflicts with ssl_disabled")
Review Comment:
**Suggestion:** URI values are strings, so `ssl_disabled=0` or `ssl_disabled=false` is truthy here and incorrectly rejects a non-disabling override.
**Assessment:** π `Major` Β· π `Occurrence: Rarely` Β· π·οΈ `Incorrect condition logic`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=aacef27aea7a4dd581e02e1a12a56d41&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=aacef27aea7a4dd581e02e1a12a56d41&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/db_engine_specs/mysql.py
**Line:** 90:91
**Comment:**
*Incorrect Condition Logic: URI values are strings, so `ssl_disabled=0` or `ssl_disabled=false` is truthy here and incorrectly rejects a non-disabling override.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44723&comment_hash=846abbc6ff85d35a0dba6e269e43f2c595c2c7272e37448c5f4ca2485e30c2ca&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44723&comment_hash=846abbc6ff85d35a0dba6e269e43f2c595c2c7272e37448c5f4ca2485e30c2ca&reaction=dislike'>π</a>
##########
superset/db_engine_specs/mysql.py:
##########
@@ -445,6 +519,8 @@ def adjust_engine_params(
if schema:
uri = uri.set(database=parse.quote(schema, safe=""))
+ if cls.engine == "mysql":
+ return require_mysql_tls(uri, new_connect_args)
Review Comment:
**Suggestion:** `AuroraMySQLDataAPI` inherits `engine = "mysql"` and produces `ssl=1`, but its `auroradataapi` driver is rejected here, breaking encrypted Aurora Data API connections.
**Assessment:** π `Major` Β· π `Occurrence: Sometimes` Β· π·οΈ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c261fa48e7bb449bad9b0bfab637da60&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c261fa48e7bb449bad9b0bfab637da60&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/db_engine_specs/mysql.py
**Line:** 522:523
**Comment:**
*Api Mismatch: `AuroraMySQLDataAPI` inherits `engine = "mysql"` and produces `ssl=1`, but its `auroradataapi` driver is rejected here, breaking encrypted Aurora Data API connections.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44723&comment_hash=4350c02df7f0f6638ffe9199f60a030373da2cd9c1ca3702cce86c44394b0a86&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44723&comment_hash=4350c02df7f0f6638ffe9199f60a030373da2cd9c1ca3702cce86c44394b0a86&reaction=dislike'>π</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mysql): require TLS when SSL is requested [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44723:
URL: https://github.com/apache/superset/pull/44723#discussion_r4120026788
##########
superset/db_engine_specs/mysql.py:
##########
@@ -445,6 +519,8 @@ def adjust_engine_params(
if schema:
uri = uri.set(database=parse.quote(schema, safe=""))
+ if cls.engine == "mysql":
+ return require_mysql_tls(uri, new_connect_args)
Review Comment:
β
**Customized review instruction saved!**
**Instruction:**
> Do not flag the explicit unsupported-driver handling for Aurora MySQL Data API TLS parameters; its HTTPS-only driver does not accept an `ssl` parameter.
**Applied to:**
- `superset/db_engine_specs/mysql.py`
---
π‘ *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mysql): require TLS when SSL is requested [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #44723:
URL: https://github.com/apache/superset/pull/44723#issuecomment-5902694783
Review summary at 0afef7e82e (reviewed 485c4272bc plus the merge 9a0e549926).
**Driver paths:** mysqlclient (REQUIRED on recognized Oracle clients, VERIFY_CA on MariaDB Connector/C and unknown clients), Connector/Python and PyMySQL (`ssl_verify_cert=True`, PyMySQL >= 1.2), the Aurora Data API (HTTPS, `ssl` consumed), and all other drivers (rejected) fail closed. Connections without the toggle or `ssl=1` are returned unchanged. I also confirmed that Connector/C 3.3.19 reports `3.3.19` from `mysql_get_client_info()`, so the version check routes it to VERIFY_CA. The IAM path keeps the enforced args because `ssl_args={}` merges into them.
**Findings and fixes (0afef7e82e):**
1. Merge resolution: the MySQL heading ended up above master's `/register/` bullet, so that bullet read as part of the MySQL section. I moved the MySQL section after the bullet, so the diff against master is a pure addition again. Apart from this, the merge kept both sides and only the three PR files differ from master.
2. Tests: three fail-closed branches had no coverage. These were the nested PyMySQL `ssl` dict with the toggle, an invalid `ssl` value type, and unsupported drivers (cymysql, mariadbconnector, aiomysql). I added a test for each.
3. rebenitez1802's low item about MariaDB: UPDATING and the metadata notes now say that `mariadb://` (MariaDB engine) and other MySQL-compatible engines keep their existing SSL handling.
**Reviewer asks at head:** everything is closed. That covers all five of EnxDev's inline notes (server_cert/CA docs, the 6.1.0 snapshot being untouched with the text in `metadata.notes`, the PyMySQL < 1.2 test, the client-library-based mode, and SSH tunnel guidance). It also covers both of rebenitez1802's items (upgrade impact/recovery and the Aurora RDS CA), plus the low REQUIRED and PyMySQL-gate items.
**Not changed (pre-existing, admin-controlled):** a `connect_args` key in encrypted extra replaces the engine `connect_args` after `adjust_engine_params`. That bypasses enforcement for such configs, the same as before this PR.
**Checks:** MySQL, AWS IAM, Aurora and MariaDB spec tests: 190 passed. Changed-file pre-commit passed, including mypy, pylint and metadata validation.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mysql): require TLS when SSL is requested [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #44723:
URL: https://github.com/apache/superset/pull/44723#issuecomment-5903827837
Following up on the two additional suggestions in Bito run #90bdb8, fixed in d647235ba1:
- **Inconsistent output variable name:** the SSL tests that bound the returned dict as `result` now use `connect_args`, the parameter name in `adjust_engine_params`.
- **Missing test docstring:** `test_ssl_request_rejects_url_ssl_disabled` now documents that a truthy URL `ssl_disabled` conflicts with the SSL request. `test_pymysql_hostname_verification_survives` and `test_pymysql_unsupported_ssl_options_fail_closed`, also added in this PR, got docstrings too.
`tests/unit_tests/db_engine_specs/test_mysql.py`: 143 passed. This is a test-only change.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mysql): require TLS when SSL is requested [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #44723:
URL: https://github.com/apache/superset/pull/44723#issuecomment-5903854236
@rebenitez1802 since your approval: 485c4272bc covers your upgrade-impact/recovery and Aurora RDS CA notes, 0afef7e82e restores the UPDATING placement and adds the fail-closed branch tests, and d647235ba1 is a test-only naming and docstring cleanup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mysql): require TLS when SSL is requested [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44723:
URL: https://github.com/apache/superset/pull/44723#issuecomment-5851784364
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44723?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `7.69231%` with `48 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 56.12%. Comparing base ([`5b9733f`](https://app.codecov.io/gh/apache/superset/commit/5b9733fd60f75c5f04df2817e0749bcbca125dcc?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`7f77060`](https://app.codecov.io/gh/apache/superset/commit/7f77060fb1605415ee0d7f6b3883f462c7418d72?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/44723?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/db\_engine\_specs/mysql.py](https://app.codecov.io/gh/apache/superset/pull/44723?src=pr&el=tree&filepath=superset%2Fdb_engine_specs%2Fmysql.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | 7.69% | [48 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44723?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (5b9733f) and HEAD (7f77060). Click for more details.
>
> <details><summary>HEAD has 112 uploads less than BASE</summary>
>
>| Flag | BASE (5b9733f) | HEAD (7f77060) |
>|------|------|------|
>|python|98|1|
>|hive|16|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44723 +/- ##
===========================================
- Coverage 81.12% 56.12% -25.01%
===========================================
Files 2956 2956
Lines 178434 178486 +52
Branches 41339 41358 +19
===========================================
- Hits 144762 100176 -44586
- Misses 30967 77791 +46824
+ Partials 2705 519 -2186
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44723/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/44723/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.76% <7.69%> (-0.02%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/44723/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/44723/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/44723/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/44723/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.76% <7.69%> (-48.65%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/44723/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/44723/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44723?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mysql): require TLS when SSL is requested [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44723:
URL: https://github.com/apache/superset/pull/44723#issuecomment-5906283365
**Bito Automatic Review Skipped β PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mysql): require TLS when SSL is requested [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44723:
URL: https://github.com/apache/superset/pull/44723#issuecomment-5886628255
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mysql): require TLS when SSL is requested [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44723:
URL: https://github.com/apache/superset/pull/44723#discussion_r4129945624
##########
superset/db_engine_specs/mysql.py:
##########
@@ -78,6 +80,84 @@
)
+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."""
+ options = {**query, **args}
+ # Connector/Python has no REQUIRED mode: certificate verification is
+ # necessary to prevent its opportunistic fallback to cleartext.
+ if options.get("ssl_disabled"):
+ raise ValueError("MySQL SSL request conflicts with ssl_disabled")
+ if "ssl_verify_cert" in options and not asbool(options["ssl_verify_cert"]):
+ raise ValueError("MySQL SSL request requires ssl_verify_cert")
+ if driver == "pymysql":
+ 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))
+ if "ssl_check_hostname" in query:
+ args.setdefault(
+ "ssl_verify_identity", asbool(query.pop("ssl_check_hostname"))
+ )
Review Comment:
<!-- Bito Reply -->
The changes in the provided code correctly address the identified issues. Handling empty strings before calling `asbool` prevents potential `ValueError` exceptions, and explicitly rejecting conflicting `ssl_verify_identity` arguments ensures that security settings are not silently overridden. These updates improve the robustness and security of the TLS verification logic.
**superset/db_engine_specs/mysql.py**
```
if "ssl_verify_cert" in options and not asbool(options["ssl_verify_cert"]):
raise ValueError("MySQL SSL request requires ssl_verify_cert")
if driver == "pymysql":
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))
if "ssl_check_hostname" in query:
args.setdefault(
"ssl_verify_identity", asbool(query.pop("ssl_check_hostname"))
)
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mysql): require TLS when SSL is requested [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #44723:
URL: https://github.com/apache/superset/pull/44723#issuecomment-5903297212
### Acceptance at 0afef7e82eb705b517f70789ec61d750b7954454
**PASS β 84/84 live cases, no fix needed.** Tested the worktree's `MySQLEngineSpec.build_sqlalchemy_uri` (toggle), `adjust_engine_params`, and SQLAlchemy `create_engine` in-process; no running web app or mocked driver. MySQL **8.4.11**, SQLAlchemy **2.0.52**, mysqlclient **2.2.8 / MariaDB Connector/C 3.3.19**, PyMySQL **1.2.0**, Connector/Python **9.3.0**.
Command: `PYTHONPATH=$PWD:$PWD/superset-core/src:/tmp/r144 SUPERSET_CONFIG_PATH=/tmp/r144/mysql_accept_config.py .venv/bin/python /tmp/r144/accept44723.py` β **84/84 PASS**. Each successful connection executed `SHOW STATUS LIKE 'Ssl_cipher'` and `SELECT 1`.
| Criterion / command | Observed result |
|---|---|
| **PASS: SSL toggle and `?ssl=1`**, all four URI spellings (`mysql://`, `mysql+mysqldb://`, `mysql+pymysql://`, `mysql+mysqlconnector://`), trusted test CA | `Ssl_cipher=TLS_AES_256_GCM_SHA384` in all **16** cases, with server TLS optional and after `SET GLOBAL require_secure_transport=ON`. |
| **PASS: fail closed**, same requests against `mysql:8 --tls-version=` | All **8** attempts rejected with error **2026**: mysqlclient βSSL is required, but the server does not support itβ; PyMySQL/Connector βSSL is required but the server doesn't support itβ. No successful cleartext connection. |
| **PASS: additional request/native paths**, `connect_args.ssl=True`; `ssl=1` plus explicit `ssl_mode=REQUIRED` (mysqlclient) / `ssl_verify_cert=True` (other drivers); native verified TLS without toggle | **36/36**: cipher above for TLS optional/required; error 2026 on non-TLS server. mysqlclient REQUIRED correctly becomes VERIFY_CA for the actual linked MariaDB client. |
| **PASS: ordinary connections**, no request and explicitly disabled TLS | **24/24**: all non-TLS-server controls connect with empty cipher; explicit TLS-disabled connections also work on TLS-optional server and are rejected with **3159** when secure transport is required. With no request, mysqlclient uses cleartext / gets 3159 when required; the other drivers negotiate TLS on the TLS server. |
`python -m pytest tests/unit_tests/db_engine_specs/test_mysql.py tests/unit_tests/db_engine_specs/test_mysql_iam.py -q` β **152 passed**.
`pre-commit run --files UPDATING.md superset/db_engine_specs/mysql.py tests/unit_tests/db_engine_specs/test_mysql.py` (with worktree `PYTHONPATH`) β **all applicable hooks passed**, including mypy and pylint.
Existing review findings remain addressed: boolean/Aurora Data API/hostname-option fixes at `42404b91a6`; migration/CA/Aurora/SSH guidance, generated-docs source, old-PyMySQL gate and client-library-specific REQUIRED handling at `485c4272bc`; UPDATING placement and further branch tests at `0afef7e82e`. All eight inline threads were already resolved; none reopened. Oracle libmysqlclient behavior is unit-covered, not live-tested here.
Cleanup: `docker stop acceptance-44723-tls acceptance-44723-plain && docker rm -v acceptance-44723-tls acceptance-44723-plain` completed; both containers removed. Datadirs used tmpfs; no volumes created. No commits or pushes.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org