From f0fbbfc047b9ee1cd7be19f6774d124c1b24307a Mon Sep 17 00:00:00 2001 From: rclod <3385524+rclod@users.noreply.github.com> Date: Sun, 16 Aug 2026 15:35:55 -0500 Subject: [PATCH] fix(relay): report the verified NIP-OA owner after any successful admission MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The agent→owner backfill ran only when `require_relay_membership` was false. On a closed relay it never ran, and `enforce_relay_membership` did not fill the gap: it reported an owner only when it had USED the delegation to admit the agent (`MembershipDecision::ViaOwner`). An agent that is also a direct relay member took the `::Member` branch, which returned `Ok(None)` and discarded a valid, cryptographically self-proving attestation. So on a closed relay, any agent enrolled directly in `relay_members` had `users.agent_owner_pubkey` left NULL however valid its attestation. That column is not cosmetic — `is_agent` in the admission path derives from it (so such agents are limited at `human_messages_per_min` rather than `agent_standard_messages_per_min`), owner-managed agent lists have no relationship to display, and `channel_add_policy = "owner_only"` has no owner to compare against, refusing `add-member` for every actor including the real owner. Fixed in the shared helper rather than per call site. The attestation is now verified once, before the admission branches, and `MembershipDecision::OpenRelay` and `::Member` carry it; `enforce_relay_membership` reports it from every admitted variant. Both WebSocket AUTH and `POST /events` consume that shared result, so the open-relay-only workaround each carried is deleted rather than duplicated — the HTTP path had the same gap and would otherwise have kept it. Membership answers "may this caller connect"; the attestation answers "who owns this agent". Deriving the second from the first was the bug. The security boundary is unchanged: the tag's own verification in `extract_nip_oa_owner`. The decision→owner mapping is split into `owner_from_decision` so the behaviour that regressed is testable without a database, with three tests over it — notably that a direct member reports its owner, which is what previously returned None. Co-authored-by: themiguelamador Signed-off-by: rclod <3385524+rclod@users.noreply.github.com> --- crates/buzz-relay/src/api/bridge.rs | 10 +-- crates/buzz-relay/src/api/mod.rs | 114 ++++++++++++++++++++++--- crates/buzz-relay/src/handlers/auth.rs | 21 +---- 3 files changed, 106 insertions(+), 39 deletions(-) diff --git a/crates/buzz-relay/src/api/bridge.rs b/crates/buzz-relay/src/api/bridge.rs index dfce484494a..9249d2d2050 100644 --- a/crates/buzz-relay/src/api/bridge.rs +++ b/crates/buzz-relay/src/api/bridge.rs @@ -811,13 +811,9 @@ async fn submit_event_authed( ) .await { - Ok(owner) => owner.or_else(|| { - if !state.config.require_relay_membership { - super::relay_members::extract_nip_oa_owner(&pubkey_bytes, auth_tag) - } else { - None - } - }), + // enforce_relay_membership reports a verified owner after ANY successful + // admission, so there is nothing left to patch up per-transport. + Ok(owner) => owner, Err(e) => { return SubmitOutcome::Err { status: e.0, diff --git a/crates/buzz-relay/src/api/mod.rs b/crates/buzz-relay/src/api/mod.rs index d9f829433b1..1ed58ee26cb 100644 --- a/crates/buzz-relay/src/api/mod.rs +++ b/crates/buzz-relay/src/api/mod.rs @@ -45,9 +45,14 @@ pub mod relay_members { #[derive(Debug, Clone, PartialEq, Eq)] pub enum MembershipDecision { /// Relay membership enforcement is disabled. - OpenRelay, - /// Caller is directly present in `relay_members`. - Member, + /// + /// Carries a verified NIP-OA owner when the caller presented one. The + /// owner relationship is independent of admission: it is proven by the + /// attestation's own signature, not by how the caller got in. + OpenRelay(Option), + /// Caller is directly present in `relay_members`. Carries a verified + /// NIP-OA owner when the caller presented one, for the same reason. + Member(Option), /// Caller is admitted through a NIP-OA owner that is a relay member. ViaOwner(nostr::PublicKey), /// Caller is not admitted. @@ -64,8 +69,17 @@ pub mod relay_members { pubkey_bytes: &[u8], auth_tag_header: Option<&str>, ) -> Result { + // Verify the attestation up front, independent of admission. + // + // Membership answers "may this caller connect"; the attestation answers + // "who owns this agent". Deriving the second from the first is what + // regressed: a direct member was admitted without the delegation being + // consulted, so a perfectly valid owner relationship was discarded and + // `users.agent_owner_pubkey` stayed NULL. + let verified_owner = extract_nip_oa_owner(pubkey_bytes, auth_tag_header); + if !state.config.require_relay_membership { - return Ok(MembershipDecision::OpenRelay); + return Ok(MembershipDecision::OpenRelay(verified_owner)); } let pubkey_hex = hex::encode(pubkey_bytes); @@ -75,7 +89,7 @@ pub mod relay_members { .await .map_err(|e| format!("relay membership check failed: {e}"))?; if is_member { - return Ok(MembershipDecision::Member); + return Ok(MembershipDecision::Member(verified_owner)); } if state.config.allow_nip_oa_auth { @@ -112,15 +126,18 @@ pub mod relay_members { /// Enforce relay membership for a pubkey, with NIP-OA agent delegation fallback. /// - /// Returns `Ok(Some(owner_pubkey))` when the agent is not a direct member but - /// its NIP-OA owner *is* — access is granted via delegation. + /// Returns `Ok(Some(owner_pubkey))` whenever the caller is admitted AND + /// presented a verifying NIP-OA attestation — whether it was admitted as a + /// direct relay member, through owner delegation, or on an open relay. /// - /// On open relays (`require_relay_membership = false`), returns `Ok(None)` - /// immediately — no membership check is performed. Callers that need NIP-OA - /// owner extraction on open relays should call [`extract_nip_oa_owner`] directly. + /// The owner relationship is deliberately independent of the admission path: + /// it is proven by the attestation's own signature. Reporting it only for + /// delegated admissions left `users.agent_owner_pubkey` NULL for every + /// directly-enrolled agent, which is what `is_agent`, owner-managed agent + /// lists and `channel_add_policy = "owner_only"` all derive from. /// - /// Returns `Ok(None)` when the caller is a direct member (closed relay) or when - /// no NIP-OA tag is present/applicable (open relay without auth tag). + /// Returns `Ok(None)` when the caller presented no attestation, or one that + /// does not verify against its pubkey. pub async fn enforce_relay_membership( state: &AppState, community: CommunityId, @@ -128,8 +145,11 @@ pub mod relay_members { auth_tag_header: Option<&str>, ) -> Result, (StatusCode, Json)> { match check_relay_membership(state, community, pubkey_bytes, auth_tag_header).await { - Ok(MembershipDecision::OpenRelay) | Ok(MembershipDecision::Member) => Ok(None), - Ok(MembershipDecision::ViaOwner(owner)) => Ok(Some(owner)), + Ok( + decision @ (MembershipDecision::OpenRelay(_) + | MembershipDecision::Member(_) + | MembershipDecision::ViaOwner(_)), + ) => Ok(owner_from_decision(&decision)), Ok(MembershipDecision::Denied) => Err(( StatusCode::FORBIDDEN, Json(serde_json::json!({ @@ -144,6 +164,21 @@ pub mod relay_members { } } + /// The owner relationship an admitted decision proves, if any. + /// + /// Split out from `enforce_relay_membership` so the behaviour that regressed + /// is testable without a database. Every admitted variant reports its + /// verified owner; only `Denied` has none, because a rejected caller proves + /// nothing. The bug was returning `None` for `Member`, which discarded a + /// valid attestation purely because admission had not needed it. + pub(crate) fn owner_from_decision(decision: &MembershipDecision) -> Option { + match decision { + MembershipDecision::OpenRelay(owner) | MembershipDecision::Member(owner) => *owner, + MembershipDecision::ViaOwner(owner) => Some(*owner), + MembershipDecision::Denied => None, + } + } + /// Extract NIP-OA owner from an auth tag without membership enforcement. /// /// Used on open relays (`require_relay_membership = false`) to opportunistically @@ -263,6 +298,57 @@ pub mod relay_members { assert_eq!(result, None); } + /// A direct relay member that presented a valid attestation must report + /// its owner. + /// + /// This is the regression. `Member` previously carried no owner and the + /// mapping returned `None` for it, so on a closed relay a + /// directly-enrolled agent never had `users.agent_owner_pubkey` + /// materialized — leaving it rate-limited as a human, absent from + /// owner-managed agent lists, and unaddable to channels under its own + /// `channel_add_policy = "owner_only"`. + #[test] + fn direct_member_reports_its_verified_owner() { + let owner = Keys::generate().public_key(); + + assert_eq!( + owner_from_decision(&MembershipDecision::Member(Some(owner))), + Some(owner), + "admission path must not decide whether an owner is reported" + ); + } + + /// The same owner is reported no matter which admitted path produced it. + #[test] + fn every_admitted_path_reports_the_same_owner() { + let owner = Keys::generate().public_key(); + + for decision in [ + MembershipDecision::OpenRelay(Some(owner)), + MembershipDecision::Member(Some(owner)), + MembershipDecision::ViaOwner(owner), + ] { + assert_eq!( + owner_from_decision(&decision), + Some(owner), + "{decision:?} should report the verified owner" + ); + } + } + + /// An admitted caller that presented no attestation has no owner, and a + /// denied caller proves nothing regardless. + #[test] + fn no_attestation_or_denied_reports_no_owner() { + for decision in [ + MembershipDecision::OpenRelay(None), + MembershipDecision::Member(None), + MembershipDecision::Denied, + ] { + assert_eq!(owner_from_decision(&decision), None, "{decision:?}"); + } + } + /// Invalid auth tag → returns None. #[test] fn invalid_auth_tag_returns_none() { diff --git a/crates/buzz-relay/src/handlers/auth.rs b/crates/buzz-relay/src/handlers/auth.rs index 127f1fc40e0..a408eb4126e 100644 --- a/crates/buzz-relay/src/handlers/auth.rs +++ b/crates/buzz-relay/src/handlers/auth.rs @@ -2,9 +2,9 @@ //! //! Relay membership enforcement uses the shared //! [`crate::api::relay_members::enforce_relay_membership`] helper, which supports -//! NIP-OA owner-delegation fallback on closed relays. On open relays, the auth -//! handler calls [`crate::api::relay_members::extract_nip_oa_owner`] directly to -//! extract the owner pubkey for agent→owner backfill (observer frame auth). +//! NIP-OA owner-delegation fallback on closed relays. That helper also reports +//! the verified owner after any successful admission, so the agent→owner +//! backfill needs no per-transport special case here. //! //! For WebSocket auth, the NIP-OA `auth` tag is extracted from the signed AUTH //! event itself (the tag is integrity-protected by the event signature). @@ -237,21 +237,6 @@ pub async fn handle_auth(event: nostr::Event, conn: Arc, state: } }; - // Open relay NIP-OA backfill: extract owner for agent→owner DB mapping - // (needed for observer frame auth). Only runs on open relays — on closed - // relays, enforce_relay_membership already handles NIP-OA delegation. - // No feature flag needed: NIP-OA is cryptographically self-proving. - let nip_oa_owner = nip_oa_owner.or_else(|| { - if !state.config.require_relay_membership && auth_tag_json.is_some() { - crate::api::relay_members::extract_nip_oa_owner( - pubkey.as_bytes(), - auth_tag_json.as_deref(), - ) - } else { - None - } - }); - // Stash NIP-OA owner on the auth context only after the shared // backfill confirms the first-write-wins relationship. if let Some(owner) = nip_oa_owner {