Skip to content
This repository was archived by the owner on Feb 24, 2025. It is now read-only.

VPN is sometimes stopped twice #1213

Merged
merged 4 commits into from
Feb 7, 2025
Merged

VPN is sometimes stopped twice #1213

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0ee8730
Removes a line that's causing huge anomalies in the VPN stop metrics
diegoreymendez Feb 5, 2025
f1d44a7
Removes some unused code
diegoreymendez Feb 6, 2025
8138a8a
Improves error information for the VPN
diegoreymendez Feb 6, 2025
2889623
Fixes the automated tests
diegoreymendez Feb 6, 2025
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
79 changes: 20 additions & 59 deletions Sources/NetworkProtection/PacketTunnelProvider.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,10 @@ open class PacketTunnelProvider: NEPacketTunnelProvider {

public enum TunnelError: LocalizedError, CustomNSError, SilentErrorConvertible {
// Tunnel Setup Errors - 0+
case startingTunnelWithoutAuthToken
case startingTunnelWithoutAuthToken(internalError: Error)
case couldNotGenerateTunnelConfiguration(internalError: Error)
case simulateTunnelFailureError
case tokenReset

// Subscription Errors - 100+
case vpnAccessRevoked
Expand All @@ -111,14 +112,16 @@ open class PacketTunnelProvider: NEPacketTunnelProvider {

public var errorDescription: String? {
switch self {
case .startingTunnelWithoutAuthToken:
return "Missing auth token at startup"
case .startingTunnelWithoutAuthToken(let internalError):
return "Missing auth token at startup: \(internalError)"
case .vpnAccessRevoked:
return "VPN disconnected due to expired subscription"
case .couldNotGenerateTunnelConfiguration(let internalError):
return "Failed to generate a tunnel configuration: \(internalError.localizedDescription)"
case .simulateTunnelFailureError:
return "Simulated a tunnel error as requested"
case .tokenReset:
Copy link
Contributor Author

@diegoreymendez diegoreymendez Feb 6, 2025

Choose a reason for hiding this comment

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

Differentiating a specific case that was previously reported as startingTunnelWithoutAuthToken.

return "Abnormal situation caused the token to be reset"
case .appRequestedCancellation:
return nil
}
Expand All @@ -130,6 +133,7 @@ open class PacketTunnelProvider: NEPacketTunnelProvider {
case .startingTunnelWithoutAuthToken: return 0
case .couldNotGenerateTunnelConfiguration: return 1
case .simulateTunnelFailureError: return 2
case .tokenReset: return 3
// Subscription Errors - 100+
case .vpnAccessRevoked: return 100
// State Reset - 200+
Expand All @@ -139,12 +143,13 @@ open class PacketTunnelProvider: NEPacketTunnelProvider {

public var errorUserInfo: [String: Any] {
switch self {
case .startingTunnelWithoutAuthToken,
.simulateTunnelFailureError,
case .simulateTunnelFailureError,
.vpnAccessRevoked,
.appRequestedCancellation:
.appRequestedCancellation,
.tokenReset:
return [:]
case .couldNotGenerateTunnelConfiguration(let underlyingError):
case .couldNotGenerateTunnelConfiguration(let underlyingError),
.startingTunnelWithoutAuthToken(let underlyingError):
return [NSUnderlyingErrorKey: underlyingError]
}
}
Expand Down Expand Up @@ -581,13 +586,13 @@ open class PacketTunnelProvider: NEPacketTunnelProvider {
do {
try await tokenHandler.getToken()
} catch {
throw TunnelError.startingTunnelWithoutAuthToken
throw TunnelError.startingTunnelWithoutAuthToken(internalError: error)
}
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 await tokenHandler.removeToken()
throw TunnelError.startingTunnelWithoutAuthToken
try? await tokenHandler.removeToken()
Copy link
Contributor Author

@diegoreymendez diegoreymendez Feb 6, 2025

Choose a reason for hiding this comment

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

Even if this fails, this isn't the real failure reason - the real failure reason is the token is being removed which, as described above, is an abnormal scenario. The removal failing shouldn't be the error we get.

throw TunnelError.tokenReset
}
}

Expand All @@ -603,22 +608,22 @@ open class PacketTunnelProvider: NEPacketTunnelProvider {
try await tokenHandler.refreshToken()
} catch {
Logger.networkProtection.fault("Error force-refreshing token container: \(error, privacy: .public)\n \(newTokenContainer.refreshToken, privacy: .public)")
throw TunnelError.startingTunnelWithoutAuthToken
throw TunnelError.startingTunnelWithoutAuthToken(internalError: error)
Copy link
Contributor Author

@diegoreymendez diegoreymendez Feb 6, 2025

Choose a reason for hiding this comment

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

Just taking advantage that there's underlying error info.

}
case .useExisting:
Logger.networkProtection.log("Use existing token")
do {
try await tokenHandler.getToken()
} catch {
Logger.networkProtection.fault("Error loading token container: \(error, privacy: .public)")
throw TunnelError.startingTunnelWithoutAuthToken
throw TunnelError.startingTunnelWithoutAuthToken(internalError: error)
}
case .reset:
Logger.networkProtection.log("Reset token")
// 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 await tokenHandler.removeToken()
throw TunnelError.startingTunnelWithoutAuthToken
throw TunnelError.tokenReset
}
}
#endif
Expand Down Expand Up @@ -700,18 +705,7 @@ open class PacketTunnelProvider: NEPacketTunnelProvider {
}

Logger.networkProtection.error("🔴 Stopping VPN due to no auth token")
await cancelTunnel(with: TunnelError.startingTunnelWithoutAuthToken)
Copy link
Contributor Author

@diegoreymendez diegoreymendez Feb 5, 2025

Choose a reason for hiding this comment

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

No need to cancel the tunnel if we're throwing an error below, which is the right way to cancel startup.

Copy link
Contributor

@samsymons samsymons Feb 6, 2025

Choose a reason for hiding this comment

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

Nice, yeah good catch.


// Check that the error is valid and able to be re-thrown to the OS before shutting the tunnel down
if let wrappedError = wrapped(error: error) {
// Wait for the provider to complete its pixel request.
try? await Task.sleep(interval: .seconds(2))
throw wrappedError
} else {
// Wait for the provider to complete its pixel request.
try? await Task.sleep(interval: .seconds(2))
throw error
}
throw error
}

do {
Expand Down Expand Up @@ -740,17 +734,7 @@ open class PacketTunnelProvider: NEPacketTunnelProvider {
self.knownFailureStore.lastKnownFailure = KnownFailure(error)

providerEvents.fire(.tunnelStartAttempt(.failure(error)))

// Check that the error is valid and able to be re-thrown to the OS before shutting the tunnel down
if let wrappedError = wrapped(error: error) {
// Wait for the provider to complete its pixel request.
try? await Task.sleep(interval: .seconds(2))
throw wrappedError
} else {
// Wait for the provider to complete its pixel request.
try? await Task.sleep(interval: .seconds(2))
throw error
}
throw error
}
}

Expand Down Expand Up @@ -1825,29 +1809,6 @@ open class PacketTunnelProvider: NEPacketTunnelProvider {
}
}
}

/// Wraps an error instance in a new error type in cases where it is malformed; i.e., doesn't use an `NSError` instance for its underlying error, etc.
private func wrapped(error: Error) -> Error? {
if containsValidUnderlyingError(error) {
return nil
} else {
return InvalidDiagnosticError.errorWithInvalidUnderlyingError(error)
}
}

private func containsValidUnderlyingError(_ error: Error) -> Bool {
let nsError = error as NSError

if let underlyingError = nsError.userInfo[NSUnderlyingErrorKey] as? Error {
return containsValidUnderlyingError(underlyingError)
} else if nsError.userInfo[NSUnderlyingErrorKey] != nil {
// If `NSUnderlyingErrorKey` exists but is not an `Error`, return false
return false
}

return true
}

}

extension WireGuardAdapterError: LocalizedError, CustomDebugStringConvertible {
Expand Down
Loading