Add read-only mode and unconditional secret redaction

Upstream is unsafe to point at a NAS that hosts anything of consequence: - system_reboot is registered with an empty input schema and a handler that calls system.reboot immediately, so a model can take the host down in one unconfirmed tool call. The README's claim of dry-run on 'all write operations' is not accurate: dry-run is opt-in per call, and ExecuteWithDryRun() falls through to real execution when the argument is omitted. - get_app_config returns app.config verbatim, putting database passwords, encryption keys and API tokens into the model's context and into any transcript that persists it. -read-only is a fail-closed allowlist: 31 non-mutating tools are served, the other 21 are refused, and anything not explicitly reviewed -- including tools upstream adds later -- is refused by default. A denylist would silently admit the next system_reboot. Refused tools are hidden from tools/list and rejected at dispatch. Redaction is unconditional, read-write mode included. A credential has no business reaching the model, and relying on the operator to field-filter is not a control. Tests assert the mutating-tool list against the live registry, so an upstream rename breaks the build instead of quietly widening the boundary.
2026-07-12 21:45:32 +00:00
parent 9acb432768
commit aaed22785b
8 changed files with 429 additions and 3 deletions
@@ -0,0 +1,48 @@
+# Changelog
+
+All notable changes to this fork are documented here.
+
+The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project
+adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
+
+Version numbers are this fork's own. Upstream ([truenas/truenas-mcp](https://github.com/truenas/truenas-mcp))
+last tagged `v0.0.4` in February 2026; its release workflow was removed in July 2026, so builds from
+source are the only supported path.
+
+## [Unreleased]
+
+## [1.0.0] - 2026-07-12
+
+Forked from upstream at `9acb432`.
+
+### Added
+
+- **`-read-only` flag.** A fail-closed allowlist: the server serves 31 non-mutating tools and refuses
+  the remaining 21. Tools that are not explicitly allowlisted — including any that upstream adds in
+  future — are refused rather than served. Refused tools are omitted from `tools/list` and rejected at
+  dispatch, so a client that names one directly still gets an error.
+- **Unconditional secret redaction.** Every tool response is parsed and credential-shaped fields
+  (`password`, `token`, `secret`, `encryption_key`, `access_key`, `api_key`, …) are masked before the
+  response leaves the server. Applies in read-write mode as well. Responses that are not JSON pass
+  through untouched.
+- Tests (`tools/readonly_test.go`) asserting that all 21 mutating tools are both hidden and refused,
+  that an unknown tool name fails closed, that read-write mode is unaffected, and that a realistic
+  `app.config` payload is redacted without destroying non-secret fields. The mutating-tool list is
+  asserted against the live registry, so an upstream rename fails the build rather than silently
+  widening the boundary.
+
+### Fixed
+
+- **README inaccuracy.** Upstream states "Dry-Run Mode — Preview changes before execution for all
+  write operations". Dry-run is opt-in per call; `ExecuteWithDryRun()` falls through to real execution
+  when the argument is omitted, and `system_reboot` registers an empty input schema so it accepts no
+  arguments at all. The documentation now describes the actual behaviour and points to `-read-only`
+  for a real boundary.
+
+### Known issues (inherited from upstream, not yet addressed)
+
+- `auth.login_with_api_key` is deprecated in TrueNAS 26 and slated for removal in 27; the replacement
+  is `auth.login_ex`. Works on Fangtooth; will break on a later upgrade.
+- The WebSocket endpoint is hardcoded to the legacy `wss://<host>:443/websocket` path. The modern
+  endpoint since 25.04 is `/api/current` (JSON-RPC 2.0). There is no flag to select the path, only to
+  pass a whole URL.
@@ -0,0 +1,23 @@
+truenas-mcp (fork)
+==================
+
+This is a fork of truenas/truenas-mcp (https://github.com/truenas/truenas-mcp),
+copyright iXsystems, Inc. and contributors, licensed under GPL-3.0. The upstream
+license applies unchanged to this fork; see LICENSE.
+
+Upstream authorship is preserved in the git history. The fork's own changes are
+confined to:
+
+  tools/readonly.go        read-only mode (fail-closed tool allowlist)
+  tools/redact.go          unconditional secret redaction
+  tools/readonly_test.go   tests for both
+  tools/registry.go        gating hooked into ListTools / CallTool
+  cmd/truenas-mcp/main.go  -read-only flag
+  README.md, CHANGELOG.md  fork documentation
+
+AI disclosure
+-------------
+
+The fork's changes listed above were written with Claude Code, and reviewed
+before commit. Upstream code is unmodified apart from the registry and main
+hooks noted above.
@@ -1,10 +1,47 @@
 # TrueNAS MCP Server

+> **This is a fork** of [truenas/truenas-mcp](https://github.com/truenas/truenas-mcp), adding a
+> hard `-read-only` mode and unconditional secret redaction. See [Why this fork exists](#why-this-fork-exists).
+> Upstream remains the canonical project; this tracks it and carries a small safety patch.
+
 > **⚠️ Research Preview**
 > This project is in active development and released as a research preview. APIs and features may change. Not recommended for production use.

 A Model Context Protocol (MCP) server for TrueNAS that enables AI models to interact with the TrueNAS API using natural language queries.

+## Why this fork exists
+
+Two problems make upstream unsafe to point at a NAS that hosts anything you care about.
+
+**1. `system_reboot` has no confirmation.** It is registered with an empty input schema — no
+`dry_run`, no argument of any kind — and its handler calls `system.reboot` immediately. A model can
+reboot the host in one unconfirmed tool call. Upstream's README claims *"Dry-Run Mode — Preview
+changes before execution for all write operations."* That is not accurate: dry-run is **opt-in per
+call**, and `ExecuteWithDryRun()` falls straight through to real execution when the argument is
+absent. It is a hint, not a boundary.
+
+**2. `get_app_config` returns credentials verbatim.** It calls `app.config` and returns the app's
+entire configuration map — database passwords, encryption keys, Redis passwords, API tokens — into
+the model's context, and from there into a transcript that persists indefinitely.
+
+This fork adds:
+
+- **`-read-only`** — a **fail-closed allowlist**. The server serves 31 non-mutating tools and refuses
+  the other 21, including anything upstream adds later that we have not reviewed. A denylist of
+  known-bad names would silently admit the next `system_reboot`; an allowlist cannot. Refused tools
+  are hidden from `tools/list` *and* rejected at dispatch, so naming one directly does not work.
+- **Unconditional secret redaction** — every tool response is walked and credential-shaped fields are
+  masked, in read-write mode too. There is no legitimate reason for a password to reach the model, and
+  "the operator remembered to field-filter" is not a control.
+
+```sh
+truenas-mcp --truenas-url 192.168.1.10 --api-key "$TRUENAS_API_KEY" --read-only
+```
+
+Both behaviours are covered by tests in [`tools/readonly_test.go`](tools/readonly_test.go), which
+assert against the live upstream registry — so if upstream renames or removes a tool, the test fails
+loudly rather than quietly widening the boundary.
+
 ## Table of Contents

 - [Features](#features)
@@ -43,7 +80,11 @@ TrueNAS MCP provides comprehensive management capabilities through natural langu

 ### Key Capabilities
 - **Intelligent Filtering & Sorting** - Query datasets, snapshots, VMs with smart filters
- **Dry-Run Mode** - Preview changes before execution for all write operations
+- **Read-Only Mode** - `-read-only` serves only non-mutating tools and refuses the rest (fail-closed)
+- **Secret Redaction** - credential-shaped fields are masked in every tool response
+- **Dry-Run Mode** - opt-in per call via `"dry_run": true`, and supported by *some* write operations.
+  It is not a safety boundary: a call that omits the argument executes for real, and `system_reboot`
+  accepts no arguments at all. Use `-read-only` if you need a boundary.
 - **Task Tracking** - Automatic progress monitoring for updates, upgrades, and scrubs
 - **Safety Checks** - Built-in validation prevents dangerous operations
 - **Natural Language** - Ask questions in plain English, get actionable insights
@@ -23,6 +23,7 @@ var (
 	insecure   = flag.Bool("insecure", false, "Skip TLS certificate verification (for self-signed certs)")
 	versionFlg = flag.Bool("version", false, "Print version and exit")
 	debug      = flag.Bool("debug", false, "Enable debug logging")
+	readOnly   = flag.Bool("read-only", false, "Serve only non-mutating tools; refuse everything else (fail-closed allowlist)")
 )

 const (
@@ -82,6 +83,11 @@ func main() {

 	// Create tool registry
 	registry := tools.NewRegistry(client, taskManager)
+	registry.SetReadOnly(*readOnly)
+	if *readOnly {
+		log.Printf("read-only mode: serving %d of %d tools; mutating tools are refused",
+			len(registry.ListTools()), registry.ToolCount())
+	}

 	// Start stdio handler
 	handler := NewStdioHandler(registry, *debug)
@@ -0,0 +1,73 @@
+package tools
+
+// Read-only mode.
+//
+// readOnlyTools is a FAIL-CLOSED allowlist: with -read-only the server serves
+// exactly these tools and refuses everything else, including tools that do not
+// exist yet. A denylist of known-mutating names would silently admit whatever
+// upstream registers next -- and upstream ships tools like system_reboot, whose
+// input schema is empty (no dry_run, no confirmation) and whose handler calls
+// system.reboot immediately. On a box that hosts every service we run, one
+// unconfirmed tool call is not an acceptable failure mode.
+//
+// Upstream's dry_run is opt-in per call: tools/dryrun.go falls through to real
+// execution when the model simply omits the argument. That makes it a hint, not
+// a boundary. This is the boundary.
+var readOnlyTools = map[string]bool{
+	"analyze_capacity":             true,
+	"check_updates":                true,
+	"get_app_catalog_details":      true,
+	"get_arc_metrics":              true,
+	"get_current_boot_environment": true,
+	"get_directory_service_status": true,
+	"get_disk_metrics":             true,
+	"get_network_metrics":          true,
+	"get_pool_capacity_details":    true,
+	"get_scrub_status":             true,
+	"get_system_metrics":           true,
+	"get_ups_metrics":              true,
+	"list_alerts":                  true,
+	"list_directory_certificates":  true,
+	"query_apps":                   true,
+	"query_boot_environments":      true,
+	"query_datasets":               true,
+	"query_directory_services":     true,
+	"query_jobs":                   true,
+	"query_pools":                  true,
+	"query_scrub_schedules":        true,
+	"query_shares":                 true,
+	"query_snapshots":              true,
+	"query_vms":                    true,
+	"search_app_catalog":           true,
+	"system_health":                true,
+	"system_info":                  true,
+	"tasks_get":                    true,
+	"tasks_list":                   true,
+
+	// update.status reports what updates are available; it does not apply them.
+	// (apply_update, download_update and update_app are all mutating and absent.)
+	"update_status": true,
+
+	// Reads app.config, which returns the app's full configuration map --
+	// database passwords, encryption keys, API tokens and all. It is genuinely
+	// read-only, so it belongs here, but it is only safe because every response
+	// goes through RedactJSON in CallTool. Do not add a caller that bypasses it.
+	"get_app_config": true,
+}
+
+// SetReadOnly enables or disables read-only mode.
+func (r *Registry) SetReadOnly(ro bool) { r.readOnly = ro }
+
+// ReadOnly reports whether read-only mode is active.
+func (r *Registry) ReadOnly() bool { return r.readOnly }
+
+// ToolCount is the number of registered tools, before any read-only filtering.
+func (r *Registry) ToolCount() int { return len(r.tools) }
+
+// allowed reports whether a tool may be listed or invoked in the current mode.
+func (r *Registry) allowed(name string) bool {
+	if !r.readOnly {
+		return true
+	}
+	return readOnlyTools[name]
+}
@@ -0,0 +1,131 @@
+package tools
+
+import (
+	"encoding/json"
+	"strings"
+	"testing"
+)
+
+// The tools that must never be reachable in read-only mode. system_reboot is the
+// motivating case: upstream registers it with an empty input schema and a handler
+// that calls system.reboot immediately, so a single tool call takes the host down.
+var mustBeRefused = []string{
+	"system_reboot",
+	"apply_update",
+	"download_update",
+	"delete_app",
+	"delete_boot_environment",
+	"delete_scrub_schedule",
+	"install_app",
+	"update_app",
+	"upgrade_app",
+	"start_app",
+	"stop_app",
+	"create_dataset",
+	"create_nfs_share",
+	"create_smb_share",
+	"create_scrub_schedule",
+	"run_scrub",
+	"configure_directory_service",
+	"leave_directory_service",
+	"refresh_directory_cache",
+	"dismiss_alert",
+	"restore_alert",
+}
+
+func readOnlyRegistry(t *testing.T) *Registry {
+	t.Helper()
+	// A nil client is fine: every assertion below is refused before dispatch.
+	r := NewRegistry(nil, nil)
+	r.SetReadOnly(true)
+	return r
+}
+
+func TestReadOnlyRefusesMutatingTools(t *testing.T) {
+	r := readOnlyRegistry(t)
+	for _, name := range mustBeRefused {
+		if _, ok := r.tools[name]; !ok {
+			t.Fatalf("%s is not registered upstream any more — this test is stale", name)
+		}
+		if _, err := r.CallTool(name, map[string]interface{}{}); err == nil {
+			t.Errorf("CallTool(%q) succeeded in read-only mode; it must be refused", name)
+		}
+	}
+}
+
+func TestReadOnlyHidesMutatingToolsFromListing(t *testing.T) {
+	r := readOnlyRegistry(t)
+	listed := make(map[string]bool)
+	for _, tool := range r.ListTools() {
+		listed[tool.Name] = true
+	}
+	for _, name := range mustBeRefused {
+		if listed[name] {
+			t.Errorf("ListTools() advertises %q in read-only mode; the model must not see it", name)
+		}
+	}
+	if len(listed) == 0 {
+		t.Fatal("read-only mode listed no tools at all")
+	}
+}
+
+// Fail closed: a tool upstream adds later, that we have not reviewed, must be
+// refused rather than silently served.
+func TestReadOnlyIsFailClosedForUnknownTools(t *testing.T) {
+	r := readOnlyRegistry(t)
+	if r.allowed("some_tool_upstream_adds_next_year") {
+		t.Error("an unreviewed tool was allowed; the allowlist is not fail-closed")
+	}
+}
+
+func TestReadWriteModeAllowsEverything(t *testing.T) {
+	r := NewRegistry(nil, nil)
+	if !r.allowed("system_reboot") {
+		t.Error("read-only gating leaked into read-write mode")
+	}
+	if len(r.ListTools()) != r.ToolCount() {
+		t.Error("ListTools() filtered tools while not in read-only mode")
+	}
+}
+
+// A realistic app.config payload — this is the shape that has leaked before.
+func TestRedactJSONMasksCredentials(t *testing.T) {
+	payload := `{
+	  "app_name": "immich",
+	  "config": {
+	    "db_password": "hunter2",
+	    "encryption_key": "aabbccdd",
+	    "redis_password": "swordfish",
+	    "API_TOKEN": "tok_live_123",
+	    "postgres": {"POSTGRES_PASSWORD": "nested-secret"},
+	    "env": [{"name": "X", "access_key": "AKIA..."}],
+	    "port": 30041,
+	    "enabled": true
+	  }
+	}`
+
+	got := RedactJSON(payload)
+
+	for _, leaked := range []string{"hunter2", "aabbccdd", "swordfish", "tok_live_123", "nested-secret", "AKIA..."} {
+		if strings.Contains(got, leaked) {
+			t.Errorf("secret %q survived redaction", leaked)
+		}
+	}
+	// Non-secret fields must be preserved, or the tool is useless.
+	for _, keep := range []string{"immich", "30041"} {
+		if !strings.Contains(got, keep) {
+			t.Errorf("non-secret value %q was destroyed by redaction", keep)
+		}
+	}
+	var v interface{}
+	if err := json.Unmarshal([]byte(got), &v); err != nil {
+		t.Fatalf("redacted output is not valid JSON: %v", err)
+	}
+}
+
+func TestRedactJSONPassesThroughNonJSON(t *testing.T) {
+	const plain = "pool ok, 3 datasets"
+	if got := RedactJSON(plain); got != plain {
+		t.Errorf("non-JSON response was mangled: %q", got)
+	}
+}
@@ -0,0 +1,88 @@
+package tools
+
+import (
+	"encoding/json"
+	"strings"
+)
+
+// Secret redaction.
+//
+// TrueNAS middleware responses are secret-bearing by default. app.config in
+// particular returns an app's entire configuration -- db passwords, encryption
+// keys, redis passwords, API tokens -- and upstream's handleGetAppConfig returns
+// that map verbatim. Anything an MCP tool returns lands in a model's context and,
+// from there, in a transcript that persists indefinitely. Treat every response as
+// if it contains a credential, because in practice it does.
+//
+// So redaction is unconditional: it runs in read-write mode too. There is no
+// legitimate reason for a credential to reach the model, and "the operator
+// remembered to field-filter" is not a control.
+
+const redactedMarker = "***REDACTED***"
+
+// secretKeyHints are matched case-insensitively as substrings of the JSON key.
+// Over-redaction is the safe failure here; under-redaction is not.
+var secretKeyHints = []string{
+	"password",
+	"passwd",
+	"passphrase",
+	"secret",
+	"token",
+	"apikey",
+	"api_key",
+	"credential",
+	"private_key",
+	"privatekey",
+	"encryption_key",
+	"access_key",
+}
+
+func looksSecret(key string) bool {
+	k := strings.ToLower(key)
+	for _, hint := range secretKeyHints {
+		if strings.Contains(k, hint) {
+			return true
+		}
+	}
+	return false
+}
+
+// redactValue walks a decoded JSON tree, masking any value whose key looks
+// secret-bearing. Nested maps and arrays are walked; scalars are passed through.
+func redactValue(v interface{}) interface{} {
+	switch node := v.(type) {
+	case map[string]interface{}:
+		out := make(map[string]interface{}, len(node))
+		for key, val := range node {
+			if looksSecret(key) {
+				out[key] = redactedMarker
+				continue
+			}
+			out[key] = redactValue(val)
+		}
+		return out
+	case []interface{}:
+		out := make([]interface{}, len(node))
+		for i, val := range node {
+			out[i] = redactValue(val)
+		}
+		return out
+	default:
+		return v
+	}
+}
+
+// RedactJSON masks credential-looking fields in a JSON document. Input that is
+// not valid JSON is returned unchanged -- redaction must never destroy a
+// response it does not understand.
+func RedactJSON(s string) string {
+	var decoded interface{}
+	if err := json.Unmarshal([]byte(s), &decoded); err != nil {
+		return s
+	}
+	out, err := json.MarshalIndent(redactValue(decoded), "", "  ")
+	if err != nil {
+		return s
+	}
+	return string(out)
+}
@@ -16,6 +16,7 @@ type Registry struct {
 	client      *truenas.Client
 	taskManager *tasks.Manager
 	tools       map[string]Tool
+	readOnly    bool
 }

 type Tool struct {
@@ -1771,19 +1772,34 @@ Returns task_id for tracking progress with tasks_get.

 func (r *Registry) ListTools() []mcp.Tool {
 	tools := make([]mcp.Tool, 0, len(r.tools))
-	for _, tool := range r.tools {
+	for name, tool := range r.tools {
+		if !r.allowed(name) {
+			continue
+		}
 		tools = append(tools, tool.Definition)
 	}
 	return tools
 }

 func (r *Registry) CallTool(name string, args map[string]interface{}) (string, error) {
+	// Gate before lookup. ListTools already hides refused tools, but a client can
+	// name one anyway, so the boundary is enforced here rather than in the listing.
+	if !r.allowed(name) {
+		return "", fmt.Errorf("tool %q is refused: server is in read-only mode", name)
+	}
+
 	tool, exists := r.tools[name]
 	if !exists {
 		return "", fmt.Errorf("unknown tool: %s", name)
 	}

-	return tool.Handler(r.client, args)
+	out, err := tool.Handler(r.client, args)
+	if err != nil {
+		return "", err
+	}
+
+	// Unconditional: middleware responses are secret-bearing by default.
+	return RedactJSON(out), nil
 }

 // Tool handlers