Skip to content

Latest commit

 

History

History
604 lines (439 loc) · 25.3 KB

v1.md

File metadata and controls

604 lines (439 loc) · 25.3 KB

ToolGate v1: Architectural Refactor

v1 ships no new user-visible features. It refactors the v0 internals to match the architectural commitments in project.md revision 2: resource-keyed locking, principal-bearing identity, checkpointed approval, and a Postgres-advisory-lock substrate. The deployment substrate remains Docker Compose. The goal is a system whose internals match the threat model the v1 docs claim — not a wider surface area.

If v0 was "build it," v1 is "build it for the reasons it should have existed in the first place." The shipping artifact is a smaller, cleaner codebase, not a bigger one.


Goals (and non-goals)

Goals

  1. Delete the per-turn RWLock and per-turn read/write classifier. Intra-turn ordering is delegated to the agent framework (LangGraph).
  2. Introduce a resource-keyed lock manager configured via per-tool YAML rules.
  3. Add the identity headers (X-ToolGate-Principal, X-ToolGate-Agent-Id, X-ToolGate-Agent-Version) and thread them through policy, audit, and SDK.
  4. Replace the synchronous approval hold with a checkpointed Postgres-ticket model. Agents receive pending(ticket_id) and resume via poll or SSE callback.
  5. Migrate the session mutex from Redis SETNX + TTL to Postgres advisory locks. Redis becomes optional infrastructure.
  6. Publish a threat model document and a reproducible benchmark.

Non-goals (deferred)

  • Kubernetes / operator / CRDs / Helm — v3.
  • OAuth-passthrough credential mode — v2.
  • OPA/Rego policy backend — v2.
  • Baseline-relative eval gating — v2.
  • Tamper-evident audit log (Merkle chain) — v2.
  • Web UI — v3.
  • Multi-tenant isolation primitives — v2.
  • TypeScript SDK — v2+.

The v1 surface is deliberately narrow because the value of the release is in what is removed and reshaped, not in what is added.


Track 1 — Concurrency refactor

Remove

  • cmd/gateway/concurrency_guard.go — the per-turn read/write enforcement handler.
  • cmd/gateway/classifier.go — the read/write classifier.
  • cmd/gateway/turn_rwlock.go — the per-turn RWLock implementation.
  • All tests bound to the above (concurrency_guard_test.go, classifier_test.go, turn_rwlock_test.go).
  • The read-class / write-class distinction from the policy YAML schema.

This is net negative LoC. The deletion is the work.

Add

A new package core/locks containing the resource-keyed lock manager:

core/locks/
├── manager.go          # LockManager interface + Postgres advisory impl
├── manager_test.go
├── extractor.go        # YAML template extraction for resource keys
├── extractor_test.go
└── doc.go

LockManager interface:

type Mode int
const (
    ModeNone Mode = iota
    ModeRead
    ModeWrite
)

type LockHolder struct {
    Tenant    string  // optional; empty for single-tenant
    Session   string
    Turn      string  // optional
    JSONRPCID string
}

type LockManager interface {
    // Acquire blocks until the lock is held or ctx is canceled.
    // Returns a release function that must be called exactly once.
    Acquire(ctx context.Context, key string, mode Mode, holder LockHolder) (release func(), err error)
}

The Postgres implementation uses pg_try_advisory_xact_lock for write locks and a shared/exclusive lock table for read locks. The advisory lock key is a deterministic uint64 hash of the canonicalized resource key (fnv64a of tenant + "|" + resource_key + "|" + mode). Read locks compose via the SQL count table; write locks block when any read or write lock is held on the same key.

A new pipeline stage cmd/gateway/resource_lock.go inserts between PolicyGate and Forwarder. Pseudocode:

func (h *ResourceLockHandler) Handle(ctx context.Context, req *mcp.JSONRPCRequest) (*mcp.JSONRPCResponse, error) {
    rule := h.policy.ConcurrencyRuleFor(req.Method, req.Tool, req.Operation)
    if rule == nil || rule.Mode == ModeNone {
        return nil, nil // continue
    }
    if rule.IdempotencySupported {
        h.injectIdempotencyKey(req, rule)
        return nil, nil // skip lock; backend handles correctness
    }
    key, err := h.extractor.Extract(rule.ResourceKeyTemplate, req.Params)
    if err != nil {
        return mcp.NewErrorResponse(req.ID, -32602, "resource key extraction failed: "+err.Error()), nil
    }
    release, err := h.lockManager.Acquire(ctx, key, rule.Mode, holderFromContext(ctx, req))
    if err != nil {
        return nil, fmt.Errorf("lock acquisition failed: %w", err)
    }
    ctx = withLockRelease(ctx, release)
    return nil, nil // continue; release fires on response write
}

The release function is invoked in a defer at the response-write site, ensuring it fires on all paths including panics.

Resource-key extractor

YAML template syntax (Go text/template with a restricted function set):

concurrency:
  resourceKey: "stripe:charge:{{ .args.charge_id }}"

Allowed template features:

  • Field access on .args (the JSON-decoded MCP params).
  • A small whitelist of functions: lower, default, coalesce.
  • No iteration, no conditionals — keep the template language declarative and reviewable.

Disallowed:

  • Arbitrary Go template features (range, if, function calls into the runtime).
  • Side effects of any kind.

Failure modes:

  • Missing required field → return -32602 (invalid params); call denied.
  • Template parse error at policy load time → fail policy load; gateway refuses to start. No silent fallback.

Track 2 — Identity model

SDK changes

The Python SDK gains a Principal parameter on the client constructor and attaches three headers to every request:

client = ToolGateClient(
    gateway_url="http://gateway:8080/mcp",
    principal="sarah@acme.com",          # required
    agent_id="support-refund-agent",     # required
    agent_version="v7",                  # required
)

Headers attached on every call:

Mcp-Session-Id:               <issued by gateway on initialize>
X-ToolGate-Principal:       sarah@acme.com
X-ToolGate-Agent-Id:        support-refund-agent
X-ToolGate-Agent-Version:   v7
X-Mcp-Turn-Id:                <optional, observability>

The SDK refuses to construct if principal, agent_id, or agent_version are missing. This is a load-bearing validation: the gateway will reject calls without them.

Gateway changes

cmd/gateway/server.go — handleMCPPost extracts the three identity headers (in addition to the existing Mcp-Session-Id and turn ID) and stores them in the request context via new helpers in core/mcp/types.go:

ContextKeyPrincipal     contextKey = "principal"
ContextKeyAgentID       contextKey = "agentID"
ContextKeyAgentVersion  contextKey = "agentVersion"

Missing principal → -32600 invalid request with message naming the missing header. No silent default.

Audit schema migration

The audit_log table gains three columns:

ALTER TABLE audit_log
    ADD COLUMN principal       TEXT NOT NULL,
    ADD COLUMN agent_id        TEXT NOT NULL,
    ADD COLUMN agent_version   TEXT NOT NULL;

CREATE INDEX idx_audit_log_principal      ON audit_log(principal, created_at DESC);
CREATE INDEX idx_audit_log_agent_version  ON audit_log(agent_id, agent_version, created_at DESC);

Migration runs at gateway startup if the columns are absent. Existing v0 rows are backfilled with the literal string "v0-unknown" so the NOT NULL constraint holds. The backfill is one-shot, idempotent, and logged.

Policy schema additions

AgentPolicy rules gain an optional requirePrincipalScope clause:

- tool: stripe
  operation: refund_customer
  requirePrincipalScope: "stripe:refund:write"   # checked against principal's granted scopes
  concurrency:
    mode: write
    resourceKey: "stripe:charge:{{ .args.charge_id }}"

In v1 the scope check is a placeholder — principal-scope lookup tables live in v2's multi-tenant work. v1 ships the policy field, validates the YAML schema, and records the declared scope in the audit row, but does not enforce. This is intentional: ship the data shape now so v2 can turn on enforcement without a second schema migration.


Track 3 — Approval flow refactor

Current (v0) behavior to remove

cmd/gateway/approval_bridge.go — the synchronous-hold implementation. The handler currently:

  • Writes a ticket to Postgres.
  • Posts to Slack via the notifier.
  • Blocks on a Redis Pub/Sub channel via select with a 5-minute timer.
  • Returns the resolved decision to the agent.

This synchronous goroutine is the artifact being replaced.

New (v1) behavior

PolicyGate returns immediately with a structured pending MCP response:

{
  "jsonrpc": "2.0",
  "id": "<original request id>",
  "result": {
    "status": "pending_approval",
    "ticketId": "tkt_8f3a...",
    "pollUrl": "/tickets/tkt_8f3a...",
    "estimatedSeconds": 300
  }
}

The agent then has two recovery paths, both supported:

Poll path (default for v1): the agent calls GET /tickets/{id} periodically. Response:

{ "status": "pending"   | "approved" | "denied" | "timeout",
  "resolvedAt": "...",
  "approver": "manager@acme.com" }

When approved, the agent re-issues the original MCP call with an X-ToolGate-Approval-Ticket: tkt_8f3a... header. The policy engine sees the header, validates the ticket is still fresh (default TTL 1 hour), and forwards without re-routing to approval.

Callback path (optional): if the agent holds an open SSE channel via GET /mcp, the gateway pushes a server-initiated notifications/approval/resolved event with the ticket ID and status. The agent retries the call as in the poll path. This is purely an optimization — correctness lives in the ticket state.

Postgres schema additions

ALTER TABLE approval_tickets
    ADD COLUMN status         TEXT NOT NULL DEFAULT 'pending',
    ADD COLUMN resolved_at    TIMESTAMPTZ,
    ADD COLUMN approver       TEXT,
    ADD COLUMN resolution_ttl TIMESTAMPTZ NOT NULL,
    ADD COLUMN original_call  JSONB NOT NULL;

CREATE INDEX idx_approval_tickets_status_resolution_ttl
    ON approval_tickets(status, resolution_ttl)
    WHERE status = 'pending';

original_call stores the MCP request envelope so the gateway can validate that the agent's retry matches the originally-approved call (no swapping a $50 refund ticket for a $50,000 refund retry).

Timeout sweeper

A background goroutine in the gateway scans pending tickets past resolution_ttl and marks them status = 'timeout'. Runs every 10s. Cheap; uses the partial index above.

Slack webhook handler

cmd/gateway/slack_webhook.go — unchanged in structure but no longer publishes to Redis Pub/Sub. It updates the ticket row directly:

UPDATE approval_tickets
   SET status = $1, resolved_at = NOW(), approver = $2
 WHERE ticket_id = $3 AND status = 'pending'
RETURNING *;

The RETURNING clause + WHERE status = 'pending' ensures idempotency under retried webhook deliveries.

Removed Redis dependency

After this refactor, Redis is no longer required by the approval path. Redis remains optional for ephemeral state (rate-limiter buckets, distributed metrics counters) but the v1 demo runs without it.


Track 4 — Lock substrate migration

What's being replaced

cmd/gateway/session_locker.go currently implements the per-session mutex via Redis SETNX with a 60-second TTL. This is the pattern flagged by Kleppmann's How to do distributed locking — if a turn takes longer than the TTL, the lock expires while the holder still believes it's held, and two workers can interleave writes.

Replacement

A new core/locks/session_locker.go using Postgres advisory locks. The lock is held for the lifetime of a single database transaction, eliminating the TTL-expiry failure mode entirely:

func (l *PostgresSessionLocker) Acquire(ctx context.Context, sessionID string) (release func(), err error) {
    key := fnv64a("session:" + sessionID)
    tx, err := l.pool.Begin(ctx)
    if err != nil { return nil, err }
    var ok bool
    if err := tx.QueryRow(ctx, "SELECT pg_try_advisory_xact_lock($1)", key).Scan(&ok); err != nil {
        _ = tx.Rollback(ctx)
        return nil, err
    }
    if !ok {
        _ = tx.Rollback(ctx)
        return nil, ErrLockHeld
    }
    return func() { _ = tx.Commit(ctx) }, nil
}

The transaction holds the lock; commit releases it. Network drop or process death rolls back the transaction, which releases the lock. No stale-lock window.

Migration plan

  1. Add core/locks/session_locker.go next to (not replacing) the Redis implementation.
  2. Wire a feature flag --session-lock-backend=postgres|redis (default postgres in v1).
  3. Run integration tests with both backends in CI.
  4. After two weeks of pilot use with postgres, delete the Redis backend.

Track 5 — Documentation and benchmarking

Threat model document

New file: docs/threat-model.md. Required reading for every interview, code review, and architectural decision. Single page. Structure:

# Threat Model

## What ToolGate defends against
- Cross-session/cross-tenant conflicts on shared external resources
- Policy violations on proxy-introspectable inputs
- Identity confusion (actor vs. principal)
- Approval bypass
- Audit-log tampering (deferred to v2; v1 documents the gap)

## What ToolGate delegates
- Intra-turn dependency ordering → agent framework (LangGraph)
- Per-resource correctness when the backend supports idempotency → MCP server / upstream API
- Prompt-injection detection → partner layer (Lakera, Model Armor)
- Content moderation → partner layer (Guardrails AI, NeMo Guardrails)

## What is out of scope entirely
- Goal hijacking
- Memory poisoning
- Model-output safety
- LLM cost routing → LiteLLM/Portkey

## Assumptions
- The agent framework correctly enforces its declared graph topology.
- The upstream API, where declared idempotency-safe, is in fact idempotency-safe.
- Postgres advisory locks are honored by all gateway replicas.
- TLS terminates before the gateway; transport security is not the gateway's responsibility.

Benchmark harness

New directory: bench/. Contents:

bench/
├── README.md                          # how to reproduce
├── docker-compose.bench.yml           # gateway + fake stripe MCP + postgres + redis (optional)
├── scenarios/
│   ├── 01-allow-passthrough.yaml      # baseline: no policy match, no lock
│   ├── 02-policy-allow.yaml           # policy match → allow
│   ├── 03-policy-deny.yaml            # policy match → deny
│   ├── 04-resource-lock-uncontended.yaml
│   ├── 05-resource-lock-contended.yaml
│   └── 06-approval-pending.yaml
└── run.sh                             # invokes vegeta or k6 across all scenarios

Published numbers (target — to validate, not assume):

  • p50 latency for allow-passthrough: < 1 ms gateway hop in-cluster
  • p95 for policy-allow: < 5 ms
  • p95 for resource-lock-uncontended: < 8 ms
  • Throughput: ≥ 5k rps on a 2-vCPU gateway pod

If reality differs from targets, the targets get rewritten and the project.md latency budget gets corrected. The benchmark is the artifact, not the targets.


YAML schema reference (v1)

The full v1 AgentPolicy schema:

apiVersion: agents.io/v1
kind: AgentPolicy
metadata:
  name: support-agent-policy
spec:
  budgets:
    maxToolCallsPerTurn: 20
    maxCostUsdPerSession: 2.00
    maxDurationSecondsPerTurn: 300

  rules:
    - tool: stripe
      operation: refund_customer
      requirePrincipalScope: "stripe:refund:write"   # placeholder in v1
      concurrency:
        mode: write
        resourceKey: "stripe:charge:{{ .args.charge_id }}"
      idempotency:
        backendSupports: true
        headerField: "Idempotency-Key"
      require:
        args.amount: "< 50"
        verificationToken:                           # issuance deferred to v2
          issuer: verify_duplicate_charge
          maxAgeSeconds: 60
      approvalRequired:
        when: args.amount >= 50
        via: slack
        channel: "#refunds-approval"
        timeoutSeconds: 3600
        onTimeout: deny

    - tool: zendesk
      operation: read_ticket
      concurrency:
        mode: read
        resourceKey: "zendesk:ticket:{{ .args.ticket_id }}"

    - tool: web
      operation: search
      concurrency:
        mode: none

    - tool: slack
      operation: send_message
      concurrency:
        mode: none
      transform:
        redactPII:
          fields: [body]
          types: [credit_card, ssn, dob]

    - tool: customer_db
      deny:
        operations: [delete_customer, update_payment_method]

  defaultAction: deny

Schema validation runs at policy-load time. Any unrecognized field is a hard error (no silent skipping). The schema is the contract; surprises happen at load time, not at request time.


Migration sequence (single-engineer pragmatic plan)

The five tracks have dependencies. Order matters more than parallelism for a one-engineer team.

Week 1–2:  Track 5a (threat model doc)
           — write it first. Every subsequent decision references it.

Week 3–4:  Track 4 (Postgres advisory locks for session mutex)
           — substrate work that other tracks depend on.

Week 5–7:  Track 1 (concurrency refactor)
           — RWLock deletion + resource-keyed lock manager + extractor.
           — Largest single chunk.

Week 8–9:  Track 2 (identity model)
           — SDK + headers + audit schema + policy field.

Week 10–11: Track 3 (approval flow refactor)
           — checkpointed tickets + timeout sweeper + ticket-fresh retry.

Week 12:    Track 5b (benchmark harness + publish)
           — exercises the entire refactored system.

Week 13:    Buffer + integration polish.

Realistic total: ~3 months. Aggressive: 8 weeks. Slipping past 4 months is a signal to cut scope rather than extend timeline — the Compose-first commitment is partly about being able to ship without operational tax.

Order rationale

  • Threat model first: anchors every other decision. Cheap to write, expensive to skip.
  • Lock substrate before concurrency refactor: the new resource-lock manager uses the same Postgres-advisory primitives. Building them once is cheaper than twice.
  • Concurrency before identity: the lock manager's holder field uses principal; defining principal first is a temptation, but the lock manager works fine with a placeholder holder string for one iteration.
  • Identity before approval: the new ticket schema records principal; defining it once is cleaner.
  • Approval last: it's the most user-visible change and benefits from the rest of the system being stable.
  • Benchmark at the end: measures the actual shipping system, not an intermediate state.

Testing strategy

Property-based tests for the lock manager

Use pgregory.net/rapid to generate sequences of (session, turn, resource_key, mode) operations and assert invariants:

  1. No two write locks held on the same (tenant, resource_key) simultaneously.
  2. A write lock blocks until all read locks on the same key release.
  3. Read locks on the same key compose (multiple readers can hold simultaneously).
  4. Every successful Acquire is followed by exactly one release.
  5. Process death (simulated via canceled context) releases the lock.

Run with rapid.Check(t, ...) for 10,000 iterations in CI.

Integration tests for the approval flow

New file: cmd/gateway/approval_flow_integration_test.go. Scenarios:

  1. Pending → approved: agent issues call → gets pending(ticket_id) → Slack webhook fires approve → agent polls and sees approved → agent retries with X-ToolGate-Approval-Ticket → call forwards.
  2. Pending → denied: same flow, Slack webhook fires deny → retry returns the policy denial.
  3. Pending → timeout: ticket TTL expires before approval → sweeper marks it timeout → agent's retry is denied.
  4. Approval-ticket integrity: agent retries with a different MCP payload than the original → gateway rejects with -32603 ("approval ticket does not match request").
  5. Approval-ticket replay: agent retries with the same ticket twice → first succeeds, second denied (single-use enforcement).

Migration tests

cmd/gateway/migration_test.go: stand up a v0-schema Postgres, run the gateway's startup migration, assert the audit_log columns exist, assert v0 rows are backfilled with v0-unknown, assert v1 rows have correct identity values.

Performance tests

bench/ (see Track 5). Run as part of CI on a nightly schedule, not on every PR. Publish results as a markdown artifact.


v1 Guardrails (explicit non-goals)

  • No new MCP server integrations. The fake Stripe/Zendesk/Slack servers from v0 are sufficient. New tools are a v2 concern.
  • No new policy operations. Existing allow / deny / approvalRequired / transform cover the v1 demo. Don't add rateLimit, quotaReserve, or other ideas mid-flight.
  • No SDK languages beyond Python. TypeScript ships in v2 with OAuth-passthrough.
  • No web UI. Trace viewing remains Jaeger/Tempo.
  • No tamper-evident audit log. Merkle chain lands in v2.
  • No baseline-relative eval gating. v0's pass-rate-against-threshold remains. Baseline comparison is v2.
  • No Rego. YAML evaluator continues. Rego is v2.
  • No Kubernetes anything. Compose only.
  • No verification token issuance. The policy schema accepts verificationToken: clauses but the gateway logs and ignores them. Real issuance lands in v2.
  • No multi-tenant isolation. The schema includes a tenant field but the v1 gateway runs single-tenant.

If any of these become tempting mid-refactor, write them down in notes/v2-candidates.md and move on. The discipline is the deliverable.


Risks

Lock manager hotspots

Postgres advisory locks scale to roughly 10k–100k locks/sec depending on tuning and contention pattern. If pilot workloads exceed this, the lock-acquisition latency dominates the call. Mitigation: the benchmark in Track 5 surfaces the ceiling; the architecture allows swapping in etcd lease-based locking in v3 without changing the LockManager interface.

Resource-key extraction footguns

A wrong extraction template silently kills parallelism or silently misses conflicts. Mitigation: extraction templates are validated at policy load (parse error → refuse to start); every extracted key is logged in the audit row; integration tests assert that example tool calls produce the expected resource keys.

Approval-ticket replay attacks

If the ticket-freshness check is wrong, an attacker who gains a valid ticket can replay it past its intended window. Mitigation: tickets are single-use (consumed on first retry), TTL-bounded, and bound to the original MCP payload via SHA-256 of the canonicalized request. Replay or payload swap → denial.

Migration windows

Adding NOT NULL columns to existing tables requires backfill. Mitigation: the migration is one-shot, idempotent, and runs at gateway startup before listening on the port. Failed migration → gateway refuses to start.

Breaking changes to v0 demo consumers

The v0 demo agent does not pass identity headers. v1 will reject calls without them. Mitigation: the demo agent is updated in lockstep; there is no "v0 SDK in production" to support.


Definition of done

v1 ships when all of the following are true:

  1. The Go module compiles with no references to ConcurrencyGuard, Classifier, or TurnRWLock.
  2. core/locks/ exists with a LockManager interface and Postgres implementation, with property-based tests passing 10k iterations.
  3. The Python SDK requires principal, agent_id, agent_version at construction; missing → constructor raises.
  4. The gateway rejects MCP calls without X-ToolGate-Principal with -32600.
  5. The audit_log table includes principal, agent_id, agent_version columns; v0 rows are backfilled.
  6. cmd/gateway/approval_bridge.go no longer contains a select on a Redis Pub/Sub channel. The flow returns pending(ticket_id) immediately.
  7. The timeout sweeper runs and marks expired tickets timeout.
  8. docs/threat-model.md exists and is referenced from README.md.
  9. bench/ produces a markdown report; the report is committed and refreshed on every release tag.
  10. make demo still works end-to-end: the four-scenario demo from v0 passes, plus a new fifth scenario where a refund is approved asynchronously (pending → approved → retry succeeds).

Anything missing from this list is a v1 release blocker, not a "ship it and fix later."


What v1 deliberately leaves on the table

A reasonable critic will note that v1 ships no new product surface. That is intentional and correct.

The architectural decisions baked into v0 — per-turn locking, synchronous approval, agent-attested identity — were not wrong as v0 decisions; they were the simplest things that worked. They became wrong as soon as the project committed to the threat model in project.md revision 2. v1 is the cost of paying down that gap.

The interesting product work (eval baseline gating, OAuth passthrough, Rego, tamper-evident audit, K8s) is queued behind v1 because each of them assumes the v1 architectural foundation. Building any of them on the v0 substrate produces a system that is harder to reason about, harder to test, and harder to certify. v1 buys the option to do v2 and v3 well.

If at the end of v1 the project looks "the same but with less code and cleaner boundaries," that is success.