Prevent iOS from stopping the tunnel if it remains in connecting state for too long

[rablador]

Since iOS 16.5 NE.startTunnel() has a 60s timeout to return from that method. To handle this the idea is to never keep the VPN extension in connecting state and instead return immediately from NE.startTunnel() to make the system think that the tunnel is established to prevent being shutdown by a system timer.

Internally the actor will use its own state and will remain in the actual state that we expect the tunnel to be in. Only NEVPNStatus will be a lie.

---

This change is 

[linear]

IOS-323 iOS stops the tunnel if it remains in connecting state for too long

Lately, while working on a tunnel state, I found that the system automatically stops the tunnel, if it cannot progress from connecting to connected state after some time. The reason given in that scenario is connectionFailed.

I am pretty sure that it was documented somewhere that the system would do that after 30 seconds but I can't really find a mention of it any longer nor do I really recall that being an issue in the past but things might have changed or slipped somehow.

The way to reproduce that is very simply:

• Make sure that network extension is not running. You can uncheck the on-demand in VPN configuration in system settings to prevent it from starting automatically.
• Enable network link conditioner and use a profile that drops network traffic. Personally I use 90% packet loss.
• Start network extension from system settings.
• After about 30 seconds the tunnel will automatically disconnect. You can either observe logs or put a breakpoint in stopTunnel() call in the PacketTunnelProvider to catch when that happens.

Another issue is that the tunnel will be killed when it enters the blocked state on startup - this should be prevented.

There are some possible solutions:

• Wait a few attempts and then regardless if we connected to a tunnel or not, issue a signal that the packet tunnel connected and then reassert the connection
• Immediately upon the start of the packet tunnel, assert that a connection has been made and then revert back to the reasserting state.

[rablador]

Reviewable status: 0 of 14 files reviewed, 1 unresolved discussion

---

ios/MullvadVPN/TunnelManager/MapConnectionStatusOperation.swift line 63 at r1 (raw file):

                    return .error(blockedState.reason)
                default:
                    return .none

What to return here?

[buggmagnet]

Reviewed 13 of 14 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @rablador)

---

ios/MullvadVPN/TunnelManager/MapConnectionStatusOperation.swift line 94 at r2 (raw file):

        default:
            interactor.updateTunnelStatus { tunnelStatus in
                let isNetworkReachable = tunnelStatus.observedState.connectionState?.isNetworkReachable ?? false

nit
Should we care about reachability when we are disconnecting, since we will soon be disconnected and shut down the VPN ?
Maybe we should just force tunnelStatus.state = .disconnecting(nothing) here.
What do you think @pronebird ?

---

ios/MullvadVPN/TunnelManager/MapConnectionStatusOperation.swift line 135 at r2 (raw file):

        mapToState: @escaping (ObservedState) -> TunnelState?
    ) {
        request = tunnel.getTunnelStatus { [weak self] completion in

nit
let's rename this result

---

ios/MullvadVPN/View controllers/Tunnel/TunnelControlView.swift line 226 at r2 (raw file):

            connectionPanel.dataSource = ConnectionPanelData(
                inAddress: "\(tunnelRelay.endpoint.ipv4Relay) UDP",

Nit
Note for future @buggmagnet , should we change this dynamically based on whether we're obfuscating the connection via TCP ?

Yes we should. That's what the desktop does.

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 105 at r2 (raw file):

        for await state in await actor.observedStates {
            switch state {
            case .connected:

As discussed with @rablador, we established that we should probably respect the previous behaviour where we were also returning normally in case we were already disconnected here.

---

ios/PacketTunnelCore/Actor/ObservedState.swift line 59 at r2 (raw file):

    }

    public init(

This is only used for the simulator, we should probably hide this behind an #if targetEnvironment(simulator) statement

[rablador]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadVPN/TunnelManager/MapConnectionStatusOperation.swift line 135 at r2 (raw file):

Previously, buggmagnet wrote…

nit
let's rename this result

Done.

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 105 at r2 (raw file):

Previously, buggmagnet wrote…

As discussed with @rablador, we established that we should probably respect the previous behaviour where we were also returning normally in case we were already disconnected here.

Done.

[buggmagnet]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @rablador)

[rablador]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet)

---

ios/PacketTunnelCore/Actor/ObservedState.swift line 59 at r2 (raw file):

Previously, buggmagnet wrote…

This is only used for the simulator, we should probably hide this behind an #if targetEnvironment(simulator) statement

Would have liked making a local extension in SimulatorTunnelProviderHost with the init, but that's not really doable with a struct. Should I just wrap it in targetEnv here and call it a day?

[pronebird]

Reviewable status: 14 of 15 files reviewed, 6 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/PacketTunnelCore/Actor/ObservedState.swift line 59 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

Would have liked making a local extension in SimulatorTunnelProviderHost with the init, but that's not really doable with a struct. Should I just wrap it in targetEnv here and call it a day?

It's unusual that the framework has to adapt to the way it's being used in the application target. I'd say we should keep init() public and not over-engineer this.

---

ios/PacketTunnelCore/Actor/ObservedState.swift line 24 at r4 (raw file):

    case error(ObservedBlockedState)

    public var connectionState: ObservedConnectionState? {

Ideally we want to encourage consumer to do the proper matching. So these helpers that extract the associated values should probably be defined as extensions in the target doing this.

---

ios/PacketTunnelCore/Actor/ObservedState.swift line 37 at r4 (raw file):

    }

    public var blockedState: ObservedBlockedState? {

Same goes here.

---

ios/PacketTunnelCore/Actor/ObservedState.swift line 56 at r4 (raw file):

    public var isNetworkReachable: Bool {
        networkReachability != .unreachable

The whole point of introducing NetworkReachability was to avoid making assumption that the undetermined state means we have internet. I understand that we may have to bridge some gaps before we start using it as it was intended, but them maybe the framework itself does not have to serve these shortcuts for the main target and instead we could define extension where it's used this way.

[pronebird]

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPN/TunnelManager/MapConnectionStatusOperation.swift line 94 at r2 (raw file):

Previously, buggmagnet wrote…

nit
Should we care about reachability when we are disconnecting, since we will soon be disconnected and shut down the VPN ?
Maybe we should just force tunnelStatus.state = .disconnecting(nothing) here.
What do you think @pronebird ?

I think it's fine. Path monitor will take care of providing the next reachability status once packet tunnel is fully disconnected.

[pronebird]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPN/TunnelManager/MapConnectionStatusOperation.swift line 63 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

What to return here?

Which states did you omit with default?

[pronebird]

Reviewed 6 of 14 files at r1, 1 of 2 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPN/SimulatorTunnelProvider/SimulatorTunnelProviderHost.swift line 44 at r4 (raw file):

    ) {
        dispatchQueue.async { [weak self] in
            guard let self else { return }

It would be simpler to use a strong reference here. Functions that accept completion handler and never bother to call it on a whim are typically the source of sleepless nights.

---

ios/MullvadVPN/SimulatorTunnelProvider/SimulatorTunnelProviderHost.swift line 76 at r4 (raw file):

    override func stopTunnel(with reason: NEProviderStopReason, completionHandler: @escaping () -> Void) {
        dispatchQueue.async { [weak self] in

It would be simpler to use strong reference here.

---

ios/MullvadVPN/TunnelManager/TunnelState.swift line 24 at r4 (raw file):

        var s = "\(state), network "

        if observedState.connectionState?.isNetworkReachable == true {

This will print that network is unreachable in .disconnected state which would be a lie. So either fix this message to reflect the actual status or remove it altogether.

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 103 at r4 (raw file):

        actor.start(options: startOptions)

        await actor.waitUntilConnected()

Nit: If we don't use waitUntilConnected() anywhere, then feel free to remove the function altogether.

[pronebird]

Reviewed 5 of 14 files at r1.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @buggmagnet and @rablador)

[rablador]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @buggmagnet and @pronebird)

---

ios/MullvadVPN/SimulatorTunnelProvider/SimulatorTunnelProviderHost.swift line 44 at r4 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

It would be simpler to use a strong reference here. Functions that accept completion handler and never bother to call it on a whim are typically the source of sleepless nights.

It's probably fine with strong ref here (especially in simulator), but I still think we should avoid it unless necessary. Added completion handler to early return.

---

ios/MullvadVPN/SimulatorTunnelProvider/SimulatorTunnelProviderHost.swift line 76 at r4 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

It would be simpler to use strong reference here.

See above.

---

ios/MullvadVPN/TunnelManager/MapConnectionStatusOperation.swift line 63 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Which states did you omit with default?

Looking at the old code we're supposedly not interested in anything but .connected, .connecting and .error. I guess getting any other state here means we've messed up pretty badly somewhere, so perhaps .error here would be ok too?

---

ios/MullvadVPN/TunnelManager/TunnelState.swift line 24 at r4 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

This will print that network is unreachable in .disconnected state which would be a lie. So either fix this message to reflect the actual status or remove it altogether.

Why is it incorrect? It only looks at whether network is reachable and prints accordingly.

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 103 at r4 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Nit: If we don't use waitUntilConnected() anywhere, then feel free to remove the function altogether.

Done.

---

ios/PacketTunnelCore/Actor/ObservedState.swift line 59 at r2 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

It's unusual that the framework has to adapt to the way it's being used in the application target. I'd say we should keep init() public and not over-engineer this.

Change reverted.

---

ios/PacketTunnelCore/Actor/ObservedState.swift line 24 at r4 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Ideally we want to encourage consumer to do the proper matching. So these helpers that extract the associated values should probably be defined as extensions in the target doing this.

Putting them in the existing ObservedState+Extensions file.

---

ios/PacketTunnelCore/Actor/ObservedState.swift line 37 at r4 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Same goes here.

Putting them in the existing ObservedState+Extensions file.

---

ios/PacketTunnelCore/Actor/ObservedState.swift line 56 at r4 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

The whole point of introducing NetworkReachability was to avoid making assumption that the undetermined state means we have internet. I understand that we may have to bridge some gaps before we start using it as it was intended, but them maybe the framework itself does not have to serve these shortcuts for the main target and instead we could define extension where it's used this way.

Would it be bad to switch it around and rely on .reachable instead? If .undetermined is sketchy, then perhaps we should only claim to have network when we're certain.

[pronebird]

Reviewed 4 of 4 files at r5.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPN/TunnelManager/MapConnectionStatusOperation.swift line 63 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Looking at the old code we're supposedly not interested in anything but .connected, .connecting and .error. I guess getting any other state here means we've messed up pretty badly somewhere, so perhaps .error here would be ok too?

You should handle ObservedState.reconnecting phase which we still have because we raise the reasserting = true flag inside of packet tunnel provider (this is what makes NEVPNStatus switch into reasserting state; see:startObservingActorState()).

Return .none for disconnecting and disconnected states as those will be mapped separately.

Remember:

1. In the initial phase before the tunnel is connected in the first time, we remain in the Actor.State.connecting phase. Upon success we move to the Actor.State.connected phase.

   connecting -> connected
   

2. The tunnel once connected can only enter Actor.State.reconnecting phase on reconnect and never returns to Actor.State.connecting phase as it's only used during the initial connection sequence.

   connected -> reconnecting -> connected
   

3. Actor.State.error state knows whether the tunnel had previously connected and would toggle between error -> connecting -> (connected | error) or error -> reconnecting -> (connected | error) states based on that knowledge.

I suggest you to expand the default case and generally avoid it because if we add a new state tomorrow, the default case will gladly swallow it, while the compiler would warn you if each state is handled explicitly.

---

ios/MullvadVPN/TunnelManager/TunnelState.swift line 24 at r4 (raw file):

Previously, rablador (Jon Petersson) wrote…

Why is it incorrect? It only looks at whether network is reachable and prints accordingly.

Because it would print unreachable in the .disconnected state.

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 104 at r5 (raw file):

        for await state in await actor.observedStates {
            switch state {

We probably need to handle error state in here too, and return immediately.

---

ios/PacketTunnelCore/Actor/ObservedState.swift line 24 at r4 (raw file):

Previously, rablador (Jon Petersson) wrote…

Putting them in the existing ObservedState+Extensions file.

Alright keep them in the framework.

---

ios/PacketTunnelCore/Actor/ObservedState.swift line 56 at r4 (raw file):

Previously, rablador (Jon Petersson) wrote…

Would it be bad to switch it around and rely on .reachable instead? If .undetermined is sketchy, then perhaps we should only claim to have network when we're certain.

I think the check you have here should maintain the existing behavior, so don't invert it. I'd much rather iterate over the codebase, than spend too much time in this PR trying to handle all of the rough edges, so I am unblocking this issue.

However note that .undetermined means that we don't know the reachability status yet. When we don't know, we cannot really make assumption whether we have network or not, it would be biased.

We used to make that assumption but it's not pretty. What should we do when the network status is unknown is a good question but perhaps not claim that we know what's going on, or maybe indicate that the network status is being determined. This seems like something for a broader discussion in the team.

[rablador]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @pronebird)

---

ios/MullvadVPN/TunnelManager/MapConnectionStatusOperation.swift line 63 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

You should handle ObservedState.reconnecting phase which we still have because we raise the reasserting = true flag inside of packet tunnel provider (this is what makes NEVPNStatus switch into reasserting state; see:startObservingActorState()).

Return .none for disconnecting and disconnected states as those will be mapped separately.

Remember:

1. In the initial phase before the tunnel is connected in the first time, we remain in the Actor.State.connecting phase. Upon success we move to the Actor.State.connected phase.

   connecting -> connected
   

2. The tunnel once connected can only enter Actor.State.reconnecting phase on reconnect and never returns to Actor.State.connecting phase as it's only used during the initial connection sequence.

   connected -> reconnecting -> connected
   

3. Actor.State.error state knows whether the tunnel had previously connected and would toggle between error -> connecting -> (connected | error) or error -> reconnecting -> (connected | error) states based on that knowledge.

I suggest you to expand the default case and generally avoid it because if we add a new state tomorrow, the default case will gladly swallow it, while the compiler would warn you if each state is handled explicitly.

Right, the .reconnecting state as well. I mapped it like the others, while mapping .initial to .none with the other two remaining states.

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 104 at r5 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

We probably need to handle error state in here too, and return immediately.

Done.

[rablador]

Reviewable status: 14 of 16 files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @pronebird)

---

ios/MullvadVPN/TunnelManager/TunnelState.swift line 24 at r4 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Because it would print unreachable in the .disconnected state.

Ok, got your meaning by digging a little. Updated the description.

[pronebird]

Reviewed 4 of 4 files at r6.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPN/TunnelManager/MapConnectionStatusOperation.swift line 66 at r3 (raw file):

                }
            }
            return

Removing that return statement would mark operation as finished right away, instead of after fetching the tunnel status! See the last line in this function.

[rablador]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @pronebird)

---

ios/MullvadVPN/TunnelManager/MapConnectionStatusOperation.swift line 66 at r3 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Removing that return statement would mark operation as finished right away, instead of after fetching the tunnel status! See the last line in this function.

Aha, reverted!

[pronebird]

Reviewed 1 of 1 files at r7.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)

[buggmagnet]

Reviewed 3 of 4 files at r5, 3 of 4 files at r6, 1 of 1 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved