Merge pull request #481 from Sovereign-Engineering/florian/obs-1505-simplify-tunnel-deactivation-race-protection

simplify tunnel activation race protection

Author: FlorianUekermann

apple/client/OsStatus.swift

@@ -14,17 +14,12 @@ class OsStatus: Encodable {
     var updaterStatus = UpdaterStatus()
     var debugBundleStatus = DebugBundleStatus()
 
-    init(
-        strictLeakPrevention: Bool,
-        osVpnStatus: NEVPNStatus,
-    ) {
+    init(strictLeakPrevention: Bool, osVpnStatus: NEVPNStatus) {
         self.osVpnStatus = osVpnStatus
         self.strictLeakPrevention = strictLeakPrevention
     }
 
-    static func watchable(
-        manager: NEVPNManager,
-    ) -> WatchableValue<OsStatus> {
+    static func watchable(manager: NEVPNManager) -> WatchableValue<OsStatus> {
         var lastIncludeAllNetworks = switch manager.protocolConfiguration {
         case let .some(proto): proto.includeAllNetworks
         case nil: false // Report safe default.

apple/shared/NetworkExtensionIpc.swift

@@ -15,7 +15,7 @@ enum NeManagerCmd: Codable {
     case getStatus(knownVersion: UUID?)
     case getTrafficStats
     case ping
-    case setTunnelArgs(args: TunnelArgs?)
+    case setTunnelArgs(args: TunnelArgs?, allowActivation: Bool = false)
 }
 
 struct TunnelArgs: Codable {

apple/system-network-extension/PacketTunnelProvider.swift

@@ -78,7 +78,7 @@ class PacketTunnelProvider: NEPacketTunnelProvider {
 
             let networkConfig = NetworkConfig(ipv4: "10.75.76.77", dns: ["10.64.0.99"], ipv6: "fc00:bbbb:bbbb:bb01::c:4c4d/128")
             try await self.setTunnelNetworkSettings(NEPacketTunnelNetworkSettings.build(networkConfig))
-            let _: Empty = try await runManagerCmd(.setTunnelArgs(args: tunnelArgs))
+            let _: Empty = try await runManagerCmd(.setTunnelArgs(args: tunnelArgs, allowActivation: true))
 
             logger.log("set tunnel active flag \(self.providerId, privacy: .public)")
             isActiveGuard.value = true
@@ -104,7 +104,7 @@ class PacketTunnelProvider: NEPacketTunnelProvider {
 
             logger.log("stopping tunnel \(self.providerId, privacy: .public)")
             do {
-                let _: Empty = try await runManagerCmd(.setTunnelArgs(args: .none))
+                let _: Empty = try await runManagerCmd(.setTunnelArgs(args: .none, allowActivation: false))
             } catch {
                 logger.critical("setting empty tunnel args failed: \(error, privacy: .public)")
             }
@@ -123,23 +123,9 @@ class PacketTunnelProvider: NEPacketTunnelProvider {
             logger.error("received app message without completion handler")
             return
         }
-        guard let isSetTunnelArgs = try? [String: Empty].self.init(json: msg).keys.contains("setTunnelArgs") else {
-            logger.error("received invalid app message, expected JSON encoded manager command")
-            let json_error = try! NeManagerCmdResult.error(errorCodeOther).json().data(using: .utf8)
-            completionHandler(json_error)
-            return
-        }
         Task {
-            let isActiveGuard = isSetTunnelArgs ? await self.isActive.lock() : nil
-            defer { withExtendedLifetime(isActiveGuard) {}}
-            if isActiveGuard?.value == .some(false) {
-                // Prevent accidental tunnel starts due to a "setTunnelArgs" command that lost a race with "stopTunnel".
-                let json_error = try! NeManagerCmdResult.error(errorCodeTunnelInactive).json().data(using: .utf8)
-                completionHandler(json_error)
-            } else {
-                let json_result = try! await ffiJsonManagerCmd(msg).json()
-                completionHandler(json_result.data(using: .utf8))
-            }
+            let json_result = try! await ffiJsonManagerCmd(msg).json()
+            completionHandler(json_result.data(using: .utf8))
         }
     }
 

rustlib/src/manager.rs

@@ -148,8 +148,26 @@ impl Manager {
         self.status_watch.subscribe()
     }
 
-    pub fn set_target_state(&self, target_args: Option<TunnelArgs>) {
-        _ = self.target_tunnel_args.send(target_args)
+    pub fn set_target_state(&self, new_target_args: Option<TunnelArgs>, allow_activation: bool) -> Result<(), ()> {
+        let mut ret = Ok(());
+        let ret_ref = &mut ret;
+        _ = self.target_tunnel_args.send_if_modified(move |target_args| {
+            if target_args == &new_target_args {
+                tracing::warn!(
+                    message_id = "oqQ8GZEE",
+                    "not setting target state, because the new one is identical to the current one"
+                );
+                return false;
+            }
+            if target_args.is_none() && new_target_args.is_some() && !allow_activation {
+                *ret_ref = Err(());
+                tracing::warn!(message_id = "juurJ3bm", "not setting target state, because activation is not allowed");
+                return false;
+            }
+            *target_args = new_target_args;
+            true
+        });
+        ret
     }
 
     pub fn send_packet(&self, packet: &[u8]) {

rustlib/src/manager_cmd.rs

@@ -31,6 +31,7 @@ pub enum ManagerCmdErrorCode {
     ApiRateLimitExceeded,
     ApiSignupLimitExceeded,
     ConfigSaveError,
+    TunnelInactive,
     Other,
 }
 
@@ -84,7 +85,7 @@ pub enum ManagerCmd {
     SetApiUrl { url: Option<String> },
     SetInNewAccountFlow { value: bool },
     SetPinnedExits { exits: Vec<PinnedLocation> },
-    SetTunnelArgs { args: Option<TunnelArgs> },
+    SetTunnelArgs { args: Option<TunnelArgs>, allow_activation: bool },
     RotateWgKey {},
 }
 
@@ -139,10 +140,10 @@ impl ManagerCmd {
                 Ok(()) => Ok(ManagerCmdOk::Empty),
                 Err(err) => Err((&err).into()),
             },
-            ManagerCmd::SetTunnelArgs { args } => {
-                manager.set_target_state(args);
-                Ok(ManagerCmdOk::Empty)
-            }
+            ManagerCmd::SetTunnelArgs { args, allow_activation } => match manager.set_target_state(args, allow_activation) {
+                Ok(()) => Ok(ManagerCmdOk::Empty),
+                Err(()) => Err(ManagerCmdErrorCode::TunnelInactive),
+            },
             ManagerCmd::RotateWgKey {} => match manager.rotate_wg_key() {
                 Ok(()) => Ok(ManagerCmdOk::Empty),
                 Err(err) => Err((&err).into()),