From 0feab42b4c9c71bf224e9c3745f91dc946927fa2 Mon Sep 17 00:00:00 2001 From: Kayshen-X Date: Sat, 1 Aug 2026 13:01:24 +0800 Subject: [PATCH] feat(collab): allow cross-account collaboration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Peer admission required the remote ticket's subject to equal the local account on both sides, so only devices of one account could pair. That made the product multi-device sync rather than collaboration. The subject equality was the authorization, so it is replaced rather than deleted. `PeerIdentityPolicy` states which accounts a peer may belong to, and the two sides get different answers because they do not have the same ability to tell who the peer is: - The owner accepting a guest admits any issued account. Nothing at this layer decides whether the guest joins — a human does, from the approval prompt, which is shown the verified identity and which the admission state machine makes unskippable (`Active` is reachable only through `OwnerAuthorized`). - A guest joining by invite or relay admits any issued account, because the invite's signed locator already pinned the owner's Noise static key and that pin is checked before admission runs. The device is authenticated whatever account is behind it, which is what makes joining a stranger's session safe. - A guest joining over an unpinned LAN discovery still requires the same account. A guest has no approval prompt — whatever it accepts, it accepts silently — and nothing else names the peer there: mDNS is spoofable and no key is known in advance. Relaxing it would let anyone on the segment holding any valid ticket pose as the owner, undetected. Opening this needs a way for the guest to confirm who it is joining, which is a user-facing decision, not a protocol change. Relaxing the account relaxes nothing else: issuer, expiry, and the binding to the observed Noise static key are unchanged, and renewal still refuses any mid-session change of issuer, subject, device id, or key. Tests cover that a foreign subject is admitted while each of those still rejects, and the unpinned-LAN case has its own regression guard. Also fixes two things this uncovered. The live MCP tests in op-host-desktop drove the endpoint with raw HTTP and no token, so they failed with 401 after bc75c765b authenticated it — they now send the instance token, which is also a live check that the authentication works. And two constant relations in the transport config were runtime asserts that clippy rejects as constant-valued; they are compile-time asserts now, which is what a relation between constants should have been. --- crates/op-collab-smoke/src/auth.rs | 4 +- crates/op-collab-transport/src/admission.rs | 41 +++++++++- .../src/admission_tests.rs | 77 ++++++++++++++++++- crates/op-collab-transport/src/config.rs | 13 ++-- .../op-collab-transport/src/driver_queue.rs | 6 +- crates/op-collab-transport/src/lib.rs | 3 +- crates/op-collab-transport/src/runtime.rs | 10 +-- .../op-collab-transport/src/runtime_tests.rs | 8 +- crates/op-collab-transport/src/tcp.rs | 8 +- crates/op-collab-transport/tests/driver.rs | 14 ++-- .../src/collab_runtime/network.rs | 1 + .../src/collab_runtime/network/connection.rs | 8 +- .../src/collab_runtime/network/guest.rs | 16 +++- .../collab_runtime/network/guest_identity.rs | 58 ++++++++++++++ .../src/collab_runtime/network/owner.rs | 19 ++--- crates/op-host-desktop/src/main_mcp_tests.rs | 21 ++--- .../p2p-collaboration-threat-model.md | 46 +++++++++-- 17 files changed, 283 insertions(+), 70 deletions(-) create mode 100644 crates/op-host-desktop/src/collab_runtime/network/guest_identity.rs diff --git a/crates/op-collab-smoke/src/auth.rs b/crates/op-collab-smoke/src/auth.rs index 9245f49c1..155d006c2 100644 --- a/crates/op-collab-smoke/src/auth.rs +++ b/crates/op-collab-smoke/src/auth.rs @@ -54,7 +54,9 @@ impl SmokeAuth { ticket.expose(), key.public_key(), TEST_COLLAB_ISSUER, - TEST_SUBJECT, + PeerIdentityPolicy::SameAccount { + subject: TEST_SUBJECT, + }, now_unix_ms, )? .to_auth_metadata(); diff --git a/crates/op-collab-transport/src/admission.rs b/crates/op-collab-transport/src/admission.rs index 758774f3a..d232b35fb 100644 --- a/crates/op-collab-transport/src/admission.rs +++ b/crates/op-collab-transport/src/admission.rs @@ -217,12 +217,45 @@ impl fmt::Debug for AdmissionIdentity { } } +/// Which accounts a peer's admission ticket may belong to. +/// +/// Independent of this, every ticket is verified against the trusted issuer, +/// checked for expiry, and bound to the Noise static key actually observed on +/// the connection. This decides only *whose* ticket is acceptable on top of +/// that. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum PeerIdentityPolicy<'a> { + /// Only this account's own devices; the subject must match exactly. + /// + /// This is the multi-device sync case, and it is the only thing + /// authenticating the peer when nothing else does — notably an + /// unpinned LAN join, where mDNS is spoofable and no key is known in + /// advance. + SameAccount { subject: &'a str }, + /// Any account the trusted issuer vouches for — cross-account + /// collaboration. + /// + /// This removes the last automatic answer to *who* the peer is, so a + /// caller may select it only when something else supplies that answer: + /// + /// * the responder (owner) gates the connection on an explicit human + /// approval that is shown the verified identity, and the admission + /// state machine makes that gate unskippable — `Active` is reachable + /// only through `OwnerAuthorized`; or + /// * the initiator (guest) has pinned the peer's Noise static key + /// out of band, from an invite's signed locator, so the account is + /// beside the point: the device itself is already authenticated. + /// + /// Selecting it without one of those is a hole, not a relaxation. + AnyIssuedAccount, +} + pub fn verify_initial_ticket( verifier: &dyn TicketVerifier, opaque_ticket: &[u8], remote_static: &[u8; 32], expected_issuer: &str, - expected_subject: &str, + identity_policy: PeerIdentityPolicy<'_>, now_unix_ms: u64, ) -> Result { let claims = verifier @@ -231,8 +264,10 @@ pub fn verify_initial_ticket( if claims.issuer != expected_issuer { return Err(AdmissionError::WrongIssuer); } - if claims.subject != expected_subject { - return Err(AdmissionError::WrongSubject); + if let PeerIdentityPolicy::SameAccount { subject } = identity_policy { + if claims.subject != subject { + return Err(AdmissionError::WrongSubject); + } } if claims.expires_at_unix_ms <= now_unix_ms { return Err(AdmissionError::Expired); diff --git a/crates/op-collab-transport/src/admission_tests.rs b/crates/op-collab-transport/src/admission_tests.rs index d703e0e25..887e1c5ab 100644 --- a/crates/op-collab-transport/src/admission_tests.rs +++ b/crates/op-collab-transport/src/admission_tests.rs @@ -78,7 +78,9 @@ fn initial_ticket_binds_identity_profile_expiry_and_remote_static() { b"ticket", &static_key, "https://issuer.example", - "00000000-0000-0000-0000-000000000001", + PeerIdentityPolicy::SameAccount { + subject: "00000000-0000-0000-0000-000000000001", + }, 1_000, ) .unwrap(); @@ -106,7 +108,9 @@ fn initial_ticket_binds_identity_profile_expiry_and_remote_static() { b"ticket", &[8_u8; 32], "https://issuer.example", - "00000000-0000-0000-0000-000000000001", + PeerIdentityPolicy::SameAccount { + subject: "00000000-0000-0000-0000-000000000001" + }, 1_000 ), Err(AdmissionError::StaticKeyMismatch) @@ -117,7 +121,7 @@ fn initial_ticket_binds_identity_profile_expiry_and_remote_static() { b"ticket", &static_key, "https://issuer.example", - "other", + PeerIdentityPolicy::SameAccount { subject: "other" }, 1_000 ), Err(AdmissionError::WrongSubject) @@ -160,7 +164,9 @@ fn renewal_requires_same_identity_and_strictly_later_expiry() { b"ticket", &static_key, "https://issuer.example", - "00000000-0000-0000-0000-000000000001", + PeerIdentityPolicy::SameAccount { + subject: "00000000-0000-0000-0000-000000000001", + }, 1_000, ) .unwrap(); @@ -203,3 +209,66 @@ fn ticket_bounds_are_enforced_before_encoding() { assert!(AdmissionHello::new(Vec::new(), JoinIntent::New).is_err()); assert!(AdmissionHello::new(vec![0; MAX_TICKET_BYTES + 1], JoinIntent::New).is_err()); } + +#[test] +fn any_issued_account_admits_a_foreign_subject_but_keeps_every_other_check() { + let static_key = [7_u8; 32]; + let verifier = FixtureVerifier { + claims: claims_with_profile(static_key, 2_000), + fail: false, + }; + + // Cross-account collaboration: the subject belongs to somebody else and is + // admitted. The caller is responsible for the human approval or the pinned + // key that decides whether this peer actually joins. + let identity = verify_initial_ticket( + &verifier, + b"ticket", + &static_key, + "https://issuer.example", + PeerIdentityPolicy::AnyIssuedAccount, + 1_000, + ) + .unwrap(); + assert_eq!( + identity.claims().subject(), + "00000000-0000-0000-0000-000000000001" + ); + + // Relaxing the account must not relax anything else: a foreign issuer, an + // expired ticket, and a ticket bound to a different Noise static key are + // all still refused. + assert!(matches!( + verify_initial_ticket( + &verifier, + b"ticket", + &static_key, + "https://attacker.example", + PeerIdentityPolicy::AnyIssuedAccount, + 1_000 + ), + Err(AdmissionError::WrongIssuer) + )); + assert!(matches!( + verify_initial_ticket( + &verifier, + b"ticket", + &static_key, + "https://issuer.example", + PeerIdentityPolicy::AnyIssuedAccount, + 3_000 + ), + Err(AdmissionError::Expired) + )); + assert!(matches!( + verify_initial_ticket( + &verifier, + b"ticket", + &[8_u8; 32], + "https://issuer.example", + PeerIdentityPolicy::AnyIssuedAccount, + 1_000 + ), + Err(AdmissionError::StaticKeyMismatch) + )); +} diff --git a/crates/op-collab-transport/src/config.rs b/crates/op-collab-transport/src/config.rs index 9516aa70c..5d8e61afe 100644 --- a/crates/op-collab-transport/src/config.rs +++ b/crates/op-collab-transport/src/config.rs @@ -57,6 +57,13 @@ pub const DEFAULT_GLOBAL_QUEUED_BYTES: usize = 256 * 1024 * 1024; /// transport-owned inbound heap to half the outbound aggregate /// (`DEFAULT_GLOBAL_QUEUED_BYTES`). pub const DEFAULT_GLOBAL_INBOUND_REASSEMBLY_BYTES: usize = 128 * 1024 * 1024; +// The aggregate has to admit one full owner snapshot or a legitimate first +// sync would be refused by its own budget, and it must stay under the outbound +// aggregate so inbound buffering cannot become the larger of the two. Both are +// relations between constants, so they belong at compile time — a runtime test +// would only assert what the compiler already knows. +const _: () = assert!(DEFAULT_GLOBAL_INBOUND_REASSEMBLY_BYTES >= MAX_SNAPSHOT_TRANSFER_BYTES); +const _: () = assert!(DEFAULT_GLOBAL_INBOUND_REASSEMBLY_BYTES <= DEFAULT_GLOBAL_QUEUED_BYTES); pub const MAX_CONFIGURED_CONNECTIONS: usize = 256; pub const MAX_CONFIGURED_QUEUE_ITEMS: usize = 1_024; pub const MAX_CONFIGURED_QUEUE_BYTES: usize = 512 * 1024 * 1024; @@ -331,12 +338,6 @@ mod tests { assert!(connections.max_pending_handshakes <= MAX_CONFIGURED_CONNECTIONS); } - #[test] - fn inbound_reassembly_aggregate_admits_a_full_snapshot() { - assert!(DEFAULT_GLOBAL_INBOUND_REASSEMBLY_BYTES >= MAX_SNAPSHOT_TRANSFER_BYTES); - assert!(DEFAULT_GLOBAL_INBOUND_REASSEMBLY_BYTES <= DEFAULT_GLOBAL_QUEUED_BYTES); - } - #[test] fn invalid_resource_limits_fail_closed() { let mut config = TransportConfig::default(); diff --git a/crates/op-collab-transport/src/driver_queue.rs b/crates/op-collab-transport/src/driver_queue.rs index e9b338c84..cdcdd870a 100644 --- a/crates/op-collab-transport/src/driver_queue.rs +++ b/crates/op-collab-transport/src/driver_queue.rs @@ -6,7 +6,7 @@ use super::ConnectionDriver; use crate::queue::QueueItem; use crate::{ verify_initial_ticket, AdmissionError, AdmissionHello, AdmissionIdentity, AdmissionPhase, - EncodedFrameTransfer, QueueError, RuntimeError, TicketVerifier, + EncodedFrameTransfer, PeerIdentityPolicy, QueueError, RuntimeError, TicketVerifier, }; impl ConnectionDriver { @@ -37,7 +37,7 @@ impl ConnectionDriver { hello: &AdmissionHello, verifier: &dyn TicketVerifier, expected_issuer: &str, - expected_subject: &str, + identity_policy: PeerIdentityPolicy<'_>, now_unix_ms: u64, now: Instant, ) -> Result { @@ -51,7 +51,7 @@ impl ConnectionDriver { hello.ticket(), self.connection.remote_static(), expected_issuer, - expected_subject, + identity_policy, now_unix_ms, ) .inspect_err(|_| self.connection.failed = true)?; diff --git a/crates/op-collab-transport/src/lib.rs b/crates/op-collab-transport/src/lib.rs index 5e0c9c682..30a90caef 100644 --- a/crates/op-collab-transport/src/lib.rs +++ b/crates/op-collab-transport/src/lib.rs @@ -33,7 +33,8 @@ mod windows_key_store; pub use admission::{ verify_initial_ticket, verify_renewal_ticket, AdmissionHello, AdmissionIdentity, - AdmissionPhase, AdmissionState, JoinIntent, ResumeHint, TicketVerifier, VerifiedTicketClaims, + AdmissionPhase, AdmissionState, JoinIntent, PeerIdentityPolicy, ResumeHint, TicketVerifier, + VerifiedTicketClaims, }; pub use chunk::{ ChunkHeader, CompletedTransfer, Reassembler, TransferChunk, TransferChunkIter, TransferClass, diff --git a/crates/op-collab-transport/src/runtime.rs b/crates/op-collab-transport/src/runtime.rs index 1b443f930..ab1a66b8c 100644 --- a/crates/op-collab-transport/src/runtime.rs +++ b/crates/op-collab-transport/src/runtime.rs @@ -9,7 +9,7 @@ use crate::record::read_ciphertext_record_until; use crate::{ decode_frame_transfer, encrypt_record, verify_initial_ticket, write_ciphertext_record, AdmissionError, AdmissionHello, AdmissionIdentity, AdmissionPhase, AdmissionState, ChunkHeader, - EncodedFrameTransfer, NoiseSession, Reassembler, RecordError, RuntimeError, + EncodedFrameTransfer, NoiseSession, PeerIdentityPolicy, Reassembler, RecordError, RuntimeError, SharedReassemblyBudget, TicketVerifier, TokenBucket, TransferChunkIter, TransferClass, TransportConfig, CHUNK_HEADER_BYTES, TRANSPORT_HEARTBEAT_PLAINTEXT, }; @@ -402,7 +402,7 @@ impl SecureConnection { local: &AdmissionHello, verifier: &dyn TicketVerifier, expected_issuer: &str, - expected_subject: &str, + identity_policy: PeerIdentityPolicy<'_>, now_unix_ms: u64, now: Instant, ) -> Result<(AdmissionHello, AdmissionIdentity), RuntimeError> { @@ -413,7 +413,7 @@ impl SecureConnection { remote.ticket(), self.remote_static(), expected_issuer, - expected_subject, + identity_policy, now_unix_ms, ) .inspect_err(|_| { @@ -429,7 +429,7 @@ impl SecureConnection { local: &AdmissionHello, verifier: &dyn TicketVerifier, expected_issuer: &str, - expected_subject: &str, + identity_policy: PeerIdentityPolicy<'_>, now_unix_ms: u64, now: Instant, ) -> Result<(AdmissionHello, AdmissionIdentity), RuntimeError> { @@ -439,7 +439,7 @@ impl SecureConnection { remote.ticket(), self.remote_static(), expected_issuer, - expected_subject, + identity_policy, now_unix_ms, ) .inspect_err(|_| { diff --git a/crates/op-collab-transport/src/runtime_tests.rs b/crates/op-collab-transport/src/runtime_tests.rs index af090160c..96afe090a 100644 --- a/crates/op-collab-transport/src/runtime_tests.rs +++ b/crates/op-collab-transport/src/runtime_tests.rs @@ -33,7 +33,9 @@ fn ticket_deadlines_are_monotonic_and_use_eighty_percent_for_renewal() { b"ticket", &[7; 32], "https://issuer.example", - "00000000-0000-0000-0000-000000000001", + PeerIdentityPolicy::SameAccount { + subject: "00000000-0000-0000-0000-000000000001", + }, 1_000, ) .unwrap(); @@ -135,7 +137,7 @@ mod heartbeat_idle { &hello, &verifier(owner_public, guest_public), ISSUER, - SUBJECT, + PeerIdentityPolicy::SameAccount { subject: SUBJECT }, NOW_UNIX_MS, Instant::now(), ) @@ -168,7 +170,7 @@ mod heartbeat_idle { &hello, &verifier(owner_public, guest_public), ISSUER, - SUBJECT, + PeerIdentityPolicy::SameAccount { subject: SUBJECT }, NOW_UNIX_MS, Instant::now(), ) diff --git a/crates/op-collab-transport/src/tcp.rs b/crates/op-collab-transport/src/tcp.rs index f8af2ecb4..49a4c8e89 100644 --- a/crates/op-collab-transport/src/tcp.rs +++ b/crates/op-collab-transport/src/tcp.rs @@ -424,8 +424,8 @@ impl Write for DeadlineTcp<'_> { mod tests { use super::*; use crate::{ - read_ciphertext_record, AdmissionHello, AdmissionPhase, JoinIntent, TicketVerifier, - TransferClass, VerifiedTicketClaims, + read_ciphertext_record, AdmissionHello, AdmissionPhase, JoinIntent, PeerIdentityPolicy, + TicketVerifier, TransferClass, VerifiedTicketClaims, }; use op_collab::{Bye, ByeReason, CollabMessage, Epoch, FrameEnvelope, Role, SessionId}; use std::net::TcpListener; @@ -492,7 +492,7 @@ mod tests { &local, &verifier(owner_public, guest_public), ISSUER, - SUBJECT, + PeerIdentityPolicy::SameAccount { subject: SUBJECT }, NOW_UNIX_MS, Instant::now(), ) @@ -523,7 +523,7 @@ mod tests { &local, &verifier(owner_public, guest_public), ISSUER, - SUBJECT, + PeerIdentityPolicy::SameAccount { subject: SUBJECT }, NOW_UNIX_MS, client_now, ) diff --git a/crates/op-collab-transport/tests/driver.rs b/crates/op-collab-transport/tests/driver.rs index 60e453499..d9e54b6ee 100644 --- a/crates/op-collab-transport/tests/driver.rs +++ b/crates/op-collab-transport/tests/driver.rs @@ -8,9 +8,9 @@ use op_collab::{ }; use op_collab_transport::{ accept_secure_tcp, connect_secure_tcp, AdmissionHello, ConnectionDriver, DeviceStaticKey, - DriverEvent, EncodedFrameTransfer, InboundTransferPolicy, JoinIntent, RuntimeError, - SecureConnection, ServerPrelude, SharedQueueBudget, TicketVerifier, TransportConfig, - VerifiedTicketClaims, + DriverEvent, EncodedFrameTransfer, InboundTransferPolicy, JoinIntent, PeerIdentityPolicy, + RuntimeError, SecureConnection, ServerPrelude, SharedQueueBudget, TicketVerifier, + TransportConfig, VerifiedTicketClaims, }; const ISSUER: &str = "https://issuer.example"; @@ -83,7 +83,7 @@ fn admitted_pair_with_expiry( &local, &verifier_with_expiry(owner_public, guest_public, expires_at_unix_ms), ISSUER, - SUBJECT, + PeerIdentityPolicy::SameAccount { subject: SUBJECT }, NOW_UNIX_MS, Instant::now(), ) @@ -106,7 +106,7 @@ fn admitted_pair_with_expiry( &local, &verifier_with_expiry(owner_public, guest_public, expires_at_unix_ms), ISSUER, - SUBJECT, + PeerIdentityPolicy::SameAccount { subject: SUBJECT }, NOW_UNIX_MS, Instant::now(), ) @@ -240,7 +240,7 @@ fn nonblocking_driver_completes_mutual_admission_without_blocking_reads() { &received_guest, &verifier(owner_public, guest_public), ISSUER, - SUBJECT, + PeerIdentityPolicy::SameAccount { subject: SUBJECT }, NOW_UNIX_MS, now, ) @@ -257,7 +257,7 @@ fn nonblocking_driver_completes_mutual_admission_without_blocking_reads() { &received_owner, &verifier(owner_public, guest_public), ISSUER, - SUBJECT, + PeerIdentityPolicy::SameAccount { subject: SUBJECT }, NOW_UNIX_MS, now, ) diff --git a/crates/op-host-desktop/src/collab_runtime/network.rs b/crates/op-host-desktop/src/collab_runtime/network.rs index d5614beb8..26736a776 100644 --- a/crates/op-host-desktop/src/collab_runtime/network.rs +++ b/crates/op-host-desktop/src/collab_runtime/network.rs @@ -3,6 +3,7 @@ mod connection; mod discovery; mod guest; +mod guest_identity; mod lifecycle; mod owner; mod owner_lifecycle; diff --git a/crates/op-host-desktop/src/collab_runtime/network/connection.rs b/crates/op-host-desktop/src/collab_runtime/network/connection.rs index 34ff8df89..a8d6c6dfe 100644 --- a/crates/op-host-desktop/src/collab_runtime/network/connection.rs +++ b/crates/op-host-desktop/src/collab_runtime/network/connection.rs @@ -457,8 +457,8 @@ mod tests { }; use op_collab_transport::{ accept_secure_tcp, connect_secure_tcp, AdmissionHello, AdmissionPhase, ConnectionDriver, - DeviceStaticKey, DriverEvent, InboundTransferPolicy, JoinIntent, ServerPrelude, - SharedQueueBudget, TransportConfig, VerifiedTicketClaims, + DeviceStaticKey, DriverEvent, InboundTransferPolicy, JoinIntent, PeerIdentityPolicy, + ServerPrelude, SharedQueueBudget, TransportConfig, VerifiedTicketClaims, }; use super::super::shutdown::{retirement_ready, TerminalDrain}; @@ -528,7 +528,7 @@ mod tests { &local, &initial_verifier(owner_static, guest_static, expires_at_unix_ms), ISSUER, - SUBJECT, + PeerIdentityPolicy::SameAccount { subject: SUBJECT }, NOW_UNIX_MS, Instant::now(), ) @@ -552,7 +552,7 @@ mod tests { &local, &initial_verifier(owner_static, guest_static, expires_at_unix_ms), ISSUER, - SUBJECT, + PeerIdentityPolicy::SameAccount { subject: SUBJECT }, NOW_UNIX_MS, Instant::now(), ) diff --git a/crates/op-host-desktop/src/collab_runtime/network/guest.rs b/crates/op-host-desktop/src/collab_runtime/network/guest.rs index 28d970088..a8a91411e 100644 --- a/crates/op-host-desktop/src/collab_runtime/network/guest.rs +++ b/crates/op-host-desktop/src/collab_runtime/network/guest.rs @@ -21,6 +21,7 @@ use super::super::types::{ use super::connection::{ drive_guest, runtime_failure, DriverControl, DriverIdentity, GuestRenewalContext, }; +use super::guest_identity::guest_identity_policy; use super::EventSink; pub(super) struct GuestTarget { @@ -295,12 +296,21 @@ fn run_inner( ) }; let now_unix_ms = unix_time_ms().map_err(|error| error.failure)?; + // The guest has no approval prompt — whatever it accepts here, it accepts + // silently — so the account check may only be dropped when the owner's + // device key was already pinned out of band and checked above. An invite + // carries that pin in its signed locator, which is what makes joining a + // stranger's session safe: the device is authenticated regardless of the + // account behind it. An unpinned LAN join has no such anchor — mDNS is + // spoofable and nothing else names the peer — so there the subject stays + // the authentication, and only this account's own devices are accepted. + let identity_policy = guest_identity_policy(expected_remote_static.as_ref(), &expected_subject); connection .exchange_admission_initiator( &hello, verifier.as_ref(), &expected_issuer, - &expected_subject, + identity_policy, now_unix_ms, Instant::now(), ) @@ -463,7 +473,7 @@ mod tests { use super::*; use op_collab::{Epoch, SessionId}; use op_collab_transport::{ - accept_secure_tcp, write_server_prelude, AdmissionHello, ServerPrelude, + accept_secure_tcp, write_server_prelude, AdmissionHello, PeerIdentityPolicy, ServerPrelude, }; use std::io::{ErrorKind, Read}; use std::net::{IpAddr, Ipv4Addr, TcpListener, TcpStream}; @@ -709,7 +719,7 @@ mod tests { &hello, &verifier, "issuer", - "subject", + PeerIdentityPolicy::SameAccount { subject: "subject" }, 1, Instant::now(), ); diff --git a/crates/op-host-desktop/src/collab_runtime/network/guest_identity.rs b/crates/op-host-desktop/src/collab_runtime/network/guest_identity.rs new file mode 100644 index 000000000..1c82c747c --- /dev/null +++ b/crates/op-host-desktop/src/collab_runtime/network/guest_identity.rs @@ -0,0 +1,58 @@ +//! Which accounts a guest may accept as the owner of a session it joins. + +use op_collab_transport::PeerIdentityPolicy; + +/// Deliberately asymmetric with the owner's side, which admits any issued +/// account because a human approves each guest against the verified identity. +/// A guest has no such prompt — whatever it accepts, it accepts silently — so +/// the account check may only be dropped when the owner's device key was +/// already pinned out of band, which an invite supplies through its signed +/// locator. An unpinned LAN join has no anchor at all: mDNS is spoofable and +/// nothing else names the peer, so there the subject *is* the authentication +/// and only this account's own devices are accepted. +pub(super) fn guest_identity_policy<'a>( + pinned_remote_static: Option<&[u8; 32]>, + local_subject: &'a str, +) -> PeerIdentityPolicy<'a> { + if pinned_remote_static.is_some() { + PeerIdentityPolicy::AnyIssuedAccount + } else { + PeerIdentityPolicy::SameAccount { + subject: local_subject, + } + } +} + +#[cfg(test)] +mod tests { + use super::guest_identity_policy; + use op_collab_transport::PeerIdentityPolicy; + + const LOCAL_SUBJECT: &str = "00000000-0000-0000-0000-000000000001"; + + #[test] + fn a_pinned_owner_key_admits_any_account() { + // An invite pins the owner's Noise static in its signed locator, and + // that pin is checked before admission runs. The device is therefore + // already authenticated and the account behind it is irrelevant, which + // is what makes cross-account collaboration by invite safe. + assert_eq!( + guest_identity_policy(Some(&[9_u8; 32]), LOCAL_SUBJECT), + PeerIdentityPolicy::AnyIssuedAccount + ); + } + + #[test] + fn an_unpinned_join_still_requires_this_account() { + // Regression guard. A guest has no approval prompt, so with no pinned + // key the subject is the only thing authenticating the owner. Relaxing + // this would let anyone on the LAN holding any valid ticket pose as the + // owner, silently. + assert_eq!( + guest_identity_policy(None, LOCAL_SUBJECT), + PeerIdentityPolicy::SameAccount { + subject: LOCAL_SUBJECT + } + ); + } +} diff --git a/crates/op-host-desktop/src/collab_runtime/network/owner.rs b/crates/op-host-desktop/src/collab_runtime/network/owner.rs index d5fc2fe27..05514294c 100644 --- a/crates/op-host-desktop/src/collab_runtime/network/owner.rs +++ b/crates/op-host-desktop/src/collab_runtime/network/owner.rs @@ -8,8 +8,8 @@ use std::time::{Duration, Instant}; use op_collab::{ByeReason, ConnectionKey, Epoch, SessionId}; use op_collab_transport::{ accept_secure_tcp_guarded, ConnectionLimiter, DeviceStaticKey, DiscoveryError, - DiscoveryPublisher, JoinIntent, PendingHandshakeGuard, ServerPrelude, SharedQueueBudget, - StaticKeyStore, TransportConfig, + DiscoveryPublisher, JoinIntent, PeerIdentityPolicy, PendingHandshakeGuard, ServerPrelude, + SharedQueueBudget, StaticKeyStore, TransportConfig, }; use socket2::{Domain, Protocol, Socket, Type}; @@ -433,7 +433,7 @@ fn run_peer_inner(args: PeerArgs) -> Option { Ok(connection) => connection, Err(error) => return Some(runtime_failure(&error)), }; - let (hello, expected_issuer, expected_subject) = { + let (hello, expected_issuer) = { let local = match local.read() { Ok(local) => local, Err(_) => return Some(CollabRuntimeFailure::AuthenticationUnavailable), @@ -442,21 +442,22 @@ fn run_peer_inner(args: PeerArgs) -> Option { Ok(hello) => hello, Err(error) => return Some(error.failure), }; - ( - hello, - local.expected_issuer().to_owned(), - local.expected_subject().to_owned(), - ) + (hello, local.expected_issuer().to_owned()) }; let now_unix_ms = match unix_time_ms() { Ok(now) => now, Err(error) => return Some(error.failure), }; + // Cross-account collaboration: a guest from any account the trusted issuer + // vouches for may reach this point. Nothing here decides whether it joins — + // the owner does, from the approval prompt below, which is shown the + // verified identity and which the admission state machine makes + // unskippable (`Active` is reachable only through `OwnerAuthorized`). let (remote, identity) = match connection.exchange_admission_responder( &hello, verifier.as_ref(), &expected_issuer, - &expected_subject, + PeerIdentityPolicy::AnyIssuedAccount, now_unix_ms, Instant::now(), ) { diff --git a/crates/op-host-desktop/src/main_mcp_tests.rs b/crates/op-host-desktop/src/main_mcp_tests.rs index 771ae0d17..284bd87ce 100644 --- a/crates/op-host-desktop/src/main_mcp_tests.rs +++ b/crates/op-host-desktop/src/main_mcp_tests.rs @@ -31,10 +31,10 @@ fn live_mcp_http_server_applies_write_requests_to_editor_state() { panic!("could not start MCP server on an unused port after 20 attempts"); } - fn post_json(port: u16, body: &str) -> String { + fn post_json(port: u16, token: &str, body: &str) -> String { let mut stream = TcpStream::connect(("127.0.0.1", port)).expect("connect MCP server"); let req = format!( - "POST /mcp HTTP/1.1\r\nHost: 127.0.0.1\r\nContent-Length: {}\r\n\r\n{}", + "POST /mcp HTTP/1.1\r\nHost: 127.0.0.1\r\nX-OpenPencil-Token: {token}\r\nContent-Length: {}\r\n\r\n{}", body.len(), body ); @@ -48,8 +48,9 @@ fn live_mcp_http_server_applies_write_requests_to_editor_state() { let mut state = op_editor_core::EditorState::new(); let body = r##"{"jsonrpc":"2.0","id":1,"method":"insert_node","params":{"kind":"rect","name":"From MCP","x":"10","y":"20","width":"100","height":"50","fill_hex":"#00ff00"}}"##; let (tx, rx) = mpsc::channel(); + let token = server.token().to_owned(); std::thread::spawn(move || { - let _ = tx.send(post_json(port, body)); + let _ = tx.send(post_json(port, &token, body)); }); let started = Instant::now(); @@ -167,10 +168,10 @@ fn live_mcp_http_server_routes_file_path_requests_to_target_file() { panic!("could not start MCP server on an unused port after 20 attempts"); } - fn post_json(port: u16, body: &str) -> String { + fn post_json(port: u16, token: &str, body: &str) -> String { let mut stream = TcpStream::connect(("127.0.0.1", port)).expect("connect MCP server"); let req = format!( - "POST /mcp HTTP/1.1\r\nHost: 127.0.0.1\r\nContent-Length: {}\r\n\r\n{}", + "POST /mcp HTTP/1.1\r\nHost: 127.0.0.1\r\nX-OpenPencil-Token: {token}\r\nContent-Length: {}\r\n\r\n{}", body.len(), body ); @@ -219,8 +220,9 @@ fn live_mcp_http_server_routes_file_path_requests_to_target_file() { r#"{{"jsonrpc":"2.0","id":31,"method":"tools/call","params":{{"name":"batch_get","arguments":{{"filePath":{file_path_json},"readDepth":1}}}}}}"# ); let (tx, rx) = mpsc::channel(); + let token = server.token().to_owned(); std::thread::spawn(move || { - let _ = tx.send(post_json(port, &body)); + let _ = tx.send(post_json(port, &token, &body)); }); let started = Instant::now(); @@ -262,10 +264,10 @@ fn live_mcp_http_server_replaces_document_via_rest_document_sync() { panic!("could not start MCP server on an unused port after 20 attempts"); } - fn post(port: u16, path: &str, body: &str) -> String { + fn post(port: u16, token: &str, path: &str, body: &str) -> String { let mut stream = TcpStream::connect(("127.0.0.1", port)).expect("connect MCP server"); let req = format!( - "POST {path} HTTP/1.1\r\nHost: 127.0.0.1\r\nContent-Length: {}\r\n\r\n{}", + "POST {path} HTTP/1.1\r\nHost: 127.0.0.1\r\nX-OpenPencil-Token: {token}\r\nContent-Length: {}\r\n\r\n{}", body.len(), body ); @@ -284,8 +286,9 @@ fn live_mcp_http_server_replaces_document_via_rest_document_sync() { // `/api/mcp/document` — the same REST shape `document.post.ts` serves. let body = r##"{"document":{"version":"1.0.0","children":[{"id":"n9","type":"rectangle","name":"Synced Rect","x":5,"y":6,"width":80,"height":40,"fill":[{"type":"solid","color":"#123456"}]}]},"sourceClientId":"ts-app"}"##; let (tx, rx) = mpsc::channel(); + let token = server.token().to_owned(); std::thread::spawn(move || { - let _ = tx.send(post(port, "/api/mcp/document", body)); + let _ = tx.send(post(port, &token, "/api/mcp/document", body)); }); let started = Instant::now(); diff --git a/docs/security/p2p-collaboration-threat-model.md b/docs/security/p2p-collaboration-threat-model.md index ed8175489..0f980f31c 100644 --- a/docs/security/p2p-collaboration-threat-model.md +++ b/docs/security/p2p-collaboration-threat-model.md @@ -301,14 +301,11 @@ than "routing metadata" in the narrow sense, and is stated here explicitly: - **The full admission ticket**, presented as the WSS `Authorization: Bearer` credential and verified by the relay. Its claims carry the global account subject, the device id, and — when present — the display name and avatar - URL. Scope this precisely: peer admission today requires the remote ticket's - subject to equal the *local* account (`expected_subject` is the local - account on both sides, and a mismatch is rejected as `WrongSubject`), so the - product currently pairs only devices of the same account. What a relay - operator reconstructs is therefore **which devices of a given account sync, - from where, and when** — not a cross-account collaboration graph. That graph - becomes possible the moment cross-account collaboration ships, which is why - the credential is worth minimizing before then. + URL. Cross-account collaboration is supported, so a relay operator + reconstructs **which accounts collaborate with each other, from which + devices, and when** — a social graph, not merely one account's device fleet. + This is the strongest argument for minimizing the credential, and it is why + the minimization is tracked as open work below rather than as a nicety. Note also that the relay reads exactly one field out of this ticket, the expiry it clamps the session deadline to. The subject, device id, `jti`, @@ -466,6 +463,39 @@ different region. If an overseas client cannot reach the CN entry for a CN-home invite, it closes or reports relay unavailable; it does not mix Global endpoint/key material or silently create a Global-home session. +### Which accounts may pair + +Collaboration is cross-account: a peer holding a valid ticket from the trusted +issuer may pair with a peer of any other account. The account is therefore no +longer the authorization, and what replaces it is deliberately **asymmetric**, +because the two sides do not have the same ability to answer "who is this?". + +- **The owner accepting a guest** admits any issued account. Nothing at this + layer decides whether the guest joins — a human does, from the approval + prompt, which is shown the verified identity, and which the admission state + machine makes unskippable (`Active` is reachable only through + `OwnerAuthorized`). +- **A guest joining by invite or relay** admits any issued account, because + the invite's signed locator has already pinned the owner's Noise static key + and that pin is checked before admission runs. The device is authenticated + regardless of the account behind it, which is precisely what makes joining a + stranger's session safe. +- **A guest joining over an unpinned LAN discovery** still requires the same + account. A guest has no approval prompt — whatever it accepts, it accepts + silently — and on an unpinned LAN join nothing else names the peer: mDNS is + spoofable and no key is known in advance. There the subject *is* the + authentication, so relaxing it would let anyone on the segment holding any + valid ticket pose as the owner, undetected. + +Relaxing the account relaxes nothing else: the issuer must still be the pinned +one, the ticket must be unexpired, and it must be bound to the Noise static key +actually observed on the connection. Renewal continuity is also unchanged — a +session's issuer, subject, device id, and static key may not change mid-session. + +Residual risk: the unpinned LAN path remains same-account only. Opening it needs +a way for the guest to confirm who it is joining, which is a user-facing +decision rather than a protocol change. + ### Unauthorized edits and identity injection Verified identity metadata is constructed only by the admission boundary.