From e3ff778d5b69c35918be1a1ad8817b382b4e6bd7 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sat, 27 Jun 2026 03:43:39 -0400 Subject: [PATCH] A25: surface a clock-skew warning instead of failing silently A validly-signed gossip payload rejected only by the 120s replay freshness window (GossipReject::OutOfWindow) now drives a room-level "clocks out of sync" warning banner, instead of silently dropping the peer so the room shows "1 in room" with no error. Observe-only: verify_gossip's accept/reject decision and GOSSIP_FRESHNESS_MS are unchanged; the payload is still dropped exactly as before. The warning is gated strictly on OutOfWindow (which, because the signature is verified first, implies a genuine authenticated peer whose clock is skewed), never on BadSignature. Policy lives in a pure, unit-tested ClockSkewMonitor seam with injected now_ms: >=3 OutOfWindow drops from the same author within 60s warn once, 5-min per-author cooldown, bounded/pruned author map. The warning rides the existing in-process RoomEvent -> UiEvent -> transient-banner path (no wire/serialization or dependency change). Implemented by Codex (gpt-5.5), senior-audited against the 5-point checklist and independently verified (452 lib tests, clippy --all-targets clean, release build green). Tests-green only; a 2-machine deliberate-skew field test is still owed. Co-Authored-By: Claude Opus 4.8 --- src/app/mod.rs | 179 ++++++++++++++++++++++++++++++++++-- src/core/messages.rs | 4 + src/core/mod.rs | 13 +++ src/network/gossip.rs | 207 +++++++++++++++++++++++++++++++++++++++++- src/network/mod.rs | 4 + 5 files changed, 398 insertions(+), 9 deletions(-) diff --git a/src/app/mod.rs b/src/app/mod.rs index 1a383de..87788c1 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -262,6 +262,8 @@ const ABOVE_CHAT_MIN_H: f32 = 300.0; const DIVIDER_THICKNESS: f32 = 8.0; /// Upper bound for waiting on orderly core shutdown before letting the window exit. const SHUTDOWN_TIMEOUT_SECS: u64 = 5; +/// How long a room-level clock-skew warning remains visible without dismissal. +const CLOCK_SKEW_WARNING_VISIBLE_SECS: u64 = 12; /// Clamp the Participants panel width so neither it nor the Controls panel drops /// below its minimum, given the current window width. @@ -404,6 +406,10 @@ pub enum AppMessage { /// Open / close the "screen sharing needs pixelpass" explainer popup (A11). OpenPixelpassHelp, ClosePixelpassHelp, + /// Dismiss the room-level clock-skew warning banner. + DismissClockSkewWarning, + /// Auto-clear cadence while the clock-skew warning banner is visible. + ClockSkewWarningTick, /// Choose a room layout (applied live + persisted, closes the popup). SelectRoomLayout(RoomLayout), /// Choose a UI theme (applied live + persisted). @@ -499,6 +505,13 @@ fn core_subscription() -> impl iced::futures::Stream { }) } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct ClockSkewBanner { + skew_secs: u64, + peer_ahead: bool, + expires_at: std::time::Instant, +} + pub struct AppState { name: String, ticket_input: String, @@ -604,6 +617,10 @@ pub struct AppState { /// would pass a flag an older pixelpass rejects (audit P2). Optimistic `true` /// until the core's `AudioAppsListed` reports otherwise. share_app_audio_supported: bool, + /// Room-level warning for a validly signed peer whose gossip timestamp falls + /// outside the replay freshness window. The peer is not yet in the roster, so + /// this is not attached to a participant card. + clock_skew_warning: Option, /// Whether the Chat drawer is open (drawer layout only). drawer_chat_open: bool, /// Raw mic level (normalized RMS, `0.0..=1.0`) for the settings meter. @@ -680,6 +697,7 @@ impl AppState { self.share_audio_dropped = false; self.share_audio_app_active = false; self.share_app_audio_supported = true; + self.clock_skew_warning = None; } fn custom_sound_path(&self, sound: Sound) -> &str { @@ -808,6 +826,7 @@ impl Default for AppState { share_audio_dropped: false, share_audio_app_active: false, share_app_audio_supported: true, + clock_skew_warning: None, drawer_chat_open: false, mic_level: 0.0, mic_test_active: false, @@ -952,7 +971,13 @@ fn subscription(state: &AppState) -> Subscription { } else { Subscription::none() }; - Subscription::batch(vec![core_sub, event_sub, audio_sub]) + let clock_skew_sub = if state.clock_skew_warning.is_some() { + iced::time::every(std::time::Duration::from_secs(1)) + .map(|_| AppMessage::ClockSkewWarningTick) + } else { + Subscription::none() + }; + Subscription::batch(vec![core_sub, event_sub, audio_sub, clock_skew_sub]) } fn shutdown_timeout_task() -> Task { @@ -1429,6 +1454,14 @@ fn update(state: &mut AppState, message: AppMessage) -> Task { state.share_audio_dropped = !active; } } + UiEvent::ClockSkewWarning { skew_secs, peer_ahead } => { + show_clock_skew_warning( + state, + skew_secs, + peer_ahead, + std::time::Instant::now(), + ); + } UiEvent::IdentityStatus { node_id, persisted, error } => { state.self_node_id = Some(node_id); state.identity_persisted = persisted; @@ -1793,6 +1826,12 @@ fn update(state: &mut AppState, message: AppMessage) -> Task { AppMessage::ClosePixelpassHelp => { state.pixelpass_help_open = false; } + AppMessage::DismissClockSkewWarning => { + state.clock_skew_warning = None; + } + AppMessage::ClockSkewWarningTick => { + clear_expired_clock_skew_warning(state, std::time::Instant::now()); + } AppMessage::SelectRoomLayout(layout) => { state.config.room_layout = layout; state.config.save(); @@ -2363,6 +2402,37 @@ fn format_duration(total_secs: u64) -> String { } } +fn format_clock_skew_duration(skew_secs: u64) -> String { + let minutes = skew_secs.max(1).saturating_add(59) / 60; + if minutes == 1 { + "1 minute".to_string() + } else { + format!("{minutes} minutes") + } +} + +fn show_clock_skew_warning( + state: &mut AppState, + skew_secs: u64, + peer_ahead: bool, + now: std::time::Instant, +) { + state.clock_skew_warning = Some(ClockSkewBanner { + skew_secs, + peer_ahead, + expires_at: now + std::time::Duration::from_secs(CLOCK_SKEW_WARNING_VISIBLE_SECS), + }); +} + +fn clear_expired_clock_skew_warning(state: &mut AppState, now: std::time::Instant) { + if state + .clock_skew_warning + .is_some_and(|warning| now >= warning.expires_at) + { + state.clock_skew_warning = None; + } +} + /// First 8 characters of an id string for compact display. Panic-free: takes /// chars (not a byte slice), so a short or non-ASCII id can never panic the /// render (security finding S1) — ids are long ASCII hex today, but this guards @@ -4887,8 +4957,43 @@ fn view(state: &AppState) -> Element<'_, AppMessage> { } }; + let clock_skew_banner: Element<'_, AppMessage> = + if let Some(warning) = state.clock_skew_warning { + let direction = if warning.peer_ahead { "ahead" } else { "behind" }; + let skew = format_clock_skew_duration(warning.skew_secs); + let copy = format!( + "A peer couldn't be seen - clocks are out of sync by ~{skew} (peer clock looks {direction}). Check your system clock (turn on automatic time sync)." + ); + column![ + vertical_space(10.0), + container( + row![ + icon(IconKind::Clock, 16.0, color_yellow), + text(copy).size(12).color(color_text).width(iced::Length::Fill), + button(text("Dismiss").size(12)) + .on_press(AppMessage::DismissClockSkewWarning) + .style(b_style(color_surface, color_blue, color_text, 6.0)) + .padding(6), + ] + .spacing(10) + .align_y(iced::alignment::Vertical::Center) + ) + .padding(10) + .width(iced::Length::Fill) + .style(move |_theme: &Theme| container::Style { + text_color: Some(color_text), + background: Some(Background::Color(Color { a: 0.14, ..color_yellow })), + border: Border { color: color_yellow, width: 1.0, radius: 8.0.into() }, + ..Default::default() + }) + ] + .into() + } else { + iced::widget::Space::new().width(0.0).height(0.0).into() + }; + let room = container( - column![top_bar, header_container, vertical_space(12.0), body] + column![top_bar, header_container, clock_skew_banner, vertical_space(12.0), body] ) .padding(15) .width(iced::Length::Fill) @@ -6222,10 +6327,11 @@ impl Program for Icon { #[cfg(test)] mod tests { use super::{ - attachment_default_name, format_duration, initial_window_position, reconnect_attempt_chime, - reconnected_chime, set_peer_gate_config, set_peer_volume_config, update, AppConfig, - AppMessage, AppState, AttachmentCache, AttachmentState, ChatEntry, GateMeter, METER_MAX, - UiEvent, + attachment_default_name, clear_expired_clock_skew_warning, format_clock_skew_duration, + format_duration, initial_window_position, reconnect_attempt_chime, reconnected_chime, + set_peer_gate_config, set_peer_volume_config, show_clock_skew_warning, update, AppConfig, + AppMessage, AppState, AttachmentCache, AttachmentState, ChatEntry, ClockSkewBanner, + GateMeter, METER_MAX, UiEvent, CLOCK_SKEW_WARNING_VISIBLE_SECS, }; use iroh::SecretKey; @@ -6392,6 +6498,11 @@ mod tests { state.share_audio_dropped = true; state.share_audio_app_active = true; state.share_app_audio_supported = false; + state.clock_skew_warning = Some(ClockSkewBanner { + skew_secs: 180, + peer_ahead: true, + expires_at: now, + }); state.clip_status.lock().unwrap().playing_id = Some(attachment_id); state.reset_room_state(); @@ -6419,6 +6530,7 @@ mod tests { assert!(!state.share_audio_dropped); assert!(!state.share_audio_app_active); assert!(state.share_app_audio_supported, "reset is optimistic by default"); + assert!(state.clock_skew_warning.is_none()); for _ in 0..50 { if crate::audio::clip_player::status_snapshot(&state.clip_status).playing_id.is_none() { @@ -6429,6 +6541,61 @@ mod tests { panic!("clip player did not stop during room reset"); } + #[test] + fn clock_skew_warning_shows_dismisses_and_expires() { + let mut state = AppState::default(); + let now = std::time::Instant::now(); + + show_clock_skew_warning(&mut state, 181, true, now); + let warning = state.clock_skew_warning.expect("warning should be visible"); + assert_eq!(warning.skew_secs, 181); + assert!(warning.peer_ahead); + assert_eq!( + warning.expires_at, + now + std::time::Duration::from_secs(CLOCK_SKEW_WARNING_VISIBLE_SECS) + ); + + let _ = update(&mut state, AppMessage::DismissClockSkewWarning); + assert!(state.clock_skew_warning.is_none()); + + show_clock_skew_warning(&mut state, 240, false, now); + clear_expired_clock_skew_warning( + &mut state, + now + std::time::Duration::from_secs(CLOCK_SKEW_WARNING_VISIBLE_SECS - 1), + ); + assert!(state.clock_skew_warning.is_some()); + clear_expired_clock_skew_warning( + &mut state, + now + std::time::Duration::from_secs(CLOCK_SKEW_WARNING_VISIBLE_SECS), + ); + assert!(state.clock_skew_warning.is_none()); + } + + #[test] + fn clock_skew_ui_event_populates_banner() { + let mut state = AppState::default(); + let _ = update( + &mut state, + AppMessage::UiEventReceived(UiEvent::ClockSkewWarning { + skew_secs: 121, + peer_ahead: false, + }), + ); + + let warning = state.clock_skew_warning.expect("event should show banner"); + assert_eq!(warning.skew_secs, 121); + assert!(!warning.peer_ahead); + } + + #[test] + fn clock_skew_duration_rounds_up_to_minutes() { + assert_eq!(format_clock_skew_duration(0), "1 minute"); + assert_eq!(format_clock_skew_duration(1), "1 minute"); + assert_eq!(format_clock_skew_duration(60), "1 minute"); + assert_eq!(format_clock_skew_duration(61), "2 minutes"); + assert_eq!(format_clock_skew_duration(181), "4 minutes"); + } + #[test] fn share_picker_startup_window_is_guarded() { // P3-1: between confirming the picker and the core's ScreenShareStarted, diff --git a/src/core/messages.rs b/src/core/messages.rs index 448d963..9dd0547 100644 --- a/src/core/messages.rs +++ b/src/core/messages.rs @@ -163,6 +163,10 @@ pub enum UiEvent { /// run viewers currently hear silence. The UI shows a transient warning while /// `false`. Only meaningful while sharing a specific app (not whole-desktop). ShareAudioActive(bool), + /// A validly signed peer cannot be admitted because its gossip timestamp is + /// outside the replay freshness window. `peer_ahead` describes the peer's + /// sender-stamped timestamp relative to this machine's clock. + ClockSkewWarning { skew_secs: u64, peer_ahead: bool }, /// Our node identity (W7): the current node id string, and whether it is /// PERSISTED to disk. Sent once at startup and again after a regenerate. /// `persisted = false` means the key file couldn't be read/written and we're diff --git a/src/core/mod.rs b/src/core/mod.rs index 58fa7e4..b665081 100644 --- a/src/core/mod.rs +++ b/src/core/mod.rs @@ -2090,6 +2090,19 @@ async fn run_core_loop( attachment, }).await; } + RoomEvent::ClockSkewSuspected { author, skew_ms } => { + crate::log_msg(&format!( + "Clock skew suspected for authenticated gossip author={} skew_ms={skew_ms}", + crate::short_id(&author.to_string()) + )); + let skew_secs = skew_ms.unsigned_abs().saturating_add(999) / 1000; + let _ = ui_tx_events + .send(UiEvent::ClockSkewWarning { + skew_secs, + peer_ahead: skew_ms > 0, + }) + .await; + } RoomEvent::PeerConnectionLost(peer_id) => { // Transient drop: do NOT tear down the peer. Its audio // supervisor stays alive and keeps redialing the diff --git a/src/network/gossip.rs b/src/network/gossip.rs index fb9db63..692436f 100644 --- a/src/network/gossip.rs +++ b/src/network/gossip.rs @@ -152,6 +152,100 @@ fn prune_stale_mutations( seen.retain(|_, last_ts| *last_ts >= floor); } +/// Three signed, out-of-window payloads inside one minute is enough to distinguish +/// a persistently skewed clock from a single delayed gossip frame without making +/// the user wait long. Repeats are suppressed for five minutes per author. +const CLOCK_SKEW_OBSERVATION_WINDOW_MS: u64 = 60_000; +const CLOCK_SKEW_WARNING_THRESHOLD: usize = 3; +const CLOCK_SKEW_COOLDOWN_MS: u64 = 5 * 60_000; +const CLOCK_SKEW_AUTHORS_SOFT_CAP: usize = 256; +const CLOCK_SKEW_AUTHORS_HARD_CAP: usize = 512; +const CLOCK_SKEW_AUTHOR_TTL_MS: u64 = CLOCK_SKEW_COOLDOWN_MS; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct ClockSkewWarning { + author: EndpointId, + /// Positive means the peer's sender-stamped clock is ahead of ours. + skew_ms: i64, +} + +#[derive(Debug, Default)] +struct ClockSkewMonitor { + authors: HashMap, +} + +#[derive(Debug, Default)] +struct ClockSkewAuthorState { + observed_at: Vec, + last_seen_ms: u64, + last_warned_ms: Option, +} + +impl ClockSkewMonitor { + fn observe( + &mut self, + author: EndpointId, + skew_ms: i64, + now_ms: u64, + ) -> Option { + if self.authors.len() > CLOCK_SKEW_AUTHORS_SOFT_CAP { + self.prune_stale_authors(now_ms); + } + + let warning = { + let state = self.authors.entry(author).or_default(); + state.last_seen_ms = now_ms; + let floor = now_ms.saturating_sub(CLOCK_SKEW_OBSERVATION_WINDOW_MS); + state.observed_at.retain(|ts| *ts >= floor); + state.observed_at.push(now_ms); + if state.observed_at.len() > CLOCK_SKEW_WARNING_THRESHOLD { + let excess = state.observed_at.len() - CLOCK_SKEW_WARNING_THRESHOLD; + state.observed_at.drain(0..excess); + } + + let threshold_met = state.observed_at.len() >= CLOCK_SKEW_WARNING_THRESHOLD; + let in_cooldown = state + .last_warned_ms + .is_some_and(|last| now_ms.saturating_sub(last) < CLOCK_SKEW_COOLDOWN_MS); + if threshold_met && !in_cooldown { + state.last_warned_ms = Some(now_ms); + Some(ClockSkewWarning { author, skew_ms }) + } else { + None + } + }; + + if self.authors.len() > CLOCK_SKEW_AUTHORS_HARD_CAP { + self.drop_oldest_authors(); + } + + warning + } + + fn prune_stale_authors(&mut self, now_ms: u64) { + let stale_before = now_ms.saturating_sub(CLOCK_SKEW_AUTHOR_TTL_MS); + self.authors.retain(|_, state| { + let last_warning_live = state + .last_warned_ms + .is_some_and(|last| now_ms.saturating_sub(last) < CLOCK_SKEW_COOLDOWN_MS); + last_warning_live || state.last_seen_ms >= stale_before + }); + } + + fn drop_oldest_authors(&mut self) { + let remove_count = self.authors.len().saturating_sub(CLOCK_SKEW_AUTHORS_SOFT_CAP); + let mut by_age: Vec<_> = self + .authors + .iter() + .map(|(author, state)| (*author, state.last_seen_ms)) + .collect(); + by_age.sort_by_key(|(_, last_seen_ms)| *last_seen_ms); + for (author, _) in by_age.into_iter().take(remove_count) { + self.authors.remove(&author); + } + } +} + /// Maximum number of distinct peers we hold in a room roster at once. /// /// Everyone with the room ticket is an authenticated *insider*: a signature only @@ -413,6 +507,7 @@ impl RoomState for IrohGossipState { let handle = tokio::spawn(async move { crate::log_msg(&format!("Spawned gossip topic loop for self_id={:?}", self_id)); let mut state_mutations_seen = HashMap::new(); + let mut clock_skew_monitor = ClockSkewMonitor::default(); // Broadcast initial state let initial_payload = { @@ -453,9 +548,28 @@ impl RoomState for IrohGossipState { // action: a forged/stale payload is dropped here // so it can't impersonate, evict, or poison // presence/address-book (security S2). - if let Err(reason) = - verify_gossip(&payload, &topic_bytes, now_millis(), GOSSIP_FRESHNESS_MS) - { + let received_now_ms = now_millis(); + if let Err(reason) = verify_gossip( + &payload, + &topic_bytes, + received_now_ms, + GOSSIP_FRESHNESS_MS, + ) { + if reason == GossipReject::OutOfWindow { + let skew_ms = payload.ts as i64 - received_now_ms as i64; + if let Some(warning) = clock_skew_monitor.observe( + payload.author, + skew_ms, + received_now_ms, + ) { + let _ = event_tx + .send(RoomEvent::ClockSkewSuspected { + author: warning.author, + skew_ms: warning.skew_ms, + }) + .await; + } + } crate::log_msg(&format!( "Gossip dropped unauthenticated/stale payload claiming author={:?}: {:?}", payload.author, reason @@ -950,6 +1064,93 @@ mod tests { assert!(!seen.contains_key(&(a, StateMutationKind::Announce))); } + #[test] + fn clock_skew_monitor_single_drop_does_not_warn() { + let author = fresh_id(); + let mut monitor = ClockSkewMonitor::default(); + + assert_eq!(monitor.observe(author, -121_000, 10_000), None); + } + + #[test] + fn clock_skew_monitor_three_drops_in_window_warn_once() { + let author = fresh_id(); + let mut monitor = ClockSkewMonitor::default(); + + assert_eq!(monitor.observe(author, -121_000, 10_000), None); + assert_eq!(monitor.observe(author, -122_000, 40_000), None); + assert_eq!( + monitor.observe(author, -123_000, 69_999), + Some(ClockSkewWarning { author, skew_ms: -123_000 }) + ); + assert_eq!(monitor.observe(author, -124_000, 70_000), None); + } + + #[test] + fn clock_skew_monitor_cooldown_suppresses_repeats() { + let author = fresh_id(); + let mut monitor = ClockSkewMonitor::default(); + + assert_eq!(monitor.observe(author, 121_000, 0), None); + assert_eq!(monitor.observe(author, 122_000, 10_000), None); + assert!(monitor.observe(author, 123_000, 20_000).is_some()); + + assert_eq!(monitor.observe(author, 124_000, 30_000), None); + assert_eq!(monitor.observe(author, 125_000, 310_000), None); + assert_eq!(monitor.observe(author, 126_000, 319_000), None); + assert_eq!(monitor.observe(author, 127_000, 319_999), None); + assert_eq!( + monitor.observe(author, 128_000, 320_000), + Some(ClockSkewWarning { author, skew_ms: 128_000 }) + ); + } + + #[test] + fn clock_skew_monitor_tracks_distinct_authors_independently() { + let a = fresh_id(); + let b = fresh_id(); + let mut monitor = ClockSkewMonitor::default(); + + assert_eq!(monitor.observe(a, -121_000, 0), None); + assert_eq!(monitor.observe(a, -121_000, 1_000), None); + assert_eq!(monitor.observe(b, 121_000, 0), None); + assert_eq!(monitor.observe(b, 121_000, 1_000), None); + assert_eq!( + monitor.observe(b, 121_000, 2_000), + Some(ClockSkewWarning { author: b, skew_ms: 121_000 }) + ); + assert_eq!( + monitor.observe(a, -121_000, 2_000), + Some(ClockSkewWarning { author: a, skew_ms: -121_000 }) + ); + } + + #[test] + fn clock_skew_monitor_prunes_stale_authors_when_over_cap() { + let mut monitor = ClockSkewMonitor::default(); + for _ in 0..=CLOCK_SKEW_AUTHORS_SOFT_CAP { + assert_eq!(monitor.observe(fresh_id(), -121_000, 1), None); + } + assert!(monitor.authors.len() > CLOCK_SKEW_AUTHORS_SOFT_CAP); + + let current = fresh_id(); + assert_eq!( + monitor.observe(current, -121_000, CLOCK_SKEW_AUTHOR_TTL_MS + 2), + None + ); + assert_eq!(monitor.authors.len(), 1); + assert!(monitor.authors.contains_key(¤t)); + } + + #[test] + fn clock_skew_monitor_hard_cap_bounds_fresh_author_growth() { + let mut monitor = ClockSkewMonitor::default(); + for now_ms in 0..(CLOCK_SKEW_AUTHORS_HARD_CAP as u64 + 10) { + let _ = monitor.observe(fresh_id(), -121_000, now_ms); + assert!(monitor.authors.len() <= CLOCK_SKEW_AUTHORS_HARD_CAP); + } + } + #[test] fn sanitize_endpoint_addr_caps_address_count() { use std::net::SocketAddr; diff --git a/src/network/mod.rs b/src/network/mod.rs index 28aeb10..4b2b902 100644 --- a/src/network/mod.rs +++ b/src/network/mod.rs @@ -106,6 +106,10 @@ pub enum RoomEvent { /// from `PeerLeft` precisely so a momentary `NeighborDown` can't tear down the /// reconnect path the way it used to. PeerConnectionLost(EndpointId), + /// A validly signed gossip payload was rejected only because its timestamp is + /// outside the replay-protection window. The peer is not in the roster yet, + /// so this surfaces as a room-level warning instead of a peer-card state. + ClockSkewSuspected { author: EndpointId, skew_ms: i64 }, /// A peer sent a room text-chat message. Carries the sender's id, their /// display name (embedded so it shows even without a presence entry), the /// text, and a sender-stamped millisecond timestamp.