Implement PQ PSK

[buggmagnet]

This PR implements Post Quantum key exchange in the iOS application.
It's using the talpid-tunnel-config-client rust crate to do the key negotiation.

@dlon @pinkisemils are assigned for reviewing the Rust side of changes.
@rablador @mojganii are assigned for reviewing the Swift side of changes.
@acb-mv and @buggmagnet co-authored this PR.

We currently have an ongoing bug in the Swift side of changes where the reflected state of the connection
is not accurate if one switches between PQ PSK and regular connection within the lifetime of the tunnel manager.
It's mostly a UI issue, the post quantum security feature is respected from the connection's point of view.

⚠️ As this was done in a feature branch, the code is not gated to only build in the DEBUG config. ⚠️
We are planning to fix the UI issues before merging this to main if we deem it good enough, otherwise we will gate keep it behind the DEBUG flag.

---

This change is 

[pinkisemils]

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

---

Cargo.lock line 4077 at r3 (raw file):

 "libc",
 "log",
 "oslog",

These commits need to be signed. Ideally, all changes to lockfiles are isolated to specific signed commites, i.e. the commits changing lock files don't change anything else but lock files.

[pinkisemils]

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

---

Cargo.toml line 83 at r3 (raw file):

windows-sys = "0.52.0"

chrono = { version = "0.4.26", default-features = false }

Useless whitespace change.

---

ios/MullvadPostQuantum/MullvadPostQuantum.h line 12 at r3 (raw file):

//! Project version number for MullvadPostQuantum.
FOUNDATION_EXPORT double MullvadPostQuantumVersionNumber;

Are these used anywhere?

[pinkisemils]

Reviewed 5 of 51 files at r1.
Reviewable status: 5 of 51 files reviewed, 6 unresolved discussions (waiting on @buggmagnet)

---

talpid-routing/src/unix/mod.rs line 21 at r3 (raw file):

#[allow(clippy::module_inception)]
#[cfg(any(target_os = "macos", target_os = "ios"))]

I don't believe we should be using talpid-routing on iOS.

---

talpid-tunnel-config-client/Cargo.toml line 33 at r3 (raw file):

[dev-dependencies]
tokio = { workspace = true, features = ["rt-multi-thread"] }

This shouldn't be a dev dependency for all other platforms. The tokio dependency should only be added for the iOS target.

---

talpid-tunnel-config-client/src/lib.rs line 203 at r3 (raw file):

    Option<(classic_mceliece_rust::SecretKey<'static>, [u8; 3168])>,
) {
    let (pq_request, kem_secrets) = if enable_post_quantum {

No need to create these variables, the if statement can be the sole expression in this function.

[buggmagnet]

Reviewable status: 5 of 51 files reviewed, 6 unresolved discussions (waiting on @pinkisemils)

---

Cargo.lock line 4077 at r3 (raw file):

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

These commits need to be signed. Ideally, all changes to lockfiles are isolated to specific signed commites, i.e. the commits changing lock files don't change anything else but lock files.

I've singled out the commit that lists those changes to sign it.

---

Cargo.toml line 83 at r3 (raw file):

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

Useless whitespace change.

Shouldn't that file be linted / formatted automatically ?
I didn't do this change intentionally, my IDE did it on my behalf. Let me know if you want me to remove the change. I think it's harmless.

[dlon]

Reviewed 4 of 51 files at r1, 1 of 2 files at r2.
Reviewable status: 10 of 51 files reviewed, 18 unresolved discussions (waiting on @buggmagnet and @pinkisemils)

---

talpid-tunnel-config-client/src/lib.rs line 129 at r3 (raw file):

    enable_post_quantum: bool,
    enable_daita: bool,
    #[cfg(target_os = "ios")] mut client: EphemeralPeerClient<Channel>,

This can be split this into two functions. One that takes a client and no service_address, and another one (the original) that takes service_address and only creates a client then calls the new function. It probably doesn't have to be conditional on iOS.

---

talpid-tunnel-config-client/src/lib.rs line 129 at r3 (raw file):

    enable_post_quantum: bool,
    enable_daita: bool,
    #[cfg(target_os = "ios")] mut client: EphemeralPeerClient<Channel>,

You can use RelayConfigService instead of EphemeralPeerClient<Channel>

---

talpid-tunnel-config-client/src/ios_ffi/ios_runtime.rs line 13 at r3 (raw file):

run_ios_runtime

Can this not just be called run_post_quantum_psk_exchange or something? I don't think the caller needs to know what "iOS runtime" is.

---

talpid-tunnel-config-client/src/ios_ffi/ios_tcp_connection.rs line 88 at r3 (raw file):

    fn is_shutdown(&self) -> bool {
        self.shutdown.load(atomic::Ordering::SeqCst)

Should this wake the read/write task?

---

talpid-tunnel-config-client/src/ios_ffi/ios_tcp_connection.rs line 104 at r3 (raw file):

        buf: &[u8],
    ) -> std::task::Poll<Result<usize>> {
        let raw_sender = Box::into_raw(Box::new(self.write_tx.clone()));

This is leaky. The box should be created in if !self.write_in_progress { ... } below.

---

talpid-tunnel-config-client/src/ios_ffi/ios_tcp_connection.rs line 104 at r3 (raw file):

        buf: &[u8],
    ) -> std::task::Poll<Result<usize>> {
        let raw_sender = Box::into_raw(Box::new(self.write_tx.clone()));

This isn't properly cleaned up unlesshandle_sent is called. Is that guaranteed to happen? I think wrapping write_tx in an Arc and passing a pointer to a Weakmight solve this.

---

talpid-tunnel-config-client/src/ios_ffi/ios_tcp_connection.rs line 119 at r3 (raw file):

                    return Poll::Ready(Err(connection_closed_err()));
                }
                if self.write_in_progress {

This can be simplified to

if !self.write_in_progress {
    let raw_sender = Box::into_raw(Box::new(self.write_tx.clone()));
    unsafe {
        swift_nw_tcp_connection_send(
            self.tcp_connection,
            buf.as_ptr() as _,
            buf.len(),
            raw_sender as _,
        );
    }
    self.write_in_progress = true;
}
Poll::Pending

---

talpid-tunnel-config-client/src/ios_ffi/ios_tcp_connection.rs line 156 at r3 (raw file):

        buf: &mut tokio::io::ReadBuf<'_>,
    ) -> std::task::Poll<std::io::Result<()>> {
        let raw_sender = Box::into_raw(Box::new(self.read_tx.clone()));

This is leaky. The box should be created in if !self.read_in_progress { ... } below.

---

talpid-tunnel-config-client/src/ios_ffi/ios_tcp_connection.rs line 156 at r3 (raw file):

        buf: &mut tokio::io::ReadBuf<'_>,
    ) -> std::task::Poll<std::io::Result<()>> {
        let raw_sender = Box::into_raw(Box::new(self.read_tx.clone()));

This isn't properly cleaned up unlesshandle_recv is called. Is that guaranteed to happen? I think wrapping read_tx in an Arc and passing a pointer to a Weakmight solve this.

---

talpid-tunnel-config-client/src/ios_ffi/ios_tcp_connection.rs line 172 at r3 (raw file):

            }
            std::task::Poll::Pending => {
                if self.read_in_progress {

This can be simplified to

if !self.read_in_progress {
    let raw_sender = Box::into_raw(Box::new(self.read_tx.clone()));
    unsafe {
        swift_nw_tcp_connection_read(self.tcp_connection, raw_sender as _);
    }
    self.read_in_progress = true;
}
Poll::Pending

---

talpid-tunnel-config-client/src/ios_ffi/mod.rs line 25 at r3 (raw file):

        let send_tx: Arc<mpsc::UnboundedSender<()>> = unsafe { Arc::from_raw(self.context as _) };
        let _ = send_tx.send(());
        std::mem::forget(send_tx);

Calling Arc::increment_strong_count(self.context) at the start of the function (before from_raw) is probably a bit nicer.

---

talpid-tunnel-config-client/src/ios_ffi/mod.rs line 88 at r3 (raw file):

/// The TCP connection must be created to go through the tunnel.
/// # Safety
/// This function is safe to call

The remark about safety should be replaced with a real explanation or be removed.

[rablador]

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

---

ios/MullvadPostQuantum/MullvadPostQuantum.h line 12 at r3 (raw file):

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

Are these used anywhere?

These are auto-generated when creating a new frameworks, but I guess we could remove them unless we actually use them. They are in most (all?) of our existing frameworks though.

---

ios/MullvadPostQuantum/PacketTunnelProvider+TCPConnection.swift line 26 at r4 (raw file):

    let rawData = Data(bytes: data, count: Int(dataLength))

    // The guarantee that no more than 2 writes happen in parallel is done by virtue of not returning the execution context

No more than 2 writes?

[rablador]

Reviewed 19 of 51 files at r1, 1 of 2 files at r2, 3 of 3 files at r4.
Reviewable status: 32 of 51 files reviewed, 27 unresolved discussions (waiting on @buggmagnet and @pinkisemils)

---

ios/MullvadPostQuantum/PacketTunnelProvider+TCPConnection.swift line 17 at r4 (raw file):

@_cdecl("swift_nw_tcp_connection_send")
func tcpConnectionSend(
    connection: UnsafeMutableRawPointer?,

Nit: We use "raw" prefix for raw pointer params in receivePostQuantumKey below. It's kind of nice info I think.

---

ios/MullvadPostQuantum/PacketTunnelProvider+TCPConnection.swift line 20 at r4 (raw file):

    data: UnsafeMutableRawPointer,
    dataLength: UInt,
    sender: UnsafeMutableRawPointer?

Nit: Perhaps sender could be renamed to something that describes what it is.

---

ios/MullvadPostQuantum/PostQuantumKeyNegotiator.swift line 30 at r4 (raw file):

        var cancelToken = PostQuantumCancelToken()

        // TODO: Any non 0 return is considered a failure, and should be handled gracefully

Is there a PR or issue for this?

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 6826 at r4 (raw file):

				PRODUCT_NAME = "$(TARGET_NAME)";
				SWIFT_OPTIMIZATION_LEVEL = "-Onone";
				SWIFT_STRICT_CONCURRENCY = minimal;

Is this intentional?

---

ios/PacketTunnel/PostQuantumKeyExchangeActor.swift line 45 at r4 (raw file):

    func startNegotiation(with privateKey: PrivateKey) {
        let negotiator = PostQuantumKeyNegotiator()

Discussed offline, but we should have gateway and port stored or derived from somewhere else.

---

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

    }

    func createTCPConnectionForPQPSK(_ gatewayAddress: String) -> NWTCPConnection {

This function is unused and can be removed.

---

ios/PacketTunnelCore/Actor/PacketTunnelActor.swift line 257 at r4 (raw file):

        state = .connecting(connectionState)

        defer {

Nit: Since the function doesn't throw we can remove defer here and put the code on the bottom instead.

---

ios/PacketTunnelCore/Actor/PacketTunnelActor.swift line 289 at r4 (raw file):

        let settings: Settings = try settingsReader.read()

        if settings.quantumResistance.isEnabled {

Can we move this to its own function instead to declutter tryStart?

[rablador]

Reviewed 22 of 51 files at r1, 1 of 2 files at r2.
Reviewable status: all files reviewed, 28 unresolved discussions (waiting on @buggmagnet and @pinkisemils)

---

ios/PacketTunnelCore/Actor/ConfigurationBuilder.swift line 30 at r4 (raw file):

    var endpoint: MullvadEndpoint?
    var allowedIPs: [IPAddressRange]
    // or should this go in MullvadEndpoint?

Seems fine to have it here since privateKey is here, no?

[acb-mv]

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

Previously, rablador (Jon Petersson) wrote…

This function is unused and can be removed.

Done

[buggmagnet]

Reviewable status: 50 of 51 files reviewed, 27 unresolved discussions (waiting on @dlon, @pinkisemils, and @rablador)

---

ios/MullvadPostQuantum/PacketTunnelProvider+TCPConnection.swift line 26 at r4 (raw file):

Previously, rablador (Jon Petersson) wrote…

No more than 2 writes?

It's actually incorrect, all writes are sequentials, so no more than a single write should happen in parallel. I will update this comment.

---

talpid-routing/src/unix/mod.rs line 21 at r3 (raw file):

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

I don't believe we should be using talpid-routing on iOS.

Yes, I must have had that change for something I thought was related, but turned out wasn't. Thanks for flagging this, I will revert this to being macos only.

---

talpid-tunnel-config-client/Cargo.toml line 33 at r3 (raw file):

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

This shouldn't be a dev dependency for all other platforms. The tokio dependency should only be added for the iOS target.

This was discussed offline. We will leave this as is for now, but in the future, we will have an abstraction of tokio in mullvad-types that will be consumed by all the iOS rust code so that the rt-multi-thread feature can be used as a dev-dependency.

---

talpid-tunnel-config-client/src/lib.rs line 129 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

You can use RelayConfigService instead of EphemeralPeerClient<Channel>

Done

---

talpid-tunnel-config-client/src/lib.rs line 203 at r3 (raw file):

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

No need to create these variables, the if statement can be the sole expression in this function.

Done.

---

talpid-tunnel-config-client/src/ios_ffi/ios_runtime.rs line 13 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

run_ios_runtime

Can this not just be called run_post_quantum_psk_exchange or something? I don't think the caller needs to know what "iOS runtime" is.

Yes, that's a good suggestion !

---

talpid-tunnel-config-client/src/ios_ffi/mod.rs line 88 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

The remark about safety should be replaced with a real explanation or be removed.

Thanks for flagging this, I will update this comment to add useful information.

[acb-mv]

ios/PacketTunnelCore/Actor/PacketTunnelActor.swift line 289 at r4 (raw file):

Previously, rablador (Jon Petersson) wrote…

Can we move this to its own function instead to declutter tryStart?

Done

[buggmagnet]

Reviewable status: 44 of 51 files reviewed, 26 unresolved discussions (waiting on @dlon, @pinkisemils, and @rablador)

---

talpid-tunnel-config-client/src/ios_ffi/ios_tcp_connection.rs line 104 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

This is leaky. The box should be created in if !self.write_in_progress { ... } below.

Done.

---

talpid-tunnel-config-client/src/ios_ffi/ios_tcp_connection.rs line 104 at r3 (raw file):
handle_sent / handle_read are guaranteed to be called, otherwise we would leak anyway since the control is in the NWTCPConnection instance's hand.

I think wrapping write_tx in an Arc and passing a pointer to a Weakmight solve this.
Ok I'll try to do that.

---

talpid-tunnel-config-client/src/ios_ffi/ios_tcp_connection.rs line 119 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

This can be simplified to

if !self.write_in_progress {
    let raw_sender = Box::into_raw(Box::new(self.write_tx.clone()));
    unsafe {
        swift_nw_tcp_connection_send(
            self.tcp_connection,
            buf.as_ptr() as _,
            buf.len(),
            raw_sender as _,
        );
    }
    self.write_in_progress = true;
}
Poll::Pending

Done.

---

talpid-tunnel-config-client/src/ios_ffi/ios_tcp_connection.rs line 156 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

This is leaky. The box should be created in if !self.read_in_progress { ... } below.

Done.

---

talpid-tunnel-config-client/src/ios_ffi/ios_tcp_connection.rs line 156 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

This isn't properly cleaned up unlesshandle_recv is called. Is that guaranteed to happen? I think wrapping read_tx in an Arc and passing a pointer to a Weakmight solve this.

Ok I will try to address that.

---

talpid-tunnel-config-client/src/ios_ffi/ios_tcp_connection.rs line 172 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

This can be simplified to

if !self.read_in_progress {
    let raw_sender = Box::into_raw(Box::new(self.read_tx.clone()));
    unsafe {
        swift_nw_tcp_connection_read(self.tcp_connection, raw_sender as _);
    }
    self.read_in_progress = true;
}
Poll::Pending

Done.

[rablador]

Reviewed 1 of 1 files at r5, 4 of 5 files at r6, 1 of 1 files at r7, 3 of 3 files at r8, all commit messages.
Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @buggmagnet, @dlon, and @pinkisemils)

[rablador]

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

[mojganii]

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

---

ios/MullvadPostQuantum/MullvadPostQuantum.h line 12 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

These are auto-generated when creating a new frameworks, but I guess we could remove them unless we actually use them. They are in most (all?) of our existing frameworks though.

Xcode applies your app's version number to all included frameworks which means this can be removed.

---

ios/MullvadPostQuantum/PostQuantumKeyNegotiator.swift line 38 at r9 (raw file):

            &cancelToken
        )
        guard result == 0 else {

don't we have error handling in a better manner?

---

ios/PacketTunnel/PostQuantumKeyExchangeActor.swift line 51 at r9 (raw file):

        let inTunnelTCPConnection = createTCPConnection(endpoint)

        let ephemeralSharedKey = PrivateKey() // This will become the new private key of the device

nit: isn't better to put the comment above of the code to keep the consistency?

---

ios/PacketTunnelCore/Actor/PacketTunnelActor.swift line 257 at r4 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: Since the function doesn't throw we can remove defer here and put the code on the bottom instead.

+1

---

ios/PacketTunnelCore/Actor/PacketTunnelActor.swift line 352 at r9 (raw file):

     connection restricted to the negotiation host and entering the negotiating state.
     */
    private func tryStartPostQuantumNegotiation(

nit : I suggest to put all functions are PQ related in an extension in the same file or PacketTunnelActor+PostQuantum

Code snippet:

/**
 summery about PQ
 */
extension PacketTunnelActor {
   
    private func tryStartPostQuantumNegotiation(
        withSettings settings: Settings,
        nextRelay: NextRelay,
        reason: ReconnectReason
    ) async throws
    {}
}

[acb-mv]

Reviewable status: 48 of 52 files reviewed, 23 unresolved discussions (waiting on @buggmagnet, @dlon, @pinkisemils, and @rablador)

---

ios/MullvadPostQuantum/PacketTunnelProvider+TCPConnection.swift line 17 at r4 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: We use "raw" prefix for raw pointer params in receivePostQuantumKey below. It's kind of nice info I think.

Done

---

ios/MullvadPostQuantum/PacketTunnelProvider+TCPConnection.swift line 20 at r4 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: Perhaps sender could be renamed to something that describes what it is.

It appears to be a Rust object of type UnboundedSender. @buggmagnet : any thoughts?

---

ios/PacketTunnelCore/Actor/PacketTunnelActor.swift line 257 at r4 (raw file):

Previously, mojganii wrote…

+1

Done

---

ios/PacketTunnelCore/Actor/PacketTunnelActor.swift line 352 at r9 (raw file):

Previously, mojganii wrote…

nit : I suggest to put all functions are PQ related in an extension in the same file or PacketTunnelActor+PostQuantum

Done

[acb-mv]

Reviewable status: 45 of 52 files reviewed, 22 unresolved discussions (waiting on @buggmagnet, @dlon, @mojganii, @pinkisemils, and @rablador)

---

ios/MullvadPostQuantum/PostQuantumKeyNegotiator.swift line 30 at r4 (raw file):

Previously, rablador (Jon Petersson) wrote…

Is there a PR or issue for this?

I've now fixed this by threading failure states through.

---

ios/MullvadPostQuantum/PostQuantumKeyNegotiator.swift line 38 at r9 (raw file):

Previously, mojganii wrote…

don't we have error handling in a better manner?

The error in this case would be failure to set up a Rust runtime; the key negotiation happens asynchronously and is not returned from this function. I'll rename the function to better reflect this and handle this case.

---

ios/PacketTunnel/PostQuantumKeyExchangeActor.swift line 51 at r9 (raw file):

Previously, mojganii wrote…

nit: isn't better to put the comment above of the code to keep the consistency?

Done

---

ios/PacketTunnelCore/Actor/ConfigurationBuilder.swift line 30 at r4 (raw file):

Previously, rablador (Jon Petersson) wrote…

Seems fine to have it here since privateKey is here, no?

Fair enough

[rablador]

Reviewed 2 of 3 files at r10, 1 of 1 files at r12, 2 of 3 files at r13, 3 of 3 files at r14, all commit messages.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @buggmagnet, @dlon, @mojganii, and @pinkisemils)

[dlon]

Reviewed 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @buggmagnet, @mojganii, and @pinkisemils)

---

talpid-routing/src/unix/mod.rs line 21 at r3 (raw file):

Previously, buggmagnet wrote…

Yes, I must have had that change for something I thought was related, but turned out wasn't. Thanks for flagging this, I will revert this to being macos only.

any() is redundant.

---

talpid-tunnel-config-client/src/ios_ffi/mod.rs line 88 at r3 (raw file):

Previously, buggmagnet wrote…

Thanks for flagging this, I will update this comment to add useful information.

Is this correct? I think they must be 32-byte arrays.

---

talpid-tunnel-config-client/src/ios_ffi/mod.rs line 94 at r15 (raw file):

/// # Safety
/// `public_key` and `ephemeral_key` must be valid respective `PublicKey` and `PrivateKey` types.
/// They will not be valid after this function is called, and thus must be copied here.

This has nothing to do with this function. The function doesn't invalidate the arrays. It does copy them so that the caller doesn't have to worry about keeping them around, but I'm not sure that's worth mentioning.

---

talpid-tunnel-config-client/src/ios_ffi/mod.rs line 95 at r15 (raw file):

a

[rablador]

Reviewed 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @buggmagnet, @mojganii, and @pinkisemils)

---

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

            // while the tunnel process is trying to connect.
            startPollingTunnelStatus(interval: establishingTunnelStatusPollInterval)

Are we not certain the connection will work anymore?

[mojganii]

Reviewable status: 47 of 52 files reviewed, all discussions resolved (waiting on @dlon and @rablador)

[rablador]

Reviewed 5 of 5 files at r25, 1 of 1 files at r29, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[pinkisemils]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[pinkisemils]

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