sadpandajoe commented on code in PR #45576:
URL: https://github.com/apache/superset/pull/45576#discussion_r4340863129
##########
tests/testcontainers/db_engine_specs/test_mariadb.py:
##########
@@ -115,3 +143,160 @@ 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) -> 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, "superset-testcontainers-ca")]
+ )
+ 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(
+ [x509.NameAttribute(NameOID.COMMON_NAME, "mariadb-testcontainer")]
+ )
+ server_cert = (
+ x509.CertificateBuilder()
+ .subject_name(server_name)
+ .issuer_name(ca_name)
+ .public_key(server_key.public_key())
+ .serial_number(x509.random_serial_number())
+ .not_valid_before(now - timedelta(minutes=5))
+ .not_valid_after(now + timedelta(hours=1))
+ .sign(ca_key, hashes.SHA256())
+ )
+
+ (cert_dir /
"ca.pem").write_bytes(ca_cert.public_bytes(serialization.Encoding.PEM))
+ (cert_dir / "server-cert.pem").write_bytes(
+ server_cert.public_bytes(serialization.Encoding.PEM)
+ )
+ (cert_dir / "server-key.pem").write_bytes(
+ server_key.private_bytes(
+ encoding=serialization.Encoding.PEM,
+ format=serialization.PrivateFormat.TraditionalOpenSSL,
+ encryption_algorithm=serialization.NoEncryption(),
+ )
+ )
+ # Readable by the container's mysql user once bind-mounted read-only.
+ for name in ("ca.pem", "server-cert.pem", "server-key.pem"):
+ os.chmod(cert_dir / name, 0o644)
+
+
[email protected](scope="module")
+def tls_container() -> Iterator[tuple[MySqlContainer, str]]:
+ """A MariaDB server with a throwaway, explicitly-configured TLS cert.
+
+ Needs its own container rather than reusing the shared `engine`
+ container the way test_mysql.py's `tls_ca_path` fixture does: the cert
+ has to be mounted in and pointed to via command-line flags at container
+ start time, and there's no running-container equivalent of "extract the
+ cert the image already auto-generated" the way there is for `mysql:8.0`.
+ """
+ with tempfile.TemporaryDirectory() as cert_dir_str:
+ cert_dir = Path(cert_dir_str)
+ _write_self_signed_cert(cert_dir)
+ container = MySqlContainer(
+ "mariadb:11",
+ command=(
+ "--ssl-cert=/certs/server-cert.pem "
+ "--ssl-key=/certs/server-key.pem "
+ "--ssl-ca=/certs/ca.pem"
+ ),
+ ).with_volume_mapping(cert_dir_str, "/certs", "ro")
+ with container as started:
+ yield started, str(cert_dir / "ca.pem")
+
+
+def test_require_mysql_tls_connects_with_verified_tls(
+ tls_container: tuple[MySqlContainer, str],
+) -> None:
+ """
+ The other side of the same guarantee as the fail-closed test above: a
+ fix that merely refused every connection would also pass that test while
+ breaking every legitimate encrypted connection. A request that actually
+ trusts the server's CA must still succeed, and must be genuinely
+ encrypted rather than merely accepted -- asserted here via `Ssl_cipher`,
+ which MariaDB reports empty for a plaintext session.
+ """
+ container, ca_path = tls_container
+ host = _tcp_host(container)
+ port = container.get_exposed_port(container.port)
+ uri, connect_args = _require_tls_connect_args(f"ssl=1&ssl_ca={ca_path}")
+ uri = uri.set(host=host, port=int(port), database=container.dbname)
+ tls_engine = create_engine(uri, connect_args=connect_args)
+ with tls_engine.connect() as conn:
+ cipher = conn.execute(text("SHOW STATUS LIKE 'Ssl_cipher'")).fetchone()
+ assert cipher is not None
+ assert cipher[1], "connection succeeded but is not actually using TLS"
Review Comment:
A non-empty `Ssl_cipher` shows the session is encrypted, not that the
certificate was verified. On CI mysqlclient links Oracle's libmysqlclient, so
this path uses `ssl_mode=REQUIRED`, which only demands encryption. A plain
`ssl=1` with no `ssl_ca`, or a change that drops `ssl_ca` from the query, would
still pass this test, and the generated CA would have no effect on the outcome.
Could you add a wrong-CA case (a second throwaway CA passed as `ssl_ca`) that
must raise `OperationalError`, or force the verify-CA branch, so the test fails
when verification is lost?
##########
tests/testcontainers/db_engine_specs/test_mariadb.py:
##########
@@ -115,3 +143,160 @@ 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) -> 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, "superset-testcontainers-ca")]
+ )
+ 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(
+ [x509.NameAttribute(NameOID.COMMON_NAME, "mariadb-testcontainer")]
+ )
+ server_cert = (
+ x509.CertificateBuilder()
+ .subject_name(server_name)
+ .issuer_name(ca_name)
+ .public_key(server_key.public_key())
+ .serial_number(x509.random_serial_number())
+ .not_valid_before(now - timedelta(minutes=5))
+ .not_valid_after(now + timedelta(hours=1))
+ .sign(ca_key, hashes.SHA256())
+ )
+
+ (cert_dir /
"ca.pem").write_bytes(ca_cert.public_bytes(serialization.Encoding.PEM))
+ (cert_dir / "server-cert.pem").write_bytes(
+ server_cert.public_bytes(serialization.Encoding.PEM)
+ )
+ (cert_dir / "server-key.pem").write_bytes(
+ server_key.private_bytes(
+ encoding=serialization.Encoding.PEM,
+ format=serialization.PrivateFormat.TraditionalOpenSSL,
+ encryption_algorithm=serialization.NoEncryption(),
+ )
+ )
+ # Readable by the container's mysql user once bind-mounted read-only.
+ for name in ("ca.pem", "server-cert.pem", "server-key.pem"):
+ os.chmod(cert_dir / name, 0o644)
+
+
[email protected](scope="module")
+def tls_container() -> Iterator[tuple[MySqlContainer, str]]:
+ """A MariaDB server with a throwaway, explicitly-configured TLS cert.
+
+ Needs its own container rather than reusing the shared `engine`
+ container the way test_mysql.py's `tls_ca_path` fixture does: the cert
+ has to be mounted in and pointed to via command-line flags at container
+ start time, and there's no running-container equivalent of "extract the
+ cert the image already auto-generated" the way there is for `mysql:8.0`.
+ """
+ with tempfile.TemporaryDirectory() as cert_dir_str:
+ cert_dir = Path(cert_dir_str)
+ _write_self_signed_cert(cert_dir)
Review Comment:
Agreed—CI's mariadb job already fails at fixture setup: the container exits
with code 1 before it emits 'ready for connections', so the verified-TLS test
errors instead of running. Should the temp directory be made traversable (for
example `cert_dir.chmod(0o755)`) before it is mounted?
--
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]
