Don't return gateway twice for manual location selection, fixes #823

To reproduce the issue:

1. Start desktop client with riseup provider
2. wait until gateways are loaded
3. select any manual location
4. connect VPN

The log files will show you:

2024-07-01T10:32:20+02:00 DBG Adding gateway to command line via --remote gateway=vpn06-ams.riseup.net ip4=51.158.144.32 port=80 proto=udp4
2024-07-01T10:32:20+02:00 DBG Adding gateway to command line via --remote gateway=vpn06-ams.riseup.net ip4=51.158.144.32 port=1194 proto=udp4
2024-07-01T10:32:20+02:00 DBG Adding gateway to command line via --remote gateway=vpn06-ams.riseup.net ip4=51.158.144.32 port=80 proto=udp4
2024-07-01T10:32:20+02:00 DBG Adding gateway to command line via --remote gateway=vpn06-ams.riseup.net ip4=51.158.144.32 port=1194 proto=udp4
2024-07-01T10:32:20+02:00 DBG Adding gateway to command line via --remote gateway=vpn04-ams.riseup.net ip4=51.15.9.205 port=80 proto=udp4
2024-07-01T10:32:20+02:00 DBG Adding gateway to command line via --remote gateway=vpn04-ams.riseup.net ip4=51.15.9.205 port=1194 proto=udp4

The problem: vpn06-ams.riseup.net shows up twice in the list.

The problem does not occur when we use automatic location selection.

Some background/confusing things on this:

curl https://api.black.riseup.net:9001/json -k | jq The reponse has a field gatways. This is a sorted list. If you run curl from a random server, the gateways are in a different order.

If we load the json into a go struct, it has two fields for gateways (Gateways and SortedGateways):

type geoLocation struct {
    IPAddress      string       `json:"ip"`
    Country        string       `json:"cc"`
    City           string       `json:"city"`
    Latitude       float64      `json:"lat"`
    Longitude      float64      `json:"lon"`
    Gateways       []string     `json:"gateways"`
    SortedGateways []geoGateway `json:"sortedGateways"`
}

Even if the api response serves a sorted list, the values are stored into the variable Gateways... This is very confusing but I think that's not a bug at this point.

There are two lists of gateways in the strct gatewayPool: available and recommended.

p.available = eip.getGateways()

In eip.getGateways() (eip_service.go), we add each gateway to p.available multiple times (for each Transport: each riseup gateway supports obfs4 and openvpn - at least that what's the api says). So the next confusing thing is: p.available does not have a list of Gateways. Gateways are listed multiple times, if they support different transport. That's my debug log:

2024-07-01T11:10:39+02:00 INF Adding gateway to p.available vpn01-sea.riseup.net with transport openvpn
2024-07-01T11:10:39+02:00 INF Adding gateway to p.available vpn01-sea.riseup.net with transport obfs4

Then there is *gatewayPool.recommended. It's set in setRecommendedGateways (gateways.go). If if len(geo.SortedGateways) != 0 (and it's always empty), we are basically just setting p.recommended = p.available.

Back to the original issue:

The function gatewayPool.getBest() returns the list of gateways we connect to. It calls *gatewayPool.getGatewaysFromMenshenByLocation(), where our bug lays:

var gateways []Gateway
for _, gw := range p.recommended {
    for _, locatedGw := range gws {
        if !locatedGw.isTransport(transport) {
            continue
        }
        if locatedGw.Host == gw.gateway.Host {
            gateways = append(gateways, *locatedGw)
            break
        }
    }
    if len(gateways) == maxGateways {
        break
    }

As each gateway is listed twice in p.recommended. And that's why each gateway gets returned twice here. We check the transport of locatedGw, not the transport of gw.

I fixed it by changing the logic: First ignore all gateways where the transport does not match. Then check if the hostname machtes.

for _, gw := range p.recommended {
    if !gw.gateway.isTransport(transport) {
        continue
    }
    for _, locatedGw := range gws {
        if locatedGw.Host == gw.gateway.Host {
            gateways = append(gateways, *locatedGw)
            break
        }
    }

changed milestone to %2024.08 LEAP VPN Release

requested review from @jkito

assigned to @peanut2

changed the description

added 2h 30m of time spent at 2024-06-30

added 1 commit

• c4daee3a - Don't return gateway twice if location is selected manually, fixes #823 (closed)

Compare with previous version

added 4 commits

• c4daee3a...e4adde88 - 3 commits from branch main
• 843bcfa3 - Don't return gateway twice if location is selected manually, fixes #823 (closed)

Compare with previous version

Very good investigation, tested it and working as expected

ca you please update the commit msg and include the link to this MR, as just by looking at the output of git log its hard to figure out which MR it refers, so the line For more information, please check the merge request description. can be changed to:

For more information, please check: https://0xacab.org/leap/bitmask-vpn/-/merge_requests/213

added 1 commit

• d1965116 - Don't return gateway twice if location is selected manually, fixes #823 (closed)

Compare with previous version

added 1 commit

• 53c30207 - Don't return gateway twice if location is selected manually, fixes #823 (closed)

Compare with previous version

resolved all threads

added 4 commits

• 53c30207...211f4aeb - 3 commits from branch main
• a0a5e930 - Don't return gateway twice if location is selected manually, fixes #823 (closed)

Compare with previous version

merged