-
Notifications
You must be signed in to change notification settings - Fork 33
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
Changes from 10 commits
71c861c
5de4c50
a817b6e
8287540
2e5f409
3d18455
37c83d3
cbed087
66088d0
432b548
7e9c307
86ddb4c
a304582
9b038e2
725579d
d9cac68
0ba1e70
2c34dbd
b717437
e89ceb7
f05ad5e
0dead1b
bc7bf68
fa6b96b
2ded3f8
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 |
---|---|---|
@@ -0,0 +1,46 @@ | ||
// | ||
// UserDefaults+internalUser.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 { | ||
@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 |
---|---|---|
|
@@ -19,7 +19,7 @@ | |
import Foundation | ||
import Network | ||
|
||
public enum AnyIPAddress: IPAddress, Hashable, CustomDebugStringConvertible, @unchecked Sendable { | ||
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 was causing issues because some IP addresses contained by this were converting to empty strings. I'm removing the |
||
public enum AnyIPAddress: Hashable, CustomDebugStringConvertible, @unchecked Sendable { | ||
/// A host specified as an IPv4 address | ||
case ipv4(IPv4Address) | ||
|
||
|
@@ -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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,10 +65,8 @@ public protocol NetworkProtectionDeviceManagement { | |
typealias GenerateTunnelConfigurationResult = (tunnelConfiguration: TunnelConfiguration, server: NetworkProtectionServer) | ||
|
||
func generateTunnelConfiguration(resolvedSelectionMethod: NetworkProtectionServerSelectionMethod, | ||
includedRoutes: [IPAddressRange], | ||
excludedRoutes: [IPAddressRange], | ||
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. 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 | ||
|
||
} | ||
|
@@ -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 | ||
|
||
|
@@ -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) | ||
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 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) | ||
|
@@ -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 | ||
|
@@ -285,21 +277,31 @@ 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( | ||
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 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. |
||
server: server, | ||
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)") | ||
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. 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 { | ||
|
@@ -311,25 +313,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 { | ||
|
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.
UserDefaults
can now be used to store the internal user state so that agents and extensions can access said state.