From 4a5b6bef5b907fc65e9474089bd28a0182396ec5 Mon Sep 17 00:00:00 2001 From: Robert Patchett Date: Mon, 28 Oct 2019 14:01:13 +0100 Subject: [PATCH 1/3] Call cancelTunnelWithError(_:) if a connection fails and won't be retried --- .../OpenVPN/AppExtension/OpenVPNTunnelProvider.swift | 12 ++++++++++++ .../Sources/Protocols/OpenVPN/OpenVPNSession.swift | 11 +++++++++-- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/TunnelKit/Sources/Protocols/OpenVPN/AppExtension/OpenVPNTunnelProvider.swift b/TunnelKit/Sources/Protocols/OpenVPN/AppExtension/OpenVPNTunnelProvider.swift index d3e804be..d45dea01 100644 --- a/TunnelKit/Sources/Protocols/OpenVPN/AppExtension/OpenVPNTunnelProvider.swift +++ b/TunnelKit/Sources/Protocols/OpenVPN/AppExtension/OpenVPNTunnelProvider.swift @@ -579,6 +579,18 @@ extension OpenVPNTunnelProvider: OpenVPNSessionDelegate { public func sessionDidStop(_: OpenVPNSession, shouldReconnect: Bool) { log.info("Session did stop") + stopSession(shouldReconnect: shouldReconnect) + } + + /// :nodoc: + public func sessionFailed(_: OpenVPNSession, error: Error) { + log.info("Session failed") + + cancelTunnelWithError(error) + stopSession(shouldReconnect: false) + } + + private func stopSession(shouldReconnect: Bool) { isCountingData = false refreshDataCount() diff --git a/TunnelKit/Sources/Protocols/OpenVPN/OpenVPNSession.swift b/TunnelKit/Sources/Protocols/OpenVPN/OpenVPNSession.swift index d7811471..5234336d 100644 --- a/TunnelKit/Sources/Protocols/OpenVPN/OpenVPNSession.swift +++ b/TunnelKit/Sources/Protocols/OpenVPN/OpenVPNSession.swift @@ -59,6 +59,11 @@ public protocol OpenVPNSessionDelegate: class { - Seealso: `OpenVPNSession.reconnect(...)` */ func sessionDidStop(_: OpenVPNSession, shouldReconnect: Bool) + + /** + Called after a session failed to start. + */ + func sessionFailed(_: OpenVPNSession, error: Error) } /// Provides methods to set up and maintain an OpenVPN session. @@ -1275,13 +1280,15 @@ public class OpenVPNSession: Session { } private func doShutdown(error: Error?) { + stopError = error + if let error = error { log.error("Trigger shutdown (error: \(error))") + delegate?.sessionFailed(self, error: error) } else { log.info("Trigger shutdown on request") + delegate?.sessionDidStop(self, shouldReconnect: false) } - stopError = error - delegate?.sessionDidStop(self, shouldReconnect: false) } private func doReconnect(error: Error?) { From bf012f6b80f806d87be926400c3c6642efb735f2 Mon Sep 17 00:00:00 2001 From: Davide De Rosa Date: Fri, 8 May 2020 21:13:57 +0200 Subject: [PATCH 2/3] Refactor with an error parameter in sessionDidStop Both versions prevent clients from compiling, but this version impacts less on existing codebase. --- .../AppExtension/OpenVPNTunnelProvider.swift | 21 +++++++------------ .../Protocols/OpenVPN/OpenVPNSession.swift | 16 +++++--------- 2 files changed, 12 insertions(+), 25 deletions(-) diff --git a/TunnelKit/Sources/Protocols/OpenVPN/AppExtension/OpenVPNTunnelProvider.swift b/TunnelKit/Sources/Protocols/OpenVPN/AppExtension/OpenVPNTunnelProvider.swift index d45dea01..73e69df2 100644 --- a/TunnelKit/Sources/Protocols/OpenVPN/AppExtension/OpenVPNTunnelProvider.swift +++ b/TunnelKit/Sources/Protocols/OpenVPN/AppExtension/OpenVPNTunnelProvider.swift @@ -576,21 +576,14 @@ extension OpenVPNTunnelProvider: OpenVPNSessionDelegate { } /// :nodoc: - public func sessionDidStop(_: OpenVPNSession, shouldReconnect: Bool) { - log.info("Session did stop") + public func sessionDidStop(_: OpenVPNSession, withError error: Error?, shouldReconnect: Bool) { + if let error = error { + log.error("Session did stop with error: \(error)") + cancelTunnelWithError(error) + } else { + log.info("Session did stop") + } - stopSession(shouldReconnect: shouldReconnect) - } - - /// :nodoc: - public func sessionFailed(_: OpenVPNSession, error: Error) { - log.info("Session failed") - - cancelTunnelWithError(error) - stopSession(shouldReconnect: false) - } - - private func stopSession(shouldReconnect: Bool) { isCountingData = false refreshDataCount() diff --git a/TunnelKit/Sources/Protocols/OpenVPN/OpenVPNSession.swift b/TunnelKit/Sources/Protocols/OpenVPN/OpenVPNSession.swift index 5234336d..2ab55d5d 100644 --- a/TunnelKit/Sources/Protocols/OpenVPN/OpenVPNSession.swift +++ b/TunnelKit/Sources/Protocols/OpenVPN/OpenVPNSession.swift @@ -55,15 +55,11 @@ public protocol OpenVPNSessionDelegate: class { /** Called after stopping a session. + - Parameter error: An optional `Error` being the reason of the stop. - Parameter shouldReconnect: When `true`, the session can/should be restarted. Usually because the stop reason was recoverable. - Seealso: `OpenVPNSession.reconnect(...)` */ - func sessionDidStop(_: OpenVPNSession, shouldReconnect: Bool) - - /** - Called after a session failed to start. - */ - func sessionFailed(_: OpenVPNSession, error: Error) + func sessionDidStop(_: OpenVPNSession, withError error: Error?, shouldReconnect: Bool) } /// Provides methods to set up and maintain an OpenVPN session. @@ -1280,15 +1276,13 @@ public class OpenVPNSession: Session { } private func doShutdown(error: Error?) { - stopError = error - if let error = error { log.error("Trigger shutdown (error: \(error))") - delegate?.sessionFailed(self, error: error) } else { log.info("Trigger shutdown on request") - delegate?.sessionDidStop(self, shouldReconnect: false) } + stopError = error + delegate?.sessionDidStop(self, withError: error, shouldReconnect: false) } private func doReconnect(error: Error?) { @@ -1298,6 +1292,6 @@ public class OpenVPNSession: Session { log.info("Trigger reconnection on request") } stopError = error - delegate?.sessionDidStop(self, shouldReconnect: true) + delegate?.sessionDidStop(self, withError: error, shouldReconnect: true) } } From c1aefcf76aa46c562d3fff0a2786202353e44df1 Mon Sep 17 00:00:00 2001 From: Davide De Rosa Date: Sat, 9 May 2020 00:46:16 +0200 Subject: [PATCH 3/3] Evaluate reconnection without touching reasserting Use a different variable to signal an upcoming reconnection. Make sure that reasserting is never set to false with the meaning of "do not reconnect", because doing so would trigger a transient "connected" state in the VPN. Reverts use of cancelTunnelWithError() in sessionDidStop. --- .../AppExtension/OpenVPNTunnelProvider.swift | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/TunnelKit/Sources/Protocols/OpenVPN/AppExtension/OpenVPNTunnelProvider.swift b/TunnelKit/Sources/Protocols/OpenVPN/AppExtension/OpenVPNTunnelProvider.swift index 73e69df2..5f33c712 100644 --- a/TunnelKit/Sources/Protocols/OpenVPN/AppExtension/OpenVPNTunnelProvider.swift +++ b/TunnelKit/Sources/Protocols/OpenVPN/AppExtension/OpenVPNTunnelProvider.swift @@ -128,6 +128,8 @@ open class OpenVPNTunnelProvider: NEPacketTunnelProvider { private var isCountingData = false + private var shouldReconnect = false + // MARK: NEPacketTunnelProvider (XPC queue) open override var reasserting: Bool { @@ -341,7 +343,7 @@ open class OpenVPNTunnelProvider: NEPacketTunnelProvider { } private func finishTunnelDisconnection(error: Error?) { - if let session = session, !(reasserting && session.canRebindLink()) { + if let session = session, !(shouldReconnect && session.canRebindLink()) { session.cleanup() } @@ -419,7 +421,7 @@ extension OpenVPNTunnelProvider: GenericSocketDelegate { /// :nodoc: public func socketDidTimeout(_ socket: GenericSocket) { log.debug("Socket timed out waiting for activity, cancelling...") - reasserting = true + shouldReconnect = true socket.shutdown() // fallback: TCP connection timeout suggests falling back @@ -438,7 +440,7 @@ extension OpenVPNTunnelProvider: GenericSocketDelegate { } if session.canRebindLink() { session.rebindLink(producer.link(withMTU: cfg.mtu)) - reasserting = false + shouldReconnect = false } else { session.setLink(producer.link(withMTU: cfg.mtu)) } @@ -478,17 +480,18 @@ extension OpenVPNTunnelProvider: GenericSocketDelegate { } // reconnect? - if reasserting { + if shouldReconnect { log.debug("Disconnection is recoverable, tunnel will reconnect in \(reconnectionDelay) milliseconds...") tunnelQueue.schedule(after: .milliseconds(reconnectionDelay)) { - log.debug("Tunnel is about to reconnect...") - // give up if reasserting cleared in the meantime - guard self.reasserting else { - log.warning("Reasserting flag was cleared in the meantime") + // give up if shouldReconnect cleared in the meantime + guard self.shouldReconnect else { + log.warning("Reconnection flag was cleared in the meantime") return } + log.debug("Tunnel is about to reconnect...") + self.reasserting = true self.connectTunnel(upgradedSocket: upgradedSocket) } return @@ -579,7 +582,6 @@ extension OpenVPNTunnelProvider: OpenVPNSessionDelegate { public func sessionDidStop(_: OpenVPNSession, withError error: Error?, shouldReconnect: Bool) { if let error = error { log.error("Session did stop with error: \(error)") - cancelTunnelWithError(error) } else { log.info("Session did stop") } @@ -587,7 +589,7 @@ extension OpenVPNTunnelProvider: OpenVPNSessionDelegate { isCountingData = false refreshDataCount() - reasserting = shouldReconnect + self.shouldReconnect = shouldReconnect socket?.shutdown() }