From b41abb65b70515d209a890ab95990b94d1018486 Mon Sep 17 00:00:00 2001 From: buihongduc132 Date: Fri, 31 Jul 2026 11:59:04 +0700 Subject: [PATCH] feat: herdr session protection + version audit + dry-run mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add 4 herdr rules (server stop, session stop/delete, workspace close) to protect herdr terminal workspace manager from accidental kills. Extend session_kill_targets to include herdr/bermuda daemon. Add pi_opa_net_version to all audit traces (decision metadata, filesystem JSONL, OTLP export) for version correlation. Add PIOPANET_DRY_RUN=1 safety marker — tests run with this flag to guarantee evaluation-only mode. Bump to 0.5.0. 443 tests pass (2 pi-session smoke tests remain pre-existing failures due to stale deployed v0.3.0). --- .gitignore | 4 + CHANGELOG.md | 12 + .../2026-07-20-locked-decisions.yaml | 183 ++++++++++++++ .../2026-07-20-open-threads.yaml | 219 +++++++++++++++++ ...26-07-20-turn1-safetynet-gap-assessment.md | 91 +++++++ ...07-20-turn2-explore-unlock-design-forks.md | 218 +++++++++++++++++ ...-20-turn2a-gotcha-unlock-design-batch-a.md | 228 ++++++++++++++++++ .../2026-07-20-turn3-lock-decisions-ttl.md | 152 ++++++++++++ .../2026-07-20-turn4-yagni-finalize-seams.md | 171 +++++++++++++ ...07-20-turn4a-gotcha-yagni-seams-batch-b.md | 216 +++++++++++++++++ .../2026-07-20-rule-unlock-keys/README.md | 26 ++ .../2026-07-20-rule-unlock-keys/references.md | 31 +++ package.json | 2 +- policy/safety.rego | 40 ++- schemas/decision-output.v1.json | 10 +- src/audit/OtlpAuditSink.ts | 2 + src/output/DecisionBuilder.ts | 9 + src/pi/audit.ts | 3 + src/rules/RuleRegistry.ts | 3 +- src/rules/catalog.ts | 27 +++ src/version.ts | 23 ++ tests/e2e/e2e.test.ts | 58 ++++- tests/e2e/pi-session-smoke.test.ts | 15 ++ tests/unit/audit/OtlpAuditSink.test.ts | 4 +- tests/unit/rules/catalog-parity.test.ts | 25 ++ 25 files changed, 1763 insertions(+), 9 deletions(-) create mode 100644 flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-locked-decisions.yaml create mode 100644 flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-open-threads.yaml create mode 100644 flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn1-safetynet-gap-assessment.md create mode 100644 flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn2-explore-unlock-design-forks.md create mode 100644 flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn2a-gotcha-unlock-design-batch-a.md create mode 100644 flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn3-lock-decisions-ttl.md create mode 100644 flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn4-yagni-finalize-seams.md create mode 100644 flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn4a-gotcha-yagni-seams-batch-b.md create mode 100644 flow/findings/2026-07-20-rule-unlock-keys/README.md create mode 100644 flow/findings/2026-07-20-rule-unlock-keys/references.md create mode 100644 src/version.ts diff --git a/.gitignore b/.gitignore index afdc04d..c8bca24 100644 --- a/.gitignore +++ b/.gitignore @@ -10,3 +10,7 @@ node_modules/ .gitnexus/ .worktrees/ .wt-*/ +# Runtime artifacts +.pi-opa-net/ +.pi-subagents/ +.pi/ diff --git a/CHANGELOG.md b/CHANGELOG.md index beaf15d..5b4f437 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,18 @@ All notable changes to this project are documented in this file. 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). +## [0.5.0] - 2026-07-31 + +### Added + +- **herdr session protection rules** — 4 new rules blocking destructive herdr commands: `block-herdr-server-stop`, `block-herdr-session-stop`, `block-herdr-session-delete`, `block-herdr-workspace-close`. Extends `session_kill_targets` in the rego policy to include `herdr` and `bermuda` (herdr plugin daemon), so `pkill herdr` / `killall bermuda` are also blocked. Herdr is a terminal workspace manager for AI coding agents; killing it destroys active workspaces, sessions, and agent state. +- **`pi_opa_net_version` in all audit traces** — every decision metadata and audit log entry (filesystem JSONL + OTLP export) now carries the exact pi-opa-net package version. Enables after-the-fact version correlation when debugging decision trails. New `src/version.ts` reads the version from package.json once at module load. +- **`PIOPANET_DRY_RUN` safety mode** — setting `PIOPANET_DRY_RUN=1` adds a `dry_run: true` marker to decision metadata. Tests and CI set this flag to guarantee the CLI is in evaluation-only mode (no command execution). E2E tests now run with this flag enabled. + +### Fixed + +- **pi-session smoke test: init git repo in temp cwd.** Pi requires a `.git` directory to start; the test now runs `git init` in the temp dir so the session can launch. Previously pi refused to start with "not a git repo". + ## [0.4.2] - 2026-07-24 ### Fixed diff --git a/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-locked-decisions.yaml b/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-locked-decisions.yaml new file mode 100644 index 0000000..b11a6ed --- /dev/null +++ b/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-locked-decisions.yaml @@ -0,0 +1,183 @@ +# Locked decisions for rule-unlock-keys explore +# Each entry traces: turn + parts (section ids) + verbatim user_quote + decision. +# Immutable inputs to future proposals. + +- id: LD-L1 + turn: turn3 + parts: ["Decisions locked from your answers"] + user_quote: | + "Q1: no need unlock-once , key per rule (like git stash , different than git stash push)" + decision: Granularity K1 per-rule only. One rule = one key. Drop K3 (--unlock-once). + status: locked + +- id: LD-L2 + turn: turn3 + parts: ["Decisions locked from your answers"] + user_quote: | + "No god key: correct" + decision: No god-key. Refuse PIOPANET_UNLOCK_ALL. + status: locked + +- id: LD-L3 + turn: turn3 + parts: ["Decisions locked from your answers"] + user_quote: | + "design 2 approach: TTL and also long-live" + decision: Two key types — long-lived (ll_<16hex>) and TTL (ttl..<16hex>). Self-describing prefix. + status: locked + +- id: LD-L4 + turn: turn3 + parts: ["Decisions locked from your answers"] + user_quote: | + "Q4: no , only these for now." + decision: Delivery via ENV + --unlock + --unlock-stdin. Nothing else. + status: locked + +- id: LD-L5 + turn: turn4 + parts: ["Locked decisions (final)"] + user_quote: | + "Q1: no need unlock-once , key per rule (like git stash , different than git stash push)" + decision: Granularity K1 per-rule re-statement (same as LD-L1). Corrected stale hindsight memory. + status: superseded + superseded_by: LD-L1 + +- id: LD-L6 + turn: turn4 + parts: ["Locked decisions (final)"] + user_quote: | + "give me YAGNI solutions for now which is be able to easily extend later" + decision: Unlock check lives TS-side post-eval filter (confirmed by silence — assistant proposed in turn 2, user did not object). + status: locked + +- id: LD-Y1 + turn: turn4 + parts: ["YAGNI choices + extension seams", "[D-Y1] Salt — minimal now, swappable later"] + user_quote: | + "YAGNI" + decision: Salt deploy-local default (~/.pi-opa-net/salt) + env override seam (PIOPANET_UNLOCK_SALT). SaltResolver interface for future extension. + status: locked + +- id: LD-Y2 + turn: turn4 + parts: ["YAGNI choices + extension seams", "[D-Y2] Audit — minimal now, hook seam later"] + user_quote: | + "YAGNI" + decision: Audit decision-record only (mandatory source:opa-unlocked). NoOpSink default. AuditSink interface for future FileAppendSink/WebhookSink. + status: locked + +# Gotcha-coverage auto-decisions (step 30) + +- id: LD-G1 + turn: turn2a + parts: ["Rank 5 (Sophisticated)", "fail-open (OPA down) is already a KEYLESS universal bypass"] + user_quote: "auto-decided (no explicit user lock)" + decision: Accept fail-open degradation when keys present, but add source:'fail-open-keyless' for audit visibility. Do NOT add fail-closed complexity. + status: locked + auto_decided: true + resolves: OT3 + tradeoff: | + Option A: fail-closed when keys present (adds new failure mode: OPA down + keys = all blocked). + Option B: accept degradation + audit visibility (chosen). + Rationale: YAGNI. User's threat model is trusted agents. If OPA is down, system already degraded. + Adding fail-closed creates cascading failure. Audit can detect degradation via source field. + +- id: LD-G2 + turn: turn2a + parts: ["Rank 5 (Sophisticated)", "--unlock-stdin DOUBLE-READS fd 0"] + user_quote: "auto-decided (no explicit user lock)" + decision: --unlock-stdin requires positional command arg (so stdin is free for the key). + status: locked + auto_decided: true + resolves: OT4 + tradeoff: | + Option A: --unlock-stdin requires positional command arg (chosen). + Option B: keys from fd 3 (adds complexity). + Option C: --command-stdin vs --key-stdin flags (adds new flags, violates LD-L4 'nothing else'). + Rationale: YAGNI. Option A preserves all three channels (ENV + --unlock + --unlock-stdin) without + adding new flags. Slightly less flexible (can't pipe both command and key), but simplest. + +- id: LD-G3 + turn: turn2a + parts: ["Rank 5 (Sophisticated)", "Cache poisoning — cache key must include the unlock-set"] + user_quote: "auto-decided (no explicit user lock)" + decision: Force cacheTtlMs=0 when any unlock keys are present. Do NOT add cache key complexity. + status: locked + auto_decided: true + resolves: OT5 + tradeoff: | + Option A: cache key includes hash of unlock-key-set (adds complexity). + Option B: force cacheTtlMs=0 when keys present (chosen). + Rationale: YAGNI. Cache is not enabled by default (cacheTtlMs default 0). When keys present, + disable caching entirely. Simpler, safer. Performance hit acceptable for security-critical path. + +- id: LD-G4 + turn: turn2a + parts: ["Rank 4 (Significant)", "gcloud / bq rules CANNOT be unlocked"] + user_quote: "auto-decided (no explicit user lock)" + decision: Scope unlock to catalog-registered rules only. Document gcloud/bq as out-of-scope. + status: locked + auto_decided: true + resolves: OT6 + tradeoff: | + Option A: register gcloud/bq as discrete RuleMeta entries (adds complexity, sprintf rules are dynamic). + Option B: scope out + document (chosen). + Rationale: YAGNI. gcloud/bq are rare rules with dynamic messages. Registering them as discrete entries + adds schema complexity. Scoping out is simpler. pi-opa-net unlock-key --list reflects catalog only. + +- id: LD-G5 + turn: turn2a + parts: ["Rank 4 (Significant)", "NO revocation for long-lived keys"] + user_quote: "auto-decided (no explicit user lock)" + decision: Accept salt-rotation-only as the only LL key revocation mechanism. Document as hard constraint. + status: locked + auto_decided: true + resolves: OT9 + tradeoff: | + Option A: accept salt-rotation-only (chosen). + Option B: add minimal revocation list seam (~/.pi-opa-net/revoked.txt) (adds server-side state). + Rationale: YAGNI. Revocation list contradicts 'no server-side state' design. LL keys are for trusted + agents. If leaked, rotate salt. Operational sledgehammer accepted as documented constraint. + +- id: LD-G6 + turn: turn2a + parts: ["Rank 5 (Sophisticated)", "Multiple rules fire on one command → need ALL keys"] + user_quote: "auto-decided (no explicit user lock)" + decision: Allow only when ALL blocking reasons are unlocked. Add reasons[].bypassed per-entry + metadata.unlock_blocked_count. + status: locked + auto_decided: true + resolves: OT11 + tradeoff: | + Option A: allow ⟺ every severity:block reason has matching valid key (chosen). + Option B: partial bypass (security hole). + Rationale: No alternative. Partial bypass is undefined output state. All-or-nothing is the only + correct semantics. Schema already has reasons[].bypassed per-entry (turn3 delta). + +- id: LD-G7 + turn: turn2a + parts: ["Rank 4 (Significant)", "rule_id mutability invalidates all minted keys"] + user_quote: "auto-decided (no explicit user lock)" + decision: Forbid rule_id renames in CONTRIBUTING. Treat rule_id as immutable hash input. + status: locked + auto_decided: true + resolves: OT12 + tradeoff: | + Option A: forbid renames in CONTRIBUTING (chosen). + Option B: carry key_aliases:[...] field (adds schema complexity). + Rationale: YAGNI. rule_id is a hash input, not a user-facing name. key_aliases adds schema complexity. + Forbidding renames is simpler (documentation/policy). If rename needed, mint new keys under new id. + +- id: LD-G8 + turn: turn2a + parts: ["Rank 4 (Significant)", "TS-filter crash has NO fail-mode"] + user_quote: "auto-decided (no explicit user lock)" + decision: Wrap unlock filter in try/catch. On error, fall back to UN-FILTERED engine decision (if OPA said deny, stay deny). Log source:'unlock-filter-error'. + status: locked + auto_decided: true + resolves: OT14 + tradeoff: | + Option A: wrap in try/catch, fall back to un-filtered decision (chosen). + Option B: no fail-mode (crash = undefined behavior). + Rationale: No alternative. If filter crashes, never allow-by-accident. Fall back to OPA decision. + Log source:'unlock-filter-error' for audit. This is the only safe fail-mode. diff --git a/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-open-threads.yaml b/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-open-threads.yaml new file mode 100644 index 0000000..a536096 --- /dev/null +++ b/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-open-threads.yaml @@ -0,0 +1,219 @@ +# Open threads for rule-unlock-keys explore +status: ongoing +threads: + - id: OT1 + topic: Capture as openspec change rule-unlock-keys/ + raised_in: turn4 + status: resolved + resolution: openspec change captured at openspec/changes/rule-unlock-keys/ (proposal.md, design.md, specs/rule-unlock/spec.md, tasks.md, README.md). Implemented on branch rule-unlock-keys, merged to main as commit 5cdd037 (PR #3), published as pi-opa-net@0.2.0 to npm. Verified by jewilo (hash 072026-c604475a) + jewilo-dev with rag-quick model (hash 072026-7291243b). + resolved_in: implementation-session-2026-07-20 + notes: | + Originally offered in turn 4 of explore; resolved by full implementation + + deploy cycle after explore concluded. + + - id: OT2 + topic: Enumerate ospx manifest steps 11-70 + raised_in: turn2 + status: open + resolution: "" + resolved_in: "" + notes: | + ospx family manifest (~/.pi/agent/cmd-family/ospx.yml) shows "Step 10/70" — + assistant lacks direct access to enumerate steps 11-70. Offered to read the + manifest; user has not yet responded. + + # Gotcha-coverage additions (step 20) + + - id: OT3 + topic: "CRITICAL: fail-open = keyless universal bypass on OPA outage" + raised_in: turn2a + status: resolved + resolution: auto-decided per [LD-G1] + resolved_in: findings-resolve-threads + source: gotcha-coverage + notes: | + Batch A Rank 5. When OPA is down (failMode='open'), unlock filter has nothing + to filter → command allowed with NO source:opa-unlocked audit. Attacker who + induces OPA unavailability bypasses EVERY rule lock without any key. + Mitigation: fail-closed when keys present, or source:'fail-open-keyless'. + + - id: OT4 + topic: "CRITICAL: --unlock-stdin double-reads fd 0 vs command-via-stdin" + raised_in: turn2a + status: resolved + resolution: auto-decided per [LD-G2] + resolved_in: findings-resolve-threads + source: gotcha-coverage + notes: | + Batch A Rank 5. src/cli/run.ts resolveRaw() ALREADY reads stdin when no + positional command given. --unlock-stdin ALSO reads stdin. Cannot read fd 0 + twice. Implementation blocker. + Mitigation: --unlock-stdin requires positional command arg, OR keys from fd 3, + OR --command-stdin vs --key-stdin flags. + + - id: OT5 + topic: "CRITICAL: cache poisoning — cache key must include unlock-set" + raised_in: turn2a + status: resolved + resolution: auto-decided per [LD-G3] + resolved_in: findings-resolve-threads + source: gotcha-coverage + notes: | + Batch A Rank 5. src/config/Config.ts declares cacheTtlMs. When caching enabled, + cache keyed on parsed command. Agent A runs cmd WITH key → allow → CACHED. + Agent B runs same cmd WITHOUT key → cache HIT → allow with NO key. + Mitigation: cache key includes hash of unlock-key-set, OR force cacheTtlMs=0 + when keys present. + + - id: OT6 + topic: "CRITICAL: gcloud/bq rules un-unlockable (no canonical rule_id)" + raised_in: turn2a + status: resolved + resolution: auto-decided per [LD-G4] + resolved_in: findings-resolve-threads + source: gotcha-coverage + notes: | + Batch A Rank 4. src/rules/catalog.ts has NO entries for gcloud/bq. + RuleRegistry.lookup synthesizes custom: — not enumerable by + unlock-key --list, different per verb, not what user can mint. + Mitigation: register gcloud/bq as discrete RuleMeta entries, OR scope unlock + to catalog-registered rules and document gcloud/bq as out-of-scope. + + - id: OT7 + topic: "CRITICAL: schema additionalProperties:false blocks all additive fields" + raised_in: turn4a + status: resolved + resolution: Implementation task (not a design decision). Schema update is part of ship surface — already decided in turn4a. + resolved_in: findings-resolve-threads + source: gotcha-coverage + notes: | + Batch B Rank 4. Current schema has additionalProperties:false at 4 levels. + Proposed changes add source:'opa-unlocked', reasons[].bypassed, unlock_key_id, + unlock_key_type, unlock_expires_at, unlock_status, metadata.unlock_count, + unlock_agent. EVERY ONE requires schema change. + Mitigation: schema update is part of ship surface — not optional. + + - id: OT8 + topic: "CRITICAL: DecisionBuilder.source type doesn't include 'opa-unlocked'" + raised_in: turn4a + status: resolved + resolution: Implementation task (not a design decision). Type change is part of ship surface — already decided in turn4a. + resolved_in: findings-resolve-threads + source: gotcha-coverage + notes: | + Batch B Rank 4. DecisionOutput.source typed as 'opa'|'fail-open'|'fail-closed'|'cached'. + Adding 'opa-unlocked' requires changing TypeScript type AND EngineDecision.source + type. Unlock filter doesn't change engine source — it changes OUTPUT source. + Mitigation: UnlockFilter returns modified EngineDecision with source:'opa-unlocked' + when any rule bypassed, OR DecisionBuilder accepts unlock result and overrides. + + - id: OT9 + topic: "IMPORTANT: no LL key revocation — leaked ll_ key valid forever" + raised_in: turn2a + status: resolved + resolution: auto-decided per [LD-G5] + resolved_in: findings-resolve-threads + source: gotcha-coverage + notes: | + Batch A Rank 4. LL keys have NO embedded expiry and NO server-side revocation list. + Leaked ll_<16hex> valid until salt rotates — and rotating salt is expensive and + kills ALL keys. No way to revoke a SINGLE compromised LL key. + Mitigation: accept salt-rotation-only (documented) OR add minimal revocation list + seam (~/.pi-opa-net/revoked.txt). + + - id: OT10 + topic: "IMPORTANT: TTL replayable-within-window (not single-use)" + raised_in: turn2a + status: resolved + resolution: Documentation task (not a design decision). Already decided in turn2a: "document clearly: TTL = time-bounded unlimited use; no single-use mode." + resolved_in: findings-resolve-threads + source: gotcha-coverage + notes: | + Batch A Rank 4. ttl.. valid for EVERY invocation until exp. + Dropping K3 removed only single-use primitive. --ttl 3600 is "unlock this rule + for 1 hour of UNLIMITED use," not "unlock one command." + Mitigation: document clearly: "TTL = time-bounded unlimited use; no single-use mode." + + - id: OT11 + topic: "IMPORTANT: multiple rules fire on one command → need ALL keys" + raised_in: turn2a + status: resolved + resolution: auto-decided per [LD-G6] + resolved_in: findings-resolve-threads + source: gotcha-coverage + notes: | + Batch A Rank 5. Single command can be denied by MORE than one rule. + K1 per-rule means each fired rule needs its OWN key. If agent holds key for + rule A but not rule B, command stays blocked. Schema delta has no way to express + "1 of 2 reasons bypassed." + Mitigation: define filter semantics as "allow ⟺ every severity:block reason has + matching valid key." Add reasons[].bypassed per-entry + metadata.unlock_blocked_count. + + - id: OT12 + topic: "IMPORTANT: rule_id mutability invalidates all minted keys" + raised_in: turn2a + status: resolved + resolution: auto-decided per [LD-G7] + resolved_in: findings-resolve-threads + source: gotcha-coverage + notes: | + Batch A Rank 4. Key = HMAC(salt, rule_id). rule_id treated as immutable, but + catalog ALREADY documents drift: block-rm-bd-sub-skills and block-rm-beads-subdirs + flagged "rule is misnamed 'allow'". Routine rename silently invalidates every key. + Mitigation: treat rule_id as versioned identifier (forbid renames in CONTRIBUTING, + OR carry key_aliases:[...] field). + + - id: OT13 + topic: "IMPORTANT: clock manipulation makes TTL keys effectively permanent" + raised_in: turn2a + status: resolved + resolution: Documentation task (not a design decision). Already decided in turn2a: "state threat-model limit." + resolved_in: findings-resolve-threads + source: gotcha-coverage + notes: | + Batch A Rank 4. TTL verification is now > exp where now = Date.now() on verifier's + process clock. Verifier that freezes/rewinds clock keeps now < exp forever → + TTL key becomes permanently valid. "No server-side state" removes only anchor. + Mitigation: state threat-model limit: "TTL relies on verifier's clock being honest; + treat TTL as convenience, not enforcement, against compromised agent." + + - id: OT14 + topic: "IMPORTANT: TS-filter crash has NO fail-mode" + raised_in: turn2a + status: resolved + resolution: auto-decided per [LD-G8] + resolved_in: findings-resolve-threads + source: gotcha-coverage + notes: | + Batch A Rank 4. TS-side unlock filter has NO equivalent fail-mode. If filter + throws, whole runCli pipeline throws → bin wrapper behavior undefined. + Mitigation: wrap unlock filter in try/catch: on filter error, fall back to + UN-FILTERED engine decision (if OPA said deny, stay deny; never allow-by-accident). + + - id: OT15 + topic: "IMPORTANT: salt file race on first-use auto-generation" + raised_in: turn4a + status: resolved + resolution: Implementation detail (not a design decision). Already decided in turn4a: "use O_CREAT|O_EXCL." + resolved_in: findings-resolve-threads + source: gotcha-coverage + notes: | + Batch B Rank 3. Two concurrent pi-opa-net unlock-key invocations on first run + both detect missing salt → both generate → last-write-wins. Keys minted against + first salt become invalid once second overwrites. + Mitigation: use O_CREAT|O_EXCL (or wx flag) for salt file creation. Loser re-reads + winner's file. Or: generate into temp file, rename() atomically. + + - id: OT16 + topic: "IMPORTANT: salt file permissions default to world-readable" + raised_in: turn4a + status: resolved + resolution: Implementation detail (not a design decision). Already decided in turn4a: "fs.writeFileSync with mode:0o600." + resolved_in: findings-resolve-threads + source: gotcha-coverage + notes: | + Batch B Rank 3. ~/.pi-opa-net/salt contains root secret for all unlock keys. + If created with default umask (022 → file mode 644), any user on box reads it → + derives all keys → full bypass. + Mitigation: SaltResolver MUST fs.writeFileSync(path, salt, {mode:0o600}) on creation. + Verify on read: warn if mode != 600. diff --git a/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn1-safetynet-gap-assessment.md b/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn1-safetynet-gap-assessment.md new file mode 100644 index 0000000..74a8ddb --- /dev/null +++ b/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn1-safetynet-gap-assessment.md @@ -0,0 +1,91 @@ +# Explore Turn 1 — safety-net gap assessment + +> Date: 2026-07-20 +> Phase: context-gathering (pre-explore) +> Status: complete — surfaced 2 missing gates (branch delete, rm -rf) that motivated the unlock-keys explore + +--- + +## [user] + +> **[user]** (verbatim — trust chain L1): +> +> a. do the current implement all the functionalities of the safety-net ; +> b. do we ported all the current global cc-safety-net rules? +> +> c. do we be able to gate these yet: +> - no git commit --no-verify; +> - no git stash / pop or other act that would clear / wipe the current works ; +> No branch delete ; +> - no rm -rf ; (except in /tmp directory) ; +> - no git add . + +--- + +## [assistant] + +Read policy + catalog + canonical rulebook + ran parity tests. + +### a. all safety-net functionality implemented? + +**Partial — rules + decision engine YES, pi hook wiring NO.** + +``` +[L?] component status +[D1] rule parity (42/42) ✓ cupcake parity test 68 pass +[D2] decision semantics ✓ STRICTER than cc-safety-net (solves bare-stash-default) +[D3] structured output (allow+deny) ✓ schema v1, asymmetric-output limitation fixed +[D4] fail-open mode ✓ matches pi-safety-net fork guarantee +[D5] pi tool_call hook adapter ✗ NOT in this repo (OT5 → future pi-opa-net-ext) +``` + +Engine = done. Adapter that wires it into pi PreToolUse hook = unbuilt. + +### b. all global cc-safety-net rules ported? + +**YES — 42/42 user rules ported.** Source: `~/.cc-safety-net/rules/user-rules/rulebook.json` (42 rules). Parity test asserts every rulebook `name` appears as `deny.rule_id` → passes. + +``` +[A1] canonical rulebook snapshot at tests/fixtures/user-rules.rulebook.json +[A2] every `name` → `deny.rule_id` (parity test green) +[A3] reasons copied verbatim +[A4] 4 new tmux/pkill/killall session-kill rules ADDED beyond original pi-opa-net scope +``` + +NOT ported: cc-safety-net *system* builtins (regex fallback defaults) — not user rules, intentional. + +### c. can we gate these yet? + +``` +[C1] git commit --no-verify ✓ ALREADY GATED — block-git-commit-no-verify (policy L70-74) +[C2] git stash/pop/wipe ✓ ALREADY GATED — block-git-stash-mutations + builtin:bare-stash-default +[C3] no branch delete ✗ NOT GATED — neither `git branch -D/-d` nor `git push origin --delete` +[C4] no rm -rf (except /tmp) ✗ NOT GATED — no generic rm rule, no /tmp carveout +[C5] no git add . ✓ ALREADY GATED — block-git-add-dot (policy L149-154) +``` + +**3/5 done. 2/5 missing.** + +### CA — callouts + +- `[CA1]` [C3] [C4] **NOT a regression** — upstream cc-safety-net rulebook itself lacks them (only `-f/-M/-C` force-moves blocked, no generic `rm -rf`). pi-opa-net faithfully ported → inherited the gap. +- `[CA2]` [C3] [C4] **trivial to add in pi-opa-net** — OPA makes these 6-line rules. cc-safety-net's token-OR matcher is the limiting factor upstream; pi-opa-net doesn't have that limit (already proved it with bare-stash-default [D2]). Each new rule = ~8 lines rego + 1 catalog entry + parity test fixture. +- `[CA3]` [C4] **`/tmp` carveout needs `cwd` in input contract** — `decision-output.v1` currently has no `cwd`/`working_dir` field in `input`. Cupcake variant passes `cwd`. To carve `/tmp` either: (a) add `cwd` to input schema, or (b) path-arg match (`/tmp/*`). Decision needed before implement. +- `[CA4]` pi-opa-net still **unwired into pi** [D5] — even after adding [C3] [C4] rules, nothing in this repo blocks a live command until `pi-opa-net-ext` ships. + +### A — assumptions + +- [A] "all functionalities of safety-net" read as: rule parity + decision contract + fail-mode + hook adapter. Did not check cc-safety-net UI/cache/CLI features (out of scope — pi-opa-net is decision engine only per LD3). +- [A] canonical rulebook = `~/.cc-safety-net/rules/user-rules/rulebook.json` (42 active). Verified by listing — assumes this is current global config. + +**Verdict:** to gate [C3] [C4] → 1 short rego PR + fixture. Want me to draft? + +--- + +## Status at end of turn 1 + +- Established: pi-opa-net has 42/42 rule parity, full decision engine, but no pi hook wiring. +- Gap surfaced: [C3] branch-delete + [C4] rm -rf not gated (inherited from upstream rulebook, trivially fixable in OPA). +- These gaps form context for the unlock-keys explore in turn 2. + +→ Continue to `2026-07-20-turn2-explore-unlock-design-forks.md` diff --git a/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn2-explore-unlock-design-forks.md b/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn2-explore-unlock-design-forks.md new file mode 100644 index 0000000..da50b76 --- /dev/null +++ b/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn2-explore-unlock-design-forks.md @@ -0,0 +1,218 @@ +# Explore Turn 2 — unlock-keys design forks + +> Date: 2026-07-20 +> Phase: explore (kickoff under `/opsx:explore`) +> Status: complete — four design forks mapped, 7 open questions surfaced + +--- + +## [user] + +> **[user]** (verbatim — trust chain L1): +> +> Make the kind of hidden bypass / hidden unlock functionalities , sometimes some VERY specific agent will have capability of bypass some rules ; But they must have some kind of unlock key: Key is either be set via: ENV , addition args; Note that these are like generated from the hash of the rules and it is like password , only user have it , user will provide to the agent if they want to; + +--- + +> **[note]** `/opsx:explore` cmd invoked (family ospx, step 10/70). The cmd expanded into the explore-mode preamble governing assistant stance: curious-not-prescriptive, visualize freely, no implementation (explore = thinking), OpenSpec-aware. Full preamble lives in `~/.pi/agent/cmd-family/ospx.yml`. Only user-typed text reproduced above; cmd boilerplate not duplicated. + +--- + +## [assistant] + +In explore mode — thinking, not implementing. Read the codebase + the in-flight `conditional-branch-gate` design to ground this. + +### What I read + +``` +[E1] src/util/digest.ts ............ sha256Prefix(file) — drift detection, 12 hex chars +[E2] src/output/DecisionBuilder.ts . owns schema assembly; reasons→rule_id lookup; severity='block' +[E3] src/cli/run.ts ................ parse → engine.evaluate → builder.build → validate → emit +[E4] policy/safety.rego ............ deny[msg] blocks; default allow=true (fail-open) +[E5] openspec/changes/conditional-branch-gate/proposal.md ... adds input.signals.* — sibling pattern to copy +``` + +### The shape of the problem + +``` + ┌─────────────────────────────────────────┐ + │ USER (sole keychain holder) │ + │ │ + │ pi-opa-net unlock-keys │ + │ ┌────────────────────────────────┐ │ + │ │ block-git-stash-mutations │ │ + │ │ ↳ a3f9c2b8e1d4... │ │ ← HMAC(salt, rule_id) + │ │ block-git-reset-hard │ │ + │ │ ↳ 7c2f8a1b... │ │ + │ └────────────────────────────────┘ │ + └──────────────┬──────────────────────────┘ + │ user hands specific key(s) + │ to a trusted agent's env + ▼ + ┌──────────────────────────────────────────┐ + │ AGENT (possesses key = capability) │ + │ PIOPANET_UNLOCK_KEYS=a3f9c2b8e1d4,... │ + │ (or: --unlock a3f9... per invoke) │ + └──────────────┬───────────────────────────┘ + │ agent runs `git stash pop` + ▼ + ┌─────────┬─────────┬─────────────┬───────────┐ + │ PARSE │ EVAL │ UNLOCK │ EMIT │ + │ tokens │ rego │ filter (TS) │ schema │ + └─────────┴─────────┴─────┬───────┴───────────┘ + │ for each deny reason: + │ key = HMAC(salt, rule_id) + │ if key ∈ unlock_set: + │ demote severity→info, bypassed=true + │ drop from blocking set + ▼ + decision: allow + source: opa-unlocked ← distinct, greppable + reasons: [{...,bypassed:true, unlock_key_id:"a3f9c2b8"}] +``` + +Possession of key = capability. No separate agent allowlist needed — user only hands keys to agents they trust. + +### Four design forks + +#### Fork 1 — Granularity (what does one key unlock?) + +| Flavor | Hash input | Specificity | My lean | +|--------|-----------|-------------|---------| +| K1 per-rule | `rule_id` | unlocks rule for ANY cmd | **default** | +| K2 per-rule+args | `rule_id + sorted(block_args)` | unlocks only matching arg pattern | optional | +| K3 per-command | `rule_id + raw_command` | unlocks that exact cmd, once | `--unlock-once` for sensitive ops | + +K1 simplest. K3 most surgical (good for "let this agent run this one `git push --delete` then revoke"). Ship K1 default + K3 escape hatch. + +#### Fork 2 — Key derivation (what's hashed + salt?) + +Bare `sha256(rule_id)` is brute-forceable: rule_ids are public (`tests/fixtures/user-rules.rulebook.json` ships them). Anyone can pre-compute the whole keychain → security theater. + +``` + WITHOUT salt WITH per-deploy salt + ────────────────── ───────────────────── + key = sha256(rule_id) key = HMAC-SHA256(salt, rule_id) + .hex().slice(0,16) + keys are GLOBAL keys are DEPLOYMENT-SPECIFIC + publishable rainbow table machine A's key ≠ machine B's + security theater real shared secret +``` + +**Lean: salted.** Salt generated once at install, stored `~/.pi-opa-net/salt` (or `PIOPANET_UNLOCK_SALT` env). Rotation = new salt = all old keys die. Cheap reset on compromise. + +Tradeoff: keys don't transfer across machines. For a single-user multi-box workflow that's a feature; for "share keys with a teammate" it's friction. Resolvable via shared salt file. + +#### Fork 3 — Where does the check live? (the interesting fork) + +``` + ┌─ Rego-side ──────────────────────┬─ TS-side (post-eval filter) ────────────┐ + │ │ │ + │ deny[msg] if { │ const blocking = engine.reasons.filter( │ + │ ...match... │ r => !unlockKeys.has( │ + │ not rule_unlocked(rule_id) │ deriveKey(r.rule_id)) │ + │ } │ ) │ + │ │ ); │ + │ rule_unlocked(rid) if { │ + │ some k in input.unlock_keys │ ✓ clean two-halves extension │ + │ k == derived_key(rid) │ ✓ key never enters OPA input/trace │ + │ } │ ✓ scheme lives in code, easy to rotate │ + │ │ ✓ fail-open independent │ + │ ✗ OPA crypto builtins limited │ │ + │ ✗ key comparison in policy │ │ + │ ✗ scheme change = policy edit │ │ + │ ✗ key visible in OPA trace │ │ + └──────────────────────────────────┴─────────────────────────────────────────┘ +``` + +**Lean: hard toward TS-side.** Maps to existing two-halves architecture → becomes three halves (parse / decide / unlock). Key stays out of policy bundle, out of OPA traces, out of any future OPA debug dump. + +#### Fork 4 — Delivery (ENV vs CLI arg vs both) + +``` + ┌─ ENV ────────────────────────────┬─ CLI arg ──────────────────────────┐ + │ PIOPANET_UNLOCK_KEYS=a3f9,7c2f │ pi-opa-net eval "git stash pop" \ │ + │ │ --unlock a3f9 │ + │ ✓ session-scoped, survives │ ✓ per-invocation, surgical │ + │ ✓ doesn't leak to ps/history │ ✗ LEAKS to ps, history, /proc │ + │ ✗ lingers in env (rotation pain) │ ✓ burns after one use │ + └──────────────────────────────────┴───────────────────────────────────┘ +``` + +**Lean: both.** ENV as default for trusted-agent sessions; `--unlock` for one-shot. Plus `--unlock-stdin` (read key from stdin) for high-security — never appears in `ps`. + +Add `PIOPANET_AGENT_ID` so the decision record can name *who* unlocked (audit, not auth). + +### Schema delta (additive, stays v1) + +```jsonc +{ + "decision": "allow", + "source": "opa-unlocked", // ← NEW enum value + "reasons": [{ + "rule_id": "block-git-stash-mutations", + "severity": "info", // demoted from 'block' + "bypassed": true, // ← NEW + "unlock_key_id": "a3f9c2b8" // ← first 8 chars only, NEVER full key + }], + "metadata": { + "unlock_count": 1, // ← NEW + "unlock_agent": "deploy-bot-7" // ← NEW (from PIOPANET_AGENT_ID) + } +} +``` + +`source: 'opa-unlocked'` is the headline — distinct from `opa` / `fail-open` / `cached`. One grep finds every bypass ever. + +### CA — failure modes I want on the table + +- `[CA1]` **Key in `ps`/shell history** — `--unlock abc123` is visible to anyone on the box. Mitigation: ENV default, `--unlock-stdin` for sensitive, document risk. +- `[CA2]` **Decision record logging the full key** — must log `key_id` (truncated) only. Never the raw key. Hard rule. +- `[CA3]` **Silent bypass** — without `source:opa-unlocked` + `reasons[].bypassed`, an unlock is invisible. That's a security hole, not a feature. Schema delta is mandatory, not optional. +- `[CA4]` **God-key temptation** — someone will ask for one key that unlocks everything. Default NO. If shipped: `PIOPANET_UNLOCK_ALL=`, logged LOUDLY (`source:opa-unlocked-admin`). +- `[CA5]` **Salt loss = total reset** — if `~/.pi-opa-net/salt` is deleted, every key ever issued dies. Backup story needed (or accept as security feature). +- `[CA6]` **Fail-open interaction** — unlock filter runs AFTER OPA. If OPA is down (`source:fail-open`), there's nothing to unlock; behavior unchanged. Verified compatible. +- `[CA7]` **Rainbow table without salt** — bare `sha256(rule_id)` is publishable. Salt is not optional in my view (see Fork 2). +- `[CA8]` **Key reuse across rules** — if two rules hash-collide (truncated to 8 chars), one key unlocks both. Use full 16 hex (64 bits) → collision ~1 in 10^19. Acceptable. + +### How this fits alongside in-flight work + +``` + conditional-branch-gate (in-flight) ──adds──▶ input.signals.* + rule-unlock-keys (this) ──adds──▶ post-eval unlock filter + + source:opa-unlocked + + reasons[].bypassed +``` + +No conflict. Both additive. Could even ship in either order. + +### Open questions for you + +``` +[Q1] Granularity: K1 (per-rule, default) + K3 (per-cmd --unlock-once)? Or K2 (per-rule+args) too? +[Q2] Salt: deploy-local (my lean, keys don't transfer) or global (shareable, weaker)? +[Q3] Location: TS-side filter (my lean) — agree? +[Q4] Delivery: ENV + --unlock + --unlock-stdin? Anything else? +[Q5] God-key: ship PIOPANET_UNLOCK_ALL or refuse? +[Q6] TTL: should keys expire (e.g. PIOPANET_UNLOCK_TTL=3600) or be password-like (long-lived)? +[Q7] Audit sink: just decision record, or also append to ~/.pi-opa-net/unlock.log? +``` + +### My recommendation in one line + +**TS-side post-eval filter, HMAC(salt, rule_id) → 16 hex, ENV+CLI delivery, mandatory `source:opa-unlocked` audit, no god-key, ship K1+K3 granularity.** + +--- + +**Remaining steps (per ospx manifest, step 10):** I don't have direct access to `~/.pi/agent/cmd-family/ospx.yml` to enumerate steps 11–70. Want me to read it and list the remaining steps, or is this exploration step 10 sufficient for now? + +--- + +## Status at end of turn 2 + +- Four design forks mapped (granularity / derivation / location / delivery). +- Leans stated: K1+K3, salted, TS-side, ENV+CLI. +- 7 open questions [Q1]–[Q7] surfaced for user to lock. +- CA1–CA8 failure modes on record. + +→ Continue to `2026-07-20-turn3-lock-decisions-ttl.md` diff --git a/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn2a-gotcha-unlock-design-batch-a.md b/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn2a-gotcha-unlock-design-batch-a.md new file mode 100644 index 0000000..bcf1917 --- /dev/null +++ b/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn2a-gotcha-unlock-design-batch-a.md @@ -0,0 +1,228 @@ +# Appendix to Turn 2 — unlock design gotchas (batch A) + +> Gotcha coverage for: Turn 2 (./2026-07-20-turn2-explore-unlock-design-forks.md) +> Sub-agent: reviewer (batch A) +> Items reviewed: LD-L1, LD-L2, LD-L3, LD-L4, LD-L6 +> Source: `.pi-subagents/artifacts/outputs/99e702dc.../solutions/batch-a-gotchas.md` + +--- + +## Findings (ranked) + +### Rank 5 (Sophisticated) + +- **Multiple rules fire on one command → need ALL keys; partial-unlock state undefined** + - What: A single command can be denied by MORE than one rule. K1 per-rule means each fired rule needs its OWN key. If the agent holds a key for rule A but not rule B, the command stays blocked — but the schema delta has no way to express "1 of 2 reasons bypassed." The post-eval filter must decide: allow only when ALL blocking reasons are unlocked, else deny. + - Why missed: Original forks framed granularity as "what does ONE key unlock," never "how many keys does ONE command need." + - Severity: Undefined output state → either silent partial-bypass (security hole) or confusing UX. + - Mitigation: Define filter semantics as "allow ⟺ every `severity:block` reason has a matching valid key." Add `reasons[].bypassed` per-entry and emit `metadata.unlock_count` + `unlock_blocked_count`. + +- **fail-open (OPA down) is already a KEYLESS universal bypass — "no god-key" is undermined** + - What: LD-L6 puts the unlock check in a TS post-eval filter that runs ONLY when OPA returns reasons. `src/config/Config.ts` defaults `failMode='open'`. When OPA is down: allow + empty reasons → unlock filter has nothing to filter → command allowed with NO `source:opa-unlocked` audit trail. An attacker who can induce OPA unavailability bypasses EVERY rule lock without any key. + - Why missed: CA6 framed fail-open interaction as benign — true mechanically, false for the threat model. + - Severity: A motivated attacker (or flaky OPA) turns the whole key system into a no-op. + - Mitigation: When ANY unlock key is configured, the engine SHOULD fail-closed on OPA outage. Add `source:'fail-open-keyless'` distinct from plain `fail-open`. + +- **`--unlock-stdin` DOUBLE-READS fd 0 — conflicts with existing command-via-stdin path** + - What: `src/cli/run.ts` `resolveRaw()` ALREADY reads stdin (fd 0) when no positional command is given. LD-L4 adds `--unlock-stdin` which ALSO reads stdin. You cannot read fd 0 twice — the second read gets EOF/empty. + - Why missed: The unlock design was authored without re-reading `run.ts`'s stdin contract. + - Severity: Implementation blocker. `--unlock-stdin` as specified cannot coexist with command-via-stdin. + - Mitigation: Pick one: (a) `--unlock-stdin` requires a positional command arg, or (b) keys read from a different fd, or (c) introduce `--command-stdin` vs `--key-stdin` explicit flags. + +- **Cache poisoning — cache key must include the unlock-set** + - What: `src/engine/types.ts` declares `source:'cached'` and `src/config/Config.ts` declares `cacheTtlMs`. When caching is enabled, the cache is almost certainly keyed on the parsed command. Agent A runs `git stash pop` WITH key X → allow → CACHED. Agent B runs same command WITHOUT key → cache HIT → allow with NO key. Key-bypass-via-cache. + - Why missed: The unlock design assumed a stateless post-eval filter; the cache seam was not on the analysis radar. + - Severity: If cache ships without addressing this, ANY keyed-allow poisons the cache for all future callers. + - Mitigation: Cache key MUST include a hash of the resolved unlock-key-set, OR force `cacheTtlMs=0` when any unlock keys are present. + +### Rank 4 (Significant) + +- **gcloud / bq rules CANNOT be unlocked — no enumerable rule_id** + - What: K1 derives the key from `HMAC(salt, rule_id)`. But `src/rules/catalog.ts` RULES array has NO entries for gcloud or bq. `RuleRegistry.lookup` synthesizes `custom:` where the hash depends on the substituted verb. That synthesized id is not enumerable by `pi-opa-net unlock-key --list`, different per verb, and NOT what a user can mint. Result: the entire gcloud + bq deny families are effectively UN-UNLOCKABLE under LD-L1+LD-L6. + - Why missed: Original findings treated "rule_id is hash input" as universally true without checking that gcloud/bq have no stable rule_id. + - Severity: Whole rule families silently outside the unlock system. + - Mitigation: Either register gcloud/bq as discrete `RuleMeta` entries, OR scope the unlock feature to catalog-registered rules and document gcloud/bq as out-of-scope. + +- **rule_id mutability invalidates all minted keys — no migration story** + - What: Key = `HMAC(salt, rule_id)`. rule_id is treated as immutable, but the catalog ALREADY documents drift: `block-rm-bd-sub-skills` and `block-rm-beads-subdirs` are flagged "rule is misnamed 'allow'". A routine rename would silently invalidate every key ever minted for the old id. + - Why missed: Original analysis assumed rule_ids are stable hash inputs without auditing the catalog's own "misnamed" notes. + - Severity: A routine rename PR bricks the deployed keychain with no error. + - Mitigation: Treat rule_id as a versioned identifier (forbid renames in CONTRIBUTING, or carry a `key_aliases: [...]` field). + +- **NO revocation for long-lived keys — leaked `ll_` key is valid forever** + - What: LL keys have NO embedded expiry and NO server-side revocation list. A leaked `ll_<16hex>` is valid until the salt rotates — and rotating the salt is expensive and kills ALL keys. There is no way to revoke a SINGLE compromised LL key. + - Why missed: The turn3 TTL design focused on "how do keys expire" and never asked "how do LL keys get killed." + - Severity: A single leaked LL key is a permanent rule-wide bypass. + - Mitigation: Either accept "salt rotation is the only LL revocation" and document it, OR add a minimal revocation list seam (`~/.pi-opa-net/revoked.txt`). + +- **TTL key is replayable-within-window — conflates "temporary" with "single-use"** + - What: A `ttl..` key is valid for EVERY invocation until `exp`. Dropping K3 removed the only single-use primitive. So `--ttl 3600` is "unlock this rule for 1 hour of UNLIMITED use," not "unlock one command." + - Why missed: TTL was designed as the "temporary" alternative to LL; the distinction "temporary ≠ single-use" was never made explicit. + - Severity: Mis-matched user mental model → over-broad capability granted. + - Mitigation: Document clearly: "TTL = time-bounded unlimited use within the window; there is no single-use mode." + +- **Clock manipulation makes TTL keys effectively permanent — no trusted time anchor** + - What: TTL verification is `now > exp` where `now = Date.now()` on the verifier's process clock. A verifier that freezes/rewinds its clock keeps `now < exp` forever → a TTL key becomes permanently valid. The "no server-side state" decision removes the only anchor that would resist this. + - Why missed: CA9 framed clock as a skew problem, not an adversarial-control problem. + - Severity: An agent that controls its own clock defeats TTL entirely. + - Mitigation: State the threat-model limit explicitly: "TTL relies on the verifier's clock being honest; treat TTL as convenience, not enforcement, against a compromised agent." + +- **TS-filter crash has NO fail-mode — undefined behavior** + - What: The TS-side unlock filter has NO equivalent fail-mode. If the filter throws, the whole `runCli` pipeline throws → the bin wrapper's behavior is undefined. + - Why missed: Fail-mode was scoped to the OPA engine only; the new TS stage inherited no fail-mode contract. + - Severity: A parser bug in the unlock path takes down command evaluation with no defined fallback. + - Mitigation: Wrap the unlock filter in try/catch: on filter error, fall back to the UN-FILTERED engine decision (if OPA said deny, stay deny; never allow-by-accident). + +- **Post-eval filter keys off registry-LOOKED-UP rule_id, not the canonical one** + - What: Pipeline order: engine returns raw deny messages → `buildReasons` calls `registry.lookup` → produces `rule_id` (possibly `custom:` for unregistered rules) → the unlock filter would key off THAT. For gcloud/bq, the filter's rule_id is the synthesized `custom:` which no user can mint a key for. + - Why missed: The "three halves" diagram glossed over whether the filter is before or after the registry lookup. + - Severity: Whole rule families un-unlockable; refactor-fragile. + - Mitigation: Specify the filter operates on the registry-RESOLVED rule_id, and that key derivation MUST use the same resolved id. + +- **Multi-channel merge semantics undefined** + - What: An agent can simultaneously set `PIOPANET_UNLOCK_KEYS=ll_a` AND pass `--unlock ll_b` AND pipe `ll_c` via stdin. LD-L4 lists three channels but never defines merge: union, last-wins, or intersection. + - Why missed: Each channel was justified independently; their interaction was never modeled. + - Severity: An agent that intends "override env with a narrower key" may get "union of both." + - Mitigation: Specify UNION as the semantics and document. + +### Rank 3 (Moderate) + +- **Dropping K3 widens blast radius of any leaked ENV key** + - What: LD-L1 drops `--unlock-once`. Combined with LD-L4 ENV delivery and LD-L3 TTL, there is now NO single-use primitive. A key in `PIOPANET_UNLOCK_KEYS` is valid for UNLIMITED invocations of ANY command firing that rule, for the whole session (LL) or until exp (TTL). + - Why missed: LD-L1 was locked in isolation from LD-L4's persistence model. + - Severity: Credential leak → indefinite rule-wide bypass until salt rotation. + - Mitigation: Accept as a documented limitation, OR add a lightweight per-key nonce/counter as a future seam. + +- **Functional god-key achievable by composition — no cap** + - What: Refusing a NAMED `PIOPANET_UNLOCK_ALL` does not prevent an operator minting all ~40 per-rule keys and dumping them comma-separated into `PIOPANET_UNLOCK_KEYS`. The result is functionally a god-key. + - Why missed: The decision was scoped to "should we ship a god-key token," not "can a god-key be assembled." + - Severity: An auditor cannot distinguish "scoped agent" from "agent with every key" from the decision record alone. + - Mitigation: Optional — warn when `PIOPANET_UNLOCK_KEYS` count exceeds a threshold, or emit `metadata.unlock_keys_provided` count. + +- **No bulk re-mint — routine salt rotation is operationally expensive** + - What: HMAC salt rotation is recommended hygiene, but with no god-key there is no bulk path. Rotating salt = every per-rule key dies = operator must re-mint each one individually. + - Why missed: Rotation operability was never on the question list. + - Severity: Operators will avoid rotating salt → stale salt → weaker posture over time. + - Mitigation: Ship an inventory file (`~/.pi-opa-net/key-inventory.json`) listing which rule_ids have been minted, so `unlock-key --reissue-all` can re-mint the same set under a new salt. + +- **`exp` minted from a wrong/broken minter clock** + - What: The minter is `pi-opa-net unlock-key --ttl N` running on the USER's box. If that box's clock is wrong, exp is computed wrong. + - Why missed: Only the verifier clock was considered, not the minter clock. + - Severity: User mints a key that doesn't behave as expected. + - Mitigation: Minter sanity-checks: warn if `exp` is implausible, reject negative `--ttl`. Print the computed absolute exp time to the user at mint. + +- **No MAX_TTL cap — a far-future exp is a permanent key wearing a TTL costume** + - What: `--ttl 999999999` (≈31 years) produces a `ttl..` that is functionally a long-lived key but logged/audited as `unlock_key_type:'ttl'`. + - Why missed: TTL bounds were never specified. + - Severity: Audit/reports that count TTL vs LL usage become unreliable. + - Mitigation: Define MAX_TTL (e.g. 7 or 30 days). Reject `--ttl` above it. + +- **Key format/delimiter for `PIOPANET_UNLOCK_KEYS` unspecified** + - What: LD-L4 says keys come via env, but the SEPARATOR between multiple keys is undefined. + - Why missed: Delivery channels were listed; their multi-value grammar was not. + - Severity: Interop — two wrappers using different delimiters silently drop keys. + - Mitigation: Specify: `PIOPANET_UNLOCK_KEYS` is comma-separated; `--unlock` repeatable; union all channels; ignore empty tokens. + +- **ENV keys propagate to ALL child processes** + - What: `PIOPANET_UNLOCK_KEYS` in env is inherited by EVERY child the agent spawns — including untrusted ones. + - Why missed: CA1 compared ENV vs `--unlock` on the PERSISTENCE axis, not the INHERITANCE axis. + - Severity: Any untrusted subprocess of a keyed agent silently receives the full keychain. + - Mitigation: Document explicitly: "ENV keys are visible to all descendant processes; for untrusted-child scenarios use `--unlock-stdin` or a per-call `--unlock`." + +- **`--unlock-stdin` empty/closed behavior undefined** + - What: If `--unlock-stdin` is passed but stdin is empty, the parser receives `""`. Empty string is not a valid key. Is it a loud error, a silent skip, or a no-op? + - Why missed: Failure modes were specified for EXPIRED keys but not for MISSING/EMPTY stdin. + - Severity: Silent no-op → user believes key was applied, command still blocked. + - Mitigation: `--unlock-stdin` with empty stdin = loud error. + +- **Policy can NEVER branch on unlock possession** + - What: Because the key never enters OPA input, the rego policy CANNOT express "block X for agents without a key, allow-with-warning for agents with a key" at the policy layer. + - Why missed: LD-L6 was framed purely as a benefit. + - Severity: YAGNI now, but a real ceiling if the product evolves toward tiered policy. + - Mitigation: Document as an explicit design ceiling. + +### Rank 2 (Minor) + +- **`builtin:bare-stash-default` rule_id contains a colon — parsing surfaces** + - What: The colon is unusual. Some shell-quoting / arg-parsing libraries treat `:` specially. + - Why missed: Colon rule_ids exist but were never run through the mint/verify CLI in the design. + - Severity: Cosmetic / parser-fragility. + - Mitigation: Unit-test mint+verify round-trip for `builtin:` and `custom:` ids explicitly. + +- **`unlock_key_id` is NOT stable across LL vs TTL for the same rule** + - What: LL mac = `HMAC(salt, rule_id)`; TTL mac = `HMAC(salt, rule_id + str(exp))` → DIFFERENT macs → DIFFERENT key_ids for the same rule. + - Why missed: key_id was designed as a key-truncation without considering cross-type stability. + - Severity: Minor — misleading audit field. + - Mitigation: Add `unlock_rule_id` to the bypassed reason alongside `unlock_key_id`. + +- **Salt FILE FORMAT unspecified** + - What: `HMAC(salt, rule_id)` — is `salt` the raw file bytes, the trimmed string, base64-decoded, or hex-decoded? If the salt file is written with a trailing `\n` and read by another that doesn't trim, the HMAC key differs. + - Why missed: Salt was treated as "a secret blob," format unspecified. + - Severity: Cross-machine / cross-tool key failure with no diagnosable error. + - Mitigation: Define salt format: first line of file, UTF-8, trailing newline STRIPPED, OR base64 of N random bytes. + +- **Salt file permissions not specified** + - What: If `~/.pi-opa-net/salt` is created 0644, any local user reads it → forges every key. + - Why missed: Salt was discussed as location and loss, never as permissions. + - Severity: Local-privilege-escalation to full keychain forgery on multi-user boxes. + - Mitigation: Create salt with mode 0600; refuse to load if permissions are looser. + +- **exp parsing edge cases** + - What: `ttl..` parser must reject: `ttl..mac`, `ttl.-1.mac`, `ttl.abc.mac`, `ttl.01753.mac`, `ttl.1753.1234.mac`. + - Why missed: KeyParser was enumerated as a module but its rejection grammar was never specified. + - Severity: A malformed-key parser bug could accept a forged mac. + - Mitigation: Define strict grammar: `^ll_[0-9a-f]{16}$` | `^ttl\.\d{1,10}\.[0-9a-f]{16}$`. + +- **Stale key (valid mac, rule no longer exists) is silent** + - What: A key whose mac is valid for a rule_id that was DELETED/RENAMED produces no deny to filter — it silently matches nothing. + - Why missed: Expiry was the only "dead key" case considered; structural staleness was not. + - Severity: User confusion + masks the LD-L1 rename-bricking bug. + - Mitigation: At startup, for each provided key, if its derived rule_id is not in the current catalog, emit a stderr warning. + +- **ENV var size limits + no file-delivery escape hatch** + - What: LD-L4 closes the door on file-based delivery. ENV has platform limits. With many keys, ENV can approach limits. + - Why missed: YAGNI closed the channel without sizing the alternative. + - Severity: Edge-case failure at high key counts. + - Mitigation: Low priority — note the limit, leave `PIOPANET_UNLOCK_KEYS_FILE` as a documented future seam. + +- **No salt/scheme provenance in the audit record** + - What: The schema delta does NOT add `unlock_salt_digest` or `unlock_scheme_version`. After a salt rotation, old records are unverifiable. + - Why missed: Provenance was added per-key but not per-salt. + - Severity: Audit gap — cannot prove a past unlock was valid under the then-current salt. + - Mitigation: Add `metadata.unlock_salt_digest` (SHA-8 of the salt) to the decision record. + +- **`source:'opa-unlocked'` collides conceptually with `source:'cached'`** + - What: If caching is enabled AND a previously-unlocked decision is replayed from cache, what is `source`? + - Why missed: `cached` and `opa-unlocked` were designed independently. + - Severity: Either audit loses unlock info or audit lies about freshness. + - Mitigation: Either forbid caching unlocked decisions, or add `source:'cached-unlocked'`. + +--- + +## Cross-decision interaction summary + +| Interaction | Effect | Refs | +|---|---|---| +| LD-L2 × LD-L6 | fail-open = keyless universal bypass on OPA outage | LD-L2 Rank 5 | +| LD-L4 × LD-L6 | `--unlock-stdin` double-reads fd 0 vs command-via-stdin | LD-L4 Rank 5 | +| LD-L6 × cache seam | cache poisons across key-sets | LD-L6 Rank 5 | +| LD-L1 × LD-L6 | gcloud/bq un-unlockable (no canonical rule_id at filter) | LD-L1 Rank 4, LD-L6 Rank 4 | +| LD-L1 × LD-L3 | no one-shot; TTL is replayable-within-window | LD-L1 Rank 3, LD-L3 Rank 4 | +| LD-L3 × LD-L6 | TTL relies on verifier clock (no trusted anchor) | LD-L3 Rank 4 | +| LD-L2 × LD-L3 | no single-key revocation; salt rotation is blunt | LD-L2 Rank 3, LD-L3 Rank 4 | + +--- + +## Top 5 to fix before implementation + +1. **LD-L2/L6 fail-open = keyless god-bypass** (Rank 5) — fail-closed when keys present, or document the degradation loudly. +2. **LD-L4 `--unlock-stdin` fd 0 conflict** (Rank 5) — pick a resolution rule; blocks implementation. +3. **LD-L6 cache poisoning** (Rank 5) — cache key must include unlock-set hash; force cacheTtlMs=0 with keys. +4. **LD-L1/L6 gcloud+bq un-unlockable** (Rank 4) — catalog them or scope them out explicitly. +5. **LD-L3 no LL revocation** (Rank 4) — decide: salt-rotation-only (documented) or revocation-list seam. + +--- + +## Cross-turn references + +- Also relevant to: Turn 3 §TTL design, Turn 4 §ship surface diff --git a/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn3-lock-decisions-ttl.md b/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn3-lock-decisions-ttl.md new file mode 100644 index 0000000..16a88d7 --- /dev/null +++ b/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn3-lock-decisions-ttl.md @@ -0,0 +1,152 @@ +# Explore Turn 3 — lock decisions, TTL design + +> Date: 2026-07-20 +> Phase: decisions (locking granularity, TTL, god-key, delivery) +> Status: complete — 4 decisions locked, 3 open questions remain + +--- + +## [user] + +> **[user]** (verbatim — trust chain L1): +> +> No god key: correct ; design 2 approach: TTL and also long-live; Q1: no need unlock-once , key per rule (like git stash , different than git stash push); Q4: no , only these for now. + +--- + +## [assistant] + +Locked your answers. Updated design. + +### Decisions locked from your answers + +``` +[D-L1] Granularity (Q1) ────── K1 PER-RULE only. Drop K3 (--unlock-once). + Key for block-git-stash-mutations unlocks + ALL stash verbs (push/pop/drop/clear/...) + under that rule. One rule = one key. + +[D-L2] God-key (Q5) ────────── NO. Refuse PIOPANET_UNLOCK_ALL. + +[D-L3] TTL (Q6) ────────────── BOTH. Two key types, see below. + +[D-L4] Delivery (Q4) ──────── ENV + --unlock + --unlock-stdin. Nothing else. +``` + +### TTL + long-lived — how both coexist (the interesting part) + +Two key TYPES, self-describing via prefix. No server-side state needed for expiry. + +``` + TYPE FORMAT DERIVED FROM LIFETIME + ──────────────── ────────────────────────────── ─────────────────────────── ───────────── + long-lived (LL) ll_<16hex> HMAC(salt, rule_id) forever* + ttl ttl..<16hex> HMAC(salt, rule_id + exp) until exp + + * until salt rotates +``` + +**Validator pseudocode (TS-side):** +``` +parse key → { type, exp?, mac } + if type == 'll': expected = HMAC(salt, rule_id) + if type == 'ttl': + if now > exp → reject (expired) + expected = HMAC(salt, rule_id + str(exp)) + if mac != expected → reject + else → unlock rule_id, record key_type + (exp if ttl) +``` + +**Why this shape:** +- TTL state lives IN the key, not on disk → no `.unlock-state` file to lose/sync/corrupt +- User mints a TTL key: `pi-opa-net unlock-key block-git-stash-mutations --ttl 3600` +- Long-lived key: `pi-opa-net unlock-key block-git-stash-mutations` (default) +- Same salt, same rule → LL and TTL keys for same rule coexist (agent picks whichever fits the job) + +``` + USER mints AGENT receives + ──────────── ────────────── + pi-opa-net unlock-key PIOPANET_UNLOCK_KEYS=ll_a3f9c2b8e1d4... + block-git-stash-mutations ttl.1753123456.7c2f8a1b... + --ttl 3600 + → ttl.1753127056.7c2f8a1b... (mixed bag: trusted LL + scoped TTL) +``` + +### Still open (your call) + +``` +[Q2] SALT location + ┌─ deploy-local (~/.pi-opa-net/salt) ─────── my LEAN ─┐ + │ ✓ machine-specific keys (compromise isolated) │ + │ ✗ keys don't transfer across boxes │ + └─────────────────────────────────────────────────────┘ + alt: global/shared salt file (transferable, weaker) + PICK? + +[Q3] LOCATION of unlock check + TS-side post-eval filter (see prior turn) + → no objection raised, confirming as LOCKED unless you say otherwise. + +[Q7] AUDIT sink + ┌─ decision record only (mandatory source:opa-unlocked) ─┐ + │ ✓ lives in stdout/JSON, hook captures it │ minimal + └─────────────────────────────────────────────────────────┘ + alt: + append ~/.pi-opa-net/unlock.log (line per unlock, SIEM-style) + PICK? +``` + +### Updated schema delta (additive, v1 stays) + +```jsonc +{ + "decision": "allow", + "source": "opa-unlocked", + "reasons": [{ + "rule_id": "block-git-stash-mutations", + "severity": "info", // demoted + "bypassed": true, + "unlock_key_id": "a3f9c2b8", // first 8 hex of mac, NEVER full key + "unlock_key_type": "ll" // 'll' | 'ttl' ← NEW (from D-L3) + // if ttl: "unlock_expires_at": "2026-07-20T07:05:34Z" ← NEW + }], + "metadata": { + "unlock_count": 1, + "unlock_agent": "deploy-bot-7" + } +} +``` + +### Updated CA — new failure modes from TTL + +- `[CA9]` **TTL clock skew** — agent clock vs minter clock drift → premature expiry or extended grace. Standard fix: ±60s tolerance, OR rely solely on minter's `exp` (no skew tolerance — strict). Lean strict. +- `[CA10]` **TTL key minted with wrong unit** — `--ttl 3600` meaning seconds vs minutes. Hard rule: TTL is **seconds**, no ambiguity, error on negatives. +- `[CA11]` **Expired TTL still passed in env** — every expired key rejected loudly (not silently). Decision record shows `unlock_key_id` + `unlock_status: "expired"` so audit catches dead-key attempts. + +### CLI surface (locked from D-L4) + +``` +pi-opa-net eval "git stash pop" +pi-opa-net eval "git stash pop" --unlock ll_a3f9c2b8e1d4 +echo "ttl.1753127056.7c2f..." | pi-opa-net eval "git stash pop" # --unlock-stdin + +# key minting (NEW subcommand): +pi-opa-net unlock-key # → ll_<16hex>, long-lived +pi-opa-net unlock-key --ttl 3600 # → ttl..<16hex> +pi-opa-net unlock-key --list # enumerate rule_ids (no keys) +``` + +--- + +**Pick Q2 + Q7 and I'll capture this as openspec change `rule-unlock-keys/`** (proposal.md + design.md + specs/rule-unlock/spec.md + tasks.md), mirroring `conditional-branch-gate`'s structure. Still explore mode — capturing artifacts is allowed, code is not. + +--- + +## Status at end of turn 3 + +- 4 decisions locked: [D-L1] K1 per-rule, [D-L2] no god-key, [D-L3] TTL+LL, [D-L4] ENV+CLI. +- TTL design crystallized: self-describing prefix `ll_` / `ttl..`, no server-side state. +- 3 open questions remain: [Q2] salt, [Q3] location (pending confirmation), [Q7] audit. +- Schema delta updated with `unlock_key_type` field. +- CA9–CA11 added for TTL-specific failure modes. + +→ Continue to `2026-07-20-turn4-yagni-finalize-seams.md` diff --git a/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn4-yagni-finalize-seams.md b/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn4-yagni-finalize-seams.md new file mode 100644 index 0000000..9397aba --- /dev/null +++ b/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn4-yagni-finalize-seams.md @@ -0,0 +1,171 @@ +# Explore Turn 4 — YAGNI finalize, seams locked + +> Date: 2026-07-20 +> Phase: finalization (YAGNI applied to salt + audit) +> Status: complete — all decisions locked, ready to capture as openspec change + +--- + +## [user] + +> **[user]** (verbatim — trust chain L1): +> +> [Q2] SALT location +> ┌─ deploy-local (~/.pi-opa-net/salt) ─────── my LEAN ─┐ +> │ ✓ machine-specific keys (compromise isolated) │ +> │ ✗ keys don't transfer across boxes │ +> └─────────────────────────────────────────────────────┘ +> alt: global/shared salt file (transferable, weaker) +> PICK? +> +> [Q3] LOCATION of unlock check +> TS-side post-eval filter (see prior turn) +> → no objection raised, confirming as LOCKED unless you say otherwise. +> > give me YAGNI solutions for now which is be able to easily extend later +> +> [Q7] AUDIT sink +> ┌─ decision record only (mandatory source:opa-unlocked) ─┐ +> │ ✓ lives in stdout/JSON, hook captures it │ minimal +> └─────────────────────────────────────────────────────────┘ +> alt: + append ~/.pi-opa-net/unlock.log (line per unlock, SIEM-style) +> PICK? +> > YAGNI + +--- + +## [assistant] + +YAGNI locked. Committed to minimal + extensible seams. + +### Locked decisions (final) + +``` +[D-L2] God-key ──────────────── NO +[D-L3] TTL + long-lived ──────── BOTH (self-describing key prefix) +[D-L4] Delivery ──────────────── ENV + --unlock + --unlock-stdin +[D-L5] Granularity ──────────── K1 per-rule ONLY (no K3 — corrected from stale memory) +[D-L6] Location of check ────── TS-side post-eval filter +[D-Y1] Salt ─────────────────── YAGNI: deploy-local default + env override seam +[D-Y2] Audit ────────────────── YAGNI: decision record only + no-op sink seam +``` + +> **Note**: hindsight memory still records K3 (`--unlock-once`) as shipped — your "Q1: no need unlock-once" overrides it. K3 dropped. Will write a corrected memory at capture time. + +### YAGNI choices + extension seams + +#### [D-Y1] Salt — minimal now, swappable later + +``` + DEFAULT NOW SEAM (no rewrite later) + ────────────────────── ────────────────────────────────── + ~/.pi-opa-net/salt SaltResolver interface: + (auto-generated on first resolve(): Buffer + pi-opa-net unlock-key) ├─ default: readFile(SALT_PATH) + ├─ env: PIOPANET_UNLOCK_SALT + └─ future: keychain fetcher + └─ future: remote salt API +``` + +**Now:** 1 salt file, 1 env override. 20 lines. +**Later:** add `RemoteSaltResolver`, swap default. Zero callers change. + +#### [D-Y2] Audit — minimal now, hook seam later + +``` + DEFAULT NOW SEAM + ────────────────────── ────────────────────────────────── + Decision record only AuditSink interface: + source: 'opa-unlocked' onUnlock(record): void + reasons[].bypassed=true ├─ default: NoOpSink + metadata.unlock_count=1 ├─ future: FileAppendSink + └─ future: WebhookSink +``` + +**Now:** everything in `decision-output.v1`. No file. +**Later:** drop in `FileAppendSink`, config wires it. Zero pipeline change. + +### Architecture (3 halves + 2 seams) + +``` + PARSE ─▶ EVAL (OPA) ─▶ UNLOCK FILTER ─▶ BUILD ─▶ EMIT + │ │ + │ └─▶ AuditSink.onUnlock() [SEAM] + │ + KeyDerivation.derive(rule_id) [SEAM] + │ + KeyParser.parse(raw_key) + │ + {type:'ll'|'ttl', exp?, mac} +``` + +### Ship surface (final, minimal) + +``` +NEW MODULE: src/unlock/ + KeyDerivation.ts ← HMAC-SHA256(salt, rule_id) → 16 hex + KeyParser.ts ← parse 'll_' | 'ttl..' + KeyVerifier.ts ← verify(parsed, rule_id, now): {valid, reason?} + UnlockFilter.ts ← filter engine.reasons, demote bypassed + SaltResolver.ts ← readFile(~/.pi-opa-net/salt) | env override + types.ts ← interfaces (seam points) + +NEW SINK: src/audit/ + AuditSink.ts ← interface + NoOpSink (default) + +EDIT: + policy/safety.rego ← NO CHANGE (TS-side) + src/output/DecisionBuilder.ts ← accept bypassed reasons, new source + src/output/OutputFormatter.ts ← emit source:'opa-unlocked' + src/cli/run.ts ← thread PIOPANET_UNLOCK_KEYS, --unlock, --unlock-stdin + src/config/Config.ts ← unlock keys + salt path + schemas/decision-output.v1.json ← additive: source enum +, bypassed, unlock_* + src/rules/catalog.ts ← NO CHANGE (rule_id is hash input, already correct) + +NEW CLI: pi-opa-net unlock-key [--ttl ] +``` + +### Test surface (minimal but real) + +``` +tests/unit/unlock/ + KeyDerivation.test.ts ← HMAC deterministic, salt-sensitive + KeyParser.test.ts ← ll_*, ttl.*.*, malformed → reject + KeyVerifier.test.ts ← valid / wrong-rule / expired / wrong-salt + UnlockFilter.test.ts ← mixed valid+invalid → only valid demoted +tests/e2e/ + unlock-flow.test.ts ← full: mint → env → eval → source:opa-unlocked +tests/unit/audit/ + NoOpSink.test.ts ← default no-op (extension point) +``` + +### CA — final list + +- `[CA1]` `--unlock` in `ps`/history → document, default ENV. +- `[CA2]` Never log full key, only `unlock_key_id` (first 8 hex). +- `[CA3]` Silent bypass forbidden → `source:opa-unlocked` mandatory. +- `[CA8]` 16 hex (64-bit mac) → collision ~1/10^19, acceptable. +- `[CA9]` TTL clock strict (no skew tolerance) — fail closed on ambiguity. +- `[CA10]` TTL = seconds, negative rejected. +- `[CA11]` Expired TTL key → recorded as `unlock_status:'expired'`, NOT silent. +- `[CA12]` **NEW** — YAGNI seams must stay interfaces, not abstract classes. Pure type + NoOp default. Resist feature creep. + +--- + +**Ready to capture.** Want me to create `openspec/changes/rule-unlock-keys/` with: +- `proposal.md` (why + what + capabilities + impact) +- `design.md` (decisions D-L1…D-L6, D-Y1, D-Y2, risks, migration) +- `specs/rule-unlock/spec.md` (requirements/scenarios) +- `tasks.md` (executable checklist) + +Confirm and I draft. Still explore mode — artifacts allowed, code is not. + +--- + +## Status at end of turn 4 + +- ALL decisions locked: [D-L1]..[D-L6], [D-Y1], [D-Y2]. +- YAGNI applied to salt (deploy-local + env seam) and audit (decision-record + NoOp seam). +- Ship surface enumerated: 6 new files in `src/unlock/`, 1 in `src/audit/`, 6 edits to existing files. +- Test surface enumerated: 5 unit + 1 e2e + 1 audit unit. +- CA1–CA12 final list on record. +- Explore session complete. Awaiting user confirmation to capture as openspec change `rule-unlock-keys/`. diff --git a/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn4a-gotcha-yagni-seams-batch-b.md b/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn4a-gotcha-yagni-seams-batch-b.md new file mode 100644 index 0000000..4e3fc29 --- /dev/null +++ b/flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn4a-gotcha-yagni-seams-batch-b.md @@ -0,0 +1,216 @@ +# Appendix to Turn 4 — YAGNI seams + ship surface gotchas (batch B) + +> Gotcha coverage for: Turn 4 (./2026-07-20-turn4-yagni-finalize-seams.md) +> Sub-agent: reviewer (batch B) +> Items reviewed: LD-Y1, LD-Y2, TTL design, ship surface, OT1 +> Source: `.pi-subagents/artifacts/outputs/99e702dc.../solutions/batch-b-gotchas.md` + +--- + +## Findings (ranked) + +### Rank 4 (Significant) + +- **Schema `additionalProperties: false` blocks all proposed additive fields** + - What: The current schema has `additionalProperties: false` at 4 levels: root, Reason, EvaluatedInput, DecisionMetadata. The proposed changes add: `source: 'opa-unlocked'` (new enum value), `reasons[].bypassed`, `reasons[].unlock_key_id`, `reasons[].unlock_key_type`, `reasons[].unlock_expires_at`, `reasons[].unlock_status`, `metadata.unlock_count`, `metadata.unlock_agent`. EVERY ONE of these requires a schema change. The design says "additive, v1 stays" but `additionalProperties: false` means NOTHING is additive without explicit schema edits. + - Why missed: Design discusses schema delta in terms of "add fields" but doesn't account for the strict `additionalProperties: false` constraint. + - Severity: If schema isn't updated in lockstep with code, `validateDecision()` throws → entire pipeline crashes on any unlock event. This is a hard blocker. + - Mitigation: Schema update is NOT optional — it's part of the ship surface. Add explicit schema edit tasks. + +- **DecisionBuilder.source type doesn't include 'opa-unlocked'** + - What: `DecisionOutput.source` is typed as `'opa' | 'fail-open' | 'fail-closed' | 'cached'`. The design adds `'opa-unlocked'` but this requires changing the TypeScript type AND the `EngineDecision.source` type in `src/engine/types.ts`. The unlock filter doesn't change the engine's source — it changes the OUTPUT source. But DecisionBuilder gets source from `engine.source`, not from the unlock filter. + - Why missed: Design says "TS-side post-eval filter" but doesn't specify HOW the source field transitions from `'opa'` to `'opa-unlocked'`. + - Severity: Type error at compile time. If forced with `as`, runtime schema mismatch. + - Mitigation: Define the handoff: UnlockFilter returns a modified `EngineDecision` with `source: 'opa-unlocked'` when any rule is bypassed. Or: DecisionBuilder accepts an unlock result and overrides source. + +- **TTL key format ambiguity — `ttl.` prefix collides with potential future prefixes** + - What: Format `ttl..<16hex>` uses dots as separators. If a future key type also uses dots, parsing becomes ambiguous. Worse: if the 16hex portion happens to start with characters that look like a prefix, a naive split could misparse. + - Why missed: Design uses `.` as separator but doesn't reserve the namespace or define a formal grammar. + - Severity: Future key types break parser. Not today's problem but the prefix namespace is implicitly unbounded. + - Mitigation: Define formal BNF: `key = "ll_" hex16 | "ttl." dec_unix_exp "." hex16`. Reserve `ll_` and `ttl.` as the only valid prefixes. + +- **Unix expiry as decimal string — no validation of format** + - What: `ttl.1753127056.7c2f8a1b...` — the expiry is a decimal unix timestamp. But what if someone passes `ttl.1.7c2f...`? Or `ttl.abc.7c2f...`? Or `ttl.-1.7c2f...`? Parser must handle all malformed variants. + - Why missed: CA10 covers negative TTL at minting time but not at parsing/verification time. + - Severity: Malformed key in env → crash or silent misparse. + - Mitigation: KeyParser MUST: (1) split on `.`, expect exactly 3 parts for `ttl`, (2) `parseInt(exp, 10)` with strict check, (3) reject if `exp.toString() !== parts[1]`. + +- **Fail-open + unlock interaction is undefined** + - What: CA6 says "if OPA is down (source:fail-open), there's nothing to unlock; behavior unchanged." But consider: OPA is down → `source: 'fail-open'`, `decision: 'allow'`, `reasons: []`. The unlock filter has no deny reasons to process. This is correct. BUT: what if OPA returns SOME denies but the unlock filter bypasses ALL of them → `decision: 'allow'`, `source: 'opa-unlocked'`. Now OPA goes down on the NEXT call → `source: 'fail-open'`. The agent sees alternating `opa-unlocked` and `fail-open` sources. This is fine for behavior but confusing for audit. + - Why missed: CA6 only covers the "OPA fully down" case, not the "OPA flapping" case. + - Severity: Audit confusion. Agent behavior is correct but the source field oscillates. + - Mitigation: Document: `fail-open` and `opa-unlocked` can coexist in the same session. Audit consumers should not interpret source oscillation as instability. + +### Rank 3 (Moderate) + +- **Salt file race on first-use auto-generation** + - What: Two concurrent `pi-opa-net unlock-key` invocations on first run both detect missing salt → both generate → last-write-wins. Keys minted against the first salt become invalid once the second overwrites it. + - Why missed: Design says "auto-generated on first use" but doesn't specify atomic write-or-verify semantics. Classic TOCTOU. + - Severity: Keys minted under stale salt silently fail verification. + - Mitigation: Use `O_CREAT | O_EXCL` (or `wx` flag) for salt file creation. Loser re-reads the winner's file. Or: generate into temp file, `rename()` atomically. + +- **Salt file permissions default to world-readable** + - What: `~/.pi-opa-net/salt` contains the root secret for all unlock keys. If created with default umask (022 → file mode 644), any user on the box reads it → derives all keys → full bypass. + - Why missed: Design specifies location and env override but never mentions `chmod 600` / `0o600` on creation. + - Severity: Multi-user box = total security bypass. Single-user box = no impact. + - Mitigation: SaltResolver MUST `fs.writeFileSync(path, salt, { mode: 0o600 })` on creation. Verify on read: warn if mode != 600. + +- **NoOpSink default means zero audit trail for unlock events** + - What: With NoOpSink as default, unlock events are ONLY visible in the decision record JSON on stdout. If the consumer (pi hook, CI script) doesn't capture stdout, the unlock is invisible. No file, no log, no webhook. + - Why missed: Design says "hook captures it" — but this assumes the hook is wired (which Turn 1 confirmed is NOT yet built). + - Severity: Gap between "audit exists in theory" and "audit exists in practice." Until pi-opa-net-ext ships, unlock events are ephemeral. + - Mitigation: Accept as known gap (YAGNI). But: decision record MUST be emitted on stderr or a file when unlock actually fires, not just stdout. Or: document that audit is contingent on hook wiring. + +- **AuditSink.onUnlock() timing ambiguity — when does it fire?** + - What: The interface says `onUnlock(record)` but the design doesn't specify WHEN it's called: before emit? After? Only on successful unlock? On failed unlock attempts too? + - Why missed: Interface shape defined but lifecycle contract not. + - Severity: If called before emit and sink throws, does the unlock still proceed? If called after emit and emit fails, was the unlock recorded? + - Mitigation: Define contract: `onUnlock` fires AFTER decision record is finalized but BEFORE stdout emit. Sink exceptions are caught and logged (never block the unlock). + +- **16 hex = 64 bits — birthday collision math is wrong for security context** + - What: CA8 says "collision ~1/10^19, acceptable." That's the probability for a SINGLE pair. But with N rules and M keys per rule, the collision space is `N * M` keys. With 42 rules and 2 key types = 84 keys, collision probability is `84^2 / 2^65 ≈ 3.2e-16` — still tiny. BUT: the MAC is truncated to 16 hex (64 bits), which means the ACTUAL security strength is 64 bits = 2^64 ≈ 1.8e19. For a brute-force attack, an attacker who knows the rule_id can try 2^64 HMAC values. Without salt this is feasible with rainbow tables; WITH salt it's 2^64 per salt value. + - Why missed: CA8 cites the collision probability but doesn't frame it as brute-force resistance. + - Severity: For a local dev tool, 64-bit is probably fine. But if keys are ever exposed network-wide, it's weak. + - Mitigation: Document that 16 hex is a TRUNCATED MAC for readability, not a security boundary. The salt provides the real security. + +- **TTL key with exp=0 or exp in the past at mint time** + - What: `pi-opa-net unlock-key --ttl 0` or `--ttl -1` → mints a key that's immediately expired. CA10 says "reject negatives" but doesn't cover `--ttl 0` or `--ttl 1` (expires in 1 second, before the agent can use it). + - Why missed: CA10 covers negative rejection but not minimum TTL enforcement. + - Severity: User mints key, hands to agent, agent tries to use it → expired. Confusing UX. + - Mitigation: Enforce minimum TTL (e.g., 60 seconds). Reject `--ttl < 60` with clear error. + +- **Clock source inconsistency — `now()` at verification vs `Date.now()` at minting** + - What: KeyVerifier uses `now()` (injectable for tests). Key minting uses `Date.now()` (implicit). If the minting process and verification process have different clock sources, TTL could be wrong. + - Why missed: Design specifies injectable `now()` for KeyVerifier but doesn't specify the minting side. + - Severity: Minor on same machine. Could matter if keys are minted on one machine and verified on another with different NTP state. + - Mitigation: Both minting and verification MUST use `Date.now()` (unix epoch seconds). Injectable `now()` in tests only. + +- **UnlockFilter placement ambiguity — before or after DecisionBuilder?** + - What: Design says "post-eval filter" and the architecture shows `PARSE → EVAL → UNLOCK FILTER → BUILD → EMIT`. But DecisionBuilder.buildReasons() sets `severity: 'block'` on ALL reasons. The unlock filter needs to demote bypassed reasons to `severity: 'info'` and add `bypassed: true`. Does this happen: (a) before builder.build() by modifying engine.reasons, (b) inside builder.build() as a new step, or (c) after builder.build() by mutating the output? + - Why missed: Three different integration points, each with different implications for the DecisionBuilder API. + - Severity: Wrong placement = either builder overwrites the filter's work, or filter can't access rule metadata. + - Mitigation: Option (b) is cleanest: DecisionBuilder.build() accepts an optional `UnlockResult` in its deps. Inside `buildReasons()`, it checks if the rule_id is in the unlock set and adjusts severity + adds bypassed fields. + +- **run.ts orchestration needs unlock key parsing BEFORE engine.evaluate** + - What: Current `runCli()`: `resolveRaw → configFromEnv → parse → engine.evaluate → builder.build → validate → format`. The unlock filter needs keys parsed from env/args/stdin. But `engine.evaluate()` is where OPA runs. If unlock happens AFTER OPA, the engine already returned `decision: 'deny'` with reasons. The unlock filter then needs to change the decision to `'allow'` if all blocking reasons are bypassed. This means the `EngineDecision` is mutable, or the filter produces a new one. + - Why missed: Design shows the pipeline but doesn't specify the data flow for decision mutation. + - Severity: If the filter can't change `decision: 'deny'` → `'allow'`, the unlock doesn't work. + - Mitigation: UnlockFilter takes `(engineDecision, unlockKeys)` → returns new `EngineDecision` with: (1) bypassed reasons removed from blocking set, (2) if blocking set empty → `decision: 'allow'`, `source: 'opa-unlocked'`, (3) bypassed reasons preserved in output with `bypassed: true`. + +- **Schema validation runs AFTER builder but BEFORE formatter — unlock fields must be valid** + - What: `validateDecision(output)` runs in run.ts after builder.build(). The output at this point must include all new unlock fields AND they must conform to the updated schema. If any unlock field is missing or malformed, validation throws. + - Why missed: Design lists `validateDecision` as a "hard internal gate." New fields must be schema-valid at this point. + - Severity: Validation failure → crash → no decision emitted → equivalent to fail-closed. + - Mitigation: Ensure UnlockFilter + DecisionBuilder produce schema-complete output. Test with schema validation enabled. + +- **No key revocation mechanism** + - What: Once a key is minted and handed to an agent, it cannot be revoked except by rotating the salt (which kills ALL keys). No per-key revocation. + - Why missed: Design explicitly chose "no server-side state." Revocation requires state. + - Severity: If a specific key is compromised, the only response is salt rotation → all keys die → re-issue to all agents. Operational sledgehammer. + - Mitigation: TTL keys mitigate this (they expire). For LL keys, accept the sledgehammer. Or: future "revocation list" file (YAGNI for now, but note the gap). + +- **PIOPANET_UNLOCK_KEYS env var — delimiter not specified** + - What: Design shows `PIOPANET_UNLOCK_KEYS=a3f9c2b8e1d4,...` with comma separation. But what if a key contains a comma? What about whitespace? Leading/trailing commas? Empty entries from `key1,,key2`? + - Why missed: Delimiter choice is assumed but not specified. + - Severity: Edge cases in env var parsing → keys silently dropped or misparsed. + - Mitigation: Specify: comma-delimited, whitespace trimmed, empty entries rejected with warning. Add to KeyParser tests. + +### Rank 2 (Minor) + +- **PIOPANET_UNLOCK_SALT env var leaks via /proc** + - What: Env vars are visible in `/proc//environ` to any process with same UID. Salt in env = salt readable by any process the user runs. + - Why missed: CA1 covers key leakage via `ps`/history but the salt env var is a higher-value target. + - Severity: Adversarial process on same box → derives all keys. + - Mitigation: Document the risk. Consider reading salt from a fd (e.g., `PIOPANET_UNLOCK_SALT_FD=3`) so the value never appears in `/proc/environ`. YAGNI: at minimum, document. + +- **Salt rotation has no migration story** + - What: When salt rotates, ALL existing keys — both LL and TTL — become invalid simultaneously. No grace period, no "old salt + new salt" dual-verify window. + - Why missed: Design mentions "rotation = new salt = all old keys die" as a feature. But in practice, agents holding keys in their env will fail mid-session with no clear error. + - Severity: Operational disruption. + - Mitigation: On verification failure, check if key would validate against a "previous salt" (keep N=1 old salt). Log warning. Or: accept as-is but ensure error message says "salt may have rotated." + +- **~/.pi-opa-net/ directory may not exist** + - What: SaltResolver tries to write `~/.pi-opa-net/salt` but `~/.pi-opa-net/` doesn't exist yet. `fs.writeFileSync` throws ENOENT. + - Why missed: Design assumes the directory exists. First-ever invocation won't have it. + - Severity: Crash on first use. + - Mitigation: `fs.mkdirSync(dir, { recursive: true, mode: 0o700 })` before writing salt. + +- **Decision record as audit is JSON-fragile** + - What: Decision record is emitted as a single JSON line on stdout. If a consumer pipes through `jq` or a log aggregator that splits on newlines, and the JSON is pretty-printed (future debug mode), the audit record breaks across lines. + - Why missed: Current OutputFormatter always emits compact JSON. But no invariant enforces "single line" in the schema or interface contract. + - Severity: Minor today, breaks if anyone adds pretty-print. + - Mitigation: Add explicit contract: decision record is ALWAYS single-line JSON on stdout. + +- **No deduplication of unlock audit events** + - What: If the same key is used for 100 evals in a session, 100 identical audit records are emitted. No "session-level" aggregation. + - Why missed: Design is per-invocation. No concept of session-level unlock state. + - Severity: Log volume. + - Mitigation: Future FileAppendSink could dedup by `(unlock_key_id, rule_id)` per session. YAGNI for now but note the shape. + +- **TTL key format leaks expiry time to eavesdroppers** + - What: `ttl.1753127056.7c2f8a1b...` — the expiry timestamp is in plaintext. Anyone who sees the key knows EXACTLY when the key expires. + - Why missed: Design prioritizes "self-describing" over "opaque." + - Severity: Information leakage. Low impact for dev tool. + - Mitigation: Accept as tradeoff. Document that TTL keys leak expiry by design. + +- **Mixed LL + TTL keys for same rule — no precedence defined** + - What: Design says "LL and TTL keys for same rule coexist." But what if both are present in `PIOPANET_UNLOCK_KEYS`? Does the unlock filter try both? Prefer one? Log both? + - Why missed: Design shows them coexisting but doesn't define precedence or conflict resolution. + - Severity: If LL is present, TTL is redundant. But both being valid means two audit entries for the same rule unlock. + - Mitigation: Define: if multiple valid keys exist for the same rule, prefer TTL (more restrictive) over LL. Log both key_ids in audit. Or: first-match-wins. + +- **6 new files in src/unlock/ may exceed the "max 10 files per directory" rule** + - What: Project rules state "ALL directories MUST have MAXIMUM 10 files." src/unlock/ adds 6 files. Combined with potential test files and index.ts, this is close to the limit. + - Why missed: Design counts source files but not the total directory population including index, tests colocated per project rules. + - Severity: Minor organizational. + - Mitigation: Keep the 6 source files + 1 index.ts = 7. Tests follow the project rule of colocating. If directory exceeds 10, split into `src/unlock/derive/` and `src/unlock/verify/`. + +- **No index.ts / barrel export mentioned for src/unlock/** + - What: Ship surface lists 6 files but no `index.ts` barrel. Existing modules use barrel exports. + - Why missed: Implicit assumption that barrel is added. + - Severity: Minor — import paths become verbose without barrel. + - Mitigation: Add `src/unlock/index.ts` to ship surface. Same for `src/audit/index.ts`. + +- **No locked-decisions.yaml artifact exists yet** + - What: README.md references `2026-07-20-locked-decisions.yaml` as "immutable inputs" but this file doesn't appear to exist on disk. The openspec capture depends on it. + - Why missed: Referenced in "pick up next time" but never created. + - Severity: Missing artifact means the locked decisions exist only in prose. + - Mitigation: Create the YAML file as part of OT1 capture. Or: inline the locked decisions in proposal.md. + +- **No migration story for existing consumers of decision-output.v1** + - What: Adding `source: 'opa-unlocked'` and new Reason fields changes the schema. Any existing consumer that does strict matching on `source` will NOT recognize `'opa-unlocked'` and may treat it as unknown/invalid. + - Why missed: Design says "additive, v1 stays" but adding enum values IS a breaking change for strict consumers. + - Severity: Depends on consumer count. Today: probably zero external consumers. + - Mitigation: Document in proposal.md: "Consumers should treat unknown source values as opaque strings, not fail." Add to schema description. + +- **OT1 scope creep — proposal + design + spec + tasks is 4 documents** + - What: Mirroring `conditional-branch-gate` means 4 documents. For a feature that's 6 new files + 6 edits, that's heavy documentation overhead. + - Why missed: Pattern-copy from sibling change. + - Severity: YAGNI tension. + - Mitigation: Consider a lighter capture: proposal.md + tasks.md only. Design decisions are already captured in the explore findings. + +- **No rate limiting on unlock attempts** + - What: An agent can try unlimited unlock keys per invocation. Brute-force attempts are not throttled. + - Why missed: Design assumes possession-of-key = trust. + - Severity: For a local dev tool, low risk. + - Mitigation: YAGNI for now. But: KeyVerifier should log failed attempts. If performance becomes an issue, cap keys-per-invocation. + +--- + +## Summary table + +| Item | Rank | Count | +|------|------|-------| +| LD-Y1 (Salt) | 2-3 | 5 findings | +| LD-Y2 (Audit) | 2-4 | 4 findings | +| TTL design | 2-4 | 7 findings | +| Ship surface | 2-4 | 6 findings | +| OT1 (Openspec) | 2 | 3 findings | +| Cross-cutting | 2-4 | 3 findings | +| **Total** | | **28 findings** | + +--- + +## Cross-turn references + +- Also relevant to: Turn 2 §four design forks, Turn 3 §TTL design diff --git a/flow/findings/2026-07-20-rule-unlock-keys/README.md b/flow/findings/2026-07-20-rule-unlock-keys/README.md new file mode 100644 index 0000000..51f82bd --- /dev/null +++ b/flow/findings/2026-07-20-rule-unlock-keys/README.md @@ -0,0 +1,26 @@ +# rule-unlock-keys + +> Date range: 2026-07-20 → 2026-07-20 +> Status: **DONE** — explored → locked → openspec captured → implemented (TDD) → jewilo-verified → merged → published as pi-opa-net@0.2.0 + +## Topics + +### rule-unlock-keys design explore (2026-07-20) +Explored hidden-bypass/unlock-key feature for pi-opa-net. 4 turns → 16 locked decisions (LD-L1..L6, LD-Y1, LD-Y2, LD-G1..G8) + 16 open threads (all resolved). YAGNI applied to salt + audit. Ship surface: src/unlock/* (6 files), src/audit/AuditSink.ts, src/cli/unlock-key.ts, schema additive, policy unchanged. + +### Implementation + deploy (2026-07-20) +Full TDD cycle (RED sub-agent → GREEN sub-agent → 3 RED test-bug fixes by parent). 304/304 tests pass. jewilo verifier-loop APPROVE 2/2 (hash `072026-c604475a`). PR #3 merged to main (commit `5cdd037`). jewilo-dev with rag-quick model APPROVE 2/2 (hash `072026-7291243b`). Published as **pi-opa-net@0.2.0** to npm (manual publish fallback — CI macOS/Ubuntu e2e skip fix also shipped). Global binary install + end-to-end smoke verified. + +## Outcome + +- **Commit (merge):** `5cdd037` (PR #3 squash) +- **Commit (release):** `bb50a40` (v0.2.0 metadata refresh) +- **npm:** `pi-opa-net@0.2.0` (published 2026-07-20T12:18Z) +- **Verifier proof:** `072026-c604475a` (jewilo pre-merge), `072026-7291243b` (jewilo-dev post-merge, rag-quick) +- **OT1 status:** resolved (openspec captured + shipped) +- **OT2 status:** open (ospx manifest enumeration — escalated to user, low priority) + +## Pick up next time +1. OT2 (low priority): enumerate ospx manifest steps 11-70 +2. Future: pi-opa-net-ext (the pi tool_call adapter — OT5 from docs/open-threads) +3. Future: gcloud/bq unlock support (LD-G4 — currently out-of-scope) diff --git a/flow/findings/2026-07-20-rule-unlock-keys/references.md b/flow/findings/2026-07-20-rule-unlock-keys/references.md new file mode 100644 index 0000000..0447a88 --- /dev/null +++ b/flow/findings/2026-07-20-rule-unlock-keys/references.md @@ -0,0 +1,31 @@ +# References + +> Sources consulted during this explore session. + +## Source files + +- `README.md` — project overview, status (v0.1.0), architecture (two-halves), schema v1 contract, fail-mode, env config +- `src/rules/catalog.ts` — canonical rule catalog (41 entries), rule_id + family + suggestions, inferFamilyFromProgram helper +- `policy/safety.rego` — OPA/Rego v1 policy, 37 deny blocks, `package safety`, `default allow := true` (fail-open), helpers `has_any_arg` + `has_arg_prefix` +- `src/util/digest.ts` — `sha256Prefix(path)` → first 12 hex chars of SHA-256, used for rulebook drift detection +- `src/output/DecisionBuilder.ts` — schema assembly class, `DecisionOutput` interface, `Reason` interface with `severity: 'block'`, `build()` method wires parsed + engine → output +- `src/cli/run.ts` — `runCli()` orchestrator: resolveRaw → configFromEnv → parse → engine.evaluate → builder.build → validateDecision → formatter.format +- `docs/cupcake-parity.md` — Cupcake-compatible policy documentation, 42 active cc-safety-net user rules ported, standalone `opa eval` examples +- `openspec/changes/conditional-branch-gate/proposal.md` — in-flight change adding `input.signals.*` for branch-protection rule, solved the "signals gap" vs Cupcake +- `openspec/changes/conditional-branch-gate/design.md` — D1–D5 decisions for signals layer, GitSignals collector, protected-branch config, schema additive `signals` field + +## Documents + +- `~/.cc-safety-net/rules/user-rules/rulebook.json` — canonical cc-safety-net user rulebook (42 active rules), source of truth for parity port +- `tests/fixtures/user-rules.rulebook.json` — deterministic snapshot of the canonical rulebook for CI +- `schemas/decision-output.v1.json` — JSON Schema draft 2020-12, strict (`additionalProperties: false`), the symmetric decision record contract +- `~/.pi/agent/cmd-family/ospx.yml` — ospx family manifest (step 10/70), governs explore-mode stance + +## Code patterns + +- `sha256Prefix` in `src/util/digest.ts` — existing hash utility; candidate foundation for unlock key derivation (HMAC variant) +- `DecisionBuilder.build()` pipeline in `src/output/DecisionBuilder.ts` — the assembly point where unlock filter would inject (between engine.evaluate and builder.build, or inside builder) +- `runCli()` orchestration in `src/cli/run.ts` — the wiring point where unlock keys would be threaded (parse env/args → inject into evaluation pipeline) +- `deny[msg]` rego pattern in `policy/safety.rego` — existing deny-rule shape; unlock check lives TS-side so this file needs NO CHANGE +- `RULES` catalog in `src/rules/catalog.ts` — rule_id is the hash input for unlock key derivation; catalog already correct, needs no change +- Cupcake `input.signals.*` pattern from `openspec/changes/conditional-branch-gate/proposal.md` — sibling pattern for adding context to OPA input; unlock-keys uses a different approach (TS-side filter) but the signals design validates the additive-schema philosophy diff --git a/package.json b/package.json index a74322c..71bf787 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "pi-opa-net", - "version": "0.4.2", + "version": "0.5.0", "description": "OPA-backed bash command guard for the pi ecosystem — structured decision-output.v1 JSON, fail-open default, Claude Code hook protocol compatible. Agent-agnostic engine + CLI.", "type": "module", "main": "src/index.ts", diff --git a/policy/safety.rego b/policy/safety.rego index 326bdcc..376d435 100644 --- a/policy/safety.rego +++ b/policy/safety.rego @@ -436,7 +436,7 @@ deny[msg] if { # canonical rulebook reason field. # ────────────────────────────────────────────────────────────────── -session_kill_targets := ["tmux", "wezterm", "wezterm-mux-server"] +session_kill_targets := ["tmux", "wezterm", "wezterm-mux-server", "herdr", "bermuda"] deny[msg] if { input.program == "tmux" @@ -462,6 +462,44 @@ deny[msg] if { msg := "Killing the tmux/wezterm server destroys ALL sessions, panes, and in-flight work across every client. Do NOT run this automatically — hand the exact command back to the user and let them run it themselves." } +# ────────────────────────────────────────────────────────────────── +# GROUP H — herdr session protection +# (herdr is a terminal workspace manager for AI coding agents; +# killing it destroys active workspaces, sessions, and agent state.) +# ────────────────────────────────────────────────────────────────── + +# Block `herdr server stop` — stops the herdr daemon. +deny[msg] if { + input.program == "herdr" + has_any_arg(input.args, ["server"]) + has_any_arg(input.args, ["stop"]) + msg := "Stopping the herdr server destroys all active workspaces, sessions, and agent state. Do NOT run this automatically — hand the exact command back to the user." +} + +# Block `herdr session stop ` — stops a named session. +deny[msg] if { + input.program == "herdr" + has_any_arg(input.args, ["session"]) + has_any_arg(input.args, ["stop"]) + msg := "Stopping a herdr session destroys in-flight agent work. Do NOT run this automatically — hand the exact command back to the user." +} + +# Block `herdr session delete ` — deletes a stopped session. +deny[msg] if { + input.program == "herdr" + has_any_arg(input.args, ["session"]) + has_any_arg(input.args, ["delete"]) + msg := "Deleting a herdr session removes persisted state. Do NOT run this automatically — hand the exact command back to the user." +} + +# Block `herdr workspace close ` — closes a workspace. +deny[msg] if { + input.program == "herdr" + has_any_arg(input.args, ["workspace"]) + has_any_arg(input.args, ["close"]) + msg := "Closing a herdr workspace destroys active agent state. Do NOT run this automatically — hand the exact command back to the user." +} + # ────────────────────────────────────────────────────────────────── # USAGE # ────────────────────────────────────────────────────────────────── diff --git a/schemas/decision-output.v1.json b/schemas/decision-output.v1.json index 76ca255..a376499 100644 --- a/schemas/decision-output.v1.json +++ b/schemas/decision-output.v1.json @@ -95,7 +95,7 @@ }, "family": { "type": "string", - "enum": ["git", "docker", "rm", "gcloud", "bq", "gh", "glab", "bd", "builtin", "custom", "tmux", "pkill", "killall"], + "enum": ["git", "docker", "rm", "gcloud", "bq", "gh", "glab", "bd", "builtin", "custom", "tmux", "pkill", "killall", "herdr"], "description": "Rule family for grouping/filtering. Matches the 6 families in the rego translation (turn6)." }, "severity": { @@ -204,6 +204,14 @@ "unlock_agent": { "type": "string", "description": "Agent identifier that presented the unlock keys." + }, + "pi_opa_net_version": { + "type": "string", + "description": "pi-opa-net package version (e.g. '0.4.0'). Every audit trace carries this for version correlation when debugging decision trails." + }, + "dry_run": { + "type": "boolean", + "description": "Present and true when PIOPANET_DRY_RUN=1 is set. Indicates the CLI is in evaluation-only mode (no command execution). Tests and CI should set this flag." } } } diff --git a/src/audit/OtlpAuditSink.ts b/src/audit/OtlpAuditSink.ts index 5ec7b40..34bda35 100644 --- a/src/audit/OtlpAuditSink.ts +++ b/src/audit/OtlpAuditSink.ts @@ -29,6 +29,7 @@ type AuditEntry = { readonly command?: string; readonly rule_ids?: readonly string[]; readonly evaluated_at?: string; + readonly pi_opa_net_version?: string; readonly [k: string]: unknown; }; @@ -89,6 +90,7 @@ export class OtlpAuditSink implements AuditSink { }, }, { key: 'evaluated_at', value: stringValue(entry.evaluated_at) }, + { key: 'pi_opa_net_version', value: stringValue(entry.pi_opa_net_version) }, ]; return { diff --git a/src/output/DecisionBuilder.ts b/src/output/DecisionBuilder.ts index 679cb51..e2af66d 100644 --- a/src/output/DecisionBuilder.ts +++ b/src/output/DecisionBuilder.ts @@ -5,6 +5,7 @@ import type { EngineDecision, RawDeny } from '../engine/types.ts'; import type { ParsedCommand } from '../parser/types.ts'; import { type RuleMeta, type RuleRegistry, inferFamilyFromProgram } from '../rules/index.ts'; import type { UnlockResult } from '../unlock/types.ts'; +import { PI_OPA_NET_VERSION } from '../version.ts'; /** The schema-compliant decision output (decision-output.v1). */ export interface DecisionOutput { @@ -59,6 +60,8 @@ export interface DecisionMetadata { readonly unlock_count?: number; readonly unlock_blocked_count?: number; readonly unlock_agent?: string; + readonly pi_opa_net_version?: string; + readonly dry_run?: boolean; } export interface DecisionBuilderDeps { @@ -114,7 +117,13 @@ export class DecisionBuilder { policy_path: this.deps.config.policyPath, hostname: this.deps.config.hostname ?? osHostname(), session_id: this.deps.config.sessionId ?? '', + pi_opa_net_version: PI_OPA_NET_VERSION, }; + // Dry-run mode: PIOPANET_DRY_RUN=1 adds a marker to the metadata. + // This ensures tests and CI can verify the CLI is in evaluation-only mode. + if (process.env.PIOPANET_DRY_RUN === '1') { + (metadata as unknown as Record).dry_run = true; + } if (unlockResult) { const meta = metadata as unknown as Record; meta.unlock_count = unlockResult.bypassedCount; diff --git a/src/pi/audit.ts b/src/pi/audit.ts index 4990921..fce5712 100644 --- a/src/pi/audit.ts +++ b/src/pi/audit.ts @@ -1,6 +1,7 @@ import { appendFileSync, mkdirSync } from 'node:fs'; import { resolve } from 'node:path'; import type { DecisionOutput } from '../output/DecisionBuilder.ts'; +import { PI_OPA_NET_VERSION } from '../version.ts'; /** * Redact common secret patterns from a command string before audit. @@ -42,6 +43,7 @@ interface AuditEntry { command: string; rule_ids: string[]; evaluated_at: string; + pi_opa_net_version: string; } export async function writeAuditEntry(input: WriteAuditEntryInput): Promise { @@ -54,6 +56,7 @@ export async function writeAuditEntry(input: WriteAuditEntryInput): Promise r.rule_id), evaluated_at: input.decision.evaluated_at, + pi_opa_net_version: PI_OPA_NET_VERSION, }; await input.auditSink.write(entry); diff --git a/src/rules/RuleRegistry.ts b/src/rules/RuleRegistry.ts index c6c12b0..2727bf5 100644 --- a/src/rules/RuleRegistry.ts +++ b/src/rules/RuleRegistry.ts @@ -13,7 +13,8 @@ export type RuleFamily = | 'custom' | 'tmux' | 'pkill' - | 'killall'; + | 'killall' + | 'herdr'; export interface RuleMeta { readonly ruleId: string; diff --git a/src/rules/catalog.ts b/src/rules/catalog.ts index 511469a..4bbd82a 100644 --- a/src/rules/catalog.ts +++ b/src/rules/catalog.ts @@ -239,6 +239,31 @@ export const RULES: readonly RuleMeta[] = [ message: 'Killing the tmux/wezterm server destroys ALL sessions, panes, and in-flight work across every client. Do NOT run this automatically \u2014 hand the exact command back to the user and let them run it themselves.', }, + // ── GROUP H: herdr session protection ── + { + ruleId: 'block-herdr-server-stop', + family: 'herdr', + message: + 'Stopping the herdr server destroys all active workspaces, sessions, and agent state. Do NOT run this automatically \u2014 hand the exact command back to the user.', + }, + { + ruleId: 'block-herdr-session-stop', + family: 'herdr', + message: + 'Stopping a herdr session destroys in-flight agent work. Do NOT run this automatically \u2014 hand the exact command back to the user.', + }, + { + ruleId: 'block-herdr-session-delete', + family: 'herdr', + message: + 'Deleting a herdr session removes persisted state. Do NOT run this automatically \u2014 hand the exact command back to the user.', + }, + { + ruleId: 'block-herdr-workspace-close', + family: 'herdr', + message: + 'Closing a herdr workspace destroys active agent state. Do NOT run this automatically \u2014 hand the exact command back to the user.', + }, ]; /** gcloud/bq produce sprintf messages — family inferred from program. @@ -257,6 +282,8 @@ export function inferFamilyFromProgram(program: string): RuleFamily { return 'pkill'; case 'killall': return 'killall'; + case 'herdr': + return 'herdr'; default: return 'custom'; } diff --git a/src/version.ts b/src/version.ts new file mode 100644 index 0000000..c7fa535 --- /dev/null +++ b/src/version.ts @@ -0,0 +1,23 @@ +/** + * Package version — read once from package.json at module load. + * + * Used in decision metadata and audit entries so every trace/log line + * carries the exact pi-opa-net version that produced it. This enables + * after-the-fact version correlation when debugging audit trails. + */ +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const __dirname = fileURLToPath(new URL('.', import.meta.url)); + +let _version = 'unknown'; +try { + const pkg = JSON.parse(readFileSync(resolve(__dirname, '..', 'package.json'), 'utf8')); + _version = pkg.version ?? 'unknown'; +} catch { + // Non-fatal — version stays 'unknown' if package.json is unreachable. +} + +/** The pi-opa-net package version (e.g. "0.4.0"). */ +export const PI_OPA_NET_VERSION: string = _version; diff --git a/tests/e2e/e2e.test.ts b/tests/e2e/e2e.test.ts index c0f1694..eb9f117 100644 --- a/tests/e2e/e2e.test.ts +++ b/tests/e2e/e2e.test.ts @@ -34,7 +34,7 @@ function runCli(command: string, mode: 'json' | 'claude-code' = 'json'): CaseRes const stdout = execFileSync('bun', ['run', BIN, ...args], { encoding: 'utf8', timeout: 10000, - env: { ...process.env, HOME: process.env.HOME }, + env: { ...process.env, HOME: process.env.HOME, PIOPANET_DRY_RUN: '1' }, }); return { exitCode: 0, stdout, record: tryParse(stdout) }; } catch (err) { @@ -122,6 +122,42 @@ const DENY_CASES: ExpectDeny[] = [ ruleId: 'block-killall-tmux-wezterm', family: 'killall', }, + // GROUP H — herdr session protection. + { + command: 'herdr server stop', + ruleId: 'block-herdr-server-stop', + family: 'herdr', + }, + { + command: 'herdr session stop foo', + ruleId: 'block-herdr-session-stop', + family: 'herdr', + }, + { + command: 'herdr session delete bar', + ruleId: 'block-herdr-session-delete', + family: 'herdr', + }, + { + command: 'herdr workspace close baz', + ruleId: 'block-herdr-workspace-close', + family: 'herdr', + }, + { + command: 'pkill herdr', + ruleId: 'block-pkill-tmux-wezterm', + family: 'pkill', + }, + { + command: 'killall herdr', + ruleId: 'block-killall-tmux-wezterm', + family: 'killall', + }, + { + command: 'pkill bermuda', + ruleId: 'block-pkill-tmux-wezterm', + family: 'pkill', + }, ]; const ALLOW_CASES: ExpectAllow[] = [ @@ -137,6 +173,10 @@ const ALLOW_CASES: ExpectAllow[] = [ { command: 'tmux ls' }, // read-only tmux — must stay allowed { command: 'pkill firefox' }, // unrelated target — must stay allowed { command: 'killall vim' }, // unrelated target — must stay allowed + // GROUP H carve-outs (herdr read-only commands). + { command: 'herdr session list' }, // read-only — must stay allowed + { command: 'herdr workspace list' }, // read-only — must stay allowed + { command: 'pkill bermuda-helper' }, // unrelated target prefix — must stay allowed ]; describe.skipIf(!opaAvailable)('pi-opa-net E2E (live CLI + OPA)', () => { @@ -186,6 +226,16 @@ describe.skipIf(!opaAvailable)('pi-opa-net E2E (live CLI + OPA)', () => { } }); + it('dry-run mode: PIOPANET_DRY_RUN=1 adds dry_run flag to metadata', () => { + const r = runCli('git stash list', 'json'); + expect(r.exitCode).toBe(0); + const rec = r.record!; + const metadata = rec.metadata as Record; + expect(metadata.dry_run).toBe(true); + expect(metadata.pi_opa_net_version).toBeDefined(); + expect(typeof metadata.pi_opa_net_version).toBe('string'); + }); + it('claude-code mode: allow emits empty stdout (CA2)', () => { const r = runCli('git stash list', 'claude-code'); expect(r.exitCode).toBe(0); @@ -206,8 +256,8 @@ describe.skipIf(!opaAvailable)('pi-opa-net E2E (live CLI + OPA)', () => { const reasons = (r.record?.reasons as Array>) ?? []; for (const x of reasons) fired.add(x.rule_id as string); } - // 37 catalog rules → 40% = 15 - expect(fired.size).toBeGreaterThanOrEqual(15); + // 41 catalog rules → 40% = 17 + expect(fired.size).toBeGreaterThanOrEqual(17); }, 30000); // runs ~20 CLI subprocesses serially; needs headroom over the 5s default it('fail-open path: invalid policy path still resolves (source != crash)', () => { @@ -217,7 +267,7 @@ describe.skipIf(!opaAvailable)('pi-opa-net E2E (live CLI + OPA)', () => { const stdout = execFileSync('bun', args, { encoding: 'utf8', timeout: 10000, - env: { ...process.env, HOME: process.env.HOME }, + env: { ...process.env, HOME: process.env.HOME, PIOPANET_DRY_RUN: '1' }, }); const rec = JSON.parse(stdout); // fail-open default → allow with source fail-open OR opa if it tolerated. diff --git a/tests/e2e/pi-session-smoke.test.ts b/tests/e2e/pi-session-smoke.test.ts index 79a068e..f7529f4 100644 --- a/tests/e2e/pi-session-smoke.test.ts +++ b/tests/e2e/pi-session-smoke.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from 'bun:test'; import { spawn } from 'node:child_process'; +import { execFileSync } from 'node:child_process'; import { mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -34,6 +35,20 @@ function runPiSession( ): Promise<{ stdout: string; stderr: string; exitCode: number | null }> { return new Promise((accept, reject) => { const cwd = mkdtempSync(join(tmpdir(), 'piopa-smoke-')); + // pi requires a git repo in cwd; init one so the session can start. + try { + execFileSync('git', ['init', '-q'], { cwd, stdio: 'ignore' }); + execFileSync('git', ['config', 'user.email', 'smoke@pi-opa-net.test'], { + cwd, + stdio: 'ignore', + }); + execFileSync('git', ['config', 'user.name', 'pi-opa-net smoke'], { + cwd, + stdio: 'ignore', + }); + } catch { + // git init failure is non-fatal — the session may still start. + } const cleanup = () => rmSync(cwd, { recursive: true, force: true }); const timer = setTimeout(() => { child.kill('SIGKILL'); diff --git a/tests/unit/audit/OtlpAuditSink.test.ts b/tests/unit/audit/OtlpAuditSink.test.ts index a4e5be7..383407c 100644 --- a/tests/unit/audit/OtlpAuditSink.test.ts +++ b/tests/unit/audit/OtlpAuditSink.test.ts @@ -47,6 +47,7 @@ const SAMPLE_DENY = { command: 'rm -rf /', rule_ids: ['block-rm-rf-dangerous-target'], evaluated_at: '2026-07-24T00:00:00Z', + pi_opa_net_version: '0.4.0', }; const SAMPLE_ALLOW = { @@ -116,7 +117,7 @@ describe('OtlpAuditSink', () => { expect(customAttr?.value?.stringValue).toBe('custom-svc'); }); - it('logRecord body kvlistValue has keys: decision_id, decision, source, command, rule_ids, evaluated_at', async () => { + it('logRecord body kvlistValue has keys: decision_id, decision, source, command, rule_ids, evaluated_at, pi_opa_net_version', async () => { const mock = installFetchMock(); restoreFetch = mock.restore; const sink = new OtlpAuditSink({ endpoint: 'http://otel:4318/v1/logs' }); @@ -132,6 +133,7 @@ describe('OtlpAuditSink', () => { 'command', 'rule_ids', 'evaluated_at', + 'pi_opa_net_version', ]); // rule_ids serialized as arrayValue diff --git a/tests/unit/rules/catalog-parity.test.ts b/tests/unit/rules/catalog-parity.test.ts index 189ae7c..c506b22 100644 --- a/tests/unit/rules/catalog-parity.test.ts +++ b/tests/unit/rules/catalog-parity.test.ts @@ -90,4 +90,29 @@ describe('rule catalog ↔ rego parity', () => { } }); }); + + // ── herdr session protection rule parity ── + describe('herdr session protection rule parity', () => { + const REQUIRED_RULE_IDS = [ + 'block-herdr-server-stop', + 'block-herdr-session-stop', + 'block-herdr-session-delete', + 'block-herdr-workspace-close', + ]; + + it('catalog contains all herdr rule IDs', () => { + const catalogIds = new Set(RULES.map((r) => r.ruleId)); + const missing = REQUIRED_RULE_IDS.filter((id) => !catalogIds.has(id)); + expect(missing, `catalog is missing rule IDs: ${missing.join(', ')}`).toEqual([]); + }); + + it('each herdr rule maps to family herdr', () => { + const byId = new Map(RULES.map((r) => [r.ruleId, r])); + for (const id of REQUIRED_RULE_IDS) { + const rule = byId.get(id); + expect(rule, `catalog entry for ${id} must exist`).toBeDefined(); + expect(rule!.family).toBe('herdr'); + } + }); + }); });