-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes tunnel startup options handling #489
Changes from 1 commit
80e96cf
db0b5fe
84b787a
c462e2d
e538993
edebae5
98d5279
c687e3d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,3 @@ public enum NetworkProtectionOptionKey { | |
public static let excludedRoutes = "excludedRoutes" | ||
public static let connectionTesterEnabled = "connectionTesterEnabled" | ||
} | ||
public enum NetworkProtectionOptionValue { | ||
public static let `true` = "true" as NSString | ||
} | ||
Comment on lines
-34
to
-36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can send bools as numbers which is the standard. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,12 +37,18 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { | |
// MARK: - Error Handling | ||
|
||
enum TunnelError: LocalizedError { | ||
case startingTunnelWithoutOptions | ||
case startingTunnelWithoutAuthToken | ||
case couldNotGenerateTunnelConfiguration(internalError: Error) | ||
case couldNotFixConnection | ||
case simulateTunnelFailureError | ||
|
||
var errorDescription: String? { | ||
switch self { | ||
case .startingTunnelWithoutOptions: | ||
return "Missing tunnel options at startup" | ||
case .startingTunnelWithoutAuthToken: | ||
return "Missing auth token at startup" | ||
case .couldNotGenerateTunnelConfiguration(let internalError): | ||
return "Failed to generate a tunnel configuration: \(internalError.localizedDescription)" | ||
case .simulateTunnelFailureError: | ||
|
@@ -184,19 +190,22 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { | |
} | ||
|
||
private func setKeyValidity(_ interval: TimeInterval?) { | ||
guard keyValidity != interval, | ||
let interval = interval else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A |
||
|
||
guard keyValidity != interval else { | ||
return | ||
} | ||
|
||
let firstExpirationDate = Date().addingTimeInterval(interval) | ||
if let interval { | ||
let firstExpirationDate = Date().addingTimeInterval(interval) | ||
|
||
os_log("Setting key validity interval to %{public}@ seconds (next expiration date %{public}@)", | ||
log: .networkProtectionKeyManagement, | ||
String(describing: interval), | ||
String(describing: firstExpirationDate)) | ||
} else { | ||
os_log("Resetting key validity interval", | ||
log: .networkProtectionKeyManagement) | ||
} | ||
|
||
os_log("Setting key validity to %{public}@ seconds (next expiration date %{public}@)", | ||
log: .networkProtectionKeyManagement, | ||
type: .info, | ||
String(describing: interval), | ||
String(describing: firstExpirationDate)) | ||
keyStore.setValidityInterval(interval) | ||
} | ||
|
||
|
@@ -327,12 +336,7 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { | |
protocolConfiguration as? NETunnelProviderProtocol | ||
} | ||
|
||
private func load(options: [String: NSObject]?) throws { | ||
guard let options = options else { | ||
os_log("🔵 Tunnel options are not set", log: .networkProtection) | ||
return | ||
} | ||
|
||
private func load(options: StartupOptions) throws { | ||
loadKeyValidity(from: options) | ||
loadSelectedServer(from: options) | ||
try loadAuthToken(from: options) | ||
|
@@ -345,29 +349,40 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { | |
self.isConnectionTesterEnabled = vendorOptions?[NetworkProtectionOptionKey.connectionTesterEnabled] as? Bool ?? true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should not have been in vendor options. In fact it was not disabled in iOS due to this. I've moved it to line 362 so it's part of the startup options instead. |
||
} | ||
|
||
private func loadKeyValidity(from options: [String: AnyObject]) { | ||
guard let keyValidityString = options[NetworkProtectionOptionKey.keyValidity] as? String, | ||
let keyValidity = TimeInterval(keyValidityString) else { | ||
return | ||
private func loadKeyValidity(from options: StartupOptions) { | ||
switch options.keyValidity { | ||
case .set(let validity): | ||
setKeyValidity(validity) | ||
case .useExisting: | ||
break | ||
case .reset: | ||
setKeyValidity(nil) | ||
Comment on lines
+375
to
+381
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now we have a basic mechanism to know whether we need to set the validity, use the existing one or reset it. |
||
} | ||
|
||
setKeyValidity(keyValidity) | ||
} | ||
|
||
private func loadSelectedServer(from options: [String: AnyObject]) { | ||
guard let serverName = options[NetworkProtectionOptionKey.selectedServer] as? String else { | ||
return | ||
private func loadSelectedServer(from options: StartupOptions) { | ||
switch options.selectedServer { | ||
case .set(let selectedServer): | ||
selectedServerStore.selectedServer = selectedServer | ||
case .useExisting: | ||
break | ||
case .reset: | ||
selectedServerStore.selectedServer = .automatic | ||
Comment on lines
+387
to
+392
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above but for server selection. |
||
} | ||
|
||
selectedServerStore.selectedServer = .endpoint(serverName) | ||
} | ||
|
||
private func loadAuthToken(from options: [String: AnyObject]) throws { | ||
guard let authToken = options[NetworkProtectionOptionKey.authToken] as? String else { | ||
return | ||
private func loadAuthToken(from options: StartupOptions) throws { | ||
switch options.authToken { | ||
case .set(let authToken): | ||
try tokenStore.store(authToken) | ||
case .useExisting: | ||
break | ||
case .reset: | ||
// This case should in theory not be possible, but it's ideal to have this in place | ||
// in case an error in the controller on the client side allows it. | ||
try tokenStore.deleteToken() | ||
throw TunnelError.startingTunnelWithoutAuthToken | ||
Comment on lines
+398
to
+406
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above but for the auth token. |
||
} | ||
|
||
try tokenStore.store(authToken) | ||
} | ||
|
||
private func loadRoutes(from options: [String: Any]?) { | ||
|
@@ -389,47 +404,50 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { | |
|
||
open override func startTunnel(options: [String: NSObject]?, completionHandler: @escaping (Error?) -> Void) { | ||
connectionStatus = .connecting | ||
|
||
// when activated by system "on-demand" the option is set | ||
var isOnDemand: Bool { | ||
options?[NetworkProtectionOptionKey.isOnDemand] as? Bool == true | ||
} | ||
var isActivatedFromSystemSettings: Bool { | ||
options?[NetworkProtectionOptionKey.activationAttemptId] == nil && !isOnDemand | ||
} | ||
Comment on lines
-394
to
-399
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is now handled within |
||
tunnelHealth.isHavingConnectivityIssues = false | ||
controllerErrorStore.lastErrorMessage = nil | ||
|
||
let internalCompletionHandler = { [weak self] (error: Error?) in | ||
if error != nil { | ||
self?.connectionStatus = .disconnected | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tis was moved to line 422. The diff in this file is really awful. Apologies! |
||
completionHandler(error) | ||
guard let error else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reverted this condition using a guard so things moved around a bit... sorry! |
||
completionHandler(nil) | ||
return | ||
} | ||
|
||
completionHandler(nil) | ||
let errorDescription = (error as? LocalizedError)?.localizedDescription ?? String(describing: error) | ||
|
||
os_log("Tunnel startup error: %{public}@", type: .error, errorDescription) | ||
self?.controllerErrorStore.lastErrorMessage = errorDescription | ||
self?.connectionStatus = .disconnected | ||
|
||
completionHandler(error) | ||
} | ||
|
||
tunnelHealth.isHavingConnectivityIssues = false | ||
controllerErrorStore.lastErrorMessage = nil | ||
guard let options else { | ||
internalCompletionHandler(TunnelError.startingTunnelWithoutOptions) | ||
return | ||
} | ||
|
||
os_log("🔵 Will load options\n%{public}@", log: .networkProtection, String(describing: options)) | ||
os_log("Will load options\n%{public}@", log: .networkProtection, String(describing: options)) | ||
let startupOptions = StartupOptions(options: options, log: .networkProtection) | ||
startTunnel(options: startupOptions, completionHandler: internalCompletionHandler) | ||
} | ||
|
||
if options?[NetworkProtectionOptionKey.tunnelFailureSimulation] == NetworkProtectionOptionValue.true { | ||
internalCompletionHandler(TunnelError.simulateTunnelFailureError) | ||
controllerErrorStore.lastErrorMessage = TunnelError.simulateTunnelFailureError.localizedDescription | ||
private func startTunnel(options: StartupOptions, completionHandler: @escaping (Error?) -> Void) { | ||
if options.startedToSimulateError { | ||
completionHandler(TunnelError.simulateTunnelFailureError) | ||
return | ||
} | ||
|
||
do { | ||
try load(options: options) | ||
try loadVendorOptions(from: tunnelProviderProtocol) | ||
} catch { | ||
internalCompletionHandler(error) | ||
completionHandler(error) | ||
return | ||
} | ||
|
||
os_log("🔵 Done! Starting tunnel from the %{public}@", log: .networkProtection, type: .info, (isActivatedFromSystemSettings ? "settings" : (isOnDemand ? "on-demand" : "app"))) | ||
|
||
startTunnel(selectedServer: selectedServerStore.selectedServer, completionHandler: internalCompletionHandler) | ||
os_log("Starting tunnel %{public}@", log: .networkProtection, options.startupMethod.debugDescription) | ||
startTunnel(selectedServer: selectedServerStore.selectedServer, completionHandler: completionHandler) | ||
} | ||
|
||
private func startTunnel(selectedServer: SelectedNetworkProtectionServer, completionHandler: @escaping (Error?) -> Void) { | ||
|
@@ -688,7 +706,6 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { | |
private func handleSetSelectedServer(_ serverName: String?, completionHandler: ((Data?) -> Void)? = nil) { | ||
Task { | ||
guard let serverName else { | ||
|
||
if case .endpoint = selectedServerStore.selectedServer { | ||
selectedServerStore.selectedServer = .automatic | ||
try? await updateTunnelConfiguration(serverSelectionMethod: .automatic) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,124 @@ | ||
// | ||
// StartupOptions.swift | ||
// | ||
// Copyright © 2023 DuckDuckGo. All rights reserved. | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
// | ||
|
||
import Foundation | ||
import Common | ||
|
||
struct StartupOptions { | ||
|
||
enum StartupMethod: CustomDebugStringConvertible { | ||
/// Case started up manually from the main app. | ||
/// | ||
case manualByMainApp | ||
|
||
/// Started up manually from a Syste provided source: it can be the VPN menu, a CLI command | ||
/// or the list of VPNs in System Settings. | ||
/// | ||
case manualByTheSystem | ||
|
||
/// Started up automatically by on-demand. | ||
/// | ||
case automaticOnDemand | ||
|
||
var debugDescription: String { | ||
switch self { | ||
case .automaticOnDemand: | ||
return "automatically by On-Demand" | ||
case .manualByMainApp: | ||
return "manually by the main app" | ||
case .manualByTheSystem: | ||
return "manually by the system" | ||
} | ||
} | ||
} | ||
|
||
enum Option<T> { | ||
case set(_ value: T) | ||
case reset | ||
case useExisting | ||
} | ||
|
||
private let log: OSLog | ||
let startupMethod: StartupMethod | ||
let startedToSimulateError: Bool | ||
let keyValidity: Option<TimeInterval> | ||
let selectedServer: Option<SelectedNetworkProtectionServer> | ||
let authToken: Option<String> | ||
|
||
init(options: [String: Any], log: OSLog) { | ||
self.log = log | ||
|
||
let startupMethod: StartupMethod = { | ||
if options[NetworkProtectionOptionKey.isOnDemand] as? Bool == true { | ||
return .automaticOnDemand | ||
} else if options[NetworkProtectionOptionKey.activationAttemptId] != nil { | ||
return .manualByMainApp | ||
} else { | ||
return .manualByTheSystem | ||
} | ||
}() | ||
|
||
self.startupMethod = startupMethod | ||
|
||
startedToSimulateError = options[NetworkProtectionOptionKey.tunnelFailureSimulation] as? Bool ?? false | ||
|
||
keyValidity = { | ||
guard let keyValidityString = options[NetworkProtectionOptionKey.keyValidity] as? String else { | ||
switch startupMethod { | ||
case .manualByMainApp: | ||
return .reset | ||
default: | ||
return .useExisting | ||
} | ||
} | ||
|
||
guard let keyValidity = TimeInterval(keyValidityString) else { | ||
os_log("The key validity startup option cannot be parsed", log: log, type: .error) | ||
return .useExisting | ||
} | ||
|
||
return .set(keyValidity) | ||
}() | ||
|
||
selectedServer = { | ||
guard let serverName = options[NetworkProtectionOptionKey.selectedServer] as? String else { | ||
switch startupMethod { | ||
case .manualByMainApp: | ||
return .reset | ||
default: | ||
return .useExisting | ||
} | ||
} | ||
|
||
return .set(.endpoint(serverName)) | ||
}() | ||
|
||
authToken = { | ||
guard let authToken = options[NetworkProtectionOptionKey.authToken] as? String else { | ||
switch startupMethod { | ||
case .manualByMainApp: | ||
return .reset | ||
default: | ||
return .useExisting | ||
} | ||
} | ||
|
||
return .set(authToken) | ||
}() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not have been ignored. It should have reset the selected server.