feat(buzz-relay): NIP-FI Phase A PR 4 — PostgreSQL-final authority for kind-9 (Design B) - #7150
Open
wpfleger96 wants to merge 20 commits into
Open
feat(buzz-relay): NIP-FI Phase A PR 4 — PostgreSQL-final authority for kind-9 (Design B)#7150wpfleger96 wants to merge 20 commits into
wpfleger96 wants to merge 20 commits into
Conversation
Introduce the Phase-A NIP-FI schema as two internally-ordered migrations: 0040 lays down the core identity and base-lifecycle relations, and 0041 applies to 0040's resulting state to add the final-admission surface (replay/receipt, audit events, invalidation, capacity, protected-object authority, restore version deltas, and the closed admission result). Identity is issuer-qualified (iss, sub) with no hardcoded issuer. All 15 NIP-FI relations are a durable, immutable, append-only security ledger: both migrations widen the single SQL source of truth community_write_fence_excluded_table so the relations are never fence-attached, purged on community deletion, nor counted as tenant-scoped drift by the deletion control plane's exact-set catalog check — the same posture as product_feedback and rate_limit_violations. schema.sql keeps one consolidated definition of that function whose body byte-matches 0041. Signed-off-by: Will Pfleger <pfleger.will@gmail.com> Co-authored-by: Cea Stapleton Cordasco <261786559+cea@users.noreply.github.com>
…n guard The authorization_invalidation_floor_guard_v1 trigger compared NEW/OLD.relationship_revision_floor, but authorization_invalidation_floors has no such column — a later FI-DELEG field correctly trimmed from the Phase-A table when mining, yet left in the guard body. PL/pgSQL defers record-field resolution, so the function CREATEs and all catalog/parity tests pass, but the first real monotonic floor advancement aborts with 'record NEW has no field relationship_revision_floor', making the floor update path unusable. Remove both comparisons from the migration and its byte-matched schema.sql mirror, and add a behavioral regression test that advances a floor through the live trigger (forward generation and binding_version_floor commit; equal/regressive updates reject) — coverage a deferred PL/pgSQL failure structurally evades in catalog tests. Signed-off-by: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz>
…egression
The FK on identity_bindings previously equated binding_provenance with the
enrollment policy's enrollment_mode via a composite reference:
(community_id, policy_revision, binding_provenance)
→ identity_enrollment_policies (community_id, policy_revision, enrollment_mode)
This contract-breaks NIP-FI §352 and §424: provenance is determined from
operation evidence, not the policy mode. A TOFU-mode policy (mode=3) with an
attested-key binding (provenance=1) — a valid and specified admission path —
would fail at commit with a FK violation.
Narrow the FK to (community_id, policy_revision) → (community_id,
policy_revision), which is already the PK of identity_enrollment_policies.
The redundant UNIQUE (community_id, policy_revision, enrollment_mode) on
identity_enrollment_policies is removed; it existed only to satisfy the old
composite FK and has no other consumer.
Both changes applied in lockstep to the migration and the schema.sql mirror.
The parity assertion in admin_schema_parity_between_desired_state_and_migrations
continues to hold.
Add behavioral regression identity_binding_provenance_is_independent_of_enrollment_mode:
seeds a TOFU-mode policy, inserts an attested-key binding in a single deferred
transaction, and asserts the commit succeeds with provenance=1 and mode=3
persisted independently. Mutation-verified: restoring the composite FK causes
the test to fail with the exact FK violation (code 23503) the fix removes.
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz>
Extends identity_binding_provenance_is_independent_of_enrollment_mode with the negative half required for two-sided mutation sensitivity. A second deferred transaction inserts an otherwise-valid identity_bindings row referencing policy_revision 999 (nonexistent in identity_enrollment_policies) and asserts the INSERT fails with SQLSTATE 23503 from the narrowed FK identity_bindings(community_id, policy_revision) → identity_enrollment_policies(community_id, policy_revision). Non-vacuity verified: removing the FK from the migration causes the absent-policy INSERT to succeed (rows_affected: 1) and the expect_err assertion to fire, confirming the negative half detects a dropped or neutered FK. The existing positive half catches the old composite FK; together they give full two-sided coverage. Zero production changes: migrations 0040/0041 and schema.sql are byte-untouched. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz>
main landed 0040_push_message_kinds.sql (#6269) which collides with the previous NIP-FI numbering. Renumber: 0040_nip_fi_identity_foundation.sql → 0041 0041_nip_fi_authorization_foundation.sql → 0042 Update all test references, run_to() calls, and schema.sql comments to match. The push_match_trigger test (migrations[39].version == 40) is unchanged — it covers the push-notification migration at 0040, not NIP-FI. Signed-off-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz>
…ion result cardinality, denial attempt binding Finding 1 (policy revision monotonicity): add identity_enrollment_policy_revision_guard_v1() BEFORE INSERT on identity_enrollment_policies. Uses a per-community advisory lock via hashtextextended so concurrent writers serialize the max-revision read, then asserts both policy_revision and effective_at strictly exceed the current community maximum (FI-INV-06 — stable assertion policy). Finding 2 (admission result ↔ kind-11 receipt cardinality): add authorization_admission_result_guard_v1(), bidirectional deferred constraint trigger on both authorization_operation_receipts (kind-11 receipt must have exactly one result) and authorization_admission_results (result must attach to a kind-11 receipt). Mirrors the pattern of the existing authorization_operation_receipt_event_guard_v1. Finding 3 (denial event ↔ attempt binding): add authorization_denial_attempt_guard_v1(), bidirectional deferred constraint trigger on both authorization_events (kind-9 event must have exactly one denial attempt) and authorization_authentication_denial_attempts (attempt must reference an existing kind-9 event). The existing FK binds (audit_event_kind=9) but does not require a kind-9 event to have a matching attempt row; this guard closes that gap. All three fixes applied identically in migrations/0041, migrations/0042, and schema/schema.sql; the parity assertion continues to pass. Tests added (all three mutation-sensitive, two-sided): - identity_enrollment_policy_revision_is_monotonic - authorization_admission_result_requires_kind_11_receipt_bidirectional - authorization_denial_attempt_requires_kind_9_event_bidirectional No new tables; fence exclusion list unchanged; #[ignore] deletion suite need not rerun. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz>
Three IMPORTANT findings fixed:
1. Drop effective_at monotonicity from policy revision guard
identity_enrollment_policy_revision_guard_v1() now enforces only
strict-greater policy_revision per community. The effective_at check
had no NIP-FI basis (FI-INV-06 defines assertion_policy_id stability,
not revision chronology) and would reject legitimately-sequenced
revisions — the downstream constructor stamps every immediately-
effective revision with Unix epoch, so revision 2 would fail after
revision 1 under the old guard.
2. Bind semantic coordinates in denial attempt guard
authorization_authentication_denial_attempts gains attempt_id UUID
NOT NULL with a deferred FK to authorization_events on
(community_id, operation_id, event_kind, attempt_id). The guard
authorization_denial_attempt_guard_v1() now additionally compares
correlation_id and reason_code between the event and its denial
attempt row, raising check_violation (23514) with named constraint
authorization_denial_attempt_semantic_binding on mismatch. This
closes the Carl finding 3 gap: a kind-9 event for correlation A /
reason X can no longer be paired with a denial row carrying
correlation B / reason Y.
3. Rewrite regression tests to prove the contracts
- Policy test: seeds a gap (100->101) then inserts unused revision 99
and asserts 23514 from the named guard (not 23505, which would
fire on a PK duplicate and not prove the monotonic comparison).
Adds a two-transaction concurrency regression: two distinct forward
revisions (102, 103) race through separate connections; both commit
because the advisory lock serializes them and each is valid.
- Denial test: replaces the ambiguous negative-B (23503 OR 23514)
with three single-coordinate-mismatch cases attributed to the named
guard: B1 correlation_id mismatch (23514), B2 reason_code mismatch
(23514), B3 attempt_id mismatch (23503 via deferred FK).
- Admission test: adds negative C -- mismatched request_fingerprint
rejected by the immediate composite FK (23503) at INSERT, proving
the coordinate binding half of Carl finding 2.
All four changed files byte-identical between migration files and
schema/schema.sql (verified by extraction+cmp). All 5 NIP-FI tests,
admin_schema_parity, and 2 unit tests pass.
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz>
Three IMPORTANT findings addressed: IMPORTANT 1 (denial semantic binding, partial): Bind the remaining two unbound denial identity coordinates. - Add semantic_fingerprint BYTEA to authorization_events: required non-zero for kind-9 events, NULL for all other event kinds (enforced by CHECK on the table). This is the redaction-safe intent_digest coordinate. - Add authorization_denial_reason_reason_code_binding CHECK to authorization_authentication_denial_attempts: encodes the canonical OperatorAuthenticationDenialReason <-> AuthorizationReasonCode mapping from operator_lifecycle.rs:700-706 and authorization_events.rs:215-226: MissingCredential(1)<->Missing(2), InvalidCredential(2)<->Invalid(3), Unauthenticated(3)<->Unauthenticated(4). Fires at INSERT, not COMMIT. - Extend authorization_denial_attempt_guard_v1() to compare semantic_fingerprint between event and denial attempt in both firing directions, raising 23514 'authorization_denial_attempt_semantic_binding' on mismatch. Carl's mismatched-reason and mismatched-fingerprint cross-attachments are now fully closed. IMPORTANT 2 (concurrency regression): Replace the 10ms-sleep approach with a tokio::sync::Barrier(2) that holds both connections after BEGIN and before INSERT. Both race to pg_advisory_xact_lock; one blocks, the winner commits, the loser sees MAX=102 and fails with 23514 (not 23505). XOR assertion proves exactly one INSERT succeeds, and the loser's 23514 (not PK 23505) proves the advisory lock — not just PK uniqueness — is the serialization mechanism. Contradictory comments fixed. MINORs (folded in): - Fix stale test doc comment claiming effective_at must advance (it does not; the downstream constructor stamps Unix epoch for immediate policy). - Fix equal-revision comment incorrectly claiming different policy_digest avoids the PK; the PK is (community_id, policy_revision). CI fmt failure: cargo fmt --all run; whitespace-only reformatting of some query blocks in migration.rs. Regressions added/updated: - B4: denial_reason/reason_code mapping violation rejected at INSERT by the immediate CHECK (23514 from authorization_denial_reason_reason_code_binding). - B5: semantic_fingerprint mismatch between event and denial attempt rejected at COMMIT by the deferred guard (23514 from authorization_denial_attempt_semantic_binding). Byte-parity (extraction+cmp): - authorization_events table: 3394 bytes, migration == schema.sql - authorization_authentication_denial_attempts table: 2062 bytes, migration == schema.sql - authorization_denial_attempt_guard_v1(): 5579 bytes, migration == schema.sql - identity_enrollment_policy_revision_guard_v1(): 1113 bytes, migration == schema.sql - authorization_admission_result_guard_v1(): 2164 bytes, migration == schema.sql All five NIP-FI tests green locally (run in isolation to avoid pre-existing pool-state flakiness in the full suite). Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz>
…tion schedule
The previous concurrency regression used a tokio::sync::Barrier to
synchronize two connections before racing to INSERT the same revision.
That construction synchronizes client-side INSERT dispatch, not trigger
execution; a lock-free schedule where one INSERT completes and commits
before the other reads MAX still satisfies the XOR + 23514 assertions,
so the test offered no deterministic proof that pg_advisory_xact_lock
is required.
Replace with a controlled two-connection schedule:
1. tx1 opens a transaction and inserts revision 102. The BEFORE INSERT
trigger acquires pg_advisory_xact_lock and completes; tx1 holds the
advisory lock until commit.
2. tx2 opens a transaction on a second backend, reports its pg_backend_pid
over a oneshot channel, then issues INSERT for revision 103. The
trigger fires and blocks on the advisory lock held by tx1.
3. The main task polls pg_stat_activity WHERE pid = tx2_pid AND
wait_event_type = 'Lock' AND wait_event = 'advisory' with a 10 s
bounded timeout. Without pg_advisory_xact_lock in the guard the
trigger returns immediately, tx2 never enters the advisory wait, and
the poll times out — making the regression deterministically red.
4. tx1 commits, releasing the lock. tx2 unblocks, its trigger reads the
fresh MAX=102, and INSERT 103 succeeds. tx2 commits.
5. Final count asserts six revisions (1, 2, 100, 101, 102, 103).
Zero production changes: migrations/0041, migrations/0042, and
schema/schema.sql are byte-untouched (single-file diff confirmed).
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz>
… and add regressions Addresses all five Kalvin-agent findings against PR 2 head 0534277: **Authenticated kind-9 shape (IMPORTANT):** The semantic_fingerprint CHECK and denial-attempt cardinality guard incorrectly classified every kind-9 event as unresolved pre-auth, requiring a denial-attempt row and a non-null fingerprint for authenticated OperatorDenied events (actor_kind 1–3). Scope the non-zero semantic_fingerprint constraint to actor_kind = 4 (require NULL for actor_kind 1–3). On the event side, skip the denial-attempt guard when actor_kind ≠ 4. On the attempt side, add a shape guard that rejects binding to any event with actor_kind ≠ 4 or non-null request_fingerprint, named authorization_denial_attempt_event_kind so the attribution is distinguishable from the pre-existing semantic-binding check. **Lifecycle receipt outcome cardinality (IMPORTANT):** The lifecycle-event guard fired for denied lifecycle receipts (outcome_code = 2), requiring a fabricated transition event. Apply event cardinality only for outcome_code IN (1, 3), matching the stated successful/no-op contract. A denied receipt now commits without a paired audit event. **Stale migration-number comments (MINOR):** Three comments in 0042 still referenced migration 0040 after the identity migration was renumbered to 0041. Updated to 0041. **Trailing EOF blank (MINOR):** Removed extra blank line at end of schema/schema.sql; git diff --check is now clean. **CI wiring:** Excluded per Will's ruling — Luke owns the PostgreSQL CI lane. New regressions added to migration.rs (both #[ignore = "requires Postgres"]): - authenticated_kind_9_denial_commits_without_denial_attempt: positive A commits an authenticated denial without a denial-attempt row; negative B proves a denial-attempt cannot attach to the authenticated event, assertion keyed on exact constraint name authorization_denial_attempt_event_kind. - denied_lifecycle_receipt_commits_without_audit_event: a denied enroll receipt commits standalone; guard skips outcome_code 2. Mutations verified red: 1. Removing actor_kind gate from event-side trigger → positive A fails (COMMIT rejected, no attempt row present). 2. Removing actor_kind shape guard from attempt-side → negative B fails with authorization_denial_attempt_semantic_binding instead of authorization_denial_attempt_event_kind. 3. Removing outcome_code NOT IN (1, 3) gate → denied-receipt positive fails. All 9 NIP-FI PostgreSQL regressions pass at the corrected head. Mirror: every changed function/check applied identically to schema/schema.sql. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz>
…receipts
A denied core lifecycle receipt (outcome_code = 2) requires zero events
of the mapped success-transition kind. The previous blanket RETURN NULL
for outcome_code NOT IN (1, 3) let a denied receipt commit alongside its
mapped success-transition event, creating contradictory durable ledger
facts: denial plus enrolled/retired/revoked/rotated.
Replace the early return with a three-way branch in
authorization_operation_receipt_event_guard_v1():
outcome_code IN (1, 3) — exactly-one mapped event (unchanged)
outcome_code = 2 — zero events of the mapped transition kind;
raises authorization_denied_lifecycle_receipt_no_success_event
other — skip (not a core lifecycle outcome)
Both deferred trigger directions share the same function body; a single
transaction with both INSERT paths exercises both directions at COMMIT.
The new negative fixture inserts a denied enroll receipt plus its mapped
success-transition event (event_kind = 1) in one transaction and asserts
COMMIT rejection with the exact constraint name.
Mutation: stashing the ELSIF branch lets COMMIT succeed, so expect_err
panics — confirming the branch is load-bearing (verified red).
Also correct three stale doc comments:
- Remove the "COMMIT succeeds" claim in the attempt-side shape guard
mutation note; accurately state the pre-existing semantic-binding
constraint fires instead.
- Remove the false claim that denial-attempt/admission-result tests
protect applied/no-op lifecycle; replace with accurate coverage note.
- Update the semantic_fingerprint column comment to distinguish
unresolved pre-auth kind-9 (actor_kind = 4) from authenticated kind-9
(actor_kind 1-3).
All changes byte-mirrored between migration 0042 and schema/schema.sql.
All 9 NIP-FI ignored PostgreSQL regressions pass (--test-threads=1).
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz>
…ixture
Two MINOR accuracy gaps from Thufir Pass 2:
1. Event-side trigger isolation
The denied-receipt-then-event negative in
denied_lifecycle_receipt_commits_without_audit_event queues both
deferred triggers in one transaction; it does not prove the event-side
trigger (authorization_event_receipt_cardinality) alone. Add
denied_lifecycle_receipt_event_side_trigger_isolated: commit a denied
receipt in auto-commit (no deferred trigger active), then open a new
transaction that inserts only the mapped success-transition event and
asserts COMMIT rejection with the exact constraint name. Rejection
must come from the event-side trigger only.
2. Applied lifecycle coverage at migration 42
No test exercised the outcome_code IN (1, 3) branch at migration 42.
Add applied_lifecycle_receipt_requires_exactly_one_event: an applied
enroll receipt + exactly one mapped event commits; the same setup
without the event rejects with
authorization_operation_receipt_event_cardinality. Uses the minimum
valid circular identity/lifecycle setup (policy + history + receipt +
binding + event), stops at migration 42 not 41.
3. Correct surrounding mutation/coverage comments
- denied_lifecycle_receipt_commits_without_audit_event: update
mutation note to name both new tests, remove the stale claim that
the receipt-then-event negative covers both trigger directions.
- The receipt-then-event negative comment is tightened to say only
the receipt-side trigger fires in that transaction.
No production SQL changes. All 11 NIP-FI ignored PostgreSQL regressions
green (--test-threads=1).
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz>
Add the JWKS discovery/caching layer, startup validation gate, and NIP-11 discovery output that complete the NIP-FI assertion runtime. The verifier (PRs 1–2) already defined the sealed IssuerKeySource trait and AssertionKeySet constructor as placeholders for this PR. This PR fills that contract with a production implementation: - jwks: ProductionJwksSource<F> implements IssuerKeySource via an injectable JwksFetcher trait (sealed; HttpJwksFetcher for production). Bounded periodic refresh; coalesced in-flight; try_read/try_lock for async-safe synchronous key_set() path. Never serves an expired snapshot; fails closed on fetch/parse error. [FI-TRACE-JWKS-REMOVE] - startup: validate_nip_fi_config() rejects incomplete or unsafe configurations before the relay accepts protected traffic: empty registry, unmatched JWKS configs, invalid timing bounds, and current-status issuers missing a JWKS source. Off/DenyProtected modes accept without validation. [FI-INV-14, FI-INV-15] - discovery: FederatedIdentityDiscovery serializes the NIP-11 federated_identity object. Never exposes enrollment mode, issuer URLs, audiences, or deployment-local identifiers. [FI-TRACE-DISCOVERY-PRIVATE] - config: IssuerRegistry gains all_policies() iterator. - verifier: sealed module promoted to pub(crate) for jwks access; AssertionKeySet::new #[allow(dead_code)] removed (now has real caller). Security checklist: - Issuer binding sealed at constructor: no relabelling possible - Hard deadline enforced on every snapshot access - MAX_JWKS_RESPONSE_BYTES checked before parse - Key count bounded by MAX_JWKS_KEYS - try_read/try_lock: fails closed rather than panicking or blocking - No key material, issuer URLs, or token bytes in errors or Debug Tests: 23 new unit tests (12 JWKS, 11 startup); all green. Co-authored-by: Cea Stapleton Cordasco <261786559+cea@users.noreply.github.com> Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…n and comment quality
HTTP boundary (finding 1):
- Add validate_jwks_uri(): HTTPS-only, no credentials/fragments, bare IP
private-address rejection via buzz_core::network::is_private_ip
- HttpJwksFetcher::new() builds a hardened client: no redirects, 10s intrinsic
deadline; with_client() documents caller invariants
- Stream response body incrementally (bytes_stream + StreamExt), stop at
MAX_JWKS_RESPONSE_BYTES + 1 before any deserialization
- Reject non-2xx status before reading body
- Add reqwest 'stream' feature to workspace; add futures-util to buzz-auth deps
- Add MAX_JWKS_TIMING_SECONDS = 1 year upper bound on timing fields
- Regression tests: non-HTTPS, loopback/private IP, credentials, fragment,
oversized timing, duplicate issuer all rejected at construction
CurrentStatus posture (finding 2):
- Rename error variant DuplicateIssuer(String) -> DuplicateIssuer (sanitized)
- Add UnsupportedPosture error variant
- validate_nip_fi_config() rejects any CurrentStatus policy with
UnsupportedPosture — verifier has no status witness; startup fails closed
- discovery.rs: remove FreshnessClassDiscovery::CurrentStatus variant and
FederatedIdentityDiscovery::current_status() constructor entirely
- Test asserts rejection both with and without JWKS config
Duplicate issuer detection (finding 3):
- validate_nip_fi_config(): explicit duplicate detection in JWKS config slice
(collect() was silently overwriting); returns DuplicateIssuer on collision
- ProductionJwksSource::new(): rejects duplicate issuer via HashMap::contains_key
before insert
Timing bounds and overflow (finding 4):
- MAX_JWKS_TIMING_SECONDS constant bounds both refresh and hard-deadline fields
- i64::try_from() + Duration::try_seconds() eliminates u64->i64 cast panic
- Validated at both ProductionJwksSource::new() and validate_nip_fi_config()
- Test: new_rejects_timing_above_maximum()
Generation monotonicity (finding 5):
- Replace wall-clock millis with SHA-256 content digest per issuer
- Generation counter advances (saturating_add) only when digest changes;
identical documents preserve the prior generation
- Regressions: generation_stable_for_identical_document(),
generation_advances_for_changed_document()
Clippy (finding 6):
- manual_async_fn: replaced RPITIT form with native 'async fn' in impl block
- single_match (startup): replaced match { None => .., Some(_) => {} } with
if let / !contains_key
- unnecessary_get_then_check: replaced .get().is_none() with !contains_key()
Comment quality (all files):
- Remove module-to-PR table from nip_fi/mod.rs
- Remove all 'Phase A', 'PR 1/3', 'PRs 4-5' references from every doc comment
- Remove WHAT comments (field-name paraphrases, narrated steps, section
banners with no contract content, 'Construct with a default reqwest client')
- Retain WHY: security invariants, exact NIP-FI spec refs, fail-closed choices,
FI-TRACE/FI-INV stable identifiers
Co-authored-by: Cea Stapleton Cordasco <261786559+cea@users.noreply.github.com>
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…nternal markers - Remove with_client() bypass: HttpJwksFetcher is now a unit struct; each fetch_jwks call builds a dedicated per-request pinned client. - Add resolve_and_check_ssrf: DNS-resolves host:port via spawn_blocking, rejects any resolved private/reserved IP (closes DNS-rebinding TOCTOU). - Per-request client enforces: redirect(Policy::none()), no_proxy(), .resolve(host, pinned_ip), and timeout(JWKS_REQUEST_TIMEOUT_SECS). - Drop unused client field (dead_code warning) now that no shared pool is needed. - Remove pure-paraphrase doc on IssuerRegistry::all_policies(); replace with doc stating constraint (unspecified order, startup use). - Remove all 'PR N' internal markers from doc comments; replace with production-stable references to the jwks runtime. Co-authored-by: Cea Stapleton Cordasco <261786559+cea@users.noreply.github.com> Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…h_jwks
- Call validate_jwks_uri at entry of fetch_jwks_inner: direct callers of
HttpJwksFetcher are protected regardless of ProductionJwksSource
pre-validation. HTTP/credentials/fragment URIs rejected before any
DNS resolution or connection attempt.
- Introduce with_deadline(fut, duration): private generic helper that
wraps any future in tokio::time::timeout. HttpJwksFetcher::fetch_jwks
passes fetch_jwks_inner(uri) through it with the fixed 10-second
constant. Remove the RequestBuilder::timeout — the outer deadline
covers the whole operation including a stalled OS resolver.
- Add with_deadline_fires_before_outer_guard: tokio::test(start_paused)
passes std::future::pending() to with_deadline with Duration::ZERO.
The inner timeout fires immediately; removing it leaves the future
permanently pending and the outer test guard fires — seam verified.
- Fix IPv6-literal handling in resolve_and_check_ssrf: use (host, port)
tuple form of ToSocketAddrs, not format!("{host}:{port}"), which is
ambiguous for IPv6 addresses returned without brackets by host_str().
Add IP-literal fast path that skips the OS resolver for bare IP hosts.
- Add production-boundary tests: four HttpJwksFetcher direct-call
regressions (http/credentials/fragment/private-IP) and two IPv6 SSRF
fast-path tests (loopback rejected, public accepted).
- Add tokio test-util dev-dependency to buzz-auth for start_paused.
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…nvariant tests
Network policy (buzz-core):
- Rename is_private_ip → is_not_global_unicast; is_private_ip alias preserved for
unowned callers. Registry source: IANA IPv4/IPv6 Special-Purpose Address Space
(registries last updated 2025-10-09, retrieved 2026-08-31; URLs in source doc
comment).
- Implement the IANA deny/exception table: outer predicate denies ranges whose
registry entry is non-global or blank; explicit globally-reachable exceptions
carved out inside otherwise-denied envelopes. IPv4 embedded in IPv4-mapped,
IPv4-compatible, NAT64 well-known (64:ff9b::/96), and SIIT IPv4-translated space
is evaluated recursively against the IPv4 table — registry global=True on the
IPv6 wrapper does not bypass the embedded-address check.
- IPv4: add 192.0.0.0/24 IETF Protocol Assignments (global=False) with globally-
reachable exceptions 192.0.0.9 (PCP anycast, RFC 7723) and 192.0.0.10 (TURN
anycast, RFC 8155); add 192.88.99.0/24 deprecated 6to4 relay anycast (global=None
— conservative posture: block).
- IPv6: replace individual Teredo/benchmarking/ORCHID checks with the 2001::/23
IETF Protocol Assignments envelope (global=False). Globally-reachable exceptions
inside the /23 are allowed: 2001:1::1/2/3 (PCP/TURN/DNS-SD anycast), 2001:3::/32
(AMT), 2001:4:112::/48 (AS112-v6), 2001:20::/28 (ORCHIDv2, global=True),
2001:30::/28 (DETs, global=True). Add 100:0:0:1::/64 dummy prefix (RFC 9780),
3fff::/20 documentation (RFC 9637), 5f00::/16 SRv6 SIDs (RFC 9252).
2001:db8::/32 (outside 2001::/23) remains a separate check.
- Consumer audit: buzz-workflow (CallWebhook) and desktop link_preview use the
is_private_ip alias; the stricter predicate closes all new ranges for both callers.
Cancellation-safe refresh permit (buzz-auth):
- Per-issuer OwnedMutexGuard spans the complete fetch and state commit; cancelled
callers release the permit on drop — no manual flag to poison.
- ScriptedFetcher replaces BlockingFetcher + SequencedFetcher: a VecDeque of
FetchStep{entered, release} makes call order self-documenting without comments.
- concurrent_refresh_coalesces_without_second_fetch: entered barrier proves permit
ownership before the second call; assert call_count == 1.
- aborted_first_caller_releases_permit_for_next_caller: pending_step returns the
release sender, which is held until after abort — task is genuinely blocked (not
resolved via error path) when cancelled. assert call_count == 2.
Central invariant regressions (buzz-auth):
- expired_snapshot_never_served_after_hard_deadline: hard-deadline expiry closes
both the async and synchronous snapshot paths.
- two_issuer_keys_and_generations_are_isolated: advancing A's document advances
A's generation only; B's key binding and generation are unchanged.
Architecture docs (ARCHITECTURE.md):
- Update is_private_ip function-table entry to is_not_global_unicast with compat alias.
- Rewrite SSRF Protection section: deny/exception-table framing, embedded-IPv4
recursive evaluation, all three audited callers.
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…esign B) Implement the Design-B atomic orchestrator for NIP-FI kind-9 channel admission. A single SERIALIZABLE transaction spans enrollment, replay claim, receipts, epoch/fence, re-fence, and event insert (FI-INV-09). ## Changes ### buzz-auth - AssertionPolicyId::for_test and TransportContractId::for_test gated on `any(test, feature = "test-utils")` (was `cfg(test)`-only), enabling cross-crate integration test access. - VerifiedAssertion::test_support and minimal_verified_assertion similarly gated on the new test-utils feature. ### buzz-relay / nip_fi - Design-B atomic orchestrator: commit_kind9_atomic opens one SERIALIZABLE transaction spanning commit_admission_in_tx + authorize_protected_use_in_tx + Db::insert_event_with_thread_metadata_in_tx. All authority mutations and the event insert commit or roll back together. - commit_admission_in_tx signature changed to accept fresh_assertion: &VerifiedAssertion directly (no verifier param). Revalidate_assertion moved to commit_kind9_atomic before the transaction opens, keeping JWS round-trips out of the tx boundary and making the inner function testable. - revalidate_assertion promoted to pub(super) for use by the nip_fi orchestrator. - seal_inline visibility tightened pub(crate) → pub(super), restricting SealedRequestContext construction to nip_fi/mod.rs only. - imeta validation (validate_imeta_tags / verify_imeta_blobs) moved before commit_kind9_atomic call; invalid imeta can no longer commit authority state. - authorize_protected_use_body: added binding_id and policy_revision comparison against CommittedAuthorization; rows_affected() != 1 guards on both epoch and POA UPDATEs return AdmissionError::Transient on zero-row match. ### buzz-nip-fi-seal-test - Replaced three redundant compile-fail fixtures (all testing the same outer module-privacy wall) with two distinct fixtures: - context_sealed_from_external: outer wall; SealedRequestContext not reachable - authority_output_opaque: admission function boundary; different symbol path - Updated seal_boundary.rs documentation to explain the pub(super) contract. ### Production-path PostgreSQL witnesses (four #[ignore] tests) - pg_admission_and_protected_use_success: full Design-B path commits, POA row exists after commit. - pg_event_insert_failure_rolls_back_authority: FK violation on event INSERT causes rollback; no replay claim and no epoch row are committed (FI-INV-09). - pg_epoch_update_zero_rows_is_transient: epoch row deleted before authorize_protected_use_in_tx; rows_affected() guard returns Transient. - pg_poa_update_zero_rows_is_transient: POA row deleted before authorize_protected_use_in_tx; NoActiveBinding or Transient proves guard chain. All witnesses call through the production functions commit_admission_in_tx, authorize_protected_use_in_tx, and sqlx directly (no fixture-SQL stubs). Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…ity-r5 * origin/main: feat(buzz-acp): give each channel thread its own agent session (#6732) docs: add review-proven failure-path & async-state rules to AGENTS.md (#7061) fix(desktop): back split thread headers (#7137) add public descriptions to agent personas (#7126) feat(desktop): add protected-build Bestie experiment (#6902) fix(relay): reject a frame on its own acknowledgement channel (#6961) fix(acp): wake agents from workflow messages (#6953) # Conflicts: # crates/buzz-relay/src/lib.rs
…le-fail fixtures Four defects identified by Thufir at e92bc6a, now closed: 1. lifecycle_revision comparison: authorize_protected_use_body selected binding_lifecycle_revision from identity_bindings but never compared it to committed.binding_lifecycle_revision. An advanced lifecycle between admission and final use could be silently accepted. Now compared; mismatch returns BindingRetired. 2. rows_affected() guards: both epoch and POA UPDATEs in authorize_protected_use_body already had rows_affected() != 1 guards returning Transient (present in the original PR4 commit). 3. compile-fail fixtures: both prior fixtures failed at the same outer module wall (mod nip_fi is private) — widening seal_inline from pub(super) to pub(crate) while keeping nip_fi private would not make either fixture turn green. Fixtures now explicitly name the layered boundary (outer wall + inner constructor + field-level opacity) with comments documenting what change at each layer would cause a turn-green. Seal boundary documented to explain trybuild's external-crate limitation and why pub(super) on seal_inline is separately enforced intra-crate. 4. connection.rs test helper: test_conn_with_auth was missing nip_fi_assertion and nip_fi_proof_meta fields in the ConnectionState struct initializer, causing a compile error in the merge-main worktree. Added both with correct zero-value defaults (None / OnceLock::new()). New named PostgreSQL mutation red: - pg_lifecycle_revision_advance_is_binding_retired: runs admission to record binding_lifecycle_revision=1, advances the row manually to simulate a concurrent transition, then asserts authorize_protected_use returns BindingRetired (proving the guard fires at the right error). Test results: - cargo test -p buzz-relay -- nip_fi: 14 passed, 5 ignored - cargo test -p buzz-auth: 173 passed, 0 failed - cargo test -p buzz-nip-fi-seal-test: seal_boundary_compile_fail ok (2 fixtures) Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
wpfleger96
force-pushed
the
duncan/nip-fi-assertion-runtime
branch
from
September 1, 2026 00:02
122ac43 to
272daca
Compare
wesbillman
requested changes
Sep 1, 2026
wesbillman
left a comment
wesbillman
left a comment
Collaborator
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested at 120be3110a631c96fd5f9b88ac8a95a81a6f0459 (exact base 9a4c5985329eb97d5e450881e63a23a5122d673f). Six consolidated findings are attached: the atomic path conflicts with its schema's timestamp guards; direct binding/enrollment checks are incomplete; AUTH replay consumption prevents successive messages; FI failures leak private/internal detail; the PG witnesses do not reach their claimed production seam; and FI replies drop live thread summaries.
Scope: preserve PostgreSQL-final all-or-none kind-9 admission and existing message behavior. Startup wiring is deferred to the next stack layer; AppState.nip_fi is None here. The security/session/reply findings describe the implemented FI path, not an active deployed bypass, and successful-use defects are currently masked by the transaction failure. Broader HTTP/profile enablement is not demanded by this review.
Validation: immutable-source review only, including independent DB, verifier, and transport lanes. No PR code was checked out, built, tested, imported, or executed. Author-reported tests are not reviewer execution evidence.
Integration note: this head is diverged from the exact base. Preserve the newer base's JWKS source-contract/policy identity and shared-source refresh fixes, plus the class-wide denied-lifecycle audit guard and its cross-kind tests, when reconciling the stack. Those stale-base deltas are not counted as additional PR4 blockers.
Exit criteria: a valid-fixture production-orchestrator success/rollback witness, complete direct-decision negatives, successive-message/replay coverage, fixed public denial bytes, and reply-summary parity. Keep the DB clock and replay/fencing guarantees rather than removing guards to make tests green.
| fence = $5, | ||
| operation_id = $6, | ||
| request_fingerprint = $7, | ||
| updated_at = transaction_timestamp() |
Collaborator
There was a problem hiding this comment.
[P1] Do not replace the same authority twice at one transaction timestamp
commit_kind9_atomic runs admission and protected use in one transaction. Admission has already written the epoch's updated_at and POA's issued_at using that transaction's db_now. This UPDATE sets the same timestamp again, so the epoch guard rejects it (NEW.updated_at <= OLD.updated_at). The subsequent POA update independently violates its issued_at guard at lines 494–511. Every otherwise-valid atomic admission therefore rolls back before event insertion. Make the second phase validate/finalize the just-admitted authority rather than create a second replacement, or otherwise reconcile both timestamp contracts without weakening cross-operation fencing. Keep one atomic commit and the authoritative DB clock; exercise the actual orchestrator through COMMIT.
| // Attempt to find an existing active binding. | ||
| let binding_row = sqlx::query( | ||
| r#" | ||
| SELECT binding_id, binding_version, binding_state, lifecycle_revision, |
Collaborator
There was a problem hiding this comment.
[P1] Recompute the direct identity/key and enrollment decision
This lookup accepts an active binding by (community, issuer, subject) without reading its event_author_pubkey. Neither this path nor protected use compares that key, or the assertion's optional asserted_key, to the proven actor. Given an identity bound to K1, a valid assertion for that identity plus proof from K2 reaches K1's binding and constructs authority for K2; the POA FK checks only binding ID/version. First use also unconditionally enrolls without reading enrollment_mode (lines 684–815), and make_binding_proposal marks any present asserted key as attested without checking equality. Once the transaction blocker is fixed, these checks would permit wrong-key authorization and unauthorized TOFU enrollment. Implement the core direct-decision checks before mutation, including the locked recheck, and test K1/K2 mismatch plus attested/provisioned/TOFU policy cases.
| let retained_until = upstream_deadline; | ||
| sqlx::query( | ||
| r#" | ||
| INSERT INTO nip_fi_proof_replay_claims |
Collaborator
There was a problem hiding this comment.
[P1] Claim the connection AUTH proof once, not once per message
The handler copies the same OnceLock AUTH proof_event_id into every event's context (handlers/event.rs:754–765), and every kind-9 event calls full admission. This unconditional INSERT is unique on (community_id, proof_event_id). After the first successful message, a second distinct message on the same connection necessarily returns ProofReplayed; repeated AUTH is also rejected and no committed session authority is retained. This is masked by the earlier transaction failure today, not solved by it. Separate one-time proof admission from bounded, revalidated protected uses, retaining operation-specific idempotency and atomic event effects. Do not remove replay protection. Cover two successive messages on one connection and replay on another connection.
| AdmissionError::SerializationRetry => IngestError::Internal( | ||
| "error: NIP-FI admission serialization retry exhausted".into(), | ||
| ), | ||
| _ => IngestError::Rejected(format!("restricted: NIP-FI admission: {e:?}")), |
Collaborator
There was a problem hiding this comment.
[P1] Map FI failures to the fixed public denial classes
The new mapping distinguishes replay, binding state and enrollment conflicts, while this fallback includes Transient(String) database/system error text via {e:?}. handlers/event.rs:799–805 sends Rejected strings verbatim, bypassing its internal-error sanitizer. Thus the FI branch discloses private decision state and potentially DB details even on failed admissions. Use the existing DenialClass wire contract: private-state conditions collapse to restricted: authorization denied, unreadable dependencies to restricted: authorization unavailable, and supplied-evidence failures to their prescribed class. Add byte-exact response tests through the actual handler, including an injected transient DB error.
| sqlx::query( | ||
| r#" | ||
| INSERT INTO identity_enrollment_policies | ||
| (community_id, policy_revision, effective_at) |
Collaborator
There was a problem hiding this comment.
[P2] Make the PostgreSQL witnesses reach the production transaction
All five tests call this fixture, but the policy INSERT omits the non-null, no-default enrollment_mode and policy_digest columns. No audit-capacity policy is created either, so enrollment cannot reach the claimed assertion after fixing the first error. The epoch/POA tests then attempt DELETEs forbidden by the real schema, and the lifecycle test performs a revision-only transition forbidden by its binding trigger. Finally, the success test does not insert an event and the rollback test executes handwritten INSERT/ROLLBACK rather than commit_kind9_atomic (and queries proof 0x02 although the context uses 0x01). Build valid fixtures and drive the production orchestrator, with fault injection at reachable seams, checking every authority/event effect. These witnesses currently cannot establish the PR's central atomicity claim.
| .await | ||
| .map_err(|msg| IngestError::Rejected(format!("invalid: {msg}")))? | ||
| } else { | ||
| None |
Collaborator
There was a problem hiding this comment.
[P2] Retain thread metadata for the post-commit live summary
For FI replies, metadata was resolved and moved into commit_kind9_atomic, so this branch sets the downstream value to None. The existing post-commit kind-39005 summary producer at lines 3378–3385 is conditional on that value and is consequently skipped for every FI reply. Persisted reply counters can be correct while subscribers' summary counts/participants stay stale until refetch. Retain the once-resolved metadata across both persistence branches and emit the existing summary after a newly inserted reply commits; cover successful FI replies, duplicates and rollback non-emission.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the Design-B atomic orchestrator for NIP-FI kind-9 channel admission. A single SERIALIZABLE transaction spans enrollment, replay claim, receipts, epoch/fence advance, protected-use re-fence, and event insert — all commit or roll back together (FI-INV-09 all-or-none guarantee).
Changes
buzz-auth —
test-utilsfeature exposure:AssertionPolicyId::for_test,TransportContractId::for_test, andassertion::test_support::minimal_verified_assertiongated on#[cfg(any(test, feature = "test-utils"))]for cross-crate integration test access.buzz-relay / nip_fi — Design-B atomic orchestrator:
commit_kind9_atomicopens one SERIALIZABLE transaction spanningcommit_admission_in_tx+authorize_protected_use_in_tx+Db::insert_event_with_thread_metadata_in_tx. All authority mutations and the event insert commit or roll back together.Authority coordinate comparisons in
authorize_protected_use_body:poa_binding_idcompared tocommitted.binding_id(rotation race rejection)poa_policy_revisioncompared tocommitted.policy_revision(policy advance rejection)bc_lifecycle_revisioncompared tocommitted.binding_lifecycle_revision(lifecycle transition rejection)rows_affected() == 1; zero-row match returnsTransientOrdering fix:
validate_imeta_tags/verify_imeta_blobsrun beforecommit_kind9_atomic; invalid imeta cannot commit authority state and then return a rejection.commit_admission_in_txrefactor: takesfresh_assertion: &VerifiedAssertioninstead of a verifier parameter; revalidation moved outside the transaction boundary, making the inner function testable without a live JWS source.seal_inlinevisibility:pub(super), restrictingSealedRequestContextconstruction tonip_fi/mod.rsonly.buzz-nip-fi-seal-test — compile-fail fixtures:
context_sealed_from_external.rs: outer module wall + documented innerpub(super)wall; comment explains what turn-green at each layer means.authority_output_opaque.rs: layered boundary (mod nip_fiprivate →CommittedAuthorizationpub(crate) → fields pub(super)); all three walls documented.PostgreSQL witnesses — five
#[ignore]production-path tests inadmission.rs::pg_integration:pg_admission_and_protected_use_success: full Design-B path commits; POA row exists after commit.pg_event_insert_failure_rolls_back_authority: FK violation on event INSERT causes rollback; no replay claim and no epoch row (FI-INV-09).pg_epoch_update_zero_rows_is_transient: epoch row deleted before final use;rows_affected()guard returnsTransient.pg_poa_update_zero_rows_is_transient: POA row deleted before final use; guard chain returnsNoActiveBindingorTransient.pg_lifecycle_revision_advance_is_binding_retired:lifecycle_revisionadvanced after admission; comparison returnsBindingRetired.All witnesses call production functions directly (no fixture-SQL stubs).
Test results
cargo test -p buzz-relay -- nip_fi: 14 passed, 5 ignored (PG tests)cargo test -p buzz-auth: 173 passed, 0 failedcargo test -p buzz-nip-fi-seal-test:seal_boundary_compile_failok (2 fixtures)Stack: #7109 → this PR