Move address cache from Swift to Rust API layer

[acb-mv]

This branch adapts the address caching mechanism in the Mullvad Rust API (as used in the daemon) to be usable with iOS (by adding a back-end using the iOS APIs via a FFI to store/recall cached data) and replaces the iOS app's Swift-based caching mechanism with the Rust-based mechanism. This removes some duplicated functionality and harmonises the Mullvad codebase between iOS and the daemon-based platforms.

---

This change is 

[linear]

IOS-1309 Use mullvad-api endpoint caching strategy

[pinkisemils]

@pinkisemils reviewed 11 files and all commit messages, and made 2 comments.
Reviewable status: 11 of 26 files reviewed, 2 unresolved discussions (waiting on @acb-mv).

---

ios/MullvadREST/ApiHandlers/SSLPinningURLSessionDelegate.swift line 42 at r1 (raw file):

            // this used to check for the current endpoint in the address �cache as well, though this has been deprecated.
            
            if hostName == "\(REST.defaultAPIEndpoint.ip)" {

Can you verify if this condition is ever hit today? I believe we only use this class with regular domain names, instead of trying to reach the endpoint via IP. If that is the case, we can simplify this further and remove this altogether.

---

ios/MullvadRustRuntime/MullvadAddressCacheKeychainStore.swift line 15 at r1 (raw file):

func storeAddressCache(_ pointer: UnsafeRawPointer, dataSize: UInt64) {
    let data = Data(bytes: pointer, count: Int(dataSize))
    // TODO: what if this throws?

I think the todo can be removed, but we should comment about

[pinkisemils]

@pinkisemils reviewed 4 files and made 3 comments.
Reviewable status: 15 of 26 files reviewed, 5 unresolved discussions (waiting on @acb-mv).

---

mullvad-api/src/address_cache.rs line 205 at r1 (raw file):

async fn read_address_backing(
    backing: &Arc<dyn AddressCacheBacking + 'static>,

This need not take an &Arc<_>, it can just take a &dyn AddressCacheBacking + 'static.

---

mullvad-ios/src/api_client/late_string_deallocator.rs line 4 at r1 (raw file):

#[repr(C)]
pub struct LateStringDeallocator {

Did we not have this already? Or was this part of address_cache_provider? But if we still need it and we're moving it around, could we call it something else besides LateStringDeallocator - how about SwiftString perhaps?

Further, it needs a Drop implementation unless something else ensures that the backing memory is cleared.

---

mullvad-ios/src/api_client/access_method_resolver.rs line 31 at r1 (raw file):

    async fn read(&self) -> Result<Vec<u8>, AddressCacheError> {
        let lsd = unsafe { swift_read_address_cache() };
        let val = unsafe { get_string(lsd.ptr) };

I believe get_string will have undefined behavior if the stored strings are not valid UTF-8 bytes. I don't think we need to coax the pointer into a &str here - we should just copy the bytes from the pointer into a fresh Vec. So, swift_read_address_cache should return a struct similar to the LateStringDeallocator, SwiftData perhaps - that contains a pointer and a size for the amount of memory that is valid to read from the pointer.

[pinkisemils]

There's some weird trailing whitespace - I suggest you run bash ios/format.sh format and cargo fmt.

@pinkisemils made 1 comment.
Reviewable status: 15 of 26 files reviewed, 5 unresolved discussions (waiting on @acb-mv).

[acb-mv]

mullvad-ios/src/api_client/late_string_deallocator.rs line 4 at r1 (raw file):

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

Did we not have this already? Or was this part of address_cache_provider? But if we still need it and we're moving it around, could we call it something else besides LateStringDeallocator - how about SwiftString perhaps?

Further, it needs a Drop implementation unless something else ensures that the backing memory is cleared.

It was part of another file which was made redundant and deleted.

Renaming it is a good idea, but SwiftString seems a bit vague to me, obscuring that what it brings is the ability to deallocate strings originating from outside of the Rust environment (not to mention that what it contains is not Swift-specific, but an old-fashioned C string). Maybe something like CStringHolder or CStringDeallocContainer?

[acb-mv]

ios/MullvadREST/ApiHandlers/SSLPinningURLSessionDelegate.swift line 42 at r1 (raw file):

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

Can you verify if this condition is ever hit today? I believe we only use this class with regular domain names, instead of trying to reach the endpoint via IP. If that is the case, we can simplify this further and remove this altogether.

Checking this with the debugger, the hostname has only ever been ipv4.am.i.mullvad.net or ipv6.am.i.mullvad.net. Could there be an unhappy path in which we cannot resolve the IP and get an IP address though?

[pinkisemils]

@pinkisemils made 1 comment.
Reviewable status: 1 of 26 files reviewed, 5 unresolved discussions (waiting on @acb-mv).

---

ios/MullvadREST/ApiHandlers/SSLPinningURLSessionDelegate.swift line 42 at r1 (raw file):

Previously, acb-mv wrote…

Checking this with the debugger, the hostname has only ever been ipv4.am.i.mullvad.net or ipv6.am.i.mullvad.net. Could there be an unhappy path in which we cannot resolve the IP and get an IP address though?

That is the only usecase for this class, reaching ipv4.am.i.mullvad.net. We will never use it with plain IPs. I think it's fine to remove this check in it's entirety. This was a workaround to enable us to use TLS pinning without issuing DNS requests - but for these lookups we want to do that. Long term, we would want to move these requests to Rust anyway.

[acb-mv]

mullvad-ios/src/api_client/access_method_resolver.rs line 31 at r1 (raw file):

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

I believe get_string will have undefined behavior if the stored strings are not valid UTF-8 bytes. I don't think we need to coax the pointer into a &str here - we should just copy the bytes from the pointer into a fresh Vec. So, swift_read_address_cache should return a struct similar to the LateStringDeallocator, SwiftData perhaps - that contains a pointer and a size for the amount of memory that is valid to read from the pointer.

That is indeed the only remaining use case for LateStringDeallocator, so I can just change it to return a buffer

[pinkisemils]

@pinkisemils reviewed 11 files and made 2 comments.
Reviewable status: 12 of 26 files reviewed, 6 unresolved discussions (waiting on @acb-mv).

---

mullvad-ios/src/api_client/late_string_deallocator.rs line 4 at r1 (raw file):

Previously, acb-mv wrote…

It was part of another file which was made redundant and deleted.

Renaming it is a good idea, but SwiftString seems a bit vague to me, obscuring that what it brings is the ability to deallocate strings originating from outside of the Rust environment (not to mention that what it contains is not Swift-specific, but an old-fashioned C string). Maybe something like CStringHolder or CStringDeallocContainer?

It is a SwiftString because it would be a heap allocated String that was created Swift side. But as we've talked before, this type will most likely not survive this code review anyway.

---

mullvad-ios/src/api_client/mod.rs line 144 at r2 (raw file):

) {
    let api_context = swift_api_context.rust_context();
    let cloned_context = api_context.clone();

I don't believe cloned_context is required here - it should be possible to get the tokio handle without a new copy. But even if a new copy is required, it could be done like so:

let handle = api_context.clone().api_client.handle();

[pinkisemils]

@pinkisemils reviewed 6 files and made 1 comment.
Reviewable status: 18 of 26 files reviewed, 7 unresolved discussions (waiting on @acb-mv).

---

mullvad-api/src/address_cache.rs line 41 at r2 (raw file):

impl Debug for dyn AddressCacheBacking {
    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
        write!(f, "[some AddressCacheBacking]")

For debug printing, I'd implement the Debug trait on the actual implementations of the trait and require Debug the same way that AddressCacheBacking requires the implementation to be Send + Sync.

[acb-mv]

@acb-mv made 6 comments.
Reviewable status: 18 of 26 files reviewed, 7 unresolved discussions (waiting on @pinkisemils).

---

ios/MullvadREST/ApiHandlers/SSLPinningURLSessionDelegate.swift line 42 at r1 (raw file):

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

That is the only usecase for this class, reaching ipv4.am.i.mullvad.net. We will never use it with plain IPs. I think it's fine to remove this check in it's entirety. This was a workaround to enable us to use TLS pinning without issuing DNS requests - but for these lookups we want to do that. Long term, we would want to move these requests to Rust anyway.

Done.

---

ios/MullvadRustRuntime/MullvadAddressCacheKeychainStore.swift line 15 at r1 (raw file):

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

I think the todo can be removed, but we should comment about

Done.

---

mullvad-api/src/address_cache.rs line 205 at r1 (raw file):

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

This need not take an &Arc<_>, it can just take a &dyn AddressCacheBacking + 'static.

Not quite: the size for values of type \dyn AddressCacheBacking` cannot be known at compilation time`

---

mullvad-ios/src/api_client/access_method_resolver.rs line 31 at r1 (raw file):

Previously, acb-mv wrote…

That is indeed the only remaining use case for LateStringDeallocator, so I can just change it to return a buffer

Done

---

mullvad-ios/src/api_client/late_string_deallocator.rs line 4 at r1 (raw file):

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

It is a SwiftString because it would be a heap allocated String that was created Swift side. But as we've talked before, this type will most likely not survive this code review anyway.

Done.

---

mullvad-ios/src/api_client/mod.rs line 144 at r2 (raw file):

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

I don't believe cloned_context is required here - it should be possible to get the tokio handle without a new copy. But even if a new copy is required, it could be done like so:

let handle = api_context.clone().api_client.handle();

I tried that, but it gives
creates a temporary value which is freed while still in use
hence the temporary variable.