Close the audit's findings: key leaked via the API-error path; redaction gaps

An independent audit of this fork found the headline claim -- 'the API key never reaches a log line' -- was broken on a path I had not covered. formatAPIErrorWithContext serialized the request params into the error string, and for auth.login_ex those params ARE the plaintext key. Not debug-gated, not redacted. Any middleware error frame on authentication (bad key, or a reconnect that re-authenticates and fails mid-session) put the key into log.Fatalf at startup AND, through CallTool's error return, into the model's context and the transcript. My redaction test only covered the debug request-log frame, which is precisely why this survived. Worse, the deprecated auth.login_with_api_key passes the key as a bare positional param -- a naked string with no key name -- so key-based redaction was structurally incapable of masking it. redactParamsForError now masks every scalar param of any auth.* method outright, leaving maps to key-based redaction so username/mechanism still show up in the error. Redaction gaps also closed: env vars come back as [{name:DB_PASSWORD,value: ...}], where the secret sits under the generic key value and no key rule could see it; bindpw/keytab/bare-key were absent from the hints (upstream's maskCredentials only masks those at the top level, so a nested one bypassed both); and the float64 round-trip silently corrupted 64-bit integers like ZFS guids. -insecure was a no-op: verification was always off while the flag claimed to be what turned it off. That undercuts the ws:// rejection entirely -- refusing plaintext to protect the key is hollow if the wss:// connection trusts any cert. Verification is now on by default; -insecure genuinely disables it and warns. TrueNAS is self-signed, so -insecure is required against a stock box -- but as an explicit choice, not a silent default. Remaining limit is documented, not hidden: redaction is key-name-based, so a secret inside an opaque string blob (a custom app's compose YAML) is not caught. A test pins that behaviour so it can't be mistaken for safety. Read-only gate audited clean: gate before dispatch, fail-closed for unknown tools, no mutating middleware method reachable from an allowlisted tool.
2026-07-13 16:03:03 +00:00
parent 2da1d6ab51
commit 26d5eda1c7
8 changed files with 330 additions and 13 deletions
@@ -11,6 +11,47 @@ source are the only supported path.

 ## [Unreleased]

+### Fixed (from an independent audit of the fork)
+
+- **HIGH — the API key leaked through the API-error formatter.** `formatAPIErrorWithContext`
+  serialized the request's params into the error string, and for `auth.login_ex` those params *are*
+  the plaintext key. That path was neither `-debug`-gated nor redacted, so a middleware error frame on
+  authentication (bad key, or a mid-session reconnect that re-authenticates and fails) put the key into
+  two sinks: `log.Fatalf` at startup, and — via `CallTool`'s error return — the model's context and the
+  transcript. The earlier redaction only covered the debug request-*log* frame, which is exactly why
+  this survived. Params and the middleware-supplied `Trace` now both go through `redactForLog`.
+- **HIGH — the deprecated auth call leaked a *bare positional* key.** `auth.login_with_api_key` passes
+  the key as a naked string param with no key name, so key-based redaction could not see it at all.
+  `redactParamsForError` now masks every scalar param of any `auth.*` method outright, while leaving
+  maps to key-based redaction so useful context (username, mechanism) survives.
+- **MEDIUM — redaction was blind to secrets under generic keys.** TrueNAS returns env vars as
+  `[{"name":"DB_PASSWORD","value":"hunter2"}]`; the secret sits under `value`, which matches no key
+  hint. Redaction now detects the name/value pair shape and masks the value when the *name* looks like
+  a credential. Non-secret env vars keep their values.
+- **MEDIUM — `bindpw`, `keytab`, and a bare `key` were not masked.** Upstream's `maskCredentials` only
+  handles these at the top level, so a nested directory-services credential bypassed both it and
+  redaction. Added as hints; bare `key` is an exact-match rule so `keyboard`/`monkey` aren't shredded.
+- **`-insecure` was a no-op.** `InsecureSkipVerify` was set unconditionally and the flag only printed a
+  log line — certificate verification was *always* off while the flag advertised itself as the thing
+  that disabled it. That silently undercut the reason we hard-reject `ws://`: refusing plaintext to
+  protect the key means little if the `wss://` connection then trusts any certificate, since an active
+  MITM can present one and capture the key during `auth.login_ex`. Verification is now ON by default and
+  `-insecure` genuinely disables it, with a warning. **TrueNAS ships a self-signed cert, so `-insecure`
+  is required against a stock box** — but it is now an explicit choice, not a silent default.
+- **Large integers were corrupted by redaction.** `RedactJSON` unmarshalled into `interface{}`, decoding
+  every number as `float64` and silently mangling 64-bit values (a ZFS guid `15032414960031428871` came
+  back as `...429000`). Now decodes with `UseNumber()`.
+- **Bracketed IPv6 without a port produced a double-bracketed URL** (`wss://[[fd00::1]]:443/...`).
+
+### Known limit (documented, not fixed)
+
+Redaction masks by **key name** (plus the name/value pair shape). A secret embedded inside an opaque
+**string blob** — e.g. a custom app's docker-compose YAML returned as one field, with
+`POSTGRES_PASSWORD: hunter2` inside it — is **not** caught, because the key holding the blob isn't
+credential-shaped and we will not regex the interior of arbitrary strings. `get_app_config` on a
+custom/compose app is therefore not fully safe; prefer `query_apps` for those. A test asserts this
+current behaviour so it can't be mistaken for safety.
+
 ## [1.0.0] - 2026-07-12

 Forked from upstream at `9acb432`.
@@ -88,12 +88,23 @@ func main() {
 			"(removed in TrueNAS 27). Pass -username to use auth.login_ex.")
 	}

-	// Configure TLS - accept self-signed certs by default (common for TrueNAS)
+	// TLS. Upstream set InsecureSkipVerify unconditionally and -insecure only
+	// printed a log line, so certificate verification was ALWAYS off while the flag
+	// advertised itself as the thing that turned it off. That quietly undercut the
+	// reason we hard-reject ws:// at all (client.go): refusing plaintext to protect
+	// the API key means little if the wss:// connection then trusts any certificate,
+	// since an active MITM can present one and capture the key during auth.login_ex.
+	//
+	// Now the flag means what it says: verification is ON unless -insecure is passed.
+	// TrueNAS ships a self-signed cert, so -insecure is genuinely required against a
+	// stock box -- but it is now an explicit, visible choice rather than a silent
+	// default. (Pinning the NAS's cert would be strictly better; not done here.)
 	tlsConfig := &tls.Config{
-		InsecureSkipVerify: true,
+		InsecureSkipVerify: *insecure,
 	}
 	if *insecure {
-		log.Println("TLS certificate verification disabled (self-signed certs accepted)")
+		log.Println("WARNING: TLS certificate verification disabled (-insecure). The API key is " +
+			"exposed to an active MITM on this connection.")
 	}

 	// Create TrueNAS client
@@ -1,6 +1,7 @@
 package tools

 import (
+	"bytes"
 	"encoding/json"
 	"strings"
 )
@@ -17,10 +18,17 @@ import (
 // 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.
+//
+// KNOWN LIMIT, stated plainly: this masks by KEY NAME (plus the name/value pair
+// shape below). A secret embedded inside an opaque string -- e.g. a custom app's
+// docker-compose YAML returned as one blob, with `POSTGRES_PASSWORD: hunter2`
+// inside it -- is NOT caught, because the key holding the blob isn't credential-
+// shaped and we will not regex the interior of arbitrary strings. get_app_config on
+// a custom/compose app is therefore not fully safe; prefer query_apps for those.

 const redactedMarker = "***REDACTED***"

-// secretKeyHints are matched case-insensitively as substrings of the JSON key.
+// 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",
@@ -35,10 +43,27 @@ var secretKeyHints = []string{
 	"privatekey",
 	"encryption_key",
 	"access_key",
+	// TrueNAS-specific: directory-services bind password and Kerberos keytab.
+	// Upstream's maskCredentials only masks these at the TOP level, so a nested
+	// credential object slipped through both it and this.
+	"bindpw",
+	"keytab",
+}
+
+// exactSecretKeys are matched case-insensitively as WHOLE keys. Kept separate from
+// the substring list so a bare "key" is caught without also masking "keyboard",
+// "monkey", or "key_count".
+var exactSecretKeys = map[string]bool{
+	"key":  true,
+	"pw":   true,
+	"pass": true,
 }

 func looksSecret(key string) bool {
 	k := strings.ToLower(key)
+	if exactSecretKeys[k] {
+		return true
+	}
 	for _, hint := range secretKeyHints {
 		if strings.Contains(k, hint) {
 			return true
@@ -47,17 +72,39 @@ func looksSecret(key string) bool {
 	return false
 }

+// envPairSecret handles the {"name": "DB_PASSWORD", "value": "hunter2"} shape used
+// for environment variables. The secret sits under the generic key "value", which
+// no key-name rule would ever catch -- the credential-ness lives in the sibling's
+// *value*, not in the key. If a map carries a name-ish field whose value looks like
+// a credential identifier, mask its value-ish field.
+func envPairSecret(m map[string]interface{}) bool {
+	for _, nameKey := range []string{"name", "key", "variable", "env"} {
+		if raw, ok := m[nameKey]; ok {
+			if s, ok := raw.(string); ok && looksSecret(s) {
+				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{}:
+		maskValueField := envPairSecret(node)
 		out := make(map[string]interface{}, len(node))
 		for key, val := range node {
 			if looksSecret(key) {
 				out[key] = redactedMarker
 				continue
 			}
+			// {"name":"DB_PASSWORD","value":"hunter2"} -> mask "value"
+			if maskValueField && strings.EqualFold(key, "value") {
+				out[key] = redactedMarker
+				continue
+			}
 			out[key] = redactValue(val)
 		}
 		return out
@@ -75,14 +122,25 @@ func redactValue(v interface{}) interface{} {
 // 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.
+//
+// Numbers are decoded with UseNumber so large integers survive the round-trip. A
+// plain unmarshal turns every number into a float64, which silently mangles 64-bit
+// values (a ZFS guid like 15032414960031428871 came back as ...429000).
 func RedactJSON(s string) string {
+	dec := json.NewDecoder(strings.NewReader(s))
+	dec.UseNumber()
+
 	var decoded interface{}
-	if err := json.Unmarshal([]byte(s), &decoded); err != nil {
+	if err := dec.Decode(&decoded); err != nil {
 		return s
 	}
-	out, err := json.MarshalIndent(redactValue(decoded), "", "  ")
-	if err != nil {
+
+	var buf bytes.Buffer
+	enc := json.NewEncoder(&buf)
+	enc.SetIndent("", "  ")
+	enc.SetEscapeHTML(false)
+	if err := enc.Encode(redactValue(decoded)); err != nil {
 		return s
 	}
-	return string(out)
+	return strings.TrimRight(buf.String(), "\n")
 }
@@ -0,0 +1,87 @@
+package tools
+
+import (
+	"strings"
+	"testing"
+)
+
+// The audit's MEDIUM-HIGH finding: RedactJSON masked by key name only, so a secret
+// sitting under the generic key "value" (the {"name":"DB_PASSWORD","value":"..."}
+// env-var shape TrueNAS uses) sailed straight through. The original test used
+// {"name":"X","access_key":"AKIA..."} -- a hint-matched key -- which is exactly why
+// the gap survived.
+func TestRedactJSONMasksEnvNameValuePairs(t *testing.T) {
+	payload := `{
+	  "config": {
+	    "environment": [
+	      {"name": "DB_PASSWORD", "value": "hunter2"},
+	      {"name": "REDIS_TOKEN",  "value": "tok_abc"},
+	      {"name": "TZ",           "value": "America/New_York"}
+	    ]
+	  }
+	}`
+	got := RedactJSON(payload)
+
+	for _, leaked := range []string{"hunter2", "tok_abc"} {
+		if strings.Contains(got, leaked) {
+			t.Errorf("secret %q survived under the generic \"value\" key:\n%s", leaked, got)
+		}
+	}
+	// A non-secret env var must keep its value, or the tool is useless.
+	if !strings.Contains(got, "America/New_York") {
+		t.Errorf("non-secret env value was destroyed:\n%s", got)
+	}
+	// The names themselves stay visible -- knowing DB_PASSWORD *exists* is useful.
+	if !strings.Contains(got, "DB_PASSWORD") {
+		t.Errorf("env var name should survive:\n%s", got)
+	}
+}
+
+// The audit's MEDIUM finding: TrueNAS directory-services credentials (bindpw,
+// keytab) matched no hint, and upstream's maskCredentials only masks them at the
+// TOP level -- so a nested one bypassed both.
+func TestRedactJSONMasksTrueNASDirectoryCredentials(t *testing.T) {
+	payload := `{"credential": {"bindpw": "ldapsecret", "keytab": "BASE64KEYTAB", "binddn": "cn=admin"}}`
+	got := RedactJSON(payload)
+	for _, leaked := range []string{"ldapsecret", "BASE64KEYTAB"} {
+		if strings.Contains(got, leaked) {
+			t.Errorf("directory-services secret %q survived:\n%s", leaked, got)
+		}
+	}
+}
+
+// A bare "key" is a credential; "keyboard"/"monkey" are not. Exact-match, not
+// substring, so we don't shred harmless fields.
+func TestRedactJSONBareKeyExactMatchOnly(t *testing.T) {
+	got := RedactJSON(`{"key": "s3cret", "keyboard_layout": "us", "monkey_count": 3}`)
+	if strings.Contains(got, "s3cret") {
+		t.Errorf("bare \"key\" was not masked:\n%s", got)
+	}
+	if !strings.Contains(got, "us") || !strings.Contains(got, "3") {
+		t.Errorf("over-redacted a harmless key containing \"key\":\n%s", got)
+	}
+}
+
+// The audit's LOW/INFO finding: unmarshalling into interface{} decodes every number
+// as float64, silently mangling 64-bit integers. A ZFS guid came back wrong.
+func TestRedactJSONPreservesLargeIntegers(t *testing.T) {
+	const guid = "15032414960031428871"
+	got := RedactJSON(`{"guid": ` + guid + `, "size": 9007199254740993}`)
+	if !strings.Contains(got, guid) {
+		t.Errorf("64-bit guid was corrupted by the float64 round-trip:\n%s", got)
+	}
+	if !strings.Contains(got, "9007199254740993") {
+		t.Errorf("integer past 2^53 was corrupted:\n%s", got)
+	}
+}
+
+// Honest documentation of the remaining gap: a secret inside an opaque string blob
+// (a custom app's compose YAML) is NOT caught. This test asserts the CURRENT
+// behaviour so nobody mistakes it for safety -- if someone later fixes it, this
+// test fails loudly and should be updated.
+func TestRedactJSONDoesNotCatchSecretsInsideStringBlobs(t *testing.T) {
+	got := RedactJSON(`{"custom_compose_config_string": "services:\n  db:\n    environment:\n      POSTGRES_PASSWORD: hunter2\n"}`)
+	if !strings.Contains(got, "hunter2") {
+		t.Skip("string-blob redaction now implemented -- update this test and the README's known-limit note")
+	}
+}
@@ -0,0 +1,64 @@
+package truenas
+
+import (
+	"strings"
+	"testing"
+)
+
+// The HIGH finding from the audit: formatAPIErrorWithContext serialized the request
+// params into the error string, and for auth.login_ex those params ARE the plaintext
+// API key. That error is not debug-gated and reaches log.Fatalf on startup and, via
+// CallTool's error return, the model's context. The original redaction test only
+// covered the debug request-LOG frame, which is precisely why this path survived.
+func TestAPIErrorDoesNotLeakApiKey(t *testing.T) {
+	const key = "3-SUPERSECRETKEYVALUE"
+	params := []interface{}{
+		map[string]interface{}{
+			"mechanism": "API_KEY_PLAIN",
+			"username":  "flan",
+			"api_key":   key,
+		},
+	}
+	apiErr := &APIError{Code: 1, Message: "authentication failed"}
+
+	err := formatAPIErrorWithContext(apiErr, "auth.login_ex", params)
+	got := err.Error()
+
+	if strings.Contains(got, key) {
+		t.Fatalf("API key leaked into the error string:\n%s", got)
+	}
+	if !strings.Contains(got, "REDACTED") {
+		t.Errorf("expected the key to be masked, got:\n%s", got)
+	}
+	// Diagnostics must survive, or the error is useless.
+	for _, keep := range []string{"auth.login_ex", "authentication failed", "flan"} {
+		if !strings.Contains(got, keep) {
+			t.Errorf("redaction destroyed useful context %q:\n%s", keep, got)
+		}
+	}
+}
+
+// The deprecated fallback passes the key as a bare positional param, not inside a
+// map. Make sure that shape is masked too.
+func TestAPIErrorDoesNotLeakBarePositionalKey(t *testing.T) {
+	const key = "3-BAREPOSITIONALKEY"
+	err := formatAPIErrorWithContext(
+		&APIError{Code: 1, Message: "bad key"}, "auth.login_with_api_key", []interface{}{key})
+	if strings.Contains(err.Error(), key) {
+		t.Errorf("bare positional API key leaked into the error string:\n%s", err.Error())
+	}
+}
+
+// The middleware-supplied Trace can echo the request back at us.
+func TestAPIErrorRedactsTrace(t *testing.T) {
+	const key = "3-TRACELEAKEDKEY"
+	apiErr := &APIError{
+		Code:    1,
+		Message: "boom",
+		Trace:   map[string]interface{}{"request": map[string]interface{}{"api_key": key}},
+	}
+	err := formatAPIErrorWithContext(apiErr, "auth.login_ex", nil)
+	if strings.Contains(err.Error(), key) {
+		t.Errorf("API key leaked via Trace:\n%s", err.Error())
+	}
+}
@@ -251,6 +251,13 @@ func (c *Client) buildConnectionURLs() ([]string, error) {
 	host, port := c.endpoint, "443"
 	if h, p, err := net.SplitHostPort(c.endpoint); err == nil {
 		host, port = h, p
+	} else {
+		// No port. If the host is a bracketed IPv6 literal ("[fd00::1]"), strip the
+		// brackets: JoinHostPort re-adds them, and without this we'd emit the
+		// double-bracketed "wss://[[fd00::1]]:443/websocket".
+		if strings.HasPrefix(host, "[") && strings.HasSuffix(host, "]") {
+			host = host[1 : len(host)-1]
+		}
 	}
 	// NOTE ON THE ENDPOINT: this client speaks the legacy DDP-style protocol
 	// ({"msg":"connect"}, {"msg":"method"}), which only the unversioned /websocket
@@ -519,24 +526,36 @@ func formatAPIError(apiErr *APIError) error {
 	return fmt.Errorf("%s", errMsg)
 }

-// formatAPIErrorWithContext formats API error with request context for debugging
+// formatAPIErrorWithContext formats an API error with request context.
+//
+// SECURITY: the params it echoes are the request's params -- and for auth.login_ex
+// those params ARE the plaintext API key. Upstream serialized them verbatim, so any
+// middleware error frame on the auth call (a bad key, a mid-session reconnect that
+// re-authenticates and fails) produced an error string containing the key. That
+// error reaches two sinks that are not debug-gated: main's log.Fatalf on startup,
+// and -- via CallTool's error return -- the model's context and the transcript. The
+// debug-log redaction added elsewhere did not cover this path.
+//
+// So every serialized blob here goes through redactForLog. The Trace is redacted
+// too: it is middleware-supplied and can echo the request.
 func formatAPIErrorWithContext(apiErr *APIError, method string, params []interface{}) error {
 	errMsg := fmt.Sprintf("API error: %s (code %d)", apiErr.Message, apiErr.Code)

 	errMsg = fmt.Sprintf("%s\n\nRequest:\n  Method: %s", errMsg, method)

 	if len(params) > 0 {
-		if paramsJSON, err := json.MarshalIndent(params, "  ", "  "); err == nil {
-			errMsg = fmt.Sprintf("%s\n  Params: %s", errMsg, string(paramsJSON))
+		safe := redactParamsForError(method, params)
+		if paramsJSON, err := json.MarshalIndent(safe, "  ", "  "); err == nil {
+			errMsg = fmt.Sprintf("%s\n  Params: %s", errMsg, redactForLog(string(paramsJSON)))
 		}
 	}

 	if apiErr.Trace != nil {
 		if traceStr, ok := apiErr.Trace.(string); ok && traceStr != "" {
-			errMsg = fmt.Sprintf("%s\n\nTrace: %s", errMsg, traceStr)
+			errMsg = fmt.Sprintf("%s\n\nTrace: %s", errMsg, redactForLog(traceStr))
 		} else {
 			if traceJSON, err := json.MarshalIndent(apiErr.Trace, "", "  "); err == nil {
-				errMsg = fmt.Sprintf("%s\n\nTrace: %s", errMsg, string(traceJSON))
+				errMsg = fmt.Sprintf("%s\n\nTrace: %s", errMsg, redactForLog(string(traceJSON)))
 			}
 		}
 	}
@@ -17,6 +17,8 @@ func TestBuildConnectionURLsHonoursExplicitPort(t *testing.T) {
 		{"truenas.local:8443", "wss://truenas.local:8443/websocket"},
 		{"truenas.local", "wss://truenas.local:443/websocket"},
 		{"[fd00::1]:444", "wss://[fd00::1]:444/websocket"},
+		{"[fd00::1]", "wss://[fd00::1]:443/websocket"},   // bracketed IPv6, no port: must not double-bracket
+		{"fd00::1", "wss://[fd00::1]:443/websocket"},     // bare IPv6, no port
 		{"wss://host:9999/websocket", "wss://host:9999/websocket"}, // full URL passes through
 	}
 	for _, tc := range cases {
@@ -18,10 +18,18 @@ import (
 var logSecretKeys = []string{
 	"api_key", "apikey", "password", "passwd", "passphrase",
 	"secret", "token", "credential", "private_key", "privatekey",
+	// TrueNAS directory-services: bind password and Kerberos keytab.
+	"bindpw", "keytab",
 }

+// Whole-key matches, so a bare "key" is caught without masking "keyboard".
+var logExactSecretKeys = map[string]bool{"key": true, "pw": true, "pass": true}
+
 func logKeyIsSecret(k string) bool {
 	k = strings.ToLower(k)
+	if logExactSecretKeys[k] {
+		return true
+	}
 	for _, h := range logSecretKeys {
 		if strings.Contains(k, h) {
 			return true
@@ -53,6 +61,33 @@ func redactLogValue(v interface{}) interface{} {
 	}
 }

+// redactParamsForError sanitises a request's params before they are echoed into an
+// error string.
+//
+// redactForLog masks by KEY NAME, which covers auth.login_ex (whose params are a map
+// containing "api_key"). But the deprecated auth.login_with_api_key passes the key as
+// a BARE POSITIONAL param -- a naked string with no key at all -- so key-based
+// redaction cannot see it, and it leaked verbatim. For any auth.* method every scalar
+// param is credential material by definition, so mask them outright. Map/array params
+// are left for redactForLog, which preserves useful context (username, mechanism).
+func redactParamsForError(method string, params []interface{}) []interface{} {
+	isAuth := strings.HasPrefix(strings.ToLower(method), "auth.")
+	out := make([]interface{}, len(params))
+	for i, p := range params {
+		switch p.(type) {
+		case map[string]interface{}, []interface{}:
+			out[i] = p // key-based redaction handles these after marshalling
+		default:
+			if isAuth {
+				out[i] = "***REDACTED***"
+			} else {
+				out[i] = p
+			}
+		}
+	}
+	return out
+}
+
 // redactForLog masks credential-shaped fields in a JSON document for safe logging.
 // Non-JSON input is returned as-is so a log line is never silently dropped.
 func redactForLog(s string) string {