Opened 5 years ago

Closed 2 years ago

#14216 closed defect (fixed)

We only "optimistically retry connections" once if we have other bridges descs

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-bridge
Cc: dcf, isis Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Start your Tor Browser using obfs3 bridges. Now you'll have descriptors for these obfs3 bridges in your state file.

Then restart and use meek bridges. Now you'll have both.

Then use meek until something times out and your Tor client marks your single meek bridge as down.

Then on the next request you make, your Tor will realize that all your bridges are down, and print out

"Application request when we haven't used client functionality "
"lately. Optimistically trying known %s again.",
options->UseBridges ? "bridges" : "entrynodes");

and call entries_retry_all().

Then entries_retry_helper() will walk through your list of entry_guards, and call
router_set_status(node->identity, 1)
on all of them -- including your obfs3 bridges!

Then use your Tor Browser a while more, until something else times out and your single meek bridge gets marked as down again.

Then on the next request you make, your Tor *doesn't* realize that all your bridges are down, since your obfs3 bridges are listed as running. So it just sits there wondering why it can't make a circuit. Oops!

Child Tickets

Change History (15)

comment:1 Changed 5 years ago by arma

The overly simple fix is just

diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c
index b18aabe..a057ae4 100644
--- a/src/or/entrynodes.c
+++ b/src/or/entrynodes.c
@@ -2357,7 +2357,8 @@ entries_retry_helper(const or_options_t *options, int act)
   SMARTLIST_FOREACH_BEGIN(entry_guards, entry_guard_t *, e) {
       node = node_get_by_id(e->identity);
       if (node && node_has_descriptor(node) &&
-          node_is_bridge(node) == need_bridges) {
+          node_is_bridge(node) == need_bridges &&
+          (!need_bridges || node_is_a_configured_bridge(node))) {
         any_known = 1;
         if (node->is_running)
           any_running = 1; /* some entry is both known and running */

I say overly simple because I think there's still an edge case where you could have a bridge with the same descriptor that offers both obfs3 and meek, and specify the fingerprint in your torrc file, and node_is_a_configured_bridge() will ignore addr:port and say "yes it's a configured bridge" if the digest matches. But I'm tempted to say that's a separate bug in node_is_a_configured_bridge().

comment:2 Changed 5 years ago by nickm

Status: newneeds_review

So you're saying that if I merge this patch, stuff gets better, but not completely better? Or you're saying that stuff might break that works now?

comment:3 Changed 5 years ago by arma

If you merge this patch, stuff gets better, but not completely better. People who switch between multiple PTs, and who try to use PTs that shoehorn their network design into our bridge design (currently meek and flashproxy, with their "I'll just say there's a bridge on 127.0.0.1:9060 and never you mind that there's a whole internet behind that one bridge"), will become much happier when they're on not-perfectly-stable network connections.

People who are in the above situation, who have two bridges in their state file which offer different PTs but which use the same bridge identity fingerprint, will not see an increase in happiness from this patch. That's because node_is_a_configured_bridge(node) says "yes" if node's identity fingerprint is a configured bridge, even if the PT offered by node isn't the one that's configured.

comment:4 Changed 4 years ago by nickm

Hm. I can take this in 0.2.6, but I'm not 100% comfortable writing a changes file for it.

comment:5 in reply to:  3 ; Changed 4 years ago by sysrqb

Replying to arma:

If you merge this patch, stuff gets better, but not completely better. People who switch between multiple PTs, and who try to use PTs that shoehorn their network design into our bridge design (currently meek and flashproxy, with their "I'll just say there's a bridge on 127.0.0.1:9060 and never you mind that there's a whole internet behind that one bridge"), will become much happier when they're on not-perfectly-stable network connections.

I think this is a good enough fix for 0.2.6. I started writing a more comprehensive one, but I think this will do fine for now. I also wonder if we should also check !e->bad_since

-          node_is_bridge(node) == need_bridges) {
+          node_is_bridge(node) == need_bridges &&
+          (!need_bridges || (!e->bad_since &&
                              node_is_a_configured_bridge(node)))) {

As far as I can tell, we only set bad_since on a bridge's entry_guard_t when we used it and then it was remove from the configuration, so it's marked as bad/unlisted.

People who are in the above situation, who have two bridges in their state file which offer different PTs but which use the same bridge identity fingerprint, will not see an increase in happiness from this patch. That's because node_is_a_configured_bridge(node) says "yes" if node's identity fingerprint is a configured bridge, even if the PT offered by node isn't the one that's configured.

Created #14581 for this.

comment:6 in reply to:  5 Changed 4 years ago by sysrqb

Replying to sysrqb:

As far as I can tell, we only set bad_since on a bridge's entry_guard_t when we used it and then it was remove from the configuration, so it's marked as bad/unlisted.

We also set it on bridges for which we haven't received a descriptor, but I think that's ok because we wouldn't want to use it anyway. Also, maybe we set it if the bridge falls into the ExcludeNodes.

So, i have two branches, one which uses arma's patch and another which also checks !e->bad_since. I'm not sure if the latter is more useful.

arma, nickm, please modify the patch and changelog until your happy with them.

bug14216
bug14216_bad_since

https://git.torproject.org/user/sysrqb/tor.git

comment:7 Changed 4 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.???

Merged the second one; thanks!

Marking this for a more comprehensive fix in a later tor.

comment:8 Changed 4 years ago by isis

Cc: isis added

comment:9 Changed 3 years ago by arma

Severity: Normal

There is no longer anything that needs-review in this ticket, right?

comment:10 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:11 Changed 3 years ago by nickm

Status: needs_reviewnew

comment:12 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:13 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:14 Changed 2 years ago by nickm

Keywords: 025-backport removed

These are not ripe for 0.2.5, even if they do get fixed now

comment:15 Changed 2 years ago by nickm

Resolution: fixed
Status: newclosed

This code was rewritten with prop271, and a partial fix got merged in 026 anyway.

Note: See TracTickets for help on using tickets.