From 7be4ed1d43f061d8ff11e3985f6d37383342ef49 Mon Sep 17 00:00:00 2001 From: Kyle Isom Date: Wed, 23 Sep 2026 11:47:37 -0700 Subject: [PATCH] p2.5: redact reason/err for permit_restricted logs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Guard messages embed the hostname (resolve , no addresses for ). Restricted callers now log a coarse reason class and err=-. Test dumps slog records and forbids the hostname anywhere in them. docs: P3 review — fail-closed search, cache class, public dialer --- docs/P2.md | 4 +- docs/P3.md | 91 +++++++++++++++++++++++++++------------- internal/api/api.go | 56 +++++++++++++++++++++++-- internal/api/api_test.go | 38 +++++++++++++---- 4 files changed, 148 insertions(+), 41 deletions(-) diff --git a/docs/P2.md b/docs/P2.md index 3d33be3..02663e2 100644 --- a/docs/P2.md +++ b/docs/P2.md @@ -150,7 +150,9 @@ context calls/hits/failures; recover attempts/successes by route; token ratio hi Logs: one INFO line per request with `caller host mode q_len raw_tokens returned_tokens digest_ms fallback`. Never log page content, questions longer than 80 chars, or keys. Callers in `context.permit_restricted` log `host=-` on scrape, fetch, refused, fetch-failed, -and internal lines (P2.5). `final_url` stays in the response body. +and internal lines (P2.5). Their `reason` is a coarse class (`blocked`, `timeout`, …) and +`err` is `-`, so guard messages like `blocked: resolve ` do not reach the journal. +`final_url` stays in the response body. ## 7. Tests diff --git a/docs/P3.md b/docs/P3.md index 1afb370..5b3f8a7 100644 --- a/docs/P3.md +++ b/docs/P3.md @@ -75,8 +75,11 @@ ignored (with a warning if `digest: true`). `digest` defaults **false** and stay ## 3. Sources -Config, not the request. Each source has `enabled` and a fixed endpoint. Missing key or +Config, not the request. Each source has `enabled` and a fixed endpoint. `enabled = false` → source is skipped, no warning. +A source that is **enabled but missing its key** is not skipped silently: log once at +startup, and `/healthz` lists it as degraded (`search.: missing key`). Requests +still run the other sources and add `warnings: : missing key`. ```toml [search] @@ -112,8 +115,10 @@ the shared fan-in. This is the Hermes SearXNG provider's request (`plugins/web/s PLAN §2, verified earlier: `GET /res/v1/llm/context?q=…` with `BRAVE_API_KEY` from `/var/lib/sift/secret.env` (0640 root:sift). **Do not read SearXNG's secret.env.** -Header `X-Subscription-Token`. Map `sources[]` / `grounding` URLs + snippets; drop the -model-written `grounding` answer text if present — we want sources, not Brave's summary. +Header `X-Subscription-Token`. Map result URLs and snippets from the live JSON +(`sources` and any URL-bearing fields). Do **not** assume a model-written +`grounding` answer exists — confirm against a live response before dropping or +keeping any field that is not a URL/title/snippet. This is **not** Brave Web Search (`/res/v1/web/search`). SearXNG already runs `braveapi` as an engine; doubling that is wasted spend. @@ -123,7 +128,8 @@ as an engine; doubling that is wasted spend. - Key only in `/var/lib/sift/secret.env` as `TAVILY_API_KEY`. Never in chat, logs, metrics labels, or the Nix store. - `POST {url}` JSON: `query`, `max_results` (min(k, 20)), `include_answer: false`, - `include_raw_content: false`, `include_images: false`. `Authorization: Bearer`. + `include_raw_content: false`, `include_images: false`, `search_depth: "basic"` + (explicit, so credit cost is fixed). `Authorization: Bearer`. - **Never request or return `answer`.** If Tavily still sends one, drop it before the fan-in. Do not call `/extract`, `/crawl`, or `/map`. - Map `results[].url|title|content` + `score`. Count `usage.credits` toward the daily cap. @@ -145,7 +151,10 @@ placeholder line the same way (empty, on-box fill). affect rerank input if they brought a longer snippet (keep the longer one). 5. Rerank (§5) unless `rerank: false` or Dixie cannot take the call. 6. Truncate to `k` (native) or `limit` (`/v2/search`, default 5, max 20). -7. Store in the search cache. Return. +7. Store in the search cache **only if** every attempted source succeeded and + rerank did not fall back. A timeout or fallback must not pin a partial result + for the rest of the hour (skip the write, or use a short TTL such as 2 m — skip + is the default). Return. Search **never GETs a result URL**. Agents `sift fetch` those through the P1 guard. @@ -170,11 +179,14 @@ only**: 1. Parse the config URL: `http`/`https` only, no userinfo. SearXNG may be loopback; Brave and Tavily must be public hostnames (`api.search.brave.com`, `api.tavily.com`). -2. Resolve that hostname once (and refresh on a long interval, e.g. 10 m). The dialer - `Control` hook allows **only those resolved IPs, only that port**. Anything else, - including a rebinding answer, is refused. -3. No cookies, no proxy env, no redirects to a different host (max 1 hop, same host). - Redirect to a different host → source failure, not a follow. +2. Resolve that hostname once (and refresh on a long interval, e.g. 10 m). **A source + marked public must resolve only to public addresses** — run the guard's public-IP + check (`CheckAddr`) on every record. A poisoned `api.tavily.com → 127.0.0.1` is + refused. The dialer `Control` hook allows **only those resolved IPs, only that port**. + Anything else, including a rebinding answer, is refused. +3. TLS against **system roots**, no override, no `InsecureSkipVerify`. No cookies, no + proxy env, no redirects to a different host (max 1 hop, same host). Redirect to a + different host → source failure, not a follow. 4. Request path/query are code-built (`/search?q=&format=json`, `/res/v1/llm/context?q=`, `POST /search`). Query text is query-escaped; it never becomes a URL. @@ -190,11 +202,17 @@ different loopback port. SQLite, same `/var/lib/sift/cache.db` as P2 digest cache, **new table** `search` (not the digest/embed tables). -- Key: `sha256(norm(query) | sources | k | hour_bucket)`. +- Key: `sha256(norm(query) | sources | k | hour_bucket | caller_class)`. `norm` = trim, collapse whitespace, lowercase (same as digest questions). `sources` = sorted list of enabled source names that were actually attempted (not the ones skipped for missing keys). `hour_bucket` = UTC `YYYY-MM-DDTHH` of the request. + `caller_class` = `restricted` if the caller is in `permit_restricted`, else `fleet`. + This blocks a fleet caller from observing `cached: true` on a query only the + restricted caller issued this hour. Restricted callers currently fail closed + (§9) and never read or write this cache; the class is still in the key so a + later option (b)/(c) cannot reopen the side channel. +- Do not cache a result if any source failed or rerank fell back (§4 step 7). - TTL: `search.cache_ttl` (default 1 h). Hour-bucket already bounds freshness; TTL is the eviction/serve window if a process spans the hour mark. - Value: URLs, titles, snippets, scores, source names. **No page bodies, no Tavily @@ -204,6 +222,10 @@ the digest/embed tables). The URL/fetch cache stays in P4. +**Follow-up (not P3):** the P2 digest cache is content-addressed by page hash + +question, so a weaker same-query side channel exists there too. Note it; do not +fix it in P3. + ## 8. Cost, metrics, logs `/metrics` (JSON, additive to P1/P2): @@ -226,27 +248,31 @@ Logs: one INFO line `search caller=… q_len=… sources=… n=… cache=hit|mis Never log the query text beyond `q_len`, never keys, never snippets. `permit_restricted` callers: `host` is not a search field; do not log result hosts either. -## 9. Restricted callers — open question for Kyle +## 9. Restricted callers — fail closed until Kyle picks -Should `permit_restricted` callers' queries go to third-party engines (Brave, Tavily) -at all? SearXNG is on-box; Brave and Tavily see the query in vendor logs. +SearXNG is **not** private: on orion it forwards every query to bing, `braveapi` +(Kyle's Brave key), wikipedia, github and arxiv. Loopback only hides the hop, not +the vendor. -Do **not** decide this in implementation. Until Kyle answers, P3 code: +Kyle (2026-09-23) confirmed these searches are sensitive. Until he picks one of: -- still serves search for those callers -- still redacts hosts in fetch logs (P2.5) -- does **not** special-case their search queries -- ships a config knob `search.permit_restricted_paid = false` **commented, unused**, - so flipping it later is one line +- **(a)** restricted callers get no search from sift at all +- **(b)** a fixed, reduced SearXNG engine set for them +- **(c)** the same sources as the fleet -List this as unresolved in the review ping. +…the code **fails closed**: `403` `search not permitted` for `permit_restricted` +callers on `/v1/search` and `/v2/search`. No upstream call, no cache read or write, +no query text in logs. Skill: do not fall through to Hermes `web_search`. + +Do not implement (b) or (c) on a guess. ## 10. Bake-off (acceptance) -A fixed set of **~30 real queries** harvested from recent fleet `web_search` use -(gateway/`agent.log` on straylight, talos, titan — the `web_search` / Tavily / -SearXNG lines). Deduped, no secrets, committed as `tests/testdata/search_queries.txt`. -Do not invent the set. +A fixed set of **~30 real queries** harvested from recent **fleet** `web_search` use +(gateway/`agent.log` on straylight, talos, titan). **Exclude** the guardian-star +profile and anything from `permit_restricted` callers. Deduped, no secrets. Kyle +sees the list before it is committed as `tests/testdata/search_queries.txt`. Do not +invent the set. For each query, run four configurations and record top-k URLs + latency + credits: @@ -277,6 +303,8 @@ Plus the mechanical gates: 5. Partial results: one source 500 + two 200 → 200 with `warnings`. 6. Dixie busy → rerank fallback, still 200. 7. Cache hit/miss metrics on a repeated query inside the hour bucket. +8. `permit_restricted` caller: `403 search not permitted`, zero outbound (httptest + spies); a fleet caller with the same query still hits the fakes. ## 11. Code layout @@ -297,14 +325,16 @@ Do not print the file. Tests use httptest upstreams, never live keys. - Canonical URL table (fragment, case, default port, trailing slash). - Fan-in: 3 fake sources, one timeout, one 500, one 200 → partial + warning. -- Tavily client: assert outbound JSON (`include_answer` false, no extract); drop - `answer` from inbound. +- Tavily client: assert outbound JSON (`include_answer` false, `search_depth` basic, + no extract); drop `answer` from inbound. +- Public-source dialer: poisoned private A record refused; TLS uses system roots. - Brave/SearXNG response mapping fixtures (from PLAN's live shapes + Hermes providers). - Dialer: configured loopback allowed; other loopback port refused; request-supplied URL ignored. - Rerank fallback when embed gate is full. - Cache key: same norm/query/sources/k/hour → hit; next hour → miss. - Daily cap: Nth Tavily call skipped. +- Restricted caller: 403, zero upstream requests, no cache access. - `/v2/search` golden: `success` + `data.web`. - Contract harness: `Firecrawl(...).search("contract", limit=3)` against sifttest (search fixtures on the local HTTP server, `-test-allow-prefix` already exists). @@ -324,7 +354,8 @@ hit, `title\nurl\nsnippet`, blank line between. `--json` = native envelope. Non- exits match fetch (5 = sift unreachable). `skill/SKILL.md`: `sift search` becomes the first step for web search; `web_search` -is the fallback when sift is unreachable. Still `sift fetch` for URLs. +is the fallback when sift is unreachable — **except** for `permit_restricted` +callers (403 from sift; do not fall through to Tavily). Still `sift fetch` for URLs. ## 14. Rollout @@ -340,8 +371,8 @@ is the fallback when sift is unreachable. Still `sift fetch` for URLs. ## 15. Open questions for Kyle -1. §9 — do `permit_restricted` queries go to Brave/Tavily? +1. §9 — restricted search: **(a)** none, **(b)** reduced SearXNG engines, **(c)** same + as fleet. Code fails closed at 403 until this is picked. 2. Tavily `daily_credit_cap` default (500 is a guess). 3. Brave LLM Context vs leaving Brave entirely inside SearXNG: bake-off should answer; confirm we should not also call Brave Web Search. -) diff --git a/internal/api/api.go b/internal/api/api.go index f6c2214..1bdb0c0 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -501,14 +501,14 @@ func (s *Server) writeFetchErr(w http.ResponseWriter, c *auth.Caller, rawURL str fe.Msg = "internal error" } if fe.Status == 401 || fe.Status == 403 { - s.Log.Warn("refused", "caller", name, "host", host, "reason", fe.Msg) + s.Log.Warn("refused", "caller", name, "host", host, "reason", s.logReason(c, fe)) } else { - s.Log.Warn("fetch failed", "caller", name, "host", host, "reason", fe.Msg, "status", fe.Status) + s.Log.Warn("fetch failed", "caller", name, "host", host, "reason", s.logReason(c, fe), "status", fe.Status) } writeErr(w, fe.Status, fe.Msg, fe.Hint) return } - s.Log.Error("internal", "caller", name, "host", host, "err", err) + s.Log.Error("internal", "caller", name, "host", host, "err", s.logErr(c, err)) writeErr(w, 500, "internal error", "") } @@ -591,6 +591,56 @@ func (s *Server) logHost(c *auth.Caller, raw string) string { return hostOf(raw) } +func (s *Server) restrictedLog(c *auth.Caller) bool { + return c != nil && permittedCaller(c.Name, s.PermitRestricted) +} + +// logReason is the reason field for fetch error logs. Restricted callers get a +// coarse class so the hostname in guard messages does not land in the journal. +func (s *Server) logReason(c *auth.Caller, fe *fetch.Error) string { + if fe == nil { + return "-" + } + if s.restrictedLog(c) { + return coarseReason(fe) + } + return fe.Msg +} + +func (s *Server) logErr(c *auth.Caller, err error) any { + if s.restrictedLog(c) { + return "-" + } + return err +} + +func coarseReason(fe *fetch.Error) string { + switch fe.Status { + case 401: + return "unauthorized" + case 403: + return "blocked" + case 400: + return "bad request" + case 413: + return "too large" + case 415: + return "unsupported" + case 429: + return "busy" + case 504: + return "timeout" + default: + if fe.Status >= 400 && fe.Status < 500 { + return "upstream 4xx" + } + if fe.Status >= 500 { + return "upstream 5xx" + } + return "-" + } +} + func writeErr(w http.ResponseWriter, status int, msg, hint string) { if status == 502 { status = 500 diff --git a/internal/api/api_test.go b/internal/api/api_test.go index b8beda0..7afc4d6 100644 --- a/internal/api/api_test.go +++ b/internal/api/api_test.go @@ -284,7 +284,7 @@ func recHost(r slog.Record) string { var host string r.Attrs(func(a slog.Attr) bool { if a.Key == "host" { - host = fmtString(a.Value) + host = a.Value.String() } return true }) @@ -293,11 +293,17 @@ func recHost(r slog.Record) string { func recMsg(r slog.Record) string { return r.Message } -func fmtString(v slog.Value) string { - if v.Kind() == slog.KindString { - return v.String() - } - return v.Any().(string) +func recDump(r slog.Record) string { + var b strings.Builder + b.WriteString(r.Message) + r.Attrs(func(a slog.Attr) bool { + b.WriteByte(' ') + b.WriteString(a.Key) + b.WriteByte('=') + b.WriteString(a.Value.String()) + return true + }) + return b.String() } func TestRestrictedCallerLogsBlankHost(t *testing.T) { @@ -345,7 +351,7 @@ func TestRestrictedCallerLogsBlankHost(t *testing.T) { src, _ := env["source"].(map[string]any) final, _ := src["final_url"].(string) if !strings.Contains(final, hostOf(ts.URL)) { - t.Fatalf("final_url still in body, got %q", final) + t.Fatalf("final_url missing from body, got %q", final) } rec = postJSON(t, s, "/v1/fetch", strings.TrimSpace(string(gsKey)), map[string]any{"url": "http://10.0.0.5/", "mode": "clean"}) @@ -353,6 +359,12 @@ func TestRestrictedCallerLogsBlankHost(t *testing.T) { t.Fatalf("gs refused: %d %s", rec.Code, rec.Body.String()) } + const badHost = "this-host-does-not-exist-sift-p25.invalid" + rec = postJSON(t, s, "/v1/fetch", strings.TrimSpace(string(gsKey)), map[string]any{"url": "http://" + badHost + "/", "mode": "clean"}) + if rec.Code != 403 { + t.Fatalf("gs nxdomain: %d %s", rec.Code, rec.Body.String()) + } + rec = postJSON(t, s, "/v1/fetch", strings.TrimSpace(string(fleetKey)), map[string]any{"url": ts.URL, "mode": "clean"}) if rec.Code != 200 { t.Fatalf("fleet fetch: %d %s", rec.Code, rec.Body.String()) @@ -386,6 +398,18 @@ func TestRestrictedCallerLogsBlankHost(t *testing.T) { if fleetFetch != wantHost { t.Fatalf("fleet fetch host=%q want %q", fleetFetch, wantHost) } + + h.mu.Lock() + for _, r := range h.recs { + if attrCaller(r) != "straylight-gs" { + continue + } + dump := recDump(r) + if strings.Contains(dump, badHost) || strings.Contains(dump, "10.0.0.5") { + t.Fatalf("restricted log leaked host: %s", dump) + } + } + h.mu.Unlock() } func attrCaller(r slog.Record) string {