-
Notifications
You must be signed in to change notification settings - Fork 196
Support mTLS in ClickHouse MCP #168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -48,6 +48,8 @@ class ClickHouseConfig: | |||||
| CLICKHOUSE_ENABLED: Enable ClickHouse server (default: true) | ||||||
| CLICKHOUSE_ALLOW_WRITE_ACCESS: Allow write operations (DDL and DML) (default: false) | ||||||
| CLICKHOUSE_ALLOW_DROP: Allow destructive operations (DROP, TRUNCATE) when writes are also enabled (default: false) | ||||||
| CLICKHOUSE_CLIENT_CERT: Path to TLS client certificate for mTLS authentication (default: None) | ||||||
| CLICKHOUSE_CLIENT_CERT_KEY: Path to TLS client private key for mTLS authentication (default: None) | ||||||
|
Comment on lines
48
to
+52
|
||||||
| """ | ||||||
|
|
||||||
| def __init__(self): | ||||||
|
|
@@ -86,8 +88,11 @@ def username(self) -> str: | |||||
|
|
||||||
| @property | ||||||
| def password(self) -> str: | ||||||
| """Get the ClickHouse password.""" | ||||||
| return os.environ["CLICKHOUSE_PASSWORD"] | ||||||
| """Get the ClickHouse password. | ||||||
|
|
||||||
| Returns empty string when not set and client certificates are configured. | ||||||
|
||||||
| Returns empty string when not set and client certificates are configured. | |
| Returns an empty string when CLICKHOUSE_PASSWORD is not set. |
Copilot
AI
Apr 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current validation only checks CLICKHOUSE_CLIENT_CERT to decide whether CLICKHOUSE_PASSWORD is required, but it doesn’t define/validate what constitutes a complete mTLS configuration (e.g., cert without key, key without cert). This can lead to configs that the README describes as invalid being accepted. Consider validating that the client-cert settings are consistent (and raising a clear ValueError that names the missing counterpart) or updating the docs to match the intended flexibility (e.g., cert file may already include the key).
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -120,3 +120,45 @@ def test_server_host_name_omitted_when_unset(monkeypatch: pytest.MonkeyPatch): | |
| client_config = config.get_client_config() | ||
|
|
||
| assert "server_host_name" not in client_config | ||
|
|
||
|
|
||
| def test_client_cert_configuration(monkeypatch: pytest.MonkeyPatch): | ||
| """Test that client cert and key are passed through when set.""" | ||
| monkeypatch.setenv("CLICKHOUSE_HOST", "localhost") | ||
| monkeypatch.setenv("CLICKHOUSE_USER", "test") | ||
| monkeypatch.setenv("CLICKHOUSE_CLIENT_CERT", "/path/to/client.pem") | ||
| monkeypatch.setenv("CLICKHOUSE_CLIENT_CERT_KEY", "/path/to/client.key") | ||
| monkeypatch.delenv("CLICKHOUSE_PASSWORD", raising=False) | ||
|
|
||
|
Comment on lines
+125
to
+132
|
||
| config = ClickHouseConfig() | ||
| client_config = config.get_client_config() | ||
|
|
||
| assert client_config["client_cert"] == "/path/to/client.pem" | ||
| assert client_config["client_cert_key"] == "/path/to/client.key" | ||
| assert client_config["password"] == "" | ||
|
|
||
|
|
||
| def test_client_cert_not_in_config_when_unset(monkeypatch: pytest.MonkeyPatch): | ||
| """Test that client cert fields are omitted when not set.""" | ||
| monkeypatch.setenv("CLICKHOUSE_HOST", "localhost") | ||
| monkeypatch.setenv("CLICKHOUSE_USER", "test") | ||
| monkeypatch.setenv("CLICKHOUSE_PASSWORD", "test") | ||
| monkeypatch.delenv("CLICKHOUSE_CLIENT_CERT", raising=False) | ||
| monkeypatch.delenv("CLICKHOUSE_CLIENT_CERT_KEY", raising=False) | ||
|
|
||
| config = ClickHouseConfig() | ||
| client_config = config.get_client_config() | ||
|
|
||
| assert "client_cert" not in client_config | ||
| assert "client_cert_key" not in client_config | ||
|
|
||
|
|
||
| def test_password_required_without_client_cert(monkeypatch: pytest.MonkeyPatch): | ||
| """Test that CLICKHOUSE_PASSWORD is required when no client cert is set.""" | ||
| monkeypatch.setenv("CLICKHOUSE_HOST", "localhost") | ||
| monkeypatch.setenv("CLICKHOUSE_USER", "test") | ||
| monkeypatch.delenv("CLICKHOUSE_PASSWORD", raising=False) | ||
| monkeypatch.delenv("CLICKHOUSE_CLIENT_CERT", raising=False) | ||
|
|
||
| with pytest.raises(ValueError, match="CLICKHOUSE_PASSWORD"): | ||
| ClickHouseConfig() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The README states
CLICKHOUSE_CLIENT_CERT“must be used with”CLICKHOUSE_CLIENT_CERT_KEY(and vice versa), but the implementation currently treats these as independently optional and only usesCLICKHOUSE_CLIENT_CERTto waive the password requirement. Either tighten validation to enforce the documented requirement, or relax/reword the README to reflect the actual supported configurations (e.g., combined cert+key PEM).