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" #3460

Merged
merged 29 commits into from
Nov 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
f7d68eb
Fixes TunnelVision
diegoreymendez Oct 20, 2024
30c6008
Enabled for internal users
diegoreymendez Oct 24, 2024
accb12c
Adds some documentation
diegoreymendez Oct 24, 2024
daac8ec
Merge branch 'main' into diego/fix-tunnelvision-2
diegoreymendez Oct 24, 2024
7a75b3b
Adds the enforceRoutes feature flag
diegoreymendez Oct 24, 2024
ccbb094
WIP
diegoreymendez Oct 24, 2024
b0129c2
Some changes to better support enforceRoutes
diegoreymendez Oct 26, 2024
659b3e1
WIP
diegoreymendez Nov 4, 2024
3db14a6
Merges the latest from main
diegoreymendez Nov 4, 2024
e033d3c
Added support for the enforce routes remote feature flag
diegoreymendez Nov 4, 2024
eeedc09
Updates BSK
diegoreymendez Nov 4, 2024
282ebb7
Enables excludeLocalNetworks by default for the new routing changes
diegoreymendez Nov 4, 2024
31b4e29
Updates BSK
diegoreymendez Nov 6, 2024
cb5f1aa
Corrects a comment
diegoreymendez Nov 6, 2024
84a23cc
Rolls back some unnecessary changes
diegoreymendez Nov 6, 2024
f59b439
Updates BSK
diegoreymendez Nov 6, 2024
39e7b80
Updates BSK
diegoreymendez Nov 6, 2024
96bde82
Updates BSK
diegoreymendez Nov 6, 2024
51bc65b
Updates BSK
diegoreymendez Nov 6, 2024
a2c27a4
Updates BSK
diegoreymendez Nov 6, 2024
d0704a6
Improvements to exclude-local-networks for the VPN
diegoreymendez Nov 8, 2024
a35f947
Updates BSK
diegoreymendez Nov 8, 2024
d1e97cc
Updates BSK
diegoreymendez Nov 8, 2024
343b23b
Updates BSK
diegoreymendez Nov 8, 2024
5cfe2e9
Made some changes suggested by review feedback
diegoreymendez Nov 8, 2024
115aa17
Merges the latest from main
diegoreymendez Nov 11, 2024
6a4131d
Updates BSK
diegoreymendez Nov 11, 2024
c42ac87
Rolls back an unintentional config change
diegoreymendez Nov 11, 2024
b42c9a6
Disabled a flaky test
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
5 changes: 5 additions & 0 deletions Core/FeatureFlag.swift
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ public enum FeatureFlag: String {

/// https://app.asana.com/0/72649045549333/1208231259093710/f
case networkProtectionUserTips

/// https://app.asana.com/0/72649045549333/1208617860225199/f
case networkProtectionEnforceRoutes
}

extension FeatureFlag: FeatureFlagSourceProviding {
Expand Down Expand Up @@ -104,6 +107,8 @@ extension FeatureFlag: FeatureFlagSourceProviding {
return .remoteReleasable(.feature(.autocompleteTabs))
case .networkProtectionUserTips:
return .remoteReleasable(.subfeature(NetworkProtectionSubfeature.userTips))
case .networkProtectionEnforceRoutes:
return .remoteDevelopment(.subfeature(NetworkProtectionSubfeature.enforceRoutes))
case .adAttributionReporting:
return .remoteReleasable(.feature(.adAttributionReporting))
}
Expand Down
2 changes: 1 addition & 1 deletion DuckDuckGo.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -10980,7 +10980,7 @@
repositoryURL = "https://github.com/DuckDuckGo/BrowserServicesKit";
requirement = {
kind = exactVersion;
version = 208.1.0;
version = 209.0.0;
};
};
9F8FE9472BAE50E50071E372 /* XCRemoteSwiftPackageReference "lottie-spm" */ = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@
"kind" : "remoteSourceControl",
"location" : "https://github.com/DuckDuckGo/BrowserServicesKit",
"state" : {
"revision" : "6be781530a2516c703b8e1bcf0c90e6e763d3300",
"version" : "208.1.0"
"revision" : "614ea57db48db644ce7f3a3de9c20c9a7fbb08ff",
"version" : "209.0.0"
}
},
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@
<Test
Identifier = "AutoconsentMessageProtocolTests/testEval()">
</Test>
<Test
Identifier = "FireButtonReferenceTests/testClearDataUsingLegacyContainer()">
</Test>
<Test
Identifier = "SyncSettingsViewControllerErrorTests/test_WhenAccountRemoved_ErrorHandlerSyncDidTurnOffCalled()">
</Test>
Expand Down
15 changes: 6 additions & 9 deletions DuckDuckGo/AppDependencyProvider.swift
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ final class AppDependencyProvider: DependencyProvider {
entitlementsCache: entitlementsCache,
subscriptionEndpointService: subscriptionService,
authEndpointService: authService)

let subscriptionManager = DefaultSubscriptionManager(storePurchaseManager: DefaultStorePurchaseManager(),
accountManager: accountManager,
subscriptionEndpointService: subscriptionService,
Expand All @@ -126,17 +126,14 @@ final class AppDependencyProvider: DependencyProvider {
let accessTokenProvider: () -> String? = {
return { accountManager.accessToken }
}()
#if os(macOS)
networkProtectionKeychainTokenStore = NetworkProtectionKeychainTokenStore(keychainType: .dataProtection(.unspecified),
serviceName: "\(Bundle.main.bundleIdentifier!).authToken",
errorEvents: .networkProtectionAppDebugEvents,
accessTokenProvider: accessTokenProvider)
#else

networkProtectionKeychainTokenStore = NetworkProtectionKeychainTokenStore(accessTokenProvider: accessTokenProvider)
#endif

networkProtectionTunnelController = NetworkProtectionTunnelController(accountManager: accountManager,
tokenStore: networkProtectionKeychainTokenStore,
persistentPixel: persistentPixel)
featureFlagger: featureFlagger,
persistentPixel: persistentPixel,
settings: vpnSettings)
vpnFeatureVisibility = DefaultNetworkProtectionVisibility(userDefaults: .networkProtectionGroupDefaults,
accountManager: accountManager)
}
Expand Down
1 change: 1 addition & 0 deletions DuckDuckGo/NetworkProtectionConvenienceInitialisers.swift
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ extension NetworkProtectionVPNSettingsViewModel {
convenience init() {
self.init(
notificationsAuthorization: NotificationsAuthorizationController(),
controller: AppDependencyProvider.shared.networkProtectionTunnelController,
settings: AppDependencyProvider.shared.vpnSettings
)
}
Expand Down
12 changes: 12 additions & 0 deletions DuckDuckGo/NetworkProtectionDebugViewController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ final class NetworkProtectionDebugViewController: UITableViewController {

enum DebugFeatureRows: Int, CaseIterable {
case toggleAlwaysOn
case enforceRoutes
}

enum SimulateFailureRows: Int, CaseIterable {
Expand Down Expand Up @@ -324,6 +325,14 @@ final class NetworkProtectionDebugViewController: UITableViewController {
} else {
cell.accessoryType = .checkmark
}
case .enforceRoutes:
cell.textLabel?.text = "Enforce Routes"

if !AppDependencyProvider.shared.vpnSettings.enforceRoutes {
cell.accessoryType = .none
} else {
cell.accessoryType = .checkmark
}
default:
break
}
Expand All @@ -334,6 +343,9 @@ final class NetworkProtectionDebugViewController: UITableViewController {
case .toggleAlwaysOn:
debugFeatures.alwaysOnDisabled.toggle()
tableView.reloadRows(at: [indexPath], with: .none)
case .enforceRoutes:
AppDependencyProvider.shared.vpnSettings.enforceRoutes.toggle()
tableView.reloadRows(at: [indexPath], with: .none)
default:
break
}
Expand Down
52 changes: 49 additions & 3 deletions DuckDuckGo/NetworkProtectionTunnelController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,10 @@
// limitations under the License.
//

import Foundation
import BrowserServicesKit
import Combine
import Core
import Foundation
import NetworkExtension
import NetworkProtection
import Subscription
Expand All @@ -34,6 +35,7 @@ enum VPNConfigurationRemovalReason: String {
final class NetworkProtectionTunnelController: TunnelController, TunnelSessionProvider {
static var shouldSimulateFailure: Bool = false

private let featureFlagger: FeatureFlagger
private var internalManager: NETunnelProviderManager?
private let debugFeatures = NetworkProtectionDebugFeatures()
private let tokenStore: NetworkProtectionKeychainTokenStore
Expand All @@ -42,6 +44,7 @@ final class NetworkProtectionTunnelController: TunnelController, TunnelSessionPr
private let notificationCenter: NotificationCenter = .default
private var previousStatus: NEVPNStatus = .invalid
private let persistentPixel: PersistentPixelFiring
private let settings: VPNSettings
private var cancellables = Set<AnyCancellable>()

// MARK: - Manager, Session, & Connection
Expand Down Expand Up @@ -119,9 +122,25 @@ final class NetworkProtectionTunnelController: TunnelController, TunnelSessionPr
}
}

init(accountManager: AccountManager, tokenStore: NetworkProtectionKeychainTokenStore, persistentPixel: PersistentPixelFiring) {
self.tokenStore = tokenStore
// MARK: - Enforce Routes

private var enforceRoutes: Bool {
featureFlagger.isFeatureOn(.networkProtectionEnforceRoutes)
}

// MARK: - Initializers

init(accountManager: AccountManager,
tokenStore: NetworkProtectionKeychainTokenStore,
featureFlagger: FeatureFlagger,
persistentPixel: PersistentPixelFiring,
settings: VPNSettings) {

self.featureFlagger = featureFlagger
self.persistentPixel = persistentPixel
self.settings = settings
self.tokenStore = tokenStore

subscribeToSnoozeTimingChanges()
subscribeToStatusChanges()
subscribeToConfigurationChanges()
Expand Down Expand Up @@ -180,6 +199,16 @@ final class NetworkProtectionTunnelController: TunnelController, TunnelSessionPr
tunnelManager.connection.stopVPNTunnel()
}

func command(_ command: VPNCommand) async throws {
guard let activeSession = await AppDependencyProvider.shared.networkProtectionTunnelController.activeSession(),
activeSession.status == .connected else {

return
}

try? await activeSession.sendProviderRequest(.command(command))
}

func removeVPN(reason: VPNConfigurationRemovalReason) async {
do {
try await tunnelManager?.removeFromPreferences()
Expand Down Expand Up @@ -293,6 +322,7 @@ final class NetworkProtectionTunnelController: TunnelController, TunnelSessionPr
return tunnelManager
}

@MainActor
private func setupAndSave(_ tunnelManager: NETunnelProviderManager) async throws {
setup(tunnelManager)

Expand All @@ -319,6 +349,7 @@ final class NetworkProtectionTunnelController: TunnelController, TunnelSessionPr

/// Setups the tunnel manager if it's not set up already.
///
@MainActor
private func setup(_ tunnelManager: NETunnelProviderManager) {
tunnelManager.localizedDescription = "DuckDuckGo VPN"
tunnelManager.isEnabled = true
Expand All @@ -327,9 +358,24 @@ final class NetworkProtectionTunnelController: TunnelController, TunnelSessionPr
let protocolConfiguration = NETunnelProviderProtocol()
protocolConfiguration.serverAddress = "127.0.0.1" // Dummy address... the NetP service will take care of grabbing a real server

protocolConfiguration.providerConfiguration = [:]

// always-on
protocolConfiguration.disconnectOnSleep = false

// Enforce routes
protocolConfiguration.enforceRoutes = enforceRoutes

// We will control excluded networks through includedRoutes / excludedRoutes
protocolConfiguration.excludeLocalNetworks = false

#if DEBUG
if #available(iOS 17.4, *) {
// This is useful to ensure debugging is never blocked by the VPN
protocolConfiguration.excludeDeviceCommunication = true
diegoreymendez marked this conversation as resolved.
Show resolved Hide resolved
}
#endif

return protocolConfiguration
}()

Expand Down
3 changes: 0 additions & 3 deletions DuckDuckGo/NetworkProtectionVPNSettingsView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,6 @@ struct NetworkProtectionVPNSettingsView: View {
footerText: UserText.netPExcludeLocalNetworksSettingFooter
) {
Toggle("", isOn: $viewModel.excludeLocalNetworks)
.onTapGesture {
viewModel.toggleExcludeLocalNetworks()
}
Comment on lines -53 to -55
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 not really working at all, and adding to the overall problems.

}

dnsSection()
Expand Down
30 changes: 24 additions & 6 deletions DuckDuckGo/NetworkProtectionVPNSettingsViewModel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ enum NetworkProtectionNotificationsViewKind: Equatable {
}

final class NetworkProtectionVPNSettingsViewModel: ObservableObject {
private let controller: TunnelController
private let settings: VPNSettings
private var cancellables: Set<AnyCancellable> = []

Expand All @@ -39,11 +40,32 @@ final class NetworkProtectionVPNSettingsViewModel: ObservableObject {
self.settings.notifyStatusChanges
}

@Published public var excludeLocalNetworks: Bool = true
@Published public var excludeLocalNetworks: Bool {
didSet {
guard oldValue != excludeLocalNetworks else {
return
}

settings.excludeLocalNetworks = excludeLocalNetworks

Task {
// We need to allow some time for the setting to propagate
// But ultimately this should actually be a user choice
try await Task.sleep(interval: 0.1)
try await controller.command(.restartAdapter)
}
}
}

@Published public var usesCustomDNS = false
@Published public var dnsServers: String = UserText.vpnSettingDNSServerDefaultValue

init(notificationsAuthorization: NotificationsAuthorizationControlling, settings: VPNSettings) {
init(notificationsAuthorization: NotificationsAuthorizationControlling,
controller: TunnelController,
settings: VPNSettings) {

self.controller = controller
self.excludeLocalNetworks = settings.excludeLocalNetworks
self.settings = settings
self.notificationsAuthorization = notificationsAuthorization

Expand Down Expand Up @@ -77,10 +99,6 @@ final class NetworkProtectionVPNSettingsViewModel: ObservableObject {
settings.notifyStatusChanges = enabled
}

func toggleExcludeLocalNetworks() {
settings.excludeLocalNetworks.toggle()
}

private static func localizedString(forRegionCode: String) -> String {
Locale.current.localizedString(forRegionCode: forRegionCode) ?? forRegionCode.capitalized
}
Expand Down
Loading