sadpandajoe commented on code in PR #45576:
URL: https://github.com/apache/superset/pull/45576#discussion_r4369989901
##########
tests/testcontainers/db_engine_specs/test_mariadb.py:
##########
@@ -115,3 +143,196 @@ def test_get_columns_maps_native_types(engine: Engine) ->
None:
assert spec is not None
assert spec.generic_type == GenericDataType.NUMERIC
assert isinstance(spec.sqla_type, Integer)
+
+
[email protected](scope="module")
+def no_tls_container() -> Iterator[MySqlContainer]:
+ """A server with TLS fully disabled, to test the fail-closed guarantee.
+
+ `--skip-ssl` turns off TLS support entirely on this image (`have_ssl`
+ reports `DISABLED`), verified directly against this image before
+ writing this fixture.
+ """
+ with MySqlContainer("mariadb:11", command="--skip-ssl") as container:
+ yield container
+
+
+def _require_tls_connect_args(uri_query: str = "ssl=1") -> tuple[URL,
dict[str, Any]]:
+ """Run a `ssl=1` request through the real `adjust_engine_params` path.
+
+ Goes through `MariaDBEngineSpec.adjust_engine_params` (inherited from
+ `MySQLEngineSpec`, but exercised here as the actual class Superset
+ dispatches to for a `mariadb` connection) rather than calling
+ `require_mysql_tls` directly, so the test exercises the exact call path
+ production code takes for this engine.
+ """
+ uri = make_url(f"mysql://root:test@placeholder:3306/test?{uri_query}")
+ return MariaDBEngineSpec.adjust_engine_params(uri, {})
+
+
+def test_require_mysql_tls_fails_closed_without_server_tls(
+ no_tls_container: MySqlContainer,
+) -> None:
+ """
+ Same guarantee as test_mysql.py's version of this test
(apache/superset#44723):
+ a server offering no TLS at all must make the connection fail, never
+ succeed silently in cleartext. A mocked cursor can't observe this -- the
+ behavior lives in the C client library's own TLS negotiation, decided
+ once, at real connection time, against a real server's real TLS posture.
+ """
+ host = _tcp_host(no_tls_container)
+ port = no_tls_container.get_exposed_port(no_tls_container.port)
+ uri, connect_args = _require_tls_connect_args()
+ uri = uri.set(host=host, port=int(port), database=no_tls_container.dbname)
+ engine = create_engine(uri, connect_args=connect_args)
+ # Match the TLS refusal itself: an unrelated auth failure (e.g. 1045 or
+ # 2061) also raises OperationalError and would pass with the fix reverted.
+ with pytest.raises(OperationalError, match="SSL is required"):
+ with engine.connect():
+ pass
+
+
+def _write_self_signed_cert(
+ cert_dir: Path, ca_common_name: str = "superset-testcontainers-ca"
+) -> None:
+ """Write a throwaway CA + server cert/key into `cert_dir`.
+
+ Unlike the `mysql:8.0` image, `mariadb`'s image does not auto-generate a
+ CA/cert pair when TLS is enabled: `have_ssl` reports `YES` out of the
+ box, but `ssl_ca`/`ssl_cert`/`ssl_key` all report empty and no cert
+ files exist anywhere under `/var/lib/mysql` (verified directly against
+ this image) -- so there's nothing to extract back out the way
+ test_mysql.py's `tls_ca_path` fixture does for `mysql:8.0`. Generate a
+ throwaway one instead and mount it into a dedicated container via
+ `--ssl-cert`/`--ssl-key`/`--ssl-ca`, verified directly to work against
+ this image.
+ """
+ now = datetime.now(timezone.utc)
+
+ ca_key = rsa.generate_private_key(public_exponent=65537, key_size=2048)
+ ca_name = x509.Name([x509.NameAttribute(NameOID.COMMON_NAME,
ca_common_name)])
+ ca_cert = (
+ x509.CertificateBuilder()
+ .subject_name(ca_name)
+ .issuer_name(ca_name)
+ .public_key(ca_key.public_key())
+ .serial_number(x509.random_serial_number())
+ .not_valid_before(now - timedelta(minutes=5))
+ .not_valid_after(now + timedelta(hours=1))
+ .add_extension(x509.BasicConstraints(ca=True, path_length=None),
critical=True)
+ .sign(ca_key, hashes.SHA256())
+ )
+
+ server_key = rsa.generate_private_key(public_exponent=65537, key_size=2048)
+ server_name = x509.Name(
Review Comment:
The server cert has CN `mariadb-testcontainer` and no SAN, but both TLS
tests connect to `127.0.0.1` (or the Docker host IP). When mysqlclient is
linked against MariaDB Connector/C, `ssl_mode=VERIFY_CA` is mapped to
`MYSQL_OPT_SSL_VERIFY_SERVER_CERT`, which also checks the hostname, so
`test_require_mysql_tls_connects_with_verified_tls` would fail on a correct
implementation. The module docstring says these tests guard that client
combination, but CI only runs the Oracle-linked one, so this wouldn't show up
until someone runs it locally against Connector/C. Should the server cert carry
a `SubjectAlternativeName` with the IP/DNS name the tests actually connect to?
--
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]
