Update relay selector for shadowsocks obfuscation

[rablador]

When shadowsocks should be used, the relay selector should only select relays that match the port settings for shadowsocks.

When selecting candidates for a given selection, only candidates able to provide the ports should be considered. In the case that there is no port specified, this filtering will do nothing.

• If a user selects a port within the port range defined in shadowsocks_port_range on the god object, all relays should be considered
• If a user selects a port outside of that port range, only the relays that provide an shadowsocks_extra_addr_in should be considered in the relay selection
• If the WireGuard over Shadowsocks obfuscation is automatic then we first select a relay, then we select a port based on whether the relay has a shadowsocks_extra_addr_in available (1-65000) or not (any port in the shadowsocks_port_range)

Acceptance criteria

• A new structure / object is created that will handle the port selection for obfuscation
• The logic held by ProtocolObfuscator for selecting the udpOverTcp port will move to that new object
• Based on Jon's prototype, a pre-filtering of relays will happen either in that new structure, or in the RelaySelectorWrapper
• This avoid changing the logic of RelaySelector and its algorithmic selection of ports based on retry attempts
• The ProtocolObfuscator won't have any logic besides creating the appropriate obfuscator based on which settings it reads (either, udpOverTcp, shadowsocks or default case of not obfuscating)
• For now, we decide to lie to the obfuscator by passing settings that are predetermined, we might revisit that later

---

This change is 

[linear]

IOS-911 Update relay selector

[buggmagnet]

Reviewed 6 of 15 files at r1.
Reviewable status: 6 of 15 files reviewed, 7 unresolved discussions (waiting on @rablador)

---

ios/MullvadREST/Relay/ObfuscationMethodSelector.swift line 18 at r1 (raw file):

        tunnelSettings: LatestTunnelSettings
    ) -> WireGuardObfuscationState {
        if tunnelSettings.wireGuardObfuscation.state == .automatic {

nit
I have to say, I really like how readable this is, that UInt extension is really doing a lot of heavy lifting, super cool ! 👍

---

ios/MullvadREST/Relay/ObfuscatorPortSelector.swift line 50 at r1 (raw file):

    }

    private func obfuscateShadowsocksRelays(tunnelSettings: LatestTunnelSettings) throws -> REST.ServerRelaysResponse {

This doesn't try, therefore we can remove the throws keyword

---

ios/MullvadREST/Relay/ObfuscatorPortSelector.swift line 96 at r1 (raw file):

        switch tunnelSettings.wireGuardObfuscation.udpOverTcpPort {
        case .automatic:
            return (connectionAttemptCount % 2 == 0) ? .only(80) : .only(5001)

nit
I would have said that we can omit the return keywords here, but swiftlint then complains about the void_function_in_ternary rule

---

ios/MullvadREST/Relay/ObfuscatorPortSelector.swift line 128 at r1 (raw file):

            port
        default:
            RelaySelector.pickRandomPort(rawPortRanges: portRanges)

nit
Should we add documentation here that specifies that the port filtering has already happened here? i.e. point out the assumptions to be made when this method is called so that we do not use it incorrectly in the future ?

---

ios/MullvadREST/Relay/RelaySelector.swift line 145 at r1 (raw file):

        }

        assertionFailure("Port selection algorithm is broken!")

nit
We never hit that assert, but we should still improve it, right now it has no saliency.
It would be better to say "expected port in range but got instead " or something along those lines

---

ios/MullvadVPNTests/MullvadREST/Relay/ObfuscatorPortSelectorTests.swift line 77 at r1 (raw file):

        )

        try Array(0 ... 10).filter { $0.isMultiple(of: 2) }.forEach { attempt in

 27> (0...10).filter { $0.isMultiple(of: 2) } 
$R24: [ClosedRange<Int><>.Element] = 6 values {
  [0] = 0
  [1] = 2
  [2] = 4
  [3] = 6
  [4] = 8
  [5] = 10
}

We don't need to wrap it in Array for it to work

---

ios/MullvadVPNTests/MullvadREST/Relay/ObfuscatorPortSelectorTests.swift line 155 at r1 (raw file):

        }

        let port = portsOutsideDefaultRange.randomElement() ?? 0

This will never fail, we can do the following instead

let port = try XCTUnwrap(portsOutsideDefaultRange.randomElement())

The same principle applies for all uses of ?? here

[rablador]

Reviewable status: 4 of 15 files reviewed, 7 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadREST/Relay/ObfuscationMethodSelector.swift line 18 at r1 (raw file):

Previously, buggmagnet wrote…

nit
I have to say, I really like how readable this is, that UInt extension is really doing a lot of heavy lifting, super cool ! 👍

🙌

---

ios/MullvadREST/Relay/ObfuscatorPortSelector.swift line 50 at r1 (raw file):

Previously, buggmagnet wrote…

This doesn't try, therefore we can remove the throws keyword

Done.

---

ios/MullvadREST/Relay/ObfuscatorPortSelector.swift line 96 at r1 (raw file):

Previously, buggmagnet wrote…

nit
I would have said that we can omit the return keywords here, but swiftlint then complains about the void_function_in_ternary rule

Seems to work though :)

---

ios/MullvadREST/Relay/ObfuscatorPortSelector.swift line 128 at r1 (raw file):

Previously, buggmagnet wrote…

nit
Should we add documentation here that specifies that the port filtering has already happened here? i.e. point out the assumptions to be made when this method is called so that we do not use it incorrectly in the future ?

I moved it inside it's calling function instead. Slighlty messier but a lot clearer as to what's going on.

---

ios/MullvadREST/Relay/RelaySelector.swift line 145 at r1 (raw file):

Previously, buggmagnet wrote…

nit
We never hit that assert, but we should still improve it, right now it has no saliency.
It would be better to say "expected port in range but got instead " or something along those lines

We could just clean up this whole method with something like:

Code snippet:

guard let port = parseRawPortRanges(rawPortRanges).randomElement()?.randomElement() {
    assertionFailure("Expected to find a port in ranges but found none")
}
return port
   

---

ios/MullvadVPNTests/MullvadREST/Relay/ObfuscatorPortSelectorTests.swift line 77 at r1 (raw file):

Previously, buggmagnet wrote…

 27> (0...10).filter { $0.isMultiple(of: 2) } 
$R24: [ClosedRange<Int><>.Element] = 6 values {
  [0] = 0
  [1] = 2
  [2] = 4
  [3] = 6
  [4] = 8
  [5] = 10
}

We don't need to wrap it in Array for it to work

Done.

---

ios/MullvadVPNTests/MullvadREST/Relay/ObfuscatorPortSelectorTests.swift line 155 at r1 (raw file):

Previously, buggmagnet wrote…

This will never fail, we can do the following instead

let port = try XCTUnwrap(portsOutsideDefaultRange.randomElement())

The same principle applies for all uses of ?? here

Done.

[buggmagnet]

Reviewed 6 of 15 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions

[buggmagnet]

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

---

ios/PacketTunnelCore/Actor/ProtocolObfuscator.swift line 52 at r2 (raw file):

        }

        remotePort = endpoint.ipv4Relay.port

We need to move this line before the guard obfuscationMethod != .off else {
the ConnectionState relies on the obfuscator returning the used port

[rablador]

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

---

ios/PacketTunnelCore/Actor/ProtocolObfuscator.swift line 52 at r2 (raw file):

Previously, buggmagnet wrote…

We need to move this line before the guard obfuscationMethod != .off else {
the ConnectionState relies on the obfuscator returning the used port

Done.

[buggmagnet]

Reviewable status: 14 of 16 files reviewed, 3 unresolved discussions (waiting on @rablador)

---

ios/MullvadVPN.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved line 2 at r3 (raw file):

{
  "originHash" : "c15149b2d59d9e9c72375f65339c04f41a19943e1117e682df27fc9f943fdc56",

Please revert this change

[rablador]

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

---

ios/MullvadVPN.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved line 2 at r3 (raw file):

Previously, buggmagnet wrote…

Please revert this change

Done.

[buggmagnet]

Reviewed 1 of 2 files at r3, all commit messages.
Reviewable status: 14 of 16 files reviewed, 2 unresolved discussions

[mojganii]

Reviewable status: 14 of 16 files reviewed, 1 unresolved discussion (waiting on @buggmagnet and @rablador)

---

ios/MullvadREST/Relay/ObfuscatorPortSelector.swift line 96 at r5 (raw file):

        switch tunnelSettings.wireGuardObfuscation.udpOverTcpPort {
        case .automatic:
            (connectionAttemptCount % 2 == 0) ? .only(80) : .only(5001)

there is SwiftLint warning here, use if instead or we can have something like this:

[.only(80) ,.only(5001)][Int(connectionAttemptCount) % 2 ]

[rablador]

Reviewable status: 14 of 16 files reviewed, 1 unresolved discussion (waiting on @buggmagnet and @mojganii)

---

ios/MullvadREST/Relay/ObfuscatorPortSelector.swift line 96 at r5 (raw file):

Previously, mojganii wrote…

there is SwiftLint warning here, use if instead or we can have something like this:

[.only(80) ,.only(5001)][Int(connectionAttemptCount) % 2 ]

I did change this at some point, so I'm surprised that it looks like this. I have now reapplied my previous fix.

[mojganii]

Reviewable status: 13 of 16 files reviewed, all discussions resolved (waiting on @buggmagnet)

[mojganii]

Reviewable status: 13 of 16 files reviewed, all discussions resolved (waiting on @buggmagnet)

[buggmagnet]

Reviewed 1 of 3 files at r6, 1 of 1 files at r7.
Reviewable status: 15 of 16 files reviewed, all discussions resolved

[buggmagnet]

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