diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e024b95..6562c98 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -72,5 +72,6 @@ jobs: - uses: Swatinem/rust-cache@d1527c8f67ef9af357290475d4af4b0fab8f12e0 # v2 - run: apple/build-bindings.sh - run: swift build --package-path apple --product Omnitun + - run: swift test --package-path apple # The bindings, without the app or a running service. - run: swift run --package-path apple Smoke diff --git a/apple/Omnitun/App.swift b/apple/Omnitun/App.swift index fa11487..0c5b8ee 100644 --- a/apple/Omnitun/App.swift +++ b/apple/Omnitun/App.swift @@ -81,6 +81,7 @@ final class AppDelegate: NSObject, NSApplicationDelegate { } statusMenu = StatusMenu(model: model) model.start() + Updates.shared.willInstall = { [weak model] in model?.holdRecord() } Updates.shared.start() for url in pendingFiles { open(url, model: model) } pendingFiles = [] @@ -131,6 +132,17 @@ enum Settings { set { defaults.set(newValue, forKey: "disconnectOnQuit") } } + static var reconnectAfterUpdate: Bool { + get { defaults.object(forKey: "reconnectAfterUpdate") as? Bool ?? true } + set { defaults.set(newValue, forKey: "reconnectAfterUpdate") } + } + + /// The VPNs up at the last look, to connect again after an update. + static var upSessions: UpSessions? { + get { defaults.data(forKey: "upSessions").flatMap { try? JSONDecoder().decode(UpSessions.self, from: $0) } } + set { defaults.set(newValue.flatMap { try? JSONEncoder().encode($0) }, forKey: "upSessions") } + } + static var notifyConnected: Bool { get { defaults.bool(forKey: "notifyConnected") } set { defaults.set(newValue, forKey: "notifyConnected") } @@ -185,6 +197,20 @@ enum Settings { @MainActor static var relaunching = false } +/// The VPNs that were up, how they were connected, and which version of +/// the app saw them up and when: only ids and options, never a secret. +struct UpSessions: Codable, Equatable { + struct Entry: Codable, Equatable { + var id: String + var direct: Bool + var split: Bool + } + + var version: String + var at: Date + var sessions: [Entry] +} + /// The routing a gateway announced, and when. struct GatewayAnnouncement: Codable, Equatable { var fullTunnel: Bool diff --git a/apple/Omnitun/Model.swift b/apple/Omnitun/Model.swift index 39af478..3bf4f09 100644 --- a/apple/Omnitun/Model.swift +++ b/apple/Omnitun/Model.swift @@ -35,6 +35,13 @@ final class AppModel: ObservableObject { func secretsDidChange() { secretsChanged += 1 } + /// The VPNs to connect again once the service answers: those up when + /// the previous version stopped for an update. + private var resuming: [UpSessions.Entry]? + /// The service is stopping or the app is being updated: the sessions + /// closing now are to come back, so what is up is no longer recorded. + private var stopping = false + private var subscription: VPNClientCore.Subscription? private var watchRetry: Task? @@ -54,6 +61,14 @@ final class AppModel: ObservableObject { func start() { Notifier.shared.setUp() reloadProfiles() + resuming = Self.toResume(Settings.upSessions, version: Self.version, now: Date()) + if resuming != nil { + // A service that never answers: nothing comes back later. + Task { + try? await Task.sleep(for: .seconds(120)) + resuming = nil + } + } watch() } @@ -121,6 +136,8 @@ final class AppModel: ObservableObject { private func watch() { watchRetry?.cancel() + // A new service: what it reports is recorded again. + stopping = false subscription = client.watch(observer: Observer(model: self)) refreshService() } @@ -145,6 +162,11 @@ final class AppModel: ObservableObject { retries[id] = nil } if case .running = service {} else { refreshService() } + if let entries = resuming { + resuming = nil + resume(entries) + } + recordUp() } fileprivate func changed(_ change: StateChange) { @@ -185,6 +207,66 @@ final class AppModel: ObservableObject { } } + // MARK: Reconnecting after an update + + private static let version = Bundle.main.object(forInfoDictionaryKey: "CFBundleShortVersionString") as? String + + /// What to connect again at launch: the VPNs a previous version saw up + /// shortly before it stopped. A crash or a quit without an update + /// connects nothing. + nonisolated static func toResume(_ up: UpSessions?, version: String?, now: Date) -> [UpSessions.Entry]? { + guard let up, let version, up.version != version, !up.sessions.isEmpty, + now.timeIntervalSince(up.at) < 15 * 60, now >= up.at else { return nil } + return up.sessions + } + + /// Records the VPNs up now, unless the service is stopping. + private func recordUp() { + guard !stopping else { return } + guard Settings.reconnectAfterUpdate, let version = Self.version else { + Settings.upSessions = nil + return + } + let up = sessions.values + .filter { [.connected, .reconnecting].contains($0.state) } + .map { session in + let options = lastOptions[session.id] + ?? (session.via == nil && !(profile(session.id)?.via.isEmpty ?? true), isSplit(session.id)) + return UpSessions.Entry(id: session.id, direct: options.direct, split: options.split) + } + .sorted { $0.id < $1.id } + let old = Settings.upSessions + guard old?.sessions != up || old?.version != version else { return } + Settings.upSessions = UpSessions(version: version, at: Date(), sessions: up) + } + + /// Keeps the record as it is while the sessions close, dated now. + func holdRecord() { + stopping = true + if var up = Settings.upSessions { + up.at = Date() + Settings.upSessions = up + } + } + + private func resume(_ entries: [UpSessions.Entry]) { + let todo = Self.resumable(entries, known: Set(profiles.map(\.id)), via: { [weak self] in self?.profile($0)?.via ?? [] }) + .filter { !isOn($0.id) } + guard !todo.isEmpty else { return } + Notifier.shared.post( + title: String(localized: "Reconnecting after the update"), + body: todo.map { displayName($0.id) }.formatted(.list(type: .and))) + for entry in todo { connect(entry.id, direct: entry.direct, split: entry.split) } + } + + /// The entries to connect, profiles that still exist only: a VPN that + /// another one goes through comes back with it. + nonisolated static func resumable(_ entries: [UpSessions.Entry], known: Set, via: (String) -> [String]) -> [UpSessions.Entry] { + let present = entries.filter { known.contains($0.id) } + let chained = Set(present.filter { !$0.direct }.flatMap { via($0.id) }) + return present.filter { !chained.contains($0.id) } + } + // MARK: Actions func toggle(_ id: String, on: Bool) { @@ -433,6 +515,10 @@ private final class Observer: SessionObserver, @unchecked Sendable { DispatchQueue.main.async { MainActor.assumeIsolated { self.model?.changed(change) } } } + func stopping() { + DispatchQueue.main.async { MainActor.assumeIsolated { self.model?.holdRecord() } } + } + func stopped(error: ClientError?) { DispatchQueue.main.async { MainActor.assumeIsolated { self.model?.stopped(error) } } } diff --git a/apple/Omnitun/SettingsView.swift b/apple/Omnitun/SettingsView.swift index 9c00145..e2fafbe 100644 --- a/apple/Omnitun/SettingsView.swift +++ b/apple/Omnitun/SettingsView.swift @@ -67,6 +67,7 @@ private struct AboutView: View { private struct GeneralSettings: View { @State private var openAtLogin = LoginItem.isEnabled @State private var disconnectOnQuit = Settings.disconnectOnQuit + @State private var reconnectAfterUpdate = Settings.reconnectAfterUpdate @State private var notifyConnected = Settings.notifyConnected @State private var notifyFailures = Settings.notifyFailures @State private var language = Settings.language @@ -84,6 +85,8 @@ private struct GeneralSettings: View { .onChange(of: disconnectOnQuit) { _, on in Settings.disconnectOnQuit = on } Text("Without the app, a code or a browser login cannot be asked again when a VPN reconnects.") .font(.caption).foregroundStyle(.secondary) + Toggle("Reconnect VPNs after an update", isOn: $reconnectAfterUpdate) + .onChange(of: reconnectAfterUpdate) { _, on in Settings.reconnectAfterUpdate = on } if Updates.shared.available { Toggle("Check for updates every day", isOn: $checkUpdates) .onChange(of: checkUpdates) { _, on in Updates.shared.checksAutomatically = on } diff --git a/apple/Omnitun/Updates.swift b/apple/Omnitun/Updates.swift index 9f7fd89..8860112 100644 --- a/apple/Omnitun/Updates.swift +++ b/apple/Omnitun/Updates.swift @@ -10,6 +10,8 @@ import Sparkle final class Updates: NSObject, SPUStandardUserDriverDelegate, SPUUpdaterDelegate { static let shared = Updates() private var controller: SPUStandardUpdaterController? + /// Called just before the installer stops the service and the app. + var willInstall: (() -> Void)? /// A build run from the package's .app: `swift run` has no bundle, no /// feed and no key to check against. @@ -45,6 +47,15 @@ final class Updates: NSObject, SPUStandardUserDriverDelegate, SPUUpdaterDelegate // active before, never to a menu bar app, and refuses it to us: the // window that then offers to install and relaunch would stay behind. // It floats above the other apps' windows until it closes. + // The VPNs up now come back after the update: quitting for it must not + // disconnect them first. + nonisolated func updater(_ updater: SPUUpdater, willInstallUpdate item: SUAppcastItem) { + MainActor.assumeIsolated { + Settings.relaunching = true + willInstall?() + } + } + nonisolated func updater(_ updater: SPUUpdater, didExtractUpdate item: SUAppcastItem) { MainActor.assumeIsolated { let sparkle = Bundle(for: SPUStandardUpdaterController.self) diff --git a/apple/Package.swift b/apple/Package.swift index aec9a42..39ae776 100644 --- a/apple/Package.swift +++ b/apple/Package.swift @@ -28,5 +28,6 @@ let package = Package( linkerSettings: [.unsafeFlags(["-Xlinker", "-rpath", "-Xlinker", "@executable_path/../Frameworks"])] ), .executableTarget(name: "Smoke", dependencies: ["VPNClientCore"], path: "Smoke"), + .testTarget(name: "OmnitunTests", dependencies: ["Omnitun"], path: "Tests", swiftSettings: [.swiftLanguageMode(.v5)]), ] ) diff --git a/apple/README.md b/apple/README.md index 1fe9833..cac52d4 100644 --- a/apple/README.md +++ b/apple/README.md @@ -27,6 +27,7 @@ override the system language. ```sh apple/build-bindings.sh # Swift bindings + universal Rust library swift run --package-path apple Smoke # checks the bindings +swift test --package-path apple # the app's unit tests swift build --package-path apple # the app alone apple/package.sh # apple/dist/Omnitun.app and Omnitun-.pkg ``` @@ -48,6 +49,14 @@ builds for another repository) and offers the new version; the package is installed only if the user accepts, with an administrator password. Menu “Check for Updates…”, setting in Settings › General. +Installing stops the service, which closes the tunnels. The app then +connects again the VPNs that were up, the same way (directly or through +their chain, in split mode or not), unless “Reconnect VPNs after an update” +is off. It keeps only their ids and options in its preferences, never a +secret; the service's `stopping` event tells it the sessions that close +next were not ended by their users. A quit or a crash without an update +connects nothing. + ```sh apple/release.sh # apple/dist/release/: notarized package, signed appcast.xml; publishes nothing apple/release.sh --publish # also creates the vVERSION Release with those two files diff --git a/apple/Resources/fr.lproj/Localizable.strings b/apple/Resources/fr.lproj/Localizable.strings index bc115a5..3d9d742 100644 --- a/apple/Resources/fr.lproj/Localizable.strings +++ b/apple/Resources/fr.lproj/Localizable.strings @@ -111,6 +111,8 @@ "General" = "Général"; "Service" = "Service"; "Disconnect VPNs when I quit %@" = "Déconnecter les VPN quand je quitte %@"; +"Reconnect VPNs after an update" = "Reconnecter les VPN après une mise à jour"; +"Reconnecting after the update" = "Reconnexion après la mise à jour"; "Without the app, a code or a browser login cannot be asked again when a VPN reconnects." = "Sans l'app, un code ou une connexion par navigateur ne peut pas être redemandé lors d'une reconnexion."; "Language" = "Langue"; "Same as macOS" = "Comme macOS"; diff --git a/apple/Tests/ReconnectTests.swift b/apple/Tests/ReconnectTests.swift new file mode 100644 index 0000000..9268d1a --- /dev/null +++ b/apple/Tests/ReconnectTests.swift @@ -0,0 +1,43 @@ +import XCTest +@testable import Omnitun + +final class ReconnectTests: XCTestCase { + private let now = Date(timeIntervalSince1970: 1_800_000_000) + private let paris = UpSessions.Entry(id: "paris", direct: false, split: true) + + private func up(_ version: String, minutesAgo: Double, _ sessions: [UpSessions.Entry]) -> UpSessions { + UpSessions(version: version, at: now.addingTimeInterval(-minutesAgo * 60), sessions: sessions) + } + + func testReconnectsAfterAnUpdate() { + XCTAssertEqual(AppModel.toResume(up("0.4.3", minutesAgo: 1, [paris]), version: "0.4.4", now: now), [paris]) + } + + func testNothingWithoutAnUpdate() { + XCTAssertNil(AppModel.toResume(up("0.4.4", minutesAgo: 1, [paris]), version: "0.4.4", now: now)) + } + + func testNothingFromLongAgo() { + XCTAssertNil(AppModel.toResume(up("0.4.3", minutesAgo: 20, [paris]), version: "0.4.4", now: now)) + XCTAssertNil(AppModel.toResume(up("0.4.3", minutesAgo: -5, [paris]), version: "0.4.4", now: now), "dated in the future") + } + + func testNothingRecorded() { + XCTAssertNil(AppModel.toResume(nil, version: "0.4.4", now: now)) + XCTAssertNil(AppModel.toResume(up("0.4.3", minutesAgo: 1, []), version: "0.4.4", now: now)) + XCTAssertNil(AppModel.toResume(up("0.4.3", minutesAgo: 1, [paris]), version: nil, now: now), "a build without a version") + } + + func testAChainBringsBackWhatItGoesThrough() { + let office = UpSessions.Entry(id: "office", direct: false, split: false) + let home = UpSessions.Entry(id: "home", direct: false, split: false) + let via = { (id: String) in id == "office" ? ["home"] : [] } + XCTAssertEqual(AppModel.resumable([home, office], known: ["home", "office"], via: via), [office]) + let alone = UpSessions.Entry(id: "office", direct: true, split: false) + XCTAssertEqual(AppModel.resumable([home, alone], known: ["home", "office"], via: via), [home, alone], "connected directly") + } + + func testDeletedProfilesAreSkipped() { + XCTAssertEqual(AppModel.resumable([paris], known: [], via: { _ in [] }), []) + } +} diff --git a/crates/daemon/src/main.rs b/crates/daemon/src/main.rs index 766fb99..ee2a381 100644 --- a/crates/daemon/src/main.rs +++ b/crates/daemon/src/main.rs @@ -181,6 +181,7 @@ async fn main() -> anyhow::Result<()> { } } tracing::info!("shutting down, closing all sessions"); + manager.stopping(); // Each engine gets a few seconds to say goodbye; past the deadline the // daemon exits anyway, well before launchd or systemd would kill it, // and the next start removes what is left. @@ -248,7 +249,7 @@ impl Drop for Slot { } /// Optional capabilities announced in `Hello`. -const FEATURES: &[&str] = &["watch", "shutdown", "trust_certificate", "failure_kinds", "prompt_socket"]; +const FEATURES: &[&str] = &["watch", "shutdown", "trust_certificate", "failure_kinds", "prompt_socket", "stopping"]; fn failed(e: anyhow::Error) -> Event { Event::Error { message: format!("{e:#}"), failure: Some(vpn_core::failure::kind_of(&e)) } diff --git a/crates/daemon/src/manager.rs b/crates/daemon/src/manager.rs index b49bf7f..c516c7a 100644 --- a/crates/daemon/src/manager.rs +++ b/crates/daemon/src/manager.rs @@ -100,7 +100,8 @@ impl Default for Manager { #[derive(Clone)] struct Watched { - owner: u32, + /// `None`: for every watcher. + owner: Option, event: Event, } @@ -261,6 +262,12 @@ impl Manager { /// Closes every session at once: the service manager kills the daemon /// if stopping takes too long, so teardowns must not queue up. + /// Tells every watcher that the sessions are about to close because the + /// daemon stops: clients bring them back after an update. + pub fn stopping(&self) { + let _ = self.events.send(Watched { owner: None, event: Event::Stopping }); + } + pub async fn disconnect_all(&self) { let tasks: Vec<_> = { let mut sessions = self.sessions.lock().unwrap(); @@ -295,7 +302,7 @@ impl Manager { _ = link.closed() => return, }; match watched { - Ok(w) if uid == 0 || w.owner == uid => { + Ok(w) if uid == 0 || w.owner.is_none_or(|owner| owner == uid) => { if !link.send(w.event) { return; } @@ -436,7 +443,7 @@ impl Runner { None => tracing::info!(session = self.profile.name, "{state:?}"), } let event = Event::State { id: self.profile.id.clone(), state, detail, failure, retry }; - let _ = self.manager.events.send(Watched { owner: self.owner, event: event.clone() }); + let _ = self.manager.events.send(Watched { owner: Some(self.owner), event: event.clone() }); self.client.notify(event); } @@ -1224,6 +1231,20 @@ mod tests { assert!(timeout(Duration::from_millis(200), theirs_rx.recv()).await.is_err(), "another user's session leaked"); } + #[tokio::test] + async fn every_watcher_hears_that_the_daemon_stops() { + let m = Arc::new(Manager::default()); + let (mine, mut mine_rx) = client(); + let (theirs, mut theirs_rx) = client(); + m.watch(1000, mine); + m.watch(1001, theirs); + assert!(matches!(next_event(&mut mine_rx).await, Event::Status { .. })); + assert!(matches!(next_event(&mut theirs_rx).await, Event::Status { .. })); + m.stopping(); + assert!(matches!(next_event(&mut mine_rx).await, Event::Stopping)); + assert!(matches!(next_event(&mut theirs_rx).await, Event::Stopping)); + } + #[test] fn the_prompt_directory_must_be_private_to_root() { use std::os::unix::fs::PermissionsExt; @@ -1596,7 +1617,7 @@ mod tests { fake_session(&m, "paris", 1000); for i in 0..WATCH_QUEUE + 10 { let event = Event::Warning { message: i.to_string() }; - let _ = m.events.send(Watched { owner: 1000, event }); + let _ = m.events.send(Watched { owner: Some(1000), event }); } let Some(Event::Status { sessions }) = timeout(Duration::from_secs(2), rx.recv()).await.unwrap() else { panic!("expected the status first") diff --git a/crates/vpn-client/src/session.rs b/crates/vpn-client/src/session.rs index c369366..13858bf 100644 --- a/crates/vpn-client/src/session.rs +++ b/crates/vpn-client/src/session.rs @@ -262,7 +262,11 @@ async fn connect_one( }); } Event::Error { message, failure } => return Err(crate::daemon::error(message, failure)), - Event::Prompt { .. } | Event::OpenUrl { .. } | Event::Status { .. } | Event::Hello { .. } => {} + Event::Prompt { .. } + | Event::OpenUrl { .. } + | Event::Status { .. } + | Event::Hello { .. } + | Event::Stopping => {} } } } diff --git a/crates/vpn-core/src/ipc.rs b/crates/vpn-core/src/ipc.rs index 8432c22..700f156 100644 --- a/crates/vpn-core/src/ipc.rs +++ b/crates/vpn-core/src/ipc.rs @@ -134,6 +134,9 @@ pub enum Event { sessions: Vec, }, Done, + /// To watchers: the daemon is stopping (an update, a restart). The + /// sessions it closes next were not ended by their users. + Stopping, Error { message: String, #[serde(default, skip_serializing_if = "Option::is_none")] diff --git a/crates/vpn-ffi/src/lib.rs b/crates/vpn-ffi/src/lib.rs index 15af53d..9c058cd 100644 --- a/crates/vpn-ffi/src/lib.rs +++ b/crates/vpn-ffi/src/lib.rs @@ -468,6 +468,10 @@ impl Client { } sessions.sort_by(|a, b| a.name.cmp(&b.name)); } + Event::Stopping => { + observer.stopping(); + continue; + } Event::Error { message, failure } => { return Err(vpn_core::failure(failure.unwrap_or(FailureKind::Other), message)); } @@ -509,6 +513,9 @@ pub trait ConnectionDelegate: Send + Sync { pub trait SessionObserver: Send + Sync { fn sessions(&self, sessions: Vec); fn changed(&self, change: StateChange); + /// The daemon is stopping (an update, a restart): the sessions that end + /// next were not ended by their users. + fn stopping(&self); /// The daemon went away or refused; subscribe again later. fn stopped(&self, error: Option); }