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

Remediate TunnelVision, TunnelCrack and fix "Exclude Local Networks" #1039

Merged
merged 25 commits into from
Nov 11, 2024
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
71c861c
Adds the VPN's enforceRoutes feature flag
diegoreymendez Oct 24, 2024
5de4c50
Adds logic to properly handle DNS route inclusion
diegoreymendez Oct 25, 2024
a817b6e
Some improvements to better support enforceRoutes
diegoreymendez Oct 26, 2024
8287540
WIP
diegoreymendez Oct 31, 2024
2e5f409
Cleaned up the code to remove a lot of unnecessary logic
diegoreymendez Nov 4, 2024
3d18455
Additional cleanup around VPN routing
diegoreymendez Nov 4, 2024
37c83d3
Cleanup and fixing an error in iOS
diegoreymendez Nov 4, 2024
cbed087
Merge branch 'main' into diego/fix-tunnelvision-2
diegoreymendez Nov 4, 2024
66088d0
Adds a memory for force-enabling enforce routes for feature flag users
diegoreymendez Nov 4, 2024
432b548
Added a new file that was missing from the previous commit
diegoreymendez Nov 4, 2024
7e9c307
Fixes an issue with exclude local networks
diegoreymendez Nov 6, 2024
86ddb4c
Fixes several swiftlint warnings
diegoreymendez Nov 6, 2024
a304582
Merge branch 'main' into diego/fix-tunnelvision-2
diegoreymendez Nov 6, 2024
9b038e2
Fixes unit tests
diegoreymendez Nov 6, 2024
725579d
Re-enabled some code that was disabled by mistake
diegoreymendez Nov 6, 2024
d9cac68
Fixes unit tests
diegoreymendez Nov 6, 2024
0ba1e70
Updates the tunnel configuration to work best against TunnelCrack
diegoreymendez Nov 7, 2024
2c34dbd
Included local networks again
diegoreymendez Nov 8, 2024
b717437
Updates the excluded routes
diegoreymendez Nov 8, 2024
e89ceb7
Fixes a swiflint warning
diegoreymendez Nov 8, 2024
f05ad5e
Rolls back an unintentional change
diegoreymendez Nov 8, 2024
0dead1b
Updates a protocol
diegoreymendez Nov 8, 2024
bc7bf68
Fixes an issue with the unit tests
diegoreymendez Nov 11, 2024
fa6b96b
Merge branch 'main' into diego/fix-tunnelvision-2
diegoreymendez Nov 11, 2024
2ded3f8
Removes some commented out code
diegoreymendez Nov 11, 2024
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
@@ -0,0 +1,46 @@
//
// UserDefaults+isInternalUser.swift
//
// Copyright © 2024 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 Combine
import Foundation

extension UserDefaults: InternalUserStoring {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

UserDefaults can now be used to store the internal user state so that agents and extensions can access said state.

@objc
public dynamic var isInternalUser: Bool {
get {
bool(forKey: #keyPath(isInternalUser))
}

set {
guard newValue != bool(forKey: #keyPath(isInternalUser)) else {
return
}

guard newValue else {
removeObject(forKey: #keyPath(isInternalUser))
return
}

set(newValue, forKey: #keyPath(isInternalUser))
}
}

var internalUserPublisher: AnyPublisher<Bool, Never> {
publisher(for: \.isInternalUser).eraseToAnyPublisher()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,10 @@ public enum NetworkProtectionSubfeature: String, Equatable, PrivacySubfeature {
/// Display user tips for Network Protection
/// https://app.asana.com/0/72649045549333/1208231259093710/f
case userTips

/// Enforce routes for the VPN to fix TunnelVision
/// https://app.asana.com/0/72649045549333/1208617860225199/f
case enforceRoutes
}

public enum SyncSubfeature: String, PrivacySubfeature {
Expand Down
4 changes: 4 additions & 0 deletions Sources/NetworkProtection/Controllers/TunnelController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ public protocol TunnelController {
///
func stop() async

/// Sends a command to the adapter
///
func command(_ command: VPNCommand) async throws

/// Whether the tunnel is connected
///
var isConnected: Bool { get async }
Expand Down
4 changes: 2 additions & 2 deletions Sources/NetworkProtection/Models/AnyIPAddress.swift
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import Foundation
import Network

public enum AnyIPAddress: IPAddress, Hashable, CustomDebugStringConvertible, @unchecked Sendable {
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 was causing issues because some IP addresses contained by this were converting to empty strings. I'm removing the IPAddress protocol and consumers will instead need to call ipAddress on an instance of this class.

public enum AnyIPAddress: Hashable, CustomDebugStringConvertible, @unchecked Sendable {
/// A host specified as an IPv4 address
case ipv4(IPv4Address)

Expand Down Expand Up @@ -68,7 +68,7 @@ public enum AnyIPAddress: IPAddress, Hashable, CustomDebugStringConvertible, @un
}
}

private var ipAddress: IPAddress {
public var ipAddress: IPAddress {
switch self {
case .ipv4(let ip):
return ip
Expand Down
64 changes: 23 additions & 41 deletions Sources/NetworkProtection/NetworkProtectionDeviceManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,8 @@ public protocol NetworkProtectionDeviceManagement {
typealias GenerateTunnelConfigurationResult = (tunnelConfiguration: TunnelConfiguration, server: NetworkProtectionServer)

func generateTunnelConfiguration(resolvedSelectionMethod: NetworkProtectionServerSelectionMethod,
includedRoutes: [IPAddressRange],
excludedRoutes: [IPAddressRange],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Included and excluded routes will no longer by passed by the client or by settings. They'll be calculated by the extension based on settings.

If we ever need to add support for custom IP ranges, we'll just add the appropriate overrides in settings but still calculate all the routing here.

excludeLocalNetworks: Bool,
dnsSettings: NetworkProtectionDNSSettings,
isKillSwitchEnabled: Bool,
regenerateKey: Bool) async throws -> GenerateTunnelConfigurationResult

}
Expand Down Expand Up @@ -129,10 +127,8 @@ public actor NetworkProtectionDeviceManager: NetworkProtectionDeviceManagement {
/// 3. If the key already existed, look up the stored set of backend servers and check if the preferred server is registered. If not, register it, and return the tunnel configuration + server info.
///
public func generateTunnelConfiguration(resolvedSelectionMethod: NetworkProtectionServerSelectionMethod,
includedRoutes: [IPAddressRange],
excludedRoutes: [IPAddressRange],
excludeLocalNetworks: Bool,
dnsSettings: NetworkProtectionDNSSettings,
isKillSwitchEnabled: Bool,
regenerateKey: Bool) async throws -> GenerateTunnelConfigurationResult {
var keyPair: KeyPair

Expand Down Expand Up @@ -168,10 +164,8 @@ public actor NetworkProtectionDeviceManager: NetworkProtectionDeviceManagement {
do {
let configuration = try tunnelConfiguration(interfacePrivateKey: keyPair.privateKey,
server: selectedServer,
includedRoutes: includedRoutes,
excludedRoutes: excludedRoutes,
dnsSettings: dnsSettings,
isKillSwitchEnabled: isKillSwitchEnabled)
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 bool is not necessary and not really useful. Enforce routes is sort of our kill switch now.

excludeLocalNetworks: excludeLocalNetworks,
dnsSettings: dnsSettings)
return (configuration, selectedServer)
} catch let error as NetworkProtectionError {
errorEvents?.fire(error)
Expand Down Expand Up @@ -259,10 +253,8 @@ public actor NetworkProtectionDeviceManager: NetworkProtectionDeviceManagement {

func tunnelConfiguration(interfacePrivateKey: PrivateKey,
server: NetworkProtectionServer,
includedRoutes: [IPAddressRange],
excludedRoutes: [IPAddressRange],
dnsSettings: NetworkProtectionDNSSettings,
isKillSwitchEnabled: Bool) throws -> TunnelConfiguration {
excludeLocalNetworks: Bool,
dnsSettings: NetworkProtectionDNSSettings) throws -> TunnelConfiguration {

guard let allowedIPs = server.allowedIPs else {
throw NetworkProtectionError.noServerRegistrationInfo
Expand All @@ -285,21 +277,30 @@ public actor NetworkProtectionDeviceManager: NetworkProtectionDeviceManagement {
let dns: [DNSServer]
switch dnsSettings {
case .default:
dns = [DNSServer(address: server.serverInfo.internalIP)]
dns = [DNSServer(address: server.serverInfo.internalIP.ipAddress)]
case .custom(let servers):
dns = servers
.compactMap { IPv4Address($0) }
.map { DNSServer(address: $0) }
}

let interface = interfaceConfiguration(privateKey: interfacePrivateKey,
addressRange: interfaceAddressRange,
includedRoutes: includedRoutes,
excludedRoutes: excludedRoutes,
dns: dns,
isKillSwitchEnabled: isKillSwitchEnabled)
let routingTableResolver = VPNRoutingTableResolver(
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 class takes care of calculating the appropriate routes. Makes it really easy to write unit tests as well.

I'll try to get some in before we merge this.

dnsServers: dns,
excludeLocalNetworks: excludeLocalNetworks)

return TunnelConfiguration(name: "DuckDuckGo VPN", interface: interface, peers: [peerConfiguration])
Logger.networkProtection.log("Routing table information:\nL Included Routes: \(routingTableResolver.includedRoutes, privacy: .public)\nL Excluded Routes: \(routingTableResolver.excludedRoutes, privacy: .public)")
Copy link
Contributor Author

@diegoreymendez diegoreymendez Nov 4, 2024

Choose a reason for hiding this comment

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

I don't think this can contain PII - it'll mostly be mostly static IP ranges and DNS ip for inclusions, then static IP ranges and server IP for exclusions.


let interface = InterfaceConfiguration(privateKey: interfacePrivateKey,
addresses: [interfaceAddressRange],
includedRoutes: routingTableResolver.includedRoutes,
excludedRoutes: routingTableResolver.excludedRoutes,
dns: dns)

let tunnelConfiguration = TunnelConfiguration(name: "DuckDuckGo VPN", interface: interface, peers: [peerConfiguration])

Logger.networkProtection.log("Tunnel configuration routing information:\nL Included Routes: \(tunnelConfiguration.interface.includedRoutes, privacy: .public)\nL Excluded Routes: \(tunnelConfiguration.interface.excludedRoutes, privacy: .public)")

return tunnelConfiguration
}

func peerConfiguration(serverPublicKey: PublicKey, serverEndpoint: Endpoint) -> PeerConfiguration {
Expand All @@ -311,25 +312,6 @@ public actor NetworkProtectionDeviceManager: NetworkProtectionDeviceManagement {
return peerConfiguration
}

func interfaceConfiguration(privateKey: PrivateKey,
addressRange: IPAddressRange,
includedRoutes: [IPAddressRange],
excludedRoutes: [IPAddressRange],
dns: [DNSServer],
isKillSwitchEnabled: Bool) -> InterfaceConfiguration {
var includedRoutes = includedRoutes
// Tunnel doesn‘t work with ‘enforceRoutes‘ option when DNS IP/addressRange is in includedRoutes
if !isKillSwitchEnabled {
includedRoutes.append(contentsOf: dns.map { IPAddressRange(address: $0.address, networkPrefixLength: 32) })
includedRoutes.append(addressRange)
}
return InterfaceConfiguration(privateKey: privateKey,
addresses: [addressRange],
includedRoutes: includedRoutes,
excludedRoutes: excludedRoutes,
dns: dns)
}

private func handle(clientError: NetworkProtectionClientError) {
#if os(macOS)
if case .invalidAuthToken = clientError {
Expand Down
3 changes: 2 additions & 1 deletion Sources/NetworkProtection/NetworkProtectionOptionKey.swift
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,13 @@ public enum NetworkProtectionOptionKey {
public static let selectedServer = "selectedServer"
public static let selectedLocation = "selectedLocation"
public static let dnsSettings = "dnsSettings"
public static let excludeLocalNetworks = "excludeLocalNetworks"
public static let authToken = "authToken"
public static let isOnDemand = "is-on-demand"
public static let activationAttemptId = "activationAttemptId"
public static let tunnelFailureSimulation = "tunnelFailureSimulation"
public static let tunnelFatalErrorCrashSimulation = "tunnelFatalErrorCrashSimulation"
public static let tunnelMemoryCrashSimulation = "tunnelMemoryCrashSimulation"
public static let includedRoutes = "includedRoutes"
public static let connectionTesterEnabled = "connectionTesterEnabled"
public static let settings = "settings"
}
Loading
Loading