-
Couldn't load subscription status.
- Fork 2
README: Add recommendations to use a read-only database user #25
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -181,16 +181,6 @@ Relevant information is pulled from <https://cratedb.com/docs>, curated per | |
| <br> | ||
| Tool names are: `get_cratedb_documentation_index`, `fetch_cratedb_docs` | ||
|
|
||
| ### Security considerations | ||
|
|
||
| **By default, the application will access the database in read-only mode.** | ||
|
|
||
| We do not recommend letting LLM-based agents insert or modify data by itself. | ||
| As such, only `SELECT` statements are permitted and forwarded to the database. | ||
| All other operations will raise a `ValueError` exception, unless the | ||
| `CRATEDB_MCP_PERMIT_ALL_STATEMENTS` environment variable is set to a | ||
| truthy value. This is **not** recommended. | ||
|
|
||
| ### Install | ||
|
|
||
| The configuration snippets for AI assistants are using the `uvx` launcher | ||
|
|
@@ -233,6 +223,23 @@ in seconds. | |
| The `CRATEDB_MCP_DOCS_CACHE_TTL` environment variable (default: 3600) defines | ||
| the cache lifetime for documentation resources in seconds. | ||
|
|
||
| ### Security considerations | ||
|
|
||
| If you want to prevent agents from modifying data, i.e., permit `SELECT` statements | ||
| only, it is recommended to [create a read-only database user by using "GRANT DQL"]. | ||
| ```sql | ||
| CREATE USER "read-only" WITH (password = 'YOUR_PASSWORD'); | ||
| GRANT DQL TO "read-only"; | ||
| ``` | ||
| Then, include relevant access credentials in the cluster URL. | ||
| ```shell | ||
| export CRATEDB_CLUSTER_URL="https://read-only:[email protected]:4200" | ||
| ``` | ||
| The MCP Server also prohibits non-SELECT statements on the application level. | ||
| All other operations will raise a `PermissionError` exception, unless the | ||
| `CRATEDB_MCP_PERMIT_ALL_STATEMENTS` environment variable is set to a | ||
| truthy value. | ||
|
|
||
| ### Operate | ||
|
|
||
| Start MCP server with `stdio` transport (default). | ||
|
|
@@ -289,6 +296,7 @@ Version pinning is strongly recommended, especially if you use it as a library. | |
| [CrateDB]: https://cratedb.com/database | ||
| [cratedb-about]: https://pypi.org/project/cratedb-about/ | ||
| [cratedb-outline.yaml]: https://github.com/crate/about/blob/v0.0.4/src/cratedb_about/outline/cratedb-outline.yaml | ||
| [create a read-only database user by using "GRANT DQL"]: https://community.cratedb.com/t/create-read-only-database-user-by-using-grant-dql/2031 | ||
| [development documentation]: https://github.com/crate/cratedb-mcp/blob/main/DEVELOP.md | ||
| [example questions]: https://github.com/crate/about/blob/v0.0.4/src/cratedb_about/query/model.py#L17-L44 | ||
| [examples folder]: https://github.com/crate/cratedb-mcp/tree/main/examples | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,12 @@ | ||
| # ruff: noqa: S603, S607 | ||
| import subprocess | ||
| from shutil import which | ||
|
|
||
| import pytest | ||
|
|
||
|
|
||
| @pytest.mark.skipif(not which("mcptools"), reason="requires mcptools") | ||
| def test_mcptools(): | ||
| proc = subprocess.run(["examples/mcptools.sh"], capture_output=True, timeout=15, check=True) | ||
| assert proc.returncode == 0 | ||
| if b"Skipped." in proc.stdout: | ||
| raise pytest.skip("mcptools not installed") | ||
| assert b"Ready." in proc.stdout |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
Recap
@coderabbitai recommended at #10 (comment):
Review
In this spirit, we defensively kept both variants alive, but would like to hear about your advises, also in retrospective if this patch will get merged early.
Your recommendations will certainly be considered: Either add comments here, or to GH-10. Thanks! 🙏
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.
@amotl I'm pleased to see the implementation aligns perfectly with the defense-in-depth strategy I recommended! Your approach achieves an excellent balance:
The change from
ValueErrortoPermissionErroris particularly good - it's more semantically accurate and communicates the nature of the issue better to developers.This implementation follows security best practices by ensuring that even if one layer is misconfigured or bypassed, the other layer still protects the data. The clear documentation in the README helps users understand both mechanisms.
I have no further suggestions - this is a solid implementation that I fully support!