codeant-ai-for-open-source[bot] commented on code in PR #45576:
URL: https://github.com/apache/superset/pull/45576#discussion_r4339021764
##########
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:
**Suggestion:** On Linux, `TemporaryDirectory` is mode 0700, so the
container’s `mysql` user cannot traverse `/certs` to read the mounted TLS files
and the fixture fails to start.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Often` · 🏷️ `Incomplete
implementation`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=28ff41dfc4cc45778d2c6b133d20bfa4&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=28ff41dfc4cc45778d2c6b133d20bfa4&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:** tests/testcontainers/db_engine_specs/test_mariadb.py
**Line:** 267:269
**Comment:**
*Incomplete Implementation: On Linux, `TemporaryDirectory` is mode
0700, so the container’s `mysql` user cannot traverse `/certs` to read the
mounted TLS files and the fixture fails to start.
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%2F45576&comment_hash=b58cda20a5e04c08e909642798e880f9c092857b8d56cd01741dd8374c422f5e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45576&comment_hash=b58cda20a5e04c08e909642798e880f9c092857b8d56cd01741dd8374c422f5e&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: [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]
