Add socks5 proxy [POC] IOS-358

[pronebird]

This PR adds a socks5 forwarding proxy and socks5 transport to communicate with our API. Similar to shadowsocks implementation, the socks5 forwarding proxy runs on a local TCP port and transparently handles all traffic, making it possible to direct URLSession at it in order to communicate with the API over the remote socks proxy.

SOCKS5 is a simple protocol and in order to establish the connection over socks proxy, the client has to perform the following steps:

Protocol

1. Initial handshake

During that phase the client sends the following information over the wire:

---

VER	NMETHODS	METHODS
1	1	1 to 255

---

• VER is the socks version, always 0x05. 1 byte.
• NMETHODS - number of authentication methods supported by the client. 1 byte.
• METHODS - variable length byte stream, depending on NMETHODS. Each byte corresponds to UInt8 constant that correlates with an authentication method defined in the protocol. See Socks5AuthenticationMethod.

The handshake response contains two bytes:

---

VER	METHOD
1	1

---

• VER is the socks version, always 0x05. 1 byte.
• METHOD is the authentication method accepted by the server. When this value is set to 0xFF, it means that the client doesn't support any of the authentication methods accepted by the server. The client should terminate connection in such case.

2. Authentication

Current implementation only supports anonymous access to socks server and therefore when authentication is not required the client can move on to the next phase.

3. Connect command

The client requests the socks proxy to establish a connection to the remote server, to which the client wants to talk to, over the proxy.

A CONNECT command should be sent over the wire to initiate the connection and the layout looks as such:

---

VER	CMD	RSV	ATYP	DST.ADDR	DST.PORT
1	1	X'00'	1	Variable	2

---

• VER - socks version. 1 byte, always 0x05.
• CMD - CONNECT command, which is represented by a byte code: 0x01. 1 byte.
• RSV - reserved field. Always zero. 1 byte.
• ATYP - address type. Socks support IPv4, IPv6 and DNS domain name (See Socks5AddressType). 1 byte.
• DST.ADDR - the address of the remote server to which the clients wants to connect.
  • IPv4: 4 octets
  • IPv6: 16 octets
  • Domain name: the first byte represents the length of domain name, the reminder of it is the domain name itself. Note that since only one byte is used to represent the length of domain name, it means that the domain name cannot be longer than 255 bytes (UInt8.max)
• DST.PORT - the port of the remote server to which the client wants to connect. Typically our API runs on port 443 (SSL). This field is UInt16 in network byte order, 2 bytes long.

The response to this request is nearly identical:

---

VER	REP	RSV	ATYP	BND.ADDR	BND.PORT
1	1	X'00'	1	Variable	2

---

• VER - socks version. 1 byte, always 0x05.
• REP - response code. 1 byte. See Socks5StatusCode. Zero byte indicates success.
• RSV - reserved byte, always zero. 1 byte.
• ATYP - address type. Socks support IPv4, IPv6 and DNS domain name (See Socks5AddressType). 1 byte.
• BND.ADDR and BND.PORT - typically seem to coincide with the socks proxy address and port.

if REP contains zero, the client can begin the data exchange with the remote server (our API).

4. Begin data exchange with the remote server

At this point the socks5 proxy turns into the tunnel to the remote server, so the client can begin sending and receiving HTTP/S or any other traffic over the wire. This is the simplest part where the client passes bytes from local to remote connection and back.

Implementation structure

• Socks5Connection - type implementing a bidirectional data connection between a local and remote endpoint over the socks proxy. it's responsible for handling the socks5 flow, local and remote connections. It delegates each individual phase to the following objects:
  • Socks5HandshakeNegotiation - a type responsible for performing a handshake negotiation.
  • Socks5ConnectNegotiation - a type responsible for performing a CONNECT command negotiation.
  • Socks5DataStreamHandler - a type responsible for bidirectional data exchange.
• Socks5ForwardingProxy - the forwarding proxy that opens a local TCP port and starts listening on it. It creates a new instance of Socks5Connection for each new connection, then it initiates the socks5 exchange by calling Socks5Connection.start().

Threading considerations

In network framework, the objects implementing networking are initialized with a dispatch queue which is used for dispatching connection-related events.

Since the socks5 flow is driven by connection related events, the execution context is guaranteed to remain the same and therefore the socks5 related types do not do any synchronization with exception of public methods, which are typically wrapped to run on the same dispatch queue.

Testing

Still looking into unit tests.

In order to activate the socks5 transport you need to:

• Return .useSocks5 from TransportStrategy.connectionTransport().
• Tweak the socks5 proxy address in TransportProvider.socks5(). When testing on device, replace .loopback with the IP of your machine in the local network or wherever you run the socks5 proxy, .loopback should work on simulator if you run socks5 proxy locally.

If you need socks5 server software, I recommend using Charles - web debugging proxy. Enable SOCKS proxy under Proxy > Proxy settings > Enable SOCKS then run the app. You don't need to configure SSL certificates or anything. You can also try one of public & free SOCKS proxies available online.

---

This change is 

[linear]

IOS-358 POC a SOCKS client for URLSession

We must find a way to proxy API traffic through a SOCKS client. URLSession cannot be configured to use SOCKS proxies, so instead, we should have a SOCKS client that port forwards from a local port, not unlike what we're doing with Shadowsocks today.

[rablador]

Reviewed 24 of 24 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @pronebird)

---

ios/MullvadTransport/Socks5/Socks5Connection.swift line 128 at r1 (raw file):

    }

    private func onLocalConnectiondState(_ connectionState: NWConnection.State) {

Nit: ConnectionD

---

ios/MullvadTransport/Socks5/Socks5ConnectNegotiation.swift line 78 at r1 (raw file):

            addressType: addressType,
            onComplete: { [self] endpoint in
                let reply = Socks5ConnectReply(status: statusCode, serverBoundEndpoint: endpoint)

Since we guard for a specific code above (.succeeded) and then pass the var here anyway, if the guard is ever removed we might end up with unexpected behavior. If we intead pass .succeeded we will never end up with anything but that no matter what.

---

ios/MullvadTransport/Socks5/Socks5ForwardingProxy.swift line 130 at r1 (raw file):

        case let .starting(listener, previousCompletion):
            // Accumulate completion handlers when requested to start multiple times in a row.

What's the reason to accumulate completions? Can't we just drop multiple calls here since we're waiting to start anyway?

---

ios/MullvadTransport/Socks5/Socks5ForwardingProxy.swift line 257 at r1 (raw file):

            )
            socks5Connection.setStateHandler { [weak self] socks5Connection, state in
                if case let .stopped(error) = state {

Only handle one state here?

[pronebird]

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

---

ios/MullvadTransport/Socks5/Socks5Connection.swift line 128 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: ConnectionD

Really need to fix my keyboard. Done.

---

ios/MullvadTransport/Socks5/Socks5ConnectNegotiation.swift line 78 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Since we guard for a specific code above (.succeeded) and then pass the var here anyway, if the guard is ever removed we might end up with unexpected behavior. If we intead pass .succeeded we will never end up with anything but that no matter what.

What "var" and why would you remove a statusCode check above?

---

ios/MullvadTransport/Socks5/Socks5ForwardingProxy.swift line 130 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

What's the reason to accumulate completions? Can't we just drop multiple calls here since we're waiting to start anyway?

To keep this call idempotent.

---

ios/MullvadTransport/Socks5/Socks5ForwardingProxy.swift line 257 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Only handle one state here?

Yeah? As far as proxy concerned, it's only interested to remove the connection from the list of connections when it stopped.

[rablador]

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

---

ios/MullvadTransport/Socks5/Socks5ConnectNegotiation.swift line 78 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

What "var" and why would you remove a statusCode check above?

Sorry, I meant the statusCode variable.
No, the guard should stay, but I think .succeeded could be passed instead of statusCode. We know that - in the current impl. - statusCode will always be .succeeded if we get to this point in the code, which implies that we should always pass .succeeded to the Socks5ConnectReply init. But what if the code above (incl where the guard is) changes? Then we might pass a status code that's not successful and thus get possibly incorrect behavior.

---

ios/MullvadTransport/Socks5/Socks5ForwardingProxy.swift line 130 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

To keep this call idempotent.

Alright, to keep track of potential errors?

[pronebird]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @rablador)

---

ios/MullvadTransport/Socks5/Socks5ConnectNegotiation.swift line 78 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Sorry, I meant the statusCode variable.
No, the guard should stay, but I think .succeeded could be passed instead of statusCode. We know that - in the current impl. - statusCode will always be .succeeded if we get to this point in the code, which implies that we should always pass .succeeded to the Socks5ConnectReply init. But what if the code above (incl where the guard is) changes? Then we might pass a status code that's not successful and thus get possibly incorrect behavior.

I don't understand the premise of your suggestion.

So you're saying that we can infer that the statusCode == .succeeded after the guard check and that we should hardcode the status code instead.

Then you say that someone could remove that guard by mistake, which could lead to logical error and that it's ok if we keep reporting that everything is hunky-dory to the call site instead of reporting the actual status that we received from server?

---

ios/MullvadTransport/Socks5/Socks5ForwardingProxy.swift line 130 at r1 (raw file):
Not exactly, but to simplify the code. I think it's worth to remind ourselves what idempotence means:

Idempotence, in programming and mathematics, is a property of some operations such that no matter how many times you execute them, you achieve the same result.

For example, when you say that we should "drop" multiple calls, what do you really mean by that? Correct me if I am wrong, but to me it sounds like you suggest to simply return and never call the completion handler given by the caller when the object is already in the starting state.

Apologize for the use of that term, but "ghosting" the caller typically leads to hard to debug problems, outside flows hit the road block and never continue, swift continuations tend to break, . Workarounds include the usage of timers, we had exactly that problem in the packet tunnel when using network extension APIs.

Sometimes it's ok this way, but not always. When designing independent components it's better to keep their behavior as straightforward as possible, to lessen the impact on the caller.

Yes we could return (aka drop?), we could even call the completion handler right away and give it the error indicating that the object is already starting but why complicate and introduce additional code paths when we can simply accumulate the completion handler and carry on?

In this particular scenario, isn't it easier when the object behaves identical and accumulate the blocks? For instance:

proxy.start { error in
  // called first
}
proxy.start { error in
  // called second
}

and not like that:

proxy.start { error in
  // called first
}
proxy.start { error in
  // never called??
}

nor

proxy.start { error in
  // called first
}
proxy.start { error in
  // called with error telling the object is already starting
}

Consumer code is a "storm", it's often complex and perhaps a mess and small components like this can be an "island" of sanity, that can keep the "storm" from the outside from ripping the "island" and everything outside apart.

I can imagine that the consumer code be sharing the same proxy, and keeping the call to start() idempotent like this means that the behavior of object is consistent regardless if there are two or more external components that call proxy.start() at the same time.

It's just practical in that case and it avoids handling additional edge cases. You can argue we know exactly how we gonna use it but things change and when someone decides to use in the other way, we have edge cases, this approach eliminates all that.

[buggmagnet]

Reviewed 12 of 24 files at r1, 1 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @pronebird and @rablador)

---

ios/MullvadTransport/AnyIPEndpoint+Socks5.swift line 13 at r2 (raw file):

import Network

extension AnyIPEndpoint {

What's the difference between AnyIPEndpoint and AnyIPAddress ?
And by the same logic, the question extends to the types IPv[4|6]Address and IPv[4|6]Endpoint

---

ios/MullvadTransport/DeferredCancellable.swift line 13 at r2 (raw file):

/// Cancellable object that defers cancellation until the other token is connected to it.
final class DeferredCancellable: Cancellable {

This doesn't do any deferred actions.
I would suggest to rename it to TokenChain or something similar since it links the lifetime of several Cancellable tokens.

---

ios/MullvadTransport/DeferredCancellable.swift line 23 at r2 (raw file):

    ///
    /// The token is cancelled immediately, if the deferred object is already cancelled.
    func connect(_ token: Cancellable) {

I see why you named it connect but I think it's a misleading name. Considering we do a lot of network related calls, we usually think of connect as "connect to a webserver".
Here rather, we link tokens together.

---

ios/MullvadTransport/Socks5/Socks5Connection.swift line 11 at r2 (raw file):

import Network

/// A bidirection data connection between a local endpoint and remote endpoint over socks proxy.

typo
bidirection -> bidirectional

---

ios/MullvadTransport/Socks5/Socks5Connection.swift line 17 at r2 (raw file):

    /**
     Initilizes a new connection passing data between local and remote TCP connection over the socks proxy.

typo
Initilizes

---

ios/MullvadTransport/Socks5/Socks5Connection.swift line 72 at r2 (raw file):

     Set a handler that receives connection state events.

     It's advised to set the state handler before starting the conncetion to avoid missing updates to the connection state.

typo
conncetion

---

ios/MullvadTransport/Socks5/Socks5Connection.swift line 128 at r2 (raw file):

    }

    private func onLocalConnectionState(_ connectionState: NWConnection.State) {

nit
What do you think of renaming thisonConnectionStateChanged ?

---

ios/MullvadTransport/Socks5/Socks5Connection.swift line 136 at r2 (raw file):

            initiateConnection()

        case let .waiting(error), let .failed(error):

The .waiting state is not fatal, we shouldn't error out until we reach the .failed state

The same comment applies for onRemoteConnectionState

---

ios/MullvadTransport/Socks5/Socks5Connection.swift line 144 at r2 (raw file):

    }

    private func onRemoteConnectionState(_ connectionState: NWConnection.State) {

Right now, both onLocalConnectionState and onRemoteConnectionState have the same implementation.
We can use a single function to do both if we don't intend to separate the behavior based on whether the connection is remote or local

---

ios/MullvadTransport/Socks5/Socks5ConnectNegotiation.swift line 98 at r2 (raw file):

        // Read status code, reserved field and address type from reply.
        guard let rawStatusCode = iterator.next(),
              let _ = iterator.next(), // skip reserved field

⚠️ 👮
Unused Optional Binding Violation: Prefer != niloverlet _ = (unused_optional_binding)

[pronebird]

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

---

ios/MullvadTransport/AnyIPEndpoint+Socks5.swift line 13 at r2 (raw file):

Previously, buggmagnet wrote…

What's the difference between AnyIPEndpoint and AnyIPAddress ?
And by the same logic, the question extends to the types IPv[4|6]Address and IPv[4|6]Endpoint

The endpoint is typically a pair of address and port or hostname and port. An IP address is just that - an address without the port component. I hope that answers your question.

Note that AnyIPAddress hasn't been used anywhere but in PreferencesViewModel.

---

ios/MullvadTransport/DeferredCancellable.swift line 13 at r2 (raw file):
The deferred cancellable postpones, or in other words defers the cancellation in the event when cancellation request arrives prior to the actual cancellable being connected to the deferred one.

This is the dictionary definition of the word defer which precisely describes what's happening here:

Defer
put off (an action or event) to a later time; postpone.
"they deferred the decision until February"

---

ios/MullvadTransport/DeferredCancellable.swift line 23 at r2 (raw file):

Previously, buggmagnet wrote…

I see why you named it connect but I think it's a misleading name. Considering we do a lot of network related calls, we usually think of connect as "connect to a webserver".
Here rather, we link tokens together.

These are synonyms. In general the word "connect" does not necessarily have anything to do with network.

In fact network framework does not even use connect anywhere and mostly sticks to start or cancel instead of connect or disconnect. I don't think it's even the case anywhere with URLSession whos data tasks are being started with a call to activate or resume.

On the other side "connect" is commonly used in Combine when connecting to the publisher that yields elements. In this particular case the deferred cancellable object is a publisher and we connect the other cancellable token to it.

---

ios/MullvadTransport/Socks5/Socks5Connection.swift line 11 at r2 (raw file):

Previously, buggmagnet wrote…

typo
bidirection -> bidirectional

Done

---

ios/MullvadTransport/Socks5/Socks5Connection.swift line 17 at r2 (raw file):

Previously, buggmagnet wrote…

typo
Initilizes

Done

---

ios/MullvadTransport/Socks5/Socks5Connection.swift line 72 at r2 (raw file):

Previously, buggmagnet wrote…

typo
conncetion

Done.

---

ios/MullvadTransport/Socks5/Socks5Connection.swift line 128 at r2 (raw file):

Previously, buggmagnet wrote…

nit
What do you think of renaming thisonConnectionStateChanged ?

Since we have two different connections, having a generic "on_Connection_..." used for state observing function is misleading imo.

---

ios/MullvadTransport/Socks5/Socks5Connection.swift line 136 at r2 (raw file):

Previously, buggmagnet wrote…

The .waiting state is not fatal, we shouldn't error out until we reach the .failed state

The same comment applies for onRemoteConnectionState

It's not fatal if you intend to wait for connectivity. The idea was to error right away and let the REST framework handle the retry instead. This aligns with URLSession configuration we use, where we do not wait for network connectivity (waitsForConnectivity in URLSessionConfiguration).

---

ios/MullvadTransport/Socks5/Socks5Connection.swift line 144 at r2 (raw file):

Previously, buggmagnet wrote…

Right now, both onLocalConnectionState and onRemoteConnectionState have the same implementation.
We can use a single function to do both if we don't intend to separate the behavior based on whether the connection is remote or local

The implementation is similar but not entirely the same. Different error is passed to handleError(). I thought about adding a factory to produce a closure mapping to the right error, but then opted-in for simplicity as it's easier to debug it this way since this way we know which connection state is being received.

---

ios/MullvadTransport/Socks5/Socks5ConnectNegotiation.swift line 98 at r2 (raw file):

Previously, buggmagnet wrote…

⚠️ 👮
Unused Optional Binding Violation: Prefer != niloverlet _ = (unused_optional_binding)

Done.

[rablador]

Reviewable status: 21 of 24 files reviewed, 9 unresolved discussions (waiting on @buggmagnet and @pronebird)

---

ios/MullvadTransport/Socks5/Socks5ForwardingProxy.swift line 130 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Not exactly, but to simplify the code. I think it's worth to remind ourselves what idempotence means:

Idempotence, in programming and mathematics, is a property of some operations such that no matter how many times you execute them, you achieve the same result.

For example, when you say that we should "drop" multiple calls, what do you really mean by that? Correct me if I am wrong, but to me it sounds like you suggest to simply return and never call the completion handler given by the caller when the object is already in the starting state.

Apologize for the use of that term, but "ghosting" the caller typically leads to hard to debug problems, outside flows hit the road block and never continue, swift continuations tend to break, . Workarounds include the usage of timers, we had exactly that problem in the packet tunnel when using network extension APIs.

Sometimes it's ok this way, but not always. When designing independent components it's better to keep their behavior as straightforward as possible, to lessen the impact on the caller.

Yes we could return (aka drop?), we could even call the completion handler right away and give it the error indicating that the object is already starting but why complicate and introduce additional code paths when we can simply accumulate the completion handler and carry on?

In this particular scenario, isn't it easier when the object behaves identical and accumulate the blocks? For instance:

proxy.start { error in
  // called first
}
proxy.start { error in
  // called second
}

and not like that:

proxy.start { error in
  // called first
}
proxy.start { error in
  // never called??
}

nor

proxy.start { error in
  // called first
}
proxy.start { error in
  // called with error telling the object is already starting
}

Consumer code is a "storm", it's often complex and perhaps a mess and small components like this can be an "island" of sanity, that can keep the "storm" from the outside from ripping the "island" and everything outside apart.

I can imagine that the consumer code be sharing the same proxy, and keeping the call to start() idempotent like this means that the behavior of object is consistent regardless if there are two or more external components that call proxy.start() at the same time.

It's just practical in that case and it avoids handling additional edge cases. You can argue we know exactly how we gonna use it but things change and when someone decides to use in the other way, we have edge cases, this approach eliminates all that.

Yes, by dropping I meant that perhaps we wouldn't need completions for a state that's not changing anyway, but you're right that it wouldn't be idempotent that way. It's not a foreign pattern, eg. "calling twice method multiple times does nothing", but I think I prefer your suggestion anyway. Let's keep it as it is.

[rablador]

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

---

ios/MullvadTransport/Socks5/Socks5ConnectNegotiation.swift line 78 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

I don't understand the premise of your suggestion.

So you're saying that we can infer that the statusCode == .succeeded after the guard check and that we should hardcode the status code instead.

Then you say that someone could remove that guard by mistake, which could lead to logical error and that it's ok if we keep reporting that everything is hunky-dory to the call site instead of reporting the actual status that we received from server?

Sure, that could happen and wouldn't be good, but the idea behind my claim still holds up I think. The way I read this function is that basically the entirety of it should be run only if status is .succeeded (the comment even says "Parse server bound endpoint when partial reply indicates success."), so in that sense the guard might as well be moved to the call site and the function be renamed "handleSuccessfulReply" or something. That way no errors would be made. It IS nice to handle all this in the same function though, and I'm not particularly adamant about it. Just pointing it out.

[pronebird]

Reviewable status: 19 of 24 files reviewed, 7 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadTransport/Socks5/Socks5ConnectNegotiation.swift line 78 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Sure, that could happen and wouldn't be good, but the idea behind my claim still holds up I think. The way I read this function is that basically the entirety of it should be run only if status is .succeeded (the comment even says "Parse server bound endpoint when partial reply indicates success."), so in that sense the guard might as well be moved to the call site and the function be renamed "handleSuccessfulReply" or something. That way no errors would be made. It IS nice to handle all this in the same function though, and I'm not particularly adamant about it. Just pointing it out.

Moved reply handling to the call site.

[rablador]

Reviewed 5 of 5 files at r4, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @buggmagnet)

[buggmagnet]

Reviewed 5 of 24 files at r1, 5 of 5 files at r4, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @pronebird)

---

ios/MullvadTransport/AnyIPEndpoint+Socks5.swift line 13 at r2 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

The endpoint is typically a pair of address and port or hostname and port. An IP address is just that - an address without the port component. I hope that answers your question.

Note that AnyIPAddress hasn't been used anywhere but in PreferencesViewModel.

Thanks for the clarification. I'm wondering then if we shouldn't deprecate AnyIPAddress and replace them with an AnyIPEndpoint with port 0 instead.

---

ios/MullvadTransport/DeferredCancellable.swift line 13 at r2 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

The deferred cancellable postpones, or in other words defers the cancellation in the event when cancellation request arrives prior to the actual cancellable being connected to the deferred one.

This is the dictionary definition of the word defer which precisely describes what's happening here:

Defer
put off (an action or event) to a later time; postpone.
"they deferred the decision until February"

Right, it still doesn't do any deferred action. It doesn't postpone cancellation. It will cancel right away all the tokens if it is cancelled. That is not a deferred mechanism.

If we look at the doc for Task, the term mentioned is "cooperative".
So this is really a CooperativeCancellable.

If you cancel one token, it cancels all tokens linked to it.

---

ios/MullvadTransport/DeferredCancellable.swift line 23 at r2 (raw file):

In general the word "connect" does not necessarily have anything to do with network.
Yes, but we're writing a VPN application. So there's a really strong contextual connotation with the word "connect".

On the other side "connect" is commonly used in Combine when connecting to the publisher that yields elements.
the deferred cancellable object is a publisher

It doesn't have any subscribers, and doesn't implement the Publisher protocol.
I don't understand why you're trying to tie Combine related terms or concept for something that has nothing to do with it.

---

ios/MullvadTransport/Socks5/Socks5Connection.swift line 136 at r2 (raw file):

This aligns with URLSession configuration we use, where we do not wait for network connectivity.

We should revisit this then, right now we have an inconsistent behaviour. Every operation that uses BackgroundObserver will always wait for connectivity, even if the flag is set to ignore it.

From the docs of waitsForConnectivity

This property is ignored by background sessions, which always wait for connectivity.

That aside, I still think we shouldn't error in the waiting state.

error right away and let the REST framework handle the retry instead

Right now, in case of error, we just call the completion handler with URLError(.cancelled)
which means a device with a bad connectivity will likely consume all the retry attempts before it can get good connectivity.

I don't see any reason to not wait it out first, and let the system decide whether the call is failed or not.
We lose nothing by waiting a bit instead of erroring out immediately.

---

ios/MullvadTransport/Socks5/Socks5ConnectNegotiation.swift line 53 at r4 (raw file):

                }
            } else {
                onFailure(Socks5Error.unexpectedEndOfStream)

nit
Technically this would be only true if we checked the contentContext isFinal message and it were true.
I would suggest either checking that, and logging an error if there is neither error, nor data to handle.

IIRC, TCP messages cannot be sent with 0 data inside, and the NWConnection API has tons of edge cases since the same API is used regardless of the protocol.
I would suggest erring on the side of caution for now.

---

ios/MullvadTransport/Socks5/Socks5ForwardingProxy.swift line 187 at r4 (raw file):

            state = .stopped
            cancelListener(listener)
            openConnections.forEach { $0.cancel() }

Shouldn't we remove all the connections after cancelling them here ?

in onEndConnection we only remove the connection if self.state == .started because otherwise we cannot access the openConnections.

However, when we call stopInner, we immediately enter the .stopped state, which means the completion handler that calls onEndConnection
when a connection is stopped won't trigger the path to remove a single connection.

---

ios/MullvadTransport/Socks5/Socks5ForwardingProxy.swift line 275 at r4 (raw file):

        case .started(let listener, var openConnections):
            guard let index = openConnections.firstIndex(where: { $0 === connection }) else { return }

Nice use of the identity operator !

[pronebird]

Reviewable status: 21 of 24 files reviewed, 8 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadTransport/Socks5/Socks5Connection.swift line 136 at r2 (raw file):

We should revisit this then, right now we have an inconsistent behaviour. Every operation that uses BackgroundObserver will always wait for connectivity, even if the flag is set to ignore it.

Background observer only starts a background task which is not the same thing as a background URL session. We configure URLSession with ephemeral configuration.

URL sessions configured with background configuration are quite different, they use a delegate and a unique identifier. Their primary purpose is to offload upload and download transfers to the system daemon which enable transfers to happen even when the app is terminated or suspended.

Right now, in case of error, we just call the completion handler with URLError(.cancelled)
which means a device with a bad connectivity will likely consume all the retry attempts before it can get good connectivity.

If you refer to URLSessionSocks5Transport, then typically we return the underlying error in any case. URLError(.cancelled) is only returned when there is no error and there is no local port opened by forwarding proxy. This scenario is very unlikely to happen.

---

ios/MullvadTransport/Socks5/Socks5ConnectNegotiation.swift line 53 at r4 (raw file):

Previously, buggmagnet wrote…

nit
Technically this would be only true if we checked the contentContext isFinal message and it were true.
I would suggest either checking that, and logging an error if there is neither error, nor data to handle.

IIRC, TCP messages cannot be sent with 0 data inside, and the NWConnection API has tons of edge cases since the same API is used regardless of the protocol.
I would suggest erring on the side of caution for now.

By the API contract, the call to receive(minimumIncompleteLength:maximumLength:completion:) is guaranteed to return an error or data. I don't think this code should be more complex than what it is.

---

ios/MullvadTransport/Socks5/Socks5ForwardingProxy.swift line 187 at r4 (raw file):

Previously, buggmagnet wrote…

Shouldn't we remove all the connections after cancelling them here ?

in onEndConnection we only remove the connection if self.state == .started because otherwise we cannot access the openConnections.

However, when we call stopInner, we immediately enter the .stopped state, which means the completion handler that calls onEndConnection
when a connection is stopped won't trigger the path to remove a single connection.

I think it's fine. All connections are cancelled right away, the state is moved to .stopped and openConnections array with all connections is released upon leaving the scope.

---

ios/MullvadTransport/Socks5/Socks5ForwardingProxy.swift line 275 at r4 (raw file):

Previously, buggmagnet wrote…

Nice use of the identity operator !

Thanks. I think it's appropriate since each connection is unique and so there is no need to use identifiers otherwise to support equality.

---

ios/MullvadTransport/DeferredCancellable.swift line 13 at r2 (raw file):

Previously, buggmagnet wrote…

Right, it still doesn't do any deferred action. It doesn't postpone cancellation. It will cancel right away all the tokens if it is cancelled. That is not a deferred mechanism.

If we look at the doc for Task, the term mentioned is "cooperative".
So this is really a CooperativeCancellable.

If you cancel one token, it cancels all tokens linked to it.

I have no energy to keep this exchange going. I have renamed the type to CancellableChain and I hope it's satisfactory for your vision.

---

ios/MullvadTransport/DeferredCancellable.swift line 23 at r2 (raw file):

Previously, buggmagnet wrote…

In general the word "connect" does not necessarily have anything to do with network.
Yes, but we're writing a VPN application. So there's a really strong contextual connotation with the word "connect".

On the other side "connect" is commonly used in Combine when connecting to the publisher that yields elements.
the deferred cancellable object is a publisher

It doesn't have any subscribers, and doesn't implement the Publisher protocol.
I don't understand why you're trying to tie Combine related terms or concept for something that has nothing to do with it.

Let it be your way. Renamed to link()

[mojganii]

Reviewable status: 21 of 24 files reviewed, 9 unresolved discussions (waiting on @buggmagnet, @pronebird, and @rablador)

---

ios/MullvadTransport/URLSessionSocks5Transport.swift line 20 at r5 (raw file):

    /// The IPv4 representation of the loopback address used by `socksProxy`.
    private let localhost = "127.0.0.1"

nit : I would suggest to use IPv4Address.loopback and assign that to the host.

[rablador]

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @buggmagnet and @pronebird)

[pronebird]

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

---

ios/MullvadTransport/URLSessionSocks5Transport.swift line 20 at r5 (raw file):

Previously, mojganii wrote…

nit : I would suggest to use IPv4Address.loopback and assign that to the host.

I'd too, but I spoke about that with @buggmagnet a while ago and he raised his concerns that a conversion from IPv*Address to String implemented via CustomDebugStringConvertible by Apple could change in the future and break the app. Shadowsocks transport uses the same approach, perhaps we could move that string into extension since we use it that often?

[buggmagnet]

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mojganii and @pronebird)

---

ios/MullvadTransport/Socks5/Socks5ConnectNegotiation.swift line 53 at r4 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

By the API contract, the call to receive(minimumIncompleteLength:maximumLength:completion:) is guaranteed to return an error or data. I don't think this code should be more complex than what it is.

Fair enough

[rablador]

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

[rablador]

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

[rablador]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: 23 of 24 files reviewed, 1 unresolved discussion (waiting on @buggmagnet, @mojganii, and @rablador)