Skip to content
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

Merged
merged 8 commits into from
Sep 12, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,10 @@ public enum ExtensionMessage: RawRepresentable {
case .isHavingConnectivityIssues:
self = .isHavingConnectivityIssues
case .setSelectedServer:
guard data.count > 1 else { return nil }
Copy link
Contributor Author

@diegoreymendez diegoreymendez Sep 3, 2023

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.

guard data.count > 1 else {
self = .setSelectedServer(nil)
return
}
let serverName = ExtensionMessageString(rawValue: data[1...])
self = .setSelectedServer(serverName?.value)

Expand Down Expand Up @@ -148,6 +151,7 @@ public enum ExtensionMessage: RawRepresentable {
assertionFailure("could not encode routes: \(error)")
}
}

case .setSelectedServer(.none),
.setKeyValidity(.none),
.resetAllState,
Expand All @@ -158,7 +162,8 @@ public enum ExtensionMessage: RawRepresentable {
.getServerAddress,
.expireRegistrationKey,
.triggerTestNotification,
.simulateTunnelFailure: break
.simulateTunnelFailure:
break
}

var data = Data([self.name.rawValue])
Expand Down
3 changes: 0 additions & 3 deletions Sources/NetworkProtection/NetworkProtectionOptionKey.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

@diegoreymendez diegoreymendez Sep 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can send bools as numbers which is the standard.

125 changes: 71 additions & 54 deletions Sources/NetworkProtection/PacketTunnelProvider.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -184,19 +190,22 @@ open class PacketTunnelProvider: NEPacketTunnelProvider {
}

private func setKeyValidity(_ interval: TimeInterval?) {
guard keyValidity != interval,
let interval = interval else {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nil interval wasn't meant to bail out... it's meant to be a request to reset the key validity interval.


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)
}

Expand Down Expand Up @@ -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)
Expand All @@ -345,29 +349,40 @@ open class PacketTunnelProvider: NEPacketTunnelProvider {
self.isConnectionTesterEnabled = vendorOptions?[NetworkProtectionOptionKey.connectionTesterEnabled] as? Bool ?? true
Copy link
Contributor Author

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 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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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]?) {
Expand All @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now handled within StartupOptions.

tunnelHealth.isHavingConnectivityIssues = false
controllerErrorStore.lastErrorMessage = nil

let internalCompletionHandler = { [weak self] (error: Error?) in
if error != nil {
self?.connectionStatus = .disconnected
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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) {
Expand Down Expand Up @@ -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)
Expand Down
124 changes: 124 additions & 0 deletions Sources/NetworkProtection/StartupOptions.swift
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)
}()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import Foundation
import Network
import NetworkExtension
import Common

#if SWIFT_PACKAGE
import WireGuard
Expand Down