Support mTLS in ClickHouse MCP - #168
Conversation
|
|
809e4d6 to
c661616
Compare
c661616 to
d31a52f
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds mutual TLS (mTLS) support for mcp-clickhouse connections by allowing ClickHouse client certificate configuration via environment variables, updating config validation behavior, and documenting the new setup path.
Changes:
- Add
CLICKHOUSE_CLIENT_CERT/CLICKHOUSE_CLIENT_CERT_KEYsupport and pass-through toclickhouse_connect.get_client(...). - Make
CLICKHOUSE_PASSWORDoptional when client cert configuration is present. - Add tests and README documentation for mTLS configuration.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
mcp_clickhouse/mcp_env.py |
Adds env var accessors for client cert/key, modifies password handling, and updates required-var validation. |
tests/test_config_interface.py |
Adds unit tests for client cert pass-through and password requirement changes. |
README.md |
Documents mTLS configuration examples and new environment variables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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) |
There was a problem hiding this comment.
The class docstring still lists CLICKHOUSE_PASSWORD as a required env var, but _validate_required_vars now makes it optional when client certs are used. Update the docstring’s “Required environment variables” section so it matches the new validation rules (and document the client-cert alternative explicitly).
| return os.environ["CLICKHOUSE_PASSWORD"] | ||
| """Get the ClickHouse password. | ||
|
|
||
| Returns empty string when not set and client certificates are configured. |
There was a problem hiding this comment.
password now returns an empty string whenever CLICKHOUSE_PASSWORD is unset, but the docstring says that only happens when client certificates are configured. Either adjust the docstring to reflect the actual behavior, or change the implementation to only default to "" when mTLS config is present (and otherwise let validation/KeyError surface).
| Returns empty string when not set and client certificates are configured. | |
| Returns an empty string when CLICKHOUSE_PASSWORD is not set. |
| if "CLICKHOUSE_PASSWORD" not in os.environ and not self.client_cert: | ||
| missing_vars.append("CLICKHOUSE_PASSWORD") | ||
|
|
There was a problem hiding this comment.
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).
| 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) | ||
|
|
There was a problem hiding this comment.
This new test suite doesn’t cover misconfigured mTLS env combinations (e.g., only CLICKHOUSE_CLIENT_CERT set, only CLICKHOUSE_CLIENT_CERT_KEY set) and what the expected behavior/error should be. Add explicit tests for these partial configurations once the validation rule is clarified, so regressions don’t silently change the authentication requirements.
| * Must be used with `CLICKHOUSE_CLIENT_CERT_KEY` | ||
| * `CLICKHOUSE_CLIENT_CERT_KEY`: Path to TLS client private key for mTLS authentication | ||
| * Default: None | ||
| * Must be used with `CLICKHOUSE_CLIENT_CERT` |
There was a problem hiding this comment.
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 uses CLICKHOUSE_CLIENT_CERT to 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).
| * Must be used with `CLICKHOUSE_CLIENT_CERT_KEY` | |
| * `CLICKHOUSE_CLIENT_CERT_KEY`: Path to TLS client private key for mTLS authentication | |
| * Default: None | |
| * Must be used with `CLICKHOUSE_CLIENT_CERT` | |
| * Can be used by itself if the PEM file contains both the client certificate and private key | |
| * Use with `CLICKHOUSE_CLIENT_CERT_KEY` when the certificate and private key are stored in separate files | |
| * `CLICKHOUSE_CLIENT_CERT_KEY`: Path to TLS client private key for mTLS authentication | |
| * Default: None | |
| * Optional when `CLICKHOUSE_CLIENT_CERT` points to a combined cert+key PEM file | |
| * Set this when the private key is stored separately from `CLICKHOUSE_CLIENT_CERT` |
Enable mutual TLS in mcp-clickhouse connections to CH server.