Commit dc06bbdc authored by juga's avatar juga
Browse files

chg: When an exit fail to exit, check CC

before selecting other exits as helpers.
In #40136 we forgot to consider the corner case in #40041, discovered
thanks to analysis#36.
This was causing that sometimes a CC circuit was built when
bwscanner_cc wasn't equal or greater than 1 or the other way around.

We didn't realize about this cause this part of the code is very
confusing. To don't make it even more confusing, i've changed the
internal API:
- `select_helper_candidates`: split funtion into one to select the
  helper candidates, knowing whether to use the relay as exit or not
  and other function `use_relay_as_entry` to decide whether to use the
  relay as entry or not checking CC params.
  Also pass a new arg `relay_as_entry`.
- `create_path_relay`: rewmove not used `cb` arg, add `candidates` arg
  to stop having to select them again later on. Move the no
  `candidates` condition here instead of checking it in
  `pick_ideal_second_hop`.
- `_pick_ideal_second_hope: remove unneded `dest` and `cont arguments,
  rename `is_exit` to `helper_is_exit`. Use the candidates instead of
  selecting them again.
- `measure_relay`: in the case an exit fails to exit, select the
  candidates knowing that they have to be exits and checking CC.
- `only_relays_with_bandwidth`: remove unneded arg `controller` so
  that there is no need to pass it through several functions.

Closes #40138
parent 52e64583
Pipeline #39044 failed with stages
in 8 minutes and 39 seconds
......@@ -2,13 +2,31 @@
start
:relay_as_entry = True;
if (consensus has `cc_alg=2`) then (yes)
if (consensus has `bwscanner_cc>=1`) then (yes)
if (relay to measure is exit AND has `FlowCtrl=2`) then (yes)
:use relay as exit;
:relay_as_entry = False;
endif
'no bwscanner>=1
else (no)
if (relay to measure is exit AND has NOT `FlowCtrl=2`) then (yes)
:relay_as_entry = False;
endif
endif
' no cc_alg=2
else (no)
if (relay to measure is exit?) then (yes)
:relay_as_entry = False;
endif
endif
if (consensus has `cc_alg=2`) then (yes)
if (consensus has `bwscanner_cc>=1`) then (yes)
if (relay to measure is exit AND has `FlowCtrl=2` AND NOT relay_as_entry) then (yes)
:obtain non-exits;
else (no)
:use relay as entry;
:obtain exits
with proto `FlowCtrol=2`
without bad flag
......@@ -17,11 +35,9 @@ if (consensus has `cc_alg=2`) then (yes)
endif
'no bwscanner>=1
else (no)
if (relay to measure is exit AND has NOT `FlowCtrl=2`) then (yes)
:use relay as exit;
if (relay to measure is exit AND has NOT `FlowCtrl=2` AND NOT relay_as_entry) then (yes)
:obtain non-exits;
else
:use relay as entry;
else (no)
:obtain exits
with proto `FlowCtrol!=2`
without bad flag
......@@ -31,18 +47,17 @@ if (consensus has `cc_alg=2`) then (yes)
endif
' no cc_alg=2
else (no)
if (relay to measure is exit?) then (yes)
:use relay as exit;
if (relay to measure is exit AND NOT relay_as_entry?) then (yes)
:obtain non-exits;
else (no)
:use relay as entry;
:obtain an exits
:obtain exits
without bad flag
that can exit
to port 443;
endif
endif
:potential second relays;
:obtain a relay
from potential
......
......@@ -58,7 +58,7 @@ def main(args, conf):
for relay in non_exits:
double_min_bw = max(exits_min_bw, relay.consensus_bandwidth * 2)
helpers = stem_utils.only_relays_with_bandwidth(
controller, exits_with_min_bw, min_bw=double_min_bw
exits_with_min_bw, min_bw=double_min_bw
)
if helpers:
log.debug(
......@@ -70,7 +70,7 @@ def main(args, conf):
else:
min_bw = max(exits_min_bw, relay.consensus_bandwidth)
helpers = stem_utils.only_relays_with_bandwidth(
controller, exits_with_min_bw, min_bw=min_bw
exits_with_min_bw, min_bw=min_bw
)
if helpers:
log.debug(
......
......@@ -211,11 +211,11 @@ def measure_bandwidth_to_server(session, conf, dest, content_length):
return results, None
def _pick_ideal_second_hop(relay, dest, rl, cont, is_exit):
def _pick_ideal_second_hop(relay, rl, relay_as_entry, candidates):
"""
Sbws builds two hop circuits. Given the **relay** to measure with
destination **dest**, pick a second relay that is or is not an exit
according to **is_exit**.
Sbws builds two hop circuits. Given the **relay** to measure, pick a helper
relay from **candidates** with minimum bandwidth depending on whether
**relay_as_entry**.
"""
# 40041: Instead of using exits that can exit to all IPs, to ensure that
# they can make requests to the Web servers, try with the exits that
......@@ -224,22 +224,14 @@ def _pick_ideal_second_hop(relay, dest, rl, cont, is_exit):
# a problem since the relay will be measured in the next loop with other
# random exit.
# #40125: Select the candidates calling again this function, call already
# in `measure_relay`, so that there is no need to change internal API.
_, candidates = select_helper_candidates(relay, rl, dest)
if not len(candidates):
log.debug("No candidates.")
return None
# While not all exits implement congestion control, the min bw might not
# correspond to the subset that implement it.
min_relay_bw = rl.exit_min_bw() if is_exit else rl.non_exit_min_bw()
min_relay_bw = rl.exit_min_bw() if relay_as_entry else rl.non_exit_min_bw()
log.debug(
"Picking a 2nd hop to measure %s from %d choices. is_exit=%s",
"Picking a 2nd hop to measure %s from %d choices. relay_as_entry=%s",
relay.nickname,
len(candidates),
is_exit,
relay_as_entry,
)
for min_bw_factor in [2, 1.75, 1.5, 1.25, 1]:
min_bw = relay.consensus_bandwidth * min_bw_factor
......@@ -249,7 +241,7 @@ def _pick_ideal_second_hop(relay, dest, rl, cont, is_exit):
if min_bw < min_relay_bw:
min_bw = min_relay_bw
new_candidates = stem_utils.only_relays_with_bandwidth(
cont, candidates, min_bw=min_bw
candidates, min_bw=min_bw
)
if len(new_candidates) > 0:
chosen = rng.choice(new_candidates)
......@@ -289,15 +281,16 @@ def error_no_helper(relay, dest, our_nick=""):
]
def create_path_relay(relay, dest, rl, cb, relay_as_entry=True):
# the helper `is_exit` arg (should be better called `helper_as_exit`),
def create_path_relay(relay, dest, rl, relay_as_entry, candidates):
# the helper `relay_as_entry` arg,
# is True when the relay is the entry (helper has to be exit)
# and False when the relay is not the entry, ie. is the exit (helper does
# not have to be an exit)
helper = _pick_ideal_second_hop(
relay, dest, rl, cb.controller, is_exit=relay_as_entry
)
if not candidates:
return error_no_helper(relay, dest)
helper = _pick_ideal_second_hop(relay, rl, relay_as_entry, candidates)
if not helper:
return error_no_helper(relay, dest)
......@@ -324,12 +317,13 @@ def error_no_circuit(circ_fps, nicknames, reason, relay, dest, our_nick):
]
def select_helper_candidates(relay, rl, dest):
"""Return whether to use the relay as entry and helper candidates list.
def use_relay_as_entry(relay, rl, dest):
"""Return whether to use the relay as entry or not.
:rtype: tuple(bool, list)
:rtype: bool
"""
log.debug("Deciding whether to use the relay as entry or not.")
relay_as_entry = True
if rl.is_consensus_cc_alg_2:
log.debug("Congestion control enabled.")
......@@ -343,6 +337,58 @@ def select_helper_candidates(relay, rl, dest):
log.debug("Exit has 2 in FlowCtrl.")
log.debug("Use relay as exit. Choose non-exits.")
relay_as_entry = False
else: # no exit or no 2 in FlowCtrl
log.debug("Relay is not exit or has NOT 2 in FlowCtrl.")
log.debug(
"Use relay as entry."
"Choose an exit that does have 2 in FlowCtrl"
)
else: # bwscanner_cc != 1
log.debug("Do not use congestion control.")
if (
relay.is_exit_not_bad_allowing_port(dest.port)
and not relay.has_2_in_flowctrl
):
log.debug("Relay to measure is exit.")
log.debug("Exit has NOT 2 in FlowCtrl.")
log.debug("Use relay as exit. Choose non-exits.")
relay_as_entry = False
else: # no exit or 2 in FlowCtrl
log.debug("Relay is not exit or it has 2 in FlowCtrl.")
log.debug(
"Use relay as entry."
"Choose an exit that does NOT have 2 in FlowCtrl"
)
else: # cc_alg!=2
log.debug("Congestion control disabled.")
if relay.is_exit_not_bad_allowing_port(dest.port):
log.debug("Relay to measure is exit.")
log.debug("Use relay as exit. Choose non-exits.")
relay_as_entry = False
else:
log.debug("Relay to measure is NOT exit.")
log.debug("Use relay as entry. Choose an exit.")
return relay_as_entry
def select_helper_candidates(relay, rl, dest, relay_as_entry=True):
"""Return helper candidates list.
:rtype: list
"""
if rl.is_consensus_cc_alg_2:
log.debug("Congestion control enabled.")
if rl.is_consensus_bwscanner_cc_gte_1:
log.debug("Use congestion control.")
if (
relay.is_exit_not_bad_allowing_port(dest.port)
and relay.has_2_in_flowctrl
and not relay_as_entry
):
log.debug("Relay to measure is exit.")
log.debug("Exit has 2 in FlowCtrl.")
log.debug("Use relay as exit. Choose non-exits.")
candidates = rl.non_exits
else: # no exit or no 2 in FlowCtrl
log.debug("Relay is not exit or has NOT 2 in FlowCtrl.")
......@@ -356,11 +402,11 @@ def select_helper_candidates(relay, rl, dest):
if (
relay.is_exit_not_bad_allowing_port(dest.port)
and not relay.has_2_in_flowctrl
and not relay_as_entry
):
log.debug("Relay to measure is exit.")
log.debug("Exit has NOT 2 in FlowCtrl.")
log.debug("Use relay as exit. Choose non-exits.")
relay_as_entry = False
candidates = rl.non_exits
else: # no exit or 2 in FlowCtrl
log.debug("Relay is not exit or it has 2 in FlowCtrl.")
......@@ -371,24 +417,27 @@ def select_helper_candidates(relay, rl, dest):
candidates = rl.exits_without_2_in_flowctrl(dest.port)
else: # cc_alg!=2
log.debug("Congestion control disabled.")
if relay.is_exit_not_bad_allowing_port(dest.port):
if (
relay.is_exit_not_bad_allowing_port(dest.port)
and not relay_as_entry
):
log.debug("Relay to measure is exit.")
log.debug("Use relay as exit. Choose non-exits.")
relay_as_entry = False
candidates = rl.non_exits
else:
log.debug("Relay to measure is NOT exit.")
log.debug("Use relay as entry. Choose an exit.")
candidates = rl.exits_not_bad_allowing_port(dest.port)
# In the case the helper is an exit, the entry could be an exit too
# In the case the relay is measured as an exit, the entry helper could be
# an exit too
# (#40041), so ensure the helper is not the same as the entry, likely to
# happen in a test network.
if not relay_as_entry:
if not relay_as_entry: # relay to measure as exit
candidates = [
c for c in candidates if c.fingerprint != relay.fingerprint
]
return (relay_as_entry, candidates)
return candidates
def measure_relay(args, conf, destinations, cb, rl, relay):
......@@ -449,8 +498,9 @@ def measure_relay(args, conf, destinations, cb, rl, relay):
# #40125 Check whether the relay will be used as entry and which obtain the
# helper candidates.
relay_as_entry, candidates = select_helper_candidates(relay, rl, dest)
r = create_path_relay(relay, dest, rl, cb, relay_as_entry=relay_as_entry)
relay_as_entry = use_relay_as_entry(relay, rl, dest)
candidates = select_helper_candidates(relay, rl, dest, relay_as_entry)
r = create_path_relay(relay, dest, rl, relay_as_entry, candidates)
# When `error_no_helper` is triggered because a helper is not found, what
# can happen in test networks with very few relays, it returns a list with
......@@ -499,7 +549,10 @@ def measure_relay(args, conf, destinations, cb, rl, relay):
nicknames,
usable_data,
)
r = create_path_relay(relay, dest, rl, cb)
relay_as_entry = True
# select new candidates as exit
candidates = select_helper_candidates(relay, rl, dest, relay_as_entry)
r = create_path_relay(relay, dest, rl, relay_as_entry, candidates)
if len(r) == 1:
return r
circ_fps, nicknames, exit_policy = r
......
......@@ -514,11 +514,20 @@ class RelayList:
in their protover consensus line.
"""
return [
exits_flowctrl2 = [
r
for r in self.exits_not_bad_allowing_port(port)
if r.has_2_in_flowctrl
]
# In chutney, just take relays with exit flag
if (
not exits_flowctrl2
and self._controller.get_conf("TestingTorNetwork") == "1"
):
exits_flowctrl2 = list(
filter(lambda r: r.has_2_in_flowctrl, self.exits)
)
return exits_flowctrl2
def exits_without_2_in_flowctrl(self, port):
"""
......
......@@ -297,7 +297,7 @@ def get_socks_info(controller):
log.debug(e)
def only_relays_with_bandwidth(controller, relays, min_bw=None, max_bw=None):
def only_relays_with_bandwidth(relays, min_bw=None, max_bw=None):
"""
Given a list of relays, only return those that optionally have above
**min_bw** and optionally have below **max_bw**, inclusively. If neither
......
......@@ -2,7 +2,12 @@ import logging
import pytest
from sbws.core.scanner import _pick_ideal_second_hop, measure_relay
from sbws.core.scanner import (
_pick_ideal_second_hop,
measure_relay,
select_helper_candidates,
use_relay_as_entry,
)
from sbws.lib.resultdump import ResultSuccess
......@@ -85,7 +90,8 @@ def test_second_hop_has_2_in_flowctrl(
assert rl.is_consensus_bwscanner_cc_gte_1
dest = dests._all_dests[0]
relay = rl._relays[0]
helper = _pick_ideal_second_hop(
relay, dest, rl, persistent_launch_tor, False
)
relay_as_entry = use_relay_as_entry(relay, rl, dest)
candidates = select_helper_candidates(relay, rl, dest, relay_as_entry)
helper = _pick_ideal_second_hop(relay, rl, relay_as_entry, candidates)
assert helper.has_2_in_flowctrl
......@@ -36,13 +36,12 @@ def test_build_circuit_with_exit_flowctrl(is_cc_tor_version, cb, rl):
assert not circuit_id
# Valid path, not valid exit
exits = rl.exits_with_2_in_flowctrl(port=443)
# Valid path and relays
exit_relay = random.choice(exits)
# See https://gitlab.torproject.org/tpo/core/chutney/-/issues/40013:
# Work around to get supposed non-exits because chutney is putting Exit
# flag to all relays
non_exits = list(set(rl.exits).difference(set(exits)))
entry = random.choice(non_exits)
# Valid path and relays
exit_relay = random.choice(exits)
entry = random.choice([e for e in exits if e != exit_relay])
path = [entry.fingerprint, exit_relay.fingerprint]
circuit_id, _ = cb.build_circuit(path)
assert circuit_id
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment