Allow settings migration from packet tunnel

[buggmagnet]

This PR adds the ability for the PacketTunnel settings to perform settings migration.
The net benefit of this is that users that have automatic app updates turned on won't have to manually disconnect and reconnect from the VPN when the updates itself, the upgrade should be transparent for them.

---

This change is 

[linear]

IOS-573 Do settings migration from the packet tunnel

[pinkisemils]

Reviewable status: 0 of 8 files reviewed, 1 unresolved discussion (waiting on @buggmagnet)

---

ios/MullvadSettings/MigrationManager.swift line 39 at r1 (raw file):

    /// In order to avoid migration happening from both the VPN and the host processes at the same time,
    /// a non existant file path is used as a lock is used to synchronize access between the processes.
    /// This file is accessed by `NSFileCoordinator` in order to prevent multiple processes accessing at the same time.

If we can lock a file path, why not use the path to the settings file instead? No need for locking unrelated files.

[buggmagnet]

Reviewable status: 0 of 8 files reviewed, 1 unresolved discussion (waiting on @pinkisemils)

---

ios/MullvadSettings/MigrationManager.swift line 39 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

If we can lock a file path, why not use the path to the settings file instead? No need for locking unrelated files.

Because the settings are not stored in a file, but in the keychain itself

[pinkisemils]

Reviewable status: 0 of 8 files reviewed, 1 unresolved discussion (waiting on @buggmagnet)

---

ios/MullvadSettings/MigrationManager.swift line 39 at r1 (raw file):

Previously, buggmagnet wrote…

Because the settings are not stored in a file, but in the keychain itself

You are correct. But in that case, shouldn't this lock be acquired anytime the settings are read?

[pinkisemils]

Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadSettings/MigrationManager.swift line 39 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

You are correct. But in that case, shouldn't this lock be acquired anytime the settings are read?

No, we do not need to do that if we ensure the migration is done when the UI process starts. This is sound now. Great work!

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 193 at r1 (raw file):

                        )
                    DispatchQueue.main.asyncAfter(deadline: .now() + delay.timeInterval) { [unowned self] in
                        performSettingsMigration()

If this does not block the startup, what does the packet tunnel read from the settings? Would it not fail to deserialize or read garbage constraints? Does it make sense to have the packet tunnel not block it's startup on this?

[buggmagnet]

Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @pinkisemils)

---

ios/MullvadSettings/MigrationManager.swift line 39 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

No, we do not need to do that if we ensure the migration is done when the UI process starts. This is sound now. Great work!

Multiple concurrent reads are fine, which is what we do in many parts of the app.
There is a very high likelihood we never paid attention to where and when we write / read settings so far.
Keychain itself is blocking the calling thread, for I assume is thread safety, so we should have thread safety within the current process.

I don't know how it behaves in a multi process scenario, but I'm making the assumption that it protects us from partial read writes, but it's up to us to make sure there are no TOCTOU issues.

That being said, it would be work for another item to make sure we control exactly when read / write operations are done on the keychain, and that they are done in a more or less atomic manner.

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 193 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

If this does not block the startup, what does the packet tunnel read from the settings? Would it not fail to deserialize or read garbage constraints? Does it make sense to have the packet tunnel not block it's startup on this?

Packet tunnel will get killed in 60 seconds if doesn't start anyway.
Our best hope is that we enter the blocked state until the user starts using their phones, at which point we're at the mercy of the retry logic.

Do keep in mind that this scenario is extremely unlikely. It would mean the phone both had a reboot AND and an app update before the user unlocked their phone after the reboot.

I would not worry about those scenarios.

[buggmagnet]

Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @pinkisemils)

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 193 at r1 (raw file):

Previously, buggmagnet wrote…

Packet tunnel will get killed in 60 seconds if doesn't start anyway.
Our best hope is that we enter the blocked state until the user starts using their phones, at which point we're at the mercy of the retry logic.

Do keep in mind that this scenario is extremely unlikely. It would mean the phone both had a reboot AND and an app update before the user unlocked their phone after the reboot.

I would not worry about those scenarios.

The other scenario is that the phone was such in a bad state that we were not able to upgrade the settings.
Our upgrade process right now is basically infaillible (since we removed the migration that did networking calls after SettingsV1).

[rablador]

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @pinkisemils)

---

ios/MullvadREST/RetryStrategy/RetryStrategy.swift line 83 at r1 (raw file):

        public static var failedMigrationRecovery = RetryStrategy(
            maxRetryCount: .max,

Are we going to retry this basically forever?

---

ios/MullvadSettings/MigrationManager.swift line 38 at r1 (raw file):

    ///
    /// In order to avoid migration happening from both the VPN and the host processes at the same time,
    /// a non existant file path is used as a lock is used to synchronize access between the processes.

Nit: "is used as a lock is used to"

---

ios/MullvadVPNTests/MullvadTypes/FileCacheTests.swift line 10 at r1 (raw file):

import Foundation
@testable import MullvadMockData

Added by mistake?

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 193 at r1 (raw file):

Previously, buggmagnet wrote…

The other scenario is that the phone was such in a bad state that we were not able to upgrade the settings.
Our upgrade process right now is basically infaillible (since we removed the migration that did networking calls after SettingsV1).

So basically, before migration happens, the tunnel is started and (possibly) enters blocked state. Then migration finishes and hopefully succeeds. The tunnel reconnection timer fires at some point and user leaves blocked state?

[buggmagnet]

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

---

ios/MullvadREST/RetryStrategy/RetryStrategy.swift line 83 at r1 (raw file):
That's what the ticket asks for:

The packet tunnel must be able to handle a reboot - when the keychain is locked. It should retry with a backoff.

---

ios/MullvadSettings/MigrationManager.swift line 38 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: "is used as a lock is used to"

I reread this 3 times and yet... nice catch !

---

ios/MullvadVPNTests/MullvadTypes/FileCacheTests.swift line 10 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Added by mistake?

yes, leftover of another approach I had tried where I wanted to add a test here, will delete.

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 193 at r1 (raw file):
I want to emphasize that the odds of this failing are astronomically low for 2 reasons:

• The upgradeToNextVersion function cannot fail. i.e. the only failure mode is keychain settings have been badly corrupted (or we messed up our next TunnelSettings by adding fields that do not deserialize), at which point we cannot guarantee the app will work reliably at all. If this fails, upgrading from the UI process will end in the same result, and the user will have to re-install the app in any case
• The scenario in which we cannot read from the settings because the keychain is still locked only happens if the user's device rebooted without the user interacting with the device (since they would otherwise unlock the device to use it). We could actually remove this restriction by using kSecAttrAccessibleAlways instead. If we're really worried about other apps reading our keychain entries (or reading them from a backup) there are better solutions around.

before migration happens, the tunnel is started and (possibly) enters blocked state.

Migration is not asynchronous, the tunnel won't start before migration is done, therefore we cannot enter the blocked state because we are waiting on migration. It either succeeds and we start the tunnel, or it fails

[rablador]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @pinkisemils)

---

ios/MullvadREST/RetryStrategy/RetryStrategy.swift line 83 at r1 (raw file):

Previously, buggmagnet wrote…

That's what the ticket asks for:

The packet tunnel must be able to handle a reboot - when the keychain is locked. It should retry with a backoff.

Still, forever...?

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 193 at r1 (raw file):

Migration is not asynchronous, the tunnel won't start before migration is done

Aha, then all is good I think. Checked the Linear issue just now and one of the acceptace criteria also states that blocked state should not happen.

[rablador]

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

[rablador]

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

[pinkisemils]

Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[github-actions]

🚨 End to end tests failed. Please check the failed workflow run.