docs(nips): complete NIP-FI authorization draft - #3726
Conversation
Signed-off-by: Cea Stapleton Cordasco <261786559+cea-block@users.noreply.github.com>
Signed-off-by: Cea Stapleton Cordasco <261786559+cea-block@users.noreply.github.com>
Signed-off-by: Cea Stapleton Cordasco <261786559+cea-block@users.noreply.github.com>
ea0f556 to
674440f
Compare
Signed-off-by: Cea Stapleton Cordasco <261786559+cea-block@users.noreply.github.com>
…t-v2-s1-continuation-local-only * commit '0db374cd3cc31d3192969fa4e2cb12bc2636ff21': docs(nips): harden NIP-FI contract Signed-off-by: Cea Stapleton Cordasco <261786559+cea-block@users.noreply.github.com> # Conflicts: # docs/nips/NIP-FI-MODEL.md # docs/nips/NIP-FI.md
Signed-off-by: Cea Stapleton Cordasco <261786559+cea-block@users.noreply.github.com>
…t-v2-s1-continuation-local-only * commit 'e81a8b7fb920de1827fd17357932a86a52c5ccfa': docs(nips): align client assertion transport Signed-off-by: Cea Stapleton Cordasco <261786559+cea-block@users.noreply.github.com>
Signed-off-by: Cea Stapleton Cordasco <261786559+cea-block@users.noreply.github.com>
Signed-off-by: Cea Stapleton Cordasco <261786559+cea-block@users.noreply.github.com>
Signed-off-by: Cea Stapleton Cordasco <261786559+cea@users.noreply.github.com>
e81a8b7 to
805f113
Compare
- CORPORATE_IDENTITY.md: document BUZZ_NIP_FI_V1_CONFIG_JSON (Off / DenyProtected / Enforce), the complete runtime document with its field bounds, JWKS refresh limits, the 120-second status renewal bound, the removed legacy identity-provider variables that now fail startup, and the delegation default and enablement rules - .env.example: replace the placeholder identity section with the commented document example and the legacy-variable removal note - NIP_FI_DEPLOYMENT.md / NIP_FI_RUNTIME_OPERATIONS.md: document the single configuration input consistently and map the install-without- discovery step to the operating modes - deploy READMEs: pgcrypto requirement for managed PostgreSQL, identity configuration pass-through with secret-handling guidance, and audit-capacity sizing guidance Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Cea Stapleton Cordasco <261786559+cea@users.noreply.github.com>
There was a problem hiding this comment.
🤖
Review
I reviewed the full files at 805f1131b8c946cfdcff4508d20f1758515a5990..aa1bf732a9751fb8f4af54f1d3db8b964c6831ad, then traced the referenced implementation stack through #4847 at 5446360bee24aaa6ae4629a2bc6d4f0b1ea37eb7. This body merges two independent full-document passes at the same refs; where the passes disagreed on severity, the higher severity stands. I’m requesting changes because the public contract and the stock runtime contract cannot currently describe one conforming implementation.
[CRITICAL]: The stock runtime requires a different wire protocol than the NIP defines
docs/nips/NIP-FI.md:54-78 defines exactly trusted-proxy-hmac-v1: a v1.<timestamp>.<nonce>.<mac> envelope whose MAC covers the assertion and HTTP request coordinates. The model, deployment guide, integration guide, conformance applicability rule, and JSON examples all use that same public profile. In contrast, docs/CORPORATE_IDENTITY.md:53-58,88-92 and .env.example:66 say production requires trusted_proxy_hmac_v2.
This is not a hyphen/underscore serialization difference. The referenced runtime rejects every transport kind except trusted_proxy_hmac_v2 (config at #4847 head); its v2 MAC additionally binds the authorization domain, proof transport, and a third authenticated-client-peer field under NIP-FI-PROXY-2 (verifier input). Production then rejects otherwise-valid v1 evidence because it has no authenticated peer (upgrade boundary). A stock Buzz deployment therefore cannot accept the public NIP profile or produce the v1 conformance report this PR requires.
Choose one exact protocol before this contract lands. If v2 is the intended security boundary, specify its field, canonical MAC input, discovery identifier, replay namespace, lease bound, negative vectors, and applicability rules normatively, then update every guide/example. Otherwise the production runtime contract must accept the exact v1 profile. A private v2 extension cannot be called the sole stock NIP-FI transport while the public conformance matrix recognizes only v1.
[CRITICAL]: Operator docs assert runtime behavior that exists nowhere at this head
docs/CORPORATE_IDENTITY.md:7-12, docs/NIP_FI_DEPLOYMENT.md:5-11, docs/NIP_FI_RUNTIME_OPERATIONS.md:5-9, .env.example:57-75, and the Compose/Helm readiness sections say the relay accepts BUZZ_NIP_FI_V1_CONFIG_JSON, that absence/deny_protected/complete JSON select three operating modes, and that legacy variables fail startup. Verified at source: BUZZ_NIP_FI_V1_CONFIG_JSON, deny_protected, trusted_proxy_hmac_v2, and canonical_admission have zero hits in any code file on origin/main, the base branch, or this PR’s head (docs/.env.example only), and git log -S shows the “removed” legacy variables (BUZZ_REQUIRE_CORPORATE_IDENTITY, BUZZ_CORPORATE_IDENTITY_*) never existed on main — yet the doc says setting any of them fails startup. The same revision correctly says elsewhere that it contains no runtime adapter and changes no runtime configuration (docs/NIP_FI_INTEGRATION.md:5-24, SECURITY.md:75-82).
The sharpest edge is the emergency brake: “DenyProtected: set the document to {"deny_protected":true}. Every protected route is denied before its handler runs.” On the tree this merges into, the relay silently ignores that variable. An operator following this doc during an incident believes protected routes are locked when nothing happened — a false security promise in present-tense operator documentation. Same class: “constructed at startup, before listeners open,” “key refreshes are single-flight,” the 120-second status-renewal bound, and all field bounds.
Make these pages explicitly a future implementation contract in the same unambiguous voice NIP_FI_INTEGRATION.md already uses (“the current relay does not read this variable”), and remove current setup/pass-through claims from .env.example, Compose, and Helm until the runtime lands — or land the implementation atomically with the docs. “Not conformance evidence” does not fix a false statement about whether configuration is parsed at all.
[IMPORTANT]: The runtime configuration cannot be mapped to the spec it claims to configure
Beyond the transport mismatch above, the runtime document’s vocabulary does not correspond to the normative set: enrollment.kind: "canonical_admission" matches none of the spec’s three enrollment modes (attested-key/provisioned/tofu); restore.kind: "operation_manifest" and the entire client_status_admission block (“current-status presentation renews within 120 seconds”) reference a presentation subsystem defined nowhere in the normative documents. The configuration is also single-issuer, single-domain (one issuer string, one jwks, no domain array), while FI-TRACE-CROSS-DOMAIN-COLLISION requires two issuers × two domains and NIP_FI_INTEGRATION.md describes multi-domain deployments — the documented config cannot express the scenarios its own release gate requires. An operator cannot configure the spec from this contract, and a reviewer cannot check conformance against it.
[IMPORTANT]: Subject stability is presented as a verifier check, but no request can prove it
docs/nips/NIP-FI.md:84-92 says validation “enforces all” listed properties, including that the issuer contract guarantees sub is stable, opaque, non-reassignable, and not derived from personal data. A JWT proves only the signed subject value; it cannot prove the issuer’s lifetime assignment policy. The previous model made non-reassignment an explicit trust assumption, but this revision removes that section. The stock configuration records only a claim name, and the downstream verifier can only check that the resulting string is non-empty and bounded.
Move this property back into explicit trust/deployment assumptions, require operators to record how each accepted issuer satisfies it, and state the consequence of issuer reassignment. If the property is meant to be mechanically enforced, define the additional authenticated metadata and validation rule. Leaving an organizational assumption inside a list of executable verifier checks makes the model and conformance claim stronger than any implementation can demonstrate.
[IMPORTANT]: The claimed JWKS rollback protection has no rollback state or conformance oracle
docs/nips/NIP-FI.md:98 and the security consideration at line 286 claim that generation revalidation prevents rollback. The model only requires g to change when effective keys change, and the matrix tests add and remove (FI-TRACE-JWKS-ADD/REMOVE) but never A → B → A rollback. If an authenticated JWKS endpoint republishes a previously removed key set, it is simply a new current generation; an old assertion signed by that key revalidates successfully. The downstream cache behaves exactly this way: every changed document advances a process-local counter and is published without a durable removed-key floor (refresh).
Either define a durable authenticated anti-rollback mechanism and add a stable rollback trace, or narrow the contract to “revalidation against the currently authenticated snapshot” and list JWKS rollback as residual issuer/key-source risk. The current text promises a security property that neither the model nor the referenced implementation can establish.
[IMPORTANT]: The documented policy_id semantics cannot be produced by the stock configuration
docs/nips/NIP-FI.md:30 requires policy_id to include allowed algorithms and authenticated key-source identity and to change whenever assertion semantics change. The “complete” runtime document in docs/CORPORATE_IDENTITY.md:30-62 has no algorithm policy, and the referenced implementation hashes issuer, audience, claim names, skew, and token lifetime but not its compiled algorithm allow-list or configured JWKS/discovery source (policy construction). A binary upgrade that changes allowed algorithms, or a configuration change to a different authenticated key source, can therefore retain the same policy_id despite changing normative verifier semantics.
Put every normative semantic input into the stable digest, or include a versioned verifier-contract fingerprint that changes with compiled acceptance rules. Add vectors proving each semantic change advances policy_id while a key-only rotation does not.
[IMPORTANT]: The referenced WebSocket session path does not perform the rechecks this contract makes mandatory
The normative session contract (docs/nips/NIP-FI.md:232-242) requires each protected use to recheck binding/lifecycle versions and the current key snapshot, and requires a changed JWKS generation to revalidate the retained assertion. The matrix specifically requires active direct leases to deny/close on key removal. At #4847 head, the WebSocket fast path checks only static domain/actor/capability/transport and lease expiry (session check). The read/write leases are also built with assertion expiry rather than the proxy provenance deadline (AUTH preparation), and those grants are not registered with the invalidation registry. The only periodic binding recheck is in optional client-status presentation; clients that do not opt in still receive the static session grants.
This does not weaken the normative text; it means the referenced implementation is not yet feasible as a conforming handoff. The WebSocket lease owner needs current binding/lifecycle/invalidation and JWKS revalidation on reuse (or a cancellation owner that provides equivalent guarantees), and every direct lease must be bounded by proxy expiry. Related feasibility expectation worth stating before the #1476 rework: reconnect-based lease renewal at ~300s maxima implies real reconnect churn for long-lived agent connections.
[IMPORTANT]: The referenced final-admission path persists the denial state that the invariant forbids
docs/nips/NIP-FI.md:162,313, docs/nips/NIP-FI-MODEL.md:79,245,466, and FI-TRACE-FINAL-DENIAL-NO-MUTATION require local-policy and final-admission denial to create no audit or denial observation. At #4847 head, a denied fresh admission rolls back to a savepoint and then commits a denied operation receipt plus ProtectedDenied authorization event (denial branch, persistence). This will fail the matrix’s required before/after audit-store comparison. It also turns attacker-reachable denials into consumption of the documented non-reclaimable lifetime audit budget.
Resolve the contract decision explicitly. Either denied admission is read-only and the implementation must not persist these records, or the model/invariant/matrix must permit a separately bounded, non-authoritative denial channel and analyze audit-capacity exhaustion. The current two designs cannot both pass conformance.
[IMPORTANT]: Audit capacity is an unrecoverable one-way budget with no documented recovery transition
docs/CORPORATE_IDENTITY.md:79-84: “no online prune, export, reset, or acknowledgement workflow — size the budget for the installation’s lifetime… exhaustion denies further authorization-affecting operations.” Combined with fail-closed semantics, a mis-estimate — or an attacker who can generate authorization-affecting operations (see the denial-persistence finding above, which makes budget consumption attacker-reachable) — converts into a permanent, domain-wide identity outage. Nothing states whether raising the bound in configuration is a legitimate recovery path. The threat model’s availability section accepts transient fail-closed DoS but does not cover unrecoverable budget exhaustion. Document a privileged recovery transition or add an explicit residual-risk entry.
[MINOR]
- SECURITY.md weakening: “channel membership is the only access control mechanism” becomes “application-level… policy may allow.” On the merged tree, membership-only is still true; the softening pre-accommodates an unshipped authority. Keep the current-state claim and note NIP-FI as optional/future.
- Scope drift: the 44-line AWS-LC build-requirements section in
CONTRIBUTING.mdand the pgcrypto notes in the deploy READMEs are accurate (verifiedaws-lc-rs1.17.0 in both lockfiles; pgcrypto inmigrations/0001) but unrelated to completing the NIP-FI authorization draft. ReenableIdentitydesign question: it forcibly creates a new binding — there is no path to clear disabled state without a target key. Inprovisionedmode that conflates two operator intents (re-enable now vs. provision later).- Conformance feasibility: the denial-timing oracle’s predeclared statistical thresholds will be hard to run deterministically in CI; worth a stated execution strategy before the matrix becomes a release gate.
What's Solid
The lifecycle redesign itself is strong, and it genuinely closes the original revocation-resurrection blocker. Walking the scenario and its variants: a revoked key with a still-valid JWT hits the tombstone selector before the enrollment-mode switch, so attested-key/tofu can never re-enroll a revoked coordinate; a fresh key for a retired identity is blocked by the durable retired lineage, which only explicit recovery consumes; revocation landing between prepare and commit is caught by the atomic recomputation-and-equivalence recheck inside final admission (FI-INV-03, FI-TRACE-TOMBSTONE-REPLAY); and lifecycle transitions invalidate dependent leases after commit with an honest polling-bound caveat. Exact pending lineage makes recovery and re-enablement explicit; administrative expiry does not free either side of the partial bijection; and the read-only preparation/atomic commit split gives concurrency a clear serialization point. The proxy MAC construction is careful (length-prefixed fields, domain-separation tag, constant-time compare, overflow-safe time handling, equality-is-expired). The 24 trace labels and 16 invariant labels are internally aligned across the spec, model, matrix, and report example.
At the exact PR head, git diff --check passed; all three JSON examples parsed; local Markdown links, fences, and tables were valid; trace/invariant label equality held (24 traces appearing exactly once in each of the four carrying documents, 16 invariants); and the runtime-symbol absence claims above were verified with git grep/git log -S against origin/main, the base branch, and this head. These are documentation-consistency checks only, as the PR correctly notes; CI at head is green for the selected docs paths, with relay/security/unit jobs skipped.
Alternatives
The simplest coherent route is to promote the already-required v2 envelope into the normative transport profile, make the stock config explicitly future-facing until its implementation merges, and use the matrix as the hard gate for #4847. If v1 must remain the interoperable NIP, keep v2 as a separately named extension with its own discovery and evidence contract rather than presenting it as stock NIP-FI. Note that nearly all of the CRITICAL/IMPORTANT documentation findings trace to the newest commit (aa1bf732, plus part of c9efaf6) — rescoping or reframing that one commit leaves the normative core intact.
Quality gate: Minimalism 7/10 — duplicated transport/runtime claims are already drifting; consolidate the canonical definitions. Elegance 6/10 — a reader cannot derive one stock wire/configuration contract. Correctness 5/10 — the transport mismatch, false present-tense runtime claims, unverifiable subject rule, unsupported rollback claim, and downstream lease/denial incompatibilities block a conforming implementation. All named defects must be resolved before approval.
The published contract named a `trusted-proxy-hmac-v1` profile that no implementation accepts. This replaces it with `trusted-proxy-hmac-v2` as the sole stock proxy transport, specified from the wire format: envelope layout, length-prefixed MAC input, canonical encodings, discovery identifier, replay namespace, and lease bounds, with negative vectors. Denial handling is re-scoped. The no-mutation invariant now covers authoritative authorization state only, and denial observability moves to a separately bounded, non-authoritative channel carrying reason codes and correlation identifiers under payload minimization. A denied admission creates no authorization receipt, and a failed observation write never blocks, retries, or latches the denial. Runtime operations, deployment, and integration guidance is framed as future contract rather than current behavior. Subject stability is stated as a verifier trust assumption with its reassignment consequence, and the key-snapshot rollback claim is narrowed to what the model enforces. This is a documentation change. It adds no runtime parser, adapter, or enforcement path. Session-path rechecks and the policy digest input set remain relay-side follow-ups. The 24 trace and 16 invariant identifiers are unchanged.
Define the stock and private registered transport contracts without adding a deployment-specific profile to public discovery. Keep HMAC-v2 optional to deploy but exact when selected, and bind conformance evidence to versioned profile contracts. Unify every adapter on one normalized authorization contract, finite deadlines, current dependency revalidation, and the existing final-admission authority. Signed-off-by: Cea Stapleton Cordasco <261786559+cea@users.noreply.github.com>
Replace the flat future configuration draft with a multi-domain revision, separate denial observation from the non-reclaimable authorization-audit budget, and clarify current runtime behavior. Document lifecycle and denial-oracle rationale and carry the complete transport, lease, invalidation, and conformance obligations into the implementation handoff. Signed-off-by: Cea Stapleton Cordasco <261786559+cea@users.noreply.github.com>
Move these sections to PR #4772 because they are build and deployment documentation rather than NIP-FI specification content. Both describe requirements already present on main: aws-lc-rs 1.17.0 is in main's Cargo.lock, and pgcrypto is required by migrations/0001_initial_schema.sql. Signed-off-by: Cea Stapleton Cordasco <261786559+cea@users.noreply.github.com>
|
🤖 The review findings are addressed across four commits:
The 24 traces and 16 invariants remain unchanged. Examples and documentation checks pass. This remains a specification/docs-only PR and does not claim the future runtime is implemented. Commits: |
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Approving at head a383fd50. Re-verified every finding from the earlier review at this exact ref:
- Both CRITICALs resolved:
trusted_proxy_hmac_v2is now the single normative transport (no remainingtrusted-proxy-hmac-v1/NIP-FI-PROXY-1references anywhere at head), and the runtime configuration document is explicitly a future contract with a stated availability boundary — no live-behavior claims remain. - All seven IMPORTANTs addressed: subject stability restated as an issuer trust assumption with an operator evidence field; JWKS A→B→A rollback narrowed to documented residual risk with required trace evidence;
policy_idnow digests algorithms, key-source identity, and the verifier-contract fingerprint; leases require per-use revalidation and an invalidation owner; denials move to the separately bounded non-authoritativeO_Dchannel (FI-INV-09); config isdomains[] × issuers[]and can expressFI-TRACE-CROSS-DOMAIN-COLLISION; audit exhaustion is an accepted residual risk and denied operations no longer consume the budget. - Hygiene re-checked at head: 24 trace / 16 invariant labels consistent across carriers, all JSON examples parse,
git diff --checkclean, still docs-only.
Thanks @cea-block.
Why
The initial NIP-FI draft needs complete lifecycle, recovery, delegation, privacy, and conformance semantics before implementations can evaluate compatibility against one public contract.
What
Review guide
Risk assessment
Low. This PR changes documentation only and does not alter runtime behavior or configuration.
Testing
References