Add timeout to the tcp connection for the pq key negotiation ios 701 #6343

buggmagnet · 2024-06-12T13:59:00Z

This PR adds an exponential backoff timeout when negotiating post quantum keys.
The timeout plays 2 important part :

Trigger a reset of the key exchange process if the in-tunnel TCP connection does not become ready in time
Trigger a reset of the key exchange process if it takes too long to exchange keys.

This PR was co-authored by @pinkisemils and @buggmagnet
@MarkusPettersson98's approval is required for the rust side changes
@acb-mv's approval is required for the swift side changes.

This change is

linear · 2024-06-12T13:59:03Z

IOS-701 Add timeout to the TCP connection for the PQ key negotiation

acb-mv

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

ios/MullvadPostQuantum/PostQuantumKeyNegotiator.swift line 17 at r1 (raw file):

// swiftlint:disable function_parameter_count
public protocol PostQuantumKeyNegotiation {

Are you sure about the name? This doesn't look like a negotiation but rather as something that negotiates. Perhaps PostQuantumKeyNegotiating would be a better name.

buggmagnet

Reviewable status: 13 of 16 files reviewed, 1 unresolved discussion (waiting on @acb-mv)

ios/MullvadPostQuantum/PostQuantumKeyNegotiator.swift line 17 at r1 (raw file):

Previously, acb-mv wrote…

Are you sure about the name? This doesn't look like a negotiation but rather as something that negotiates. Perhaps PostQuantumKeyNegotiating would be a better name.

Done

MarkusPettersson98

Reviewed 3 of 16 files at r1, all commit messages.
Reviewable status: 13 of 16 files reviewed, 8 unresolved discussions (waiting on @acb-mv and @buggmagnet)

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

    }
    /// Creates a `RelayConfigService` using the in-tunnel TCP Connection provided by the Packet
    /// Tunnel Provider # Safety

Nit A bit of a weird line formatting here 🤓

Code quote:

   /// Tunnel Provider # Safety

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

                                    log::error!("No suitable peer was found");
                                    swift_post_quantum_key_ready(packet_tunnel_ptr, ptr::null(), ptr::null());
                                }

Nit The log-statement shouldn't be in the unsafe block. Consider moving the unsafe token to the line where it is actually needed

None => {
    log::error!("No suitable peer was found");
    unsafe { swift_post_quantum_key_ready(packet_tunnel_ptr, ptr::null(), ptr::null()) };
}

Code quote:

                                None => unsafe {
                                    log::error!("No suitable peer was found");
                                    swift_post_quantum_key_ready(packet_tunnel_ptr, ptr::null(), ptr::null());
                                }

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

                            log::error!("Key exchange failed {}", error);
                            swift_post_quantum_key_ready(packet_tunnel_ptr, ptr::null(), ptr::null());
                        }

Dito

Code quote:

                        Err(error) => unsafe {
                            log::error!("Key exchange failed {}", error);
                            swift_post_quantum_key_ready(packet_tunnel_ptr, ptr::null(), ptr::null());
                        }

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

    /// `tcp_connection` must be pointing to a valid instance of a `NWTCPConnection`, created by the
    /// `PacketTunnelProvider`
    pub unsafe fn new(connection: Arc<Mutex<ConnectionContext>>) -> (Self, IosTcpShutdownHandle) {

Nit The comment says tcp_connection, but the function parameter is named connection 😊

Code quote:

    /// # Safety
    /// `tcp_connection` must be pointing to a valid instance of a `NWTCPConnection`, created by the
    /// `PacketTunnelProvider`
    pub unsafe fn new(connection: Arc<Mutex<ConnectionContext>>) -> (Self, IosTcpShutdownHandle) {

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

            std::mem::drop(context);
        }
    }

Nit Seems like there is an unncecessary level of indentation here?

pub fn shutdown(self) {
    let Ok(mut context) = self.context.lock() else {
        return;
    };

    context.tcp_connection = None;
    if let Some(waker) = context.waker.take() {
        waker.wake();
    }
    std::mem::drop(context);
}

Code quote:

    pub fn shutdown(self) {
        {
            let Ok(mut context) = self.context.lock() else {
                return;
            };

            context.tcp_connection = None;
            if let Some(waker) = context.waker.take() {
                waker.wake();
            }
            std::mem::drop(context);
        }
    }

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

                    unsafe {
                        swift_nw_tcp_connection_send(
                            // self.tcp_connection,

Comment

Code quote:

                            // self.tcp_connection,

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

                    unsafe {
                        // TODO
                        swift_nw_tcp_connection_read(tcp_ptr, raw_sender as _);

TODO

Code quote:

                        // TODO
                        swift_nw_tcp_connection_read(tcp_ptr, raw_sender as _);

buggmagnet

Reviewable status: 13 of 16 files reviewed, 6 unresolved discussions (waiting on @acb-mv, @MarkusPettersson98, and @pinkisemils)

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