Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#2511 closed defect (fixed)

Tor will use an unconfigured bridge if it was a configured bridge last time you ran Tor

Reported by: arma Owned by:
Priority: High Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-bridge
Cc: twilde@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by arma)

If you configure your Tor client with

usebridges 1
bridge 128.31.0.34:9009

and you run it and it works, then Tor will end up writing two things to disk: 1) a @purpose bridge descriptor for 128.31.0.34 in your cached-descriptors file:

@downloaded-at 2011-02-08 07:54:52
@source "128.31.0.34"
@purpose bridge
router bridge 128.31.0.34 9009 0 0
...

and 2) an entry guard stanza in your state file:

EntryGuard bridge 4C17FB532E20B2A8AC199441ECD2B0177B39E4B1
EntryGuardAddedBy 4C17FB532E20B2A8AC199441ECD2B0177B39E4B1 0.2.3.0-alpha-dev 2011-02-01 18:43:23

Then if you kill your Tor and run it with

usebridges 1
bridge 150.150.150.150:9009

it will successfully bootstrap -- using the bridge that worked before but isn't your requested bridge.

Child Tickets

Change History (17)

comment:1 Changed 9 years ago by arma

This happens because we originally designed bridges to be robust ("work if you can no matter what"), rather than be precise ("use this bridge or don't work").

The exact chain of events here is that we read in the bridge descriptor, read in our entry guards, and call entry_guard_set_status(). That sets all but the one of our entry guards to Down:

  else if (options->UseBridges && (!node->ri ||
                                   node->ri->purpose != ROUTER_PURPOSE_BRIDGE))
    *reason = "not a bridge";

but that one gets set as Up.

Then various places like update_router_have_minimum_dir_info() call should_delay_dir_fetches(), which asks

  if (options->UseBridges && !any_bridge_descriptors_known()) {

But that function just checks

any_bridge_descriptors_known(void)
{
  tor_assert(get_options()->UseBridges);
  return choose_random_entry(NULL)!=NULL ? 1 : 0;
}

and sure enough, there is an "up" entry guard.

So we do a directory fetch to get the consensus and/or more descriptors from it that we don't have. In directory_get_from_dirserver() the directory mirror we'd chosen gets overwritten with our bridge:

    if (options->UseBridges && type != BRIDGE_AUTHORITY) {
      /* want to ask a running bridge for which we have a descriptor. */

which is why it looks to all intents and purposes like that bridge is our configured bridge.

comment:2 Changed 9 years ago by arma

Option 1 to fix it is to change entry_guard_set_status() so it sets you as Down if you're not a configured bridge, even if we have a descriptor and it's purpose bridge. But that fix is going to produce peripheral bugs, for example where we fetch the new descriptor but then discard it (see #2510 for an example scenario where that can happen in practice).

Option 2 to fix it is to drop the descriptor when we read it, if it's a bridge descriptor that isn't a currently configured bridge. Then that entry guard will get marked down (reason "unlisted"), and it won't get used until we get a fresh descriptor (which presumably we'd only get by receiving it in response to a /tor/server/authority request to one of our configured bridges).

Option 3 is to drop bridge descriptors for unconfigured bridges and also nuke their stanza from the entry guard list. That would force them to get readded to the entry guard list (once we get a fresh descriptor) if we want to, and would reduce other weird side-effect bugs.

Currently I think option 2 should be sufficient.

comment:3 Changed 9 years ago by arma

Status: newneeds_review

bug2511 in my arma fixes this.

comment:4 Changed 9 years ago by arma

Description: modified (diff)

comment:5 Changed 9 years ago by nickm

Will this break bridge authorities?

I personally don't like the approach of mucking up router_add_to_routerlist even more: it's too hairy by half as is, and we should be looking for ways to make it dumber, not smarter. Option 3 (or a variant option 1 where instead of calling the bridge Down we call it "Invalid" or something) seems much better.

comment:6 Changed 9 years ago by nickm

Also, we shouldn't say "ROUTER_WAS_NOT_NEW" unless we mean it.

comment:7 in reply to:  5 ; Changed 9 years ago by arma

Replying to nickm:

Will this break bridge authorities?

Yes! Good catch. We'll want to either check if UseBridges, or if authdir_mode_bridge(). I don't have an opinion on which one we should check.

I personally don't like the approach of mucking up router_add_to_routerlist even more: it's too hairy by half as is, and we should be looking for ways to make it dumber, not smarter. Option 3 (or a variant option 1 where instead of calling the bridge Down we call it "Invalid" or something) seems much better.

Hm. Well, Option 3 was "do this patch and also add another patch elsewhere", so I don't see how that will simplify router_add_to_routerlist(). The real issue is that we have a growing set of reasons why we wouldn't want to keep a router desc that we just learned about. One way to simplify is to stop having so many reasons, but I don't see a good way around that. What are some other ways to simplify? We could break the checks out into several functions based on topic. Is there another place in the code that we should be doing these checks instead? Hm.

Specifically, if we go with option 1 and we want to handle bug2510, we're going to need some messier special-casing either in router_add_to_routerlist() or elsewhere that says "if we have a bridge descriptor and we just got a descriptor that's the same as it but actually we got it from this other address then we want to drop the old one so we don't drop the new one before we can rewrite it". Maybe that means we should be thinking about bug 2510 in parallel to this one, not after.

comment:8 in reply to:  7 Changed 9 years ago by nickm

Replying to arma:

Replying to nickm:

I personally don't like the approach of mucking up router_add_to_routerlist even more: it's too hairy by half as is, and we should be looking for ways to make it dumber, not smarter. Option 3 (or a variant option 1 where instead of calling the bridge Down we call it "Invalid" or something) seems much better.

Hm. Well, Option 3 was "do this patch and also add another patch elsewhere", so I don't see how that will simplify router_add_to_routerlist(). The real issue is that we have a growing set of reasons why we wouldn't want to keep a router desc that we just learned about.

IMO the *real* real issue is that we're assuming that "router descriptors we would use" and "router descriptors we know about" need to stay the same here, so that when we discover that a descriptor is not one we should use, we try not to know about it at all. That's why a function that should have been simple "Here is a new descriptor; learn about it!" has grown so much policy over the years.

Is there really no way to say of a descriptor, "Do not use this!" Is there really no way to look at our configured bridges rather than the set of all inserted descriptors with purpose==bridge when we go to pick a first hop?

comment:9 Changed 9 years ago by twilde

Cc: twilde@… added

comment:10 Changed 9 years ago by arma

Component: Tor ClientTor Bridge

comment:11 Changed 9 years ago by arma

The simple way of saying "do not use this descriptor" is to annotate it somehow on our side, e.g. in its purpose field or by making a new is_invalid bit. But the problem there is that when we try to add it when it's already in our various descriptor_maps, we need special logic to recognize that it's there-but-annotated and it needs to become there-but-annotated-differently. This logic has proved hard to figure out in #1776.

I agree that we should make a better overall way of handling descriptors. But I don't think we want to try to force that into Tor 0.2.2.x. And I think we *do* want to get this patch into 0.2.2.x, or one of our main development directions for SponsorF will get derailed.

In that new light, is this patch adequate for 0.2.2, and we can still plan to do some grand redesign later?

I just pushed a bug2511 branch that patches your two issues above.

comment:12 Changed 9 years ago by arma

Priority: normalmajor

I take back any plans of wanting to put this, or #2510, into 0.2.1.x.

comment:13 Changed 9 years ago by nickm

It's still icky, but if we've got to, we've got to.

I'm having a quick look through things that use the was_router_added_t values to make sure it's okay to add a new one. All I can see is the WRA_WAS* functinos in routerlist.h. Do we want to add ROUTER_WAS_NOT_WANTED to one of these?

comment:14 in reply to:  13 Changed 9 years ago by arma

Replying to nickm:

It's still icky

I'd be in favor of an 0.2.3.x-or-later refactoring of how we decide which relays go in which local relay lists. I don't think it will be easy, but that's all the more reason to look at it.

I'm having a quick look through things that use the was_router_added_t values to make sure it's okay to add a new one. All I can see is the WRA_WAS* functinos in routerlist.h. Do we want to add ROUTER_WAS_NOT_WANTED to one of these?

I think we don't. It wasn't accepted, it wasn't rejected-by-the-authority, and it wasn't outdated. I guess we could call it OUTDATED if we want. Nothing in the code that we do for OUTDATED descs (all of two things) is something we want to do for this situation, but nothing we do is harmful either. Those functions seem kind of fuzzy anyway in terms of what they mean. Perhaps they should be included in the hypothetical future refactor above. :)

comment:15 Changed 9 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merging and closing. Thanks!

comment:16 Changed 7 years ago by nickm

Keywords: tor-bridge added

comment:17 Changed 7 years ago by nickm

Component: Tor BridgeTor
Note: See TracTickets for help on using tickets.