From 8b59fe6f4c15c5a8ebe424dc10750b02d252a33d Mon Sep 17 00:00:00 2001 From: Davide De Rosa Date: Wed, 24 Oct 2018 09:19:50 +0200 Subject: [PATCH 1/3] Use RawRepresentable where adequate --- .../TunnelKitProvider+Configuration.swift | 35 ++++++++++++------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/TunnelKit/Sources/AppExtension/TunnelKitProvider+Configuration.swift b/TunnelKit/Sources/AppExtension/TunnelKitProvider+Configuration.swift index 5ac1e7d..42d1316 100644 --- a/TunnelKit/Sources/AppExtension/TunnelKitProvider+Configuration.swift +++ b/TunnelKit/Sources/AppExtension/TunnelKitProvider+Configuration.swift @@ -56,7 +56,7 @@ extension TunnelKitProvider { } /// Defines the communication protocol of an endpoint. - public struct EndpointProtocol: Equatable, CustomStringConvertible { + public struct EndpointProtocol: RawRepresentable, Equatable, CustomStringConvertible { /// The socket type. public let socketType: SocketType @@ -70,23 +70,25 @@ extension TunnelKitProvider { self.port = port } + // MARK: RawRepresentable + /// :nodoc: - public static func deserialized(_ string: String) throws -> EndpointProtocol { - let components = string.components(separatedBy: ":") + public init?(rawValue: String) { + let components = rawValue.components(separatedBy: ":") guard components.count == 2 else { - throw ProviderConfigurationError.parameter(name: "endpointProtocol") + return nil } guard let socketType = SocketType(rawValue: components[0]) else { - throw ProviderConfigurationError.parameter(name: "endpointProtocol.socketType") + return nil } guard let port = UInt16(components[1]) else { - throw ProviderConfigurationError.parameter(name: "endpointProtocol.port") + return nil } - return EndpointProtocol(socketType, port) + self.init(socketType, port) } /// :nodoc: - public func serialized() -> String { + public var rawValue: String { return "\(socketType.rawValue):\(port)" } @@ -101,7 +103,7 @@ extension TunnelKitProvider { /// :nodoc: public var description: String { - return serialized() + return rawValue } } @@ -229,7 +231,12 @@ extension TunnelKitProvider { guard let endpointProtocolsStrings = providerConfiguration[S.endpointProtocols] as? [String], !endpointProtocolsStrings.isEmpty else { throw ProviderConfigurationError.parameter(name: "protocolConfiguration.providerConfiguration[\(S.endpointProtocols)] is nil or empty") } - endpointProtocols = try endpointProtocolsStrings.map { try EndpointProtocol.deserialized($0) } + endpointProtocols = try endpointProtocolsStrings.map { + guard let ep = EndpointProtocol(rawValue: $0) else { + throw ProviderConfigurationError.parameter(name: "protocolConfiguration.providerConfiguration[\(S.endpointProtocols)] has a badly formed element") + } + return ep + } self.cipher = cipher self.digest = digest @@ -444,7 +451,7 @@ extension TunnelKitProvider { var dict: [String: Any] = [ S.appGroup: appGroup, S.prefersResolvedAddresses: prefersResolvedAddresses, - S.endpointProtocols: endpointProtocols.map { $0.serialized() }, + S.endpointProtocols: endpointProtocols.map { $0.rawValue }, S.cipherAlgorithm: cipher.rawValue, S.digestAlgorithm: digest.rawValue, S.ca: ca.pem, @@ -602,12 +609,14 @@ extension TunnelKitProvider.Configuration: Equatable { extension TunnelKitProvider.EndpointProtocol: Codable { public init(from decoder: Decoder) throws { let container = try decoder.singleValueContainer() - let proto = try TunnelKitProvider.EndpointProtocol.deserialized(container.decode(String.self)) + guard let proto = try TunnelKitProvider.EndpointProtocol(rawValue: container.decode(String.self)) else { + throw TunnelKitProvider.ProviderConfigurationError.parameter(name: "endpointProtocol.decodable") + } self.init(proto.socketType, proto.port) } public func encode(to encoder: Encoder) throws { var container = encoder.singleValueContainer() - try container.encode(serialized()) + try container.encode(rawValue) } } From 91349fd780de9d77f2d62765c02ae01d92a5e1ab Mon Sep 17 00:00:00 2001 From: Davide De Rosa Date: Wed, 24 Oct 2018 09:29:39 +0200 Subject: [PATCH 2/3] Take shouldChangeProtocol out of GenericSocket Behavior is not exactly similar in UDP and TCP. --- .../Sources/AppExtension/GenericSocket.swift | 2 -- .../Transport/NETCPInterface.swift | 1 - .../AppExtension/TunnelKitProvider.swift | 25 ++++++++++++------- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/TunnelKit/Sources/AppExtension/GenericSocket.swift b/TunnelKit/Sources/AppExtension/GenericSocket.swift index 87aa9e2..4a59c02 100644 --- a/TunnelKit/Sources/AppExtension/GenericSocket.swift +++ b/TunnelKit/Sources/AppExtension/GenericSocket.swift @@ -44,8 +44,6 @@ protocol LinkProducer { protocol GenericSocketDelegate: class { func socketDidTimeout(_ socket: GenericSocket) - func socketShouldChangeProtocol(_ socket: GenericSocket) -> Bool - func socketDidBecomeActive(_ socket: GenericSocket) func socket(_ socket: GenericSocket, didShutdownWithFailure failure: Bool) diff --git a/TunnelKit/Sources/AppExtension/Transport/NETCPInterface.swift b/TunnelKit/Sources/AppExtension/Transport/NETCPInterface.swift index e74f4ae..3fd949c 100644 --- a/TunnelKit/Sources/AppExtension/Transport/NETCPInterface.swift +++ b/TunnelKit/Sources/AppExtension/Transport/NETCPInterface.swift @@ -79,7 +79,6 @@ class NETCPSocket: NSObject, GenericSocket { return } guard _self.isActive else { - _ = _self.delegate?.socketShouldChangeProtocol(_self) _self.delegate?.socketDidTimeout(_self) return } diff --git a/TunnelKit/Sources/AppExtension/TunnelKitProvider.swift b/TunnelKit/Sources/AppExtension/TunnelKitProvider.swift index f8f25a1..dfd3005 100644 --- a/TunnelKit/Sources/AppExtension/TunnelKitProvider.swift +++ b/TunnelKit/Sources/AppExtension/TunnelKitProvider.swift @@ -400,14 +400,14 @@ extension TunnelKitProvider: GenericSocketDelegate { log.debug("Socket timed out waiting for activity, cancelling...") reasserting = true socket.shutdown() - } - - func socketShouldChangeProtocol(_ socket: GenericSocket) -> Bool { - guard strategy.tryNextProtocol() else { - disposeTunnel(error: ProviderError.exhaustedProtocols) - return false + + // fallback: TCP connection timeout suggests falling back + if let _ = socket as? NETCPSocket { + guard tryNextProtocol() else { + // disposeTunnel + return + } } - return true } func socketDidBecomeActive(_ socket: GenericSocket) { @@ -448,9 +448,9 @@ extension TunnelKitProvider: GenericSocketDelegate { // clean up finishTunnelDisconnection(error: shutdownError) - // treat negotiation timeout as socket timeout, UDP is connection-less + // fallback: UDP is connection-less, treat negotiation timeout as socket timeout if didTimeoutNegotiation { - guard socketShouldChangeProtocol(socket) else { + guard tryNextProtocol() else { // disposeTunnel return } @@ -574,6 +574,13 @@ extension TunnelKitProvider: SessionProxyDelegate { } extension TunnelKitProvider { + private func tryNextProtocol() -> Bool { + guard strategy.tryNextProtocol() else { + disposeTunnel(error: ProviderError.exhaustedProtocols) + return false + } + return true + } // MARK: Logging From d829247e6e3096993baeee7e29aa205928e4f987 Mon Sep 17 00:00:00 2001 From: Davide De Rosa Date: Wed, 24 Oct 2018 09:36:34 +0200 Subject: [PATCH 3/3] Simplify socket shutdown code Drop weird (old?) linkFailures check. --- .../AppExtension/TunnelKitProvider.swift | 26 ++++++------------- 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/TunnelKit/Sources/AppExtension/TunnelKitProvider.swift b/TunnelKit/Sources/AppExtension/TunnelKitProvider.swift index dfd3005..47b87d4 100644 --- a/TunnelKit/Sources/AppExtension/TunnelKitProvider.swift +++ b/TunnelKit/Sources/AppExtension/TunnelKitProvider.swift @@ -106,8 +106,6 @@ open class TunnelKitProvider: NEPacketTunnelProvider { private var socket: GenericSocket? - private var linkFailures = 0 - private var pendingStartHandler: ((Error?) -> Void)? private var pendingStopHandler: (() -> Void)? @@ -428,19 +426,17 @@ extension TunnelKitProvider: GenericSocketDelegate { } var shutdownError: Error? - if !failure { - shutdownError = proxy.stopError - } else { - shutdownError = proxy.stopError ?? ProviderError.linkError - linkFailures += 1 - log.debug("Link failures so far: \(linkFailures) (max = \(maxLinkFailures))") + let didTimeoutNegotiation: Bool + var upgradedSocket: GenericSocket? + + // look for error causing shutdown + shutdownError = proxy.stopError + if failure && (shutdownError == nil) { + shutdownError = ProviderError.linkError } - - // neg timeout? - let didTimeoutNegotiation = (proxy.stopError as? SessionError == .negotiationTimeout) + didTimeoutNegotiation = (shutdownError as? SessionError == .negotiationTimeout) // only try upgrade on network errors - var upgradedSocket: GenericSocket? = nil if shutdownError as? SessionError == nil { upgradedSocket = socket.upgraded() } @@ -458,12 +454,6 @@ extension TunnelKitProvider: GenericSocketDelegate { // reconnect? if reasserting { - guard (linkFailures < maxLinkFailures) else { - log.debug("Too many link failures (\(linkFailures)), tunnel will die now") - reasserting = false - disposeTunnel(error: shutdownError) - return - } log.debug("Disconnection is recoverable, tunnel will reconnect in \(reconnectionDelay) milliseconds...") tunnelQueue.schedule(after: .milliseconds(reconnectionDelay)) { self.connectTunnel(upgradedSocket: upgradedSocket, preferredAddress: socket.remoteAddress)