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.