[tor-commits] [tor/master] guard: Don't check bridge transport name when selecting eligible guards

[nickm at torproject.org]

commit 218f9f90fb58ee521296fdc3b00189c70e4e4297
Author: David Goulet <dgoulet at torproject.org>
Date:   Thu Apr 8 14:08:28 2021 -0400

    guard: Don't check bridge transport name when selecting eligible guards
    
    This is related to ticket #40360 which found this problem when a Bridge entry
    with a transport name (let say obfs4) is set without a fingerprint:
    
      Bridge obfs4 <IP>:<PORT> cert=<...> iat-mode=0
    
    (Notice, no fingerprint between PORT and "cert=")
    
    Problem: commit 09c6d0324626ffa349c7eed66d9ede92ecd71583 added a check in
    get_sampled_guard_for_bridge() that would return NULL if the selected bridge
    did not have a valid transport name (that is the Bridge transport name that
    corresponds to a ClientTransportPlugin).
    
    Unfortuantely, this function is also used when selecting our eligible guards
    which is done *before* the transport list is populated and so the added check
    for the bridge<->transport name is querying an empty list of transports
    resulting in always returning NULL.
    
    For completion, the logic is: Pick eligible guards (use bridge(s) if need be)
    then for those, initiate a connection to the pluggable transport proxy and
    then populate the transport list once we've connected.
    
    Back to get_sampled_guard_for_bridge(). As said earlier, it is used when
    selecting our eligible guards in a way that prevents us from selecting
    duplicates. In other words, if that function returns non-NULL, the selection
    continues considering the bridge was sampled before. But if it returns NULL,
    the relay is added to the eligible list.
    
    This bug made it that our eligible guard list was populated with the *same*
    bridge 3 times like so (remember no fingerprint):
    
      [info] entry_guards_update_primary(): Primary entry guards have changed. New primary guard list is:
      [info] entry_guards_update_primary():   1/3: [bridge] ($0000000000000000000000000000000000000000)
      [info] entry_guards_update_primary():   2/3: [bridge] ($0000000000000000000000000000000000000000)
      [info] entry_guards_update_primary():   3/3: [bridge] ($0000000000000000000000000000000000000000)
    
    When tor starts, it will find the bridge fingerprint by connecting to it and
    will then update the primary guard list by calling
    entry_guard_learned_bridge_identity() which then goes and update only 1 single
    entry resulting in this list:
    
      [debug] sampled_guards_update_consensus_presence(): Sampled guard [bridge] ($<FINGERPRINT>) is still listed.
      [debug] sampled_guards_update_consensus_presence(): Sampled guard [bridge] ($0000000000000000000000000000000000000000) is still listed.
      [debug] sampled_guards_update_consensus_presence(): Sampled guard [bridge] ($0000000000000000000000000000000000000000) is still listed.
    
    And here lies the problem, now tor is stuck attempting to wait for a valid
    descriptor for at least 2 guards where the second one is a bunch of zeroes and
    thus tor will never fully bootstraps:
    
      [info] I learned some more directory information, but not enough to build a
      circuit: We're missing descriptors for 1/2 of our primary entry guards
      (total microdescriptors: 6671/6703). That's ok. We will try to fetch missing
      descriptors soon.
    
    Now, why passing the fingerprint then works? This is because the list of
    guards contains 3 times the same bridge but they all have a fingerprint and so
    the descriptor can be found and tor can bootstraps.
    
    The solution here is to entirely remove the transport name check in
    get_sampled_guard_for_bridge() since the transport_list is empty at that
    point. That way, the eligible guard list only gets 1 entry, the bridge, and
    can then go on to bootstrap properly.
    
    It is OK to do so since when launching a bridge descriptor fetch, we validate
    that the bridge transport name is OK and thus avoid connecting to a bridge
    without a ClientTransportPlugin. If we wanted to keep the check in place, we
    would need to populate the transport_list much earlier and this would require
    a much bigger refactoring.
    
    Fixes #40360
    
    Signed-off-by: David Goulet <dgoulet at torproject.org>
---
 changes/ticket40360             | 5 +++++
 src/feature/client/entrynodes.c | 3 ---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/changes/ticket40360 b/changes/ticket40360
new file mode 100644
index 0000000000..4a1b2c3446
--- /dev/null
+++ b/changes/ticket40360
@@ -0,0 +1,5 @@
+  o Minor bugfixes (bridge, pluggable transport):
+    - Fix a regression that prevented to configure a Bridge line without a
+      fingerprint (which is a normal use case). Fixes bug 40360; bugfix on
+      0.4.5.4-rc.
+
diff --git a/src/feature/client/entrynodes.c b/src/feature/client/entrynodes.c
index 232216c521..82866ea668 100644
--- a/src/feature/client/entrynodes.c
+++ b/src/feature/client/entrynodes.c
@@ -804,9 +804,6 @@ get_sampled_guard_for_bridge(guard_selection_t *gs,
   entry_guard_t *guard;
   if (BUG(!addrport))
     return NULL; // LCOV_EXCL_LINE
-  if (bridge_has_invalid_transport(bridge)) {
-    return NULL;
-  }
   guard = get_sampled_guard_by_bridge_addr(gs, addrport);
   if (! guard || (id && tor_memneq(id, guard->identity, DIGEST_LEN)))
     return NULL;