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

diegoreymendez · 2024-10-29T14:21:35Z

Task/Issue URL: https://app.asana.com/0/1206580121312550/1208686409805161/f
Tech Design URL: https://app.asana.com/0/481882893211075/1208643192597095/f

iOS PR: duckduckgo/iOS#3460
macOS PR: duckduckgo/macos-browser#3422
What kind of version bump will this require?: Major

Description

Remediate TunnelVision, TunnelCrack and fix "Exclude Local Networks".

Testing

Please refer to the platform-specific PRs for testing instructions.

Internal references:

Software Engineering Expectations
Technical Design Template

diegoreymendez · 2024-11-04T15:42:19Z

Sources/BrowserServicesKit/InternalUserDecider/UserDefaults+isInternalUser.swift

+import Combine
+import Foundation
+
+extension UserDefaults: InternalUserStoring {


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

diegoreymendez · 2024-11-04T15:43:25Z

Sources/NetworkProtection/Models/AnyIPAddress.swift

@@ -19,7 +19,7 @@
 import Foundation
 import Network

-public enum AnyIPAddress: IPAddress, Hashable, CustomDebugStringConvertible, @unchecked Sendable {


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.

diegoreymendez · 2024-11-04T15:44:28Z

Sources/NetworkProtection/NetworkProtectionDeviceManager.swift

@@ -65,10 +65,8 @@ public protocol NetworkProtectionDeviceManagement {
    typealias GenerateTunnelConfigurationResult = (tunnelConfiguration: TunnelConfiguration, server: NetworkProtectionServer)

    func generateTunnelConfiguration(resolvedSelectionMethod: NetworkProtectionServerSelectionMethod,
-                                     includedRoutes: [IPAddressRange],
-                                     excludedRoutes: [IPAddressRange],


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.

diegoreymendez · 2024-11-04T15:44:59Z

Sources/NetworkProtection/NetworkProtectionDeviceManager.swift

-                                                        includedRoutes: includedRoutes,
-                                                        excludedRoutes: excludedRoutes,
-                                                        dnsSettings: dnsSettings,
-                                                        isKillSwitchEnabled: isKillSwitchEnabled)


This bool is not necessary and not really useful. Enforce routes is sort of our kill switch now.

diegoreymendez · 2024-11-04T15:46:09Z

Sources/NetworkProtection/NetworkProtectionDeviceManager.swift


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


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.

diegoreymendez · 2024-11-04T15:47:03Z

Sources/NetworkProtection/NetworkProtectionDeviceManager.swift

-                                               excludedRoutes: excludedRoutes,
-                                               dns: dns,
-                                               isKillSwitchEnabled: isKillSwitchEnabled)
+        let routingTableResolver = VPNRoutingTableResolver(


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.

Sources/NetworkProtection/PacketTunnelProvider.swift

diegoreymendez · 2024-11-04T15:48:59Z

Sources/NetworkProtection/PacketTunnelProvider.swift

            Logger.networkProtection.log("Generating tunnel config")
-            Logger.networkProtection.log("Excluded ranges are: \(String(describing: self.settings.excludedRanges), privacy: .public)")
            Logger.networkProtection.log("Server selection method: \(self.currentServerSelectionMethod.debugDescription, privacy: .public)")
            Logger.networkProtection.log("DNS server: \(String(describing: self.settings.dnsSettings), privacy: .public)")
-            let tunnelConfiguration = try await generateTunnelConfiguration(serverSelectionMethod: currentServerSelectionMethod,
-                                                                            includedRoutes: includedRoutes ?? [],
-                                                                            excludedRoutes: settings.excludedRanges,
-                                                                            dnsSettings: settings.dnsSettings,
-                                                                            regenerateKey: true)
+            let tunnelConfiguration = try await generateTunnelConfiguration(
+                serverSelectionMethod: currentServerSelectionMethod,
+                dnsSettings: settings.dnsSettings,
+                regenerateKey: true)
+


Routes are no longer passed into the PacketTunnelProvider.

diegoreymendez · 2024-11-04T15:49:56Z

Sources/NetworkProtection/PacketTunnelProvider.swift

-        case .setExcludeLocalNetworks:
-            if case .connected = connectionStatus {
-                try await updateTunnelConfiguration(
-                    updateMethod: .selectServer(currentServerSelectionMethod),
-                    reassert: false)
-            }


Not sure if the extension or main app should decide when to restart the adapter. Maybe I should just do it here?

But doing it here prevents us from doing batch changes - although to be honest we don't do them in the main app either as settings are individual :)

diegoreymendez · 2024-11-04T15:50:45Z

Sources/NetworkProtection/PacketTunnelProvider.swift

+    private func handleRestartAdapter() async throws {
+        let tunnelConfiguration = try await generateTunnelConfiguration(
+            serverSelectionMethod: currentServerSelectionMethod,
+            dnsSettings: settings.dnsSettings,
+            regenerateKey: false)
+
+        try await updateTunnelConfiguration(updateMethod: .useConfiguration(tunnelConfiguration),
+                                            reassert: false,
+                                            regenerateKey: false)
+    }


Convenience method for restarting the adapter very quickly and without going through server re-registration.

diegoreymendez · 2024-11-04T15:51:51Z

Sources/NetworkProtection/Settings/RoutingRange.swift

-
-import Foundation
-
-public enum RoutingRange {


This whole doesn't make sense anymore. We don't need to offer to the user this level of granularity and if we ever do, it's likely to be using a different mechanism.

...NetworkProtection/Status/ConnectionErrorObserver/ConnectionErrorObserverThroughSession.swift

not-a-rootkit

LGTM, just some open questions on the 10.0.0.0 IP range and a nit with left-in commented-out code.

Not a blocker, but open question: do you think there is a way to test the routing table in isolation? I don't see an obvious way that doesn't involve quite a bit of work, but you know more than me :)

not-a-rootkit · 2024-11-08T17:03:14Z

Sources/NetworkProtection/Routing/VPNRoutingRange.swift

+    ]
+
+    public static let localNetworkRange: [NetworkProtection.IPAddressRange] = [
+        // "10.0.0.0/8",     /* 255.0.0.0   */


Is this commented out because it clashes with our VPN tunnel IP range? Should we add ranges surrounding the tunnel range?

I'm matching Android here, where they exclude the 10.x.x.x range because it's used for DNS and generally reserved for out VPN.

I do think we should included everything but the DNS but since Android is doing this, I'm assuming it's good enough for us, for now.

Tests/NetworkProtectionTests/Mocks/NetworkProtectionServerMocks.swift

diegoreymendez · 2024-11-11T20:03:27Z

Not a blocker, but open question: do you think there is a way to test the routing table in isolation? I don't see an obvious way that doesn't involve quite a bit of work, but you know more than me :)

I think we should add tests to make sure the ranges are fine, but I also don't want to be blocking this PR for that now. I promise I'll push something soon to cover this.

Testing what the system does with the ranges is, IMO, a loosing battle - but we can test that our system is behaving well and delivering the correct routes to the OS.

…3422) Task/Issue URL: https://app.asana.com/0/1206580121312550/1208686409805161/f Tech Design URL: https://app.asana.com/0/481882893211075/1208643192597095/f iOS PR: iOS PR: duckduckgo/iOS#3460 BSK PR: duckduckgo/BrowserServicesKit#1039 ## Description Remediate TunnelVision, TunnelCrack and fix "Exclude Local Networks".

…3460) Task/Issue URL: https://app.asana.com/0/1206580121312550/1208686409805161/f Tech Design URL: https://app.asana.com/0/481882893211075/1208643192597095/f macOS PR: duckduckgo/macos-browser#3422 BSK PR: duckduckgo/BrowserServicesKit#1039 ## Description Remediate TunnelVision, TunnelCrack and fix "Exclude Local Networks".

* main: Remediate TunnelVision, TunnelCrack and fix "Exclude Local Networks" (#1039) Sync: Send pixels for account removal + decoding issues (#1065) Bump github.com/duckduckgo/content-scope-scripts from 6.31.0 to 6.32.0 (#1067) Behavioral toast updates (#1063) point to CSS branch (#1051)

diegoreymendez added 3 commits October 24, 2024 12:57

Adds the VPN's enforceRoutes feature flag

71c861c

Adds logic to properly handle DNS route inclusion

5de4c50

Some improvements to better support enforceRoutes

a817b6e

diegoreymendez changed the title ~~Diego/fix tunnelvision 2~~ Fix TunnelVision using enforceRoutes Oct 29, 2024

diegoreymendez added 7 commits October 31, 2024 14:24

WIP

8287540

Cleaned up the code to remove a lot of unnecessary logic

2e5f409

Additional cleanup around VPN routing

3d18455

Cleanup and fixing an error in iOS

37c83d3

Merge branch 'main' into diego/fix-tunnelvision-2

cbed087

Adds a memory for force-enabling enforce routes for feature flag users

66088d0

Added a new file that was missing from the previous commit

432b548

This was referenced Nov 4, 2024

Remediate TunnelVision, TunnelCrack and fix "Exclude Local Networks" duckduckgo/iOS#3460

Merged

Remediate TunnelVision, TunnelCrack and fix "Exclude Local Networks" duckduckgo/macos-browser#3422

Merged

diegoreymendez commented Nov 4, 2024

View reviewed changes

Sources/NetworkProtection/PacketTunnelProvider.swift Outdated Show resolved Hide resolved

diegoreymendez commented Nov 4, 2024

View reviewed changes

Sources/NetworkProtection/PacketTunnelProvider.swift Outdated Show resolved Hide resolved

diegoreymendez commented Nov 4, 2024

View reviewed changes

...NetworkProtection/Status/ConnectionErrorObserver/ConnectionErrorObserverThroughSession.swift Outdated Show resolved Hide resolved

diegoreymendez assigned samsymons Nov 4, 2024

diegoreymendez marked this pull request as ready for review November 4, 2024 15:54

Fixes an issue with exclude local networks

7e9c307

diegoreymendez changed the title ~~Fix TunnelVision using enforceRoutes~~ Remediate TunnelVision, TunnelCrack and fix "Exclude Local Networks" Nov 6, 2024

diegoreymendez added 4 commits November 6, 2024 14:54

Fixes several swiftlint warnings

86ddb4c

Merge branch 'main' into diego/fix-tunnelvision-2

a304582

Fixes unit tests

9b038e2

Re-enabled some code that was disabled by mistake

725579d

diegoreymendez requested a review from samsymons November 6, 2024 14:28

Fixes unit tests

d9cac68

diegoreymendez assigned diegoreymendez and unassigned samsymons Nov 6, 2024

diegoreymendez added 5 commits November 8, 2024 00:00

Updates the tunnel configuration to work best against TunnelCrack

0ba1e70

Included local networks again

2c34dbd

Updates the excluded routes

b717437

Fixes a swiflint warning

e89ceb7

Rolls back an unintentional change

f05ad5e

diegoreymendez requested a review from not-a-rootkit November 8, 2024 12:42

Updates a protocol

0dead1b

not-a-rootkit approved these changes Nov 8, 2024

View reviewed changes

diegoreymendez added 3 commits November 11, 2024 20:05

Fixes an issue with the unit tests

bc7bf68

Merge branch 'main' into diego/fix-tunnelvision-2

fa6b96b

Removes some commented out code

2ded3f8

diegoreymendez merged commit 614ea57 into main Nov 11, 2024
7 checks passed

diegoreymendez deleted the diego/fix-tunnelvision-2 branch November 11, 2024 20:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

diegoreymendez commented Oct 29, 2024 •

edited

Loading

diegoreymendez Nov 4, 2024

diegoreymendez Nov 4, 2024

diegoreymendez Nov 4, 2024

diegoreymendez Nov 4, 2024

diegoreymendez Nov 4, 2024 •

edited

Loading

diegoreymendez Nov 4, 2024

diegoreymendez Nov 4, 2024

diegoreymendez Nov 4, 2024 •

edited

Loading

diegoreymendez Nov 4, 2024

diegoreymendez Nov 4, 2024 •

edited

Loading

not-a-rootkit left a comment

not-a-rootkit Nov 8, 2024

diegoreymendez Nov 11, 2024

diegoreymendez commented Nov 11, 2024


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

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

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

Conversation

diegoreymendez commented Oct 29, 2024 • edited Loading

Description

Testing

Internal references:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegoreymendez Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegoreymendez Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegoreymendez Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

not-a-rootkit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegoreymendez commented Nov 11, 2024

diegoreymendez commented Oct 29, 2024 •

edited

Loading

diegoreymendez Nov 4, 2024 •

edited

Loading

diegoreymendez Nov 4, 2024 •

edited

Loading

diegoreymendez Nov 4, 2024 •

edited

Loading