Change UseBridges to prevent any access attempts of public tor network

changed milestone to %Tor: 0.2.2.x-final

added component::core tor/tor milestone::Tor: 0.2.2.x-final priority::high resolution::fixed status::closed type::defect labels

It'd be nice to have a fix for this, though since it's pretty specialized and there is (IIUC?) a working workaround, I'm not going to call it urgent.

Trac:
Priority: normal to minor
Milestone: N/A to Tor: unspecified

Replying to nickm:

It'd be nice to have a fix for this, though since it's pretty specialized and there is (IIUC?) a working workaround, I'm not going to call it urgent.

The workaround that was initially devised (and that I described to you over IRC) was the following:

Add "ReachableAddresses reject :" to torrc
Start Tor
Let the user add bridges through vidalia

As detailed in bug #2357 (moved), this approach started out promising (bridges seems to be exempt of rejection from ReachableAddresses so It Just Works(TM) as soon as a bridge is added), but it breaks when Tor is restarted.

The only alternative workaround I can think of is to add "UseBridges 1" and a bogus bridge (e.g. "Bridge 127.0.0.1:12345") to torrc which will prevent the Tor client from connecting to the Tor network until the user adds a proper bridge through vidalia. This seems to work fine, but users might get confused when they see that strange bridge turn up in vidalia.

Trac:
Points: N/A to N/A

This seems like an important feature that is worth adding.

Another approach I'd heard pondered was that Vidalia would give you a popup on your first start, asking if you were planning to put some bridges in or if it should just proceed to try the public Tor network.

If we go with that design, I guess Vidalia could start Tor with this 'usebridges 1' option while it waited for the user to choose one. Or Vidalia could simply not start Tor until it knows how it wants it to be configured.

Would that be a helpful feature for tails as well, or is it orthogonal?

Trac:
Actualpointsdone: N/A to N/A
Pointsdone: N/A to N/A
Actualpoints: N/A to N/A

Replying to arma:

This seems like an important feature that is worth adding.

Agreed.

Another approach I'd heard pondered was that Vidalia would give you a popup on your first start, asking if you were planning to put some bridges in or if it should just proceed to try the public Tor network.

Yeah, I wrote a patch for this: a pop-up (that is T(A)ILS specific, but it can easily be made generic of course) is shown if UseBridges is set, but no bridge is configured, and Vidalia is started with a new "-bridgeconf" parameter. If this enhancement to UseBridges is implemented I could send my Vidalia patch upstream with some modifications to make it more generic.

If we go with that design, I guess Vidalia could start Tor with this 'usebridges 1' option while it waited for the user to choose one. Or Vidalia could simply not start Tor until it knows how it wants it to be configured.

Would that be a helpful feature for tails as well, or is it orthogonal?

We want something like this, yes. Exactly what we want, I guess, is this:

UseBridges is a tristate with possible values 0, 1 and auto, defaulting to auto: 1 and 0 behaves as before, auto behaves as 1 iff any bridges are configured. Furthermore, it's possible to start Tor with UseBridges 1 and no bridges configured (old behavious is to reject the config) -- in this situation Tor simply idles as it waits for a bridge to be configured through the control port.

I have played around with the Tor sources, but it was the first time I really looked into them, so it's very possible I have overlooked some things. I have got the above to work (I think, see attached patch), but there are some FIXME's that need further work. In addition, the patch makes two other bridge related things:

Tor only chooses a bridge as entry if it is configured. So if you add a bridge to Tor, get its descriptors etc. so it's ready for use, and then remove it and add another bridge, new circuits will be build using only the new bridge. At least it appears to me that Tor without the patch continues to use the removed bridge.
Whenever a bridge is added/removeded, all circuits are torn down (just like with ExcludeNodes and similar options). It's a bit crude as if you have a bridge A configured, and then add a bridge B (so you now have A and B configured), all circuits using A will be torn down, which is a bit unnecessary.

As I said, it my first look into the Tor sources, so I might have done this in suboptimal way. And as I said there are some parts (marked with FIXME's) that I know something probably must be done about. Input on those issues are very welcome.

Oh, and one thing I really want, but haven't figured out how to do properly, is to make Tor faster in bootstrapping with added bridges. I want Tor to immediately and explicitly try to fetch the bridge descriptors when a bridge is added instead of having that happen during the scheduled dir updates (which can take some time). Any input on how to do that properly would be greatly appreciated.

New patch.

Now bridge descriptors are (re)fetched as soon as the list of bridges are changed. Additionally, since there are constraints on how you can combine UseBridges with some other options, the following seemed like reasonable to me:

TunnelDirConns set to 0 only works with UseBridges set to 0
If we run a server, we can't have UseBridges set to 1, as before. With UseBridges set to auto (which is the default value), bridges will not be used, just like if we had UseBridges set to 0.

There are a couple of issues with my patch:

There are two (related) "FIXME" items detailing one issue -- see the patch.
When bridge switching (i.e. repeatedly changing bridges by removing the old ones and adding new ones) Vidalia starts to list one " New" entry in the connection list for every abandoned bridge, or something like that. This seems to be more of an issue in Vidalia, though. I've check with netstat, though, and it seems like one connections per old bridge stays for some time before they die (perhaps the one used for directory fetches?), but I don't know how to kill those connections.
And here's the big one: when doing frequent bridge switching it can occasionally happen that bootstrapping to a known working bridge fails. It will eventually be fetched by the scheduled fetches, but then we won't fetch its descriptor before download_status_is_ready() thinks it's ok, which seems to be 10+ minutes.

I've managed to reproduce it a couple of times, but I can see no pattern. So, given this seemingly random nature of the problem, my only guess is that there's some sort of race condition or other type of collision affecting the call I added to fetch_bridge_descriptors() in options_act(), config.c:1457. I have observed two things which somewhat convince me of this:

3.1) This seems to happen more often when I have loglevel notice than info. Hence the (admittedly very small) times required to output stuff seems to affect this.

3.2) If I put that call earlier in options_act() (I've tried several places) it fails every time.

Instead of calling fetch_bridge_descriptors(), another approach would be to do a download_status_reset() on each bridge and then let run_scheduled_events() (main.c:856) call fetch_bridge_descriptors() for us, but that isn't as instantaneous (up to 10 seconds delay).

Any ideas?

Trac:
Status: new to needs_review

I had a bit of time to look at this once more and I found a minor issue that I fixed, so new patch.

The bug was: if Tor starts with a few bridges configured in torrc, these are ignored until the options are updated (i.e. they we're only added if there was an old_config).

Replying to anonym:

When bridge switching (i.e. repeatedly changing bridges by removing the old ones and adding new ones) Vidalia starts to list one " New" entry in the connection list for every abandoned bridge, or something like that.

I've done some debugging and it's quite clear that these spurious entries are due to connections started with launch_direct_bridge_descriptor_fetch() -- when the decriptor has been fetched, I believe the connection is closed by Tor, but Vidalia never learns about that happening, so these entries remain. This also happens when adding a bridge in vanilla Tor, so it's not introduced by my patch. The issue does, however, become more apparent with my patch since more direct bridge fetches are done.

And here's the big one: when doing frequent bridge switching it can occasionally happen that bootstrapping to a known working bridge fails.

Just to be clear, if this problem occurs we can just remove the bridge through Vidalia and then re-add it and then its descriptor will be fetched (I guess the probability that it will fail is about the same as in any other situation, but I've never got two of these bridge fetch stalls in a row). Similarly, if we remove the failed bridge and add another known working bridge, the descriptor will be fetched (again, presumably with the same fail probability).

Trac:
Cc: anonym to anonym, amnesia@boum.org

Reading through the patch...

This looks weird:

-  if (options->UseBridges && !options->Bridges)
-    REJECT("If you set UseBridges, you must specify at least one bridge.");
-  if (options->UseBridges && !options->TunnelDirConns)
-    REJECT("If you set UseBridges, you must set TunnelDirConns.");
+  if (!strcmp(options->UseBridges, "0") && !options->TunnelDirConns)
+    REJECT("TunnelDirConns set to 0 only works with UseBridges set to 0.");

In the new check, we give the warning if UseBridges is 0. But previously, we gave it if it was nonzero. Maybe a strcmp mistake?

I'm not sure about launching the bridge descriptor fetch from config.c ; the more stuff we launch from configuration parsing, the more spaghettiish our code has gotten historically. We already retry launching downloads every 10 or 60 seconds from main.c; perhaps what we need is a function to tell main that it needs to retry downloads right away.

The any_bridge_descriptors_known() logic is fragile in depending on have_bridge_descriptors anc choose_random_entry; instead IMO it should probably just iterate through bridge_list and see whether we have a descriptor for any member.

Trac:
Component: Tor Client to Tor Bridge
Description: In T(A)ILS (https://amnesia.boum.org) we want to enable a bridge-only mode (chosen at the boot menu, or at least before Tor starts) which ensures that the Tor network is never directly connected to. We figure some people don't want to disclose that they are using Tor for various reasons.

Hence we'd like to have an option that can be set in torrc that makes Tor only use bridges, but without it being neccessary to specify a bridge in torrc -- the user should be able to do that through Vidalia at a later point, and then have Tor bootstrap as soon as a bridge has been set through the control port.

Here follows the discussion on #tor-dev which suggests that a change of the meaning of UseBridges might be the way:

(17:52:11) nickm: It seems like you may also want a "I am using bridges, even though I haven't configured any bridges yet" option (17:52:36) nickm: That seems much closer to what you are trying to achieve than "ReachableAddresses reject :" (17:53:14) nickm: You could even fake it, I bet, with something like Bridge 127.0.0.1:x, where x is an unused port. (17:53:17) anonym: yes, exactly (17:53:31) nickm: that's not a great solution, of course (17:56:30) anonym: a proper "EnforceBridges" or whatever would be best, yes. is that likely to get implemented if I file a feature request? (17:56:43) nickm: EnforceBridges is not really what you mean (17:56:57) nickm: Because Bridge settings are and should be enforced, always (17:57:10) nickm: You want "EnforceTheBridgesIHaventEvenToldYouAboutYet" (17:57:13) nickm: or something (17:57:18) anonym: hence my "or whatever" (17:58:10) nickm: hang on. (17:58:13) nickm: what about UseBridges 1 (17:58:32) nickm: ah. (17:58:53) nickm: if usebridges 1 is set, and you list no bridges, we reject the torrc (17:59:20) Sebastian: The value of the UseBridges config option is kind of debatable (17:59:44) nickm: Sebastian: you mean, if they specify a bridge, UseBridges should automatically turn on? (17:59:56) Sebastian: yes (17:59:56) nickm: or something else? (18:00:15) nickm: if we agreed on that, then this sounds like a fine value for a tristate, with "auto" being the default. (18:00:40) nickm: I don't know if our existing code does the right thing with UseBridges set but Bridges empty; changing this shouldn't be too hard though (18:00:42) anonym: and 1 being what was intended with "EnforceTheBridgesIHaventEvenToldYouAboutYet" (18:00:42) anonym: ? (18:00:52) nickm: anonym: hypothetically yes

to

In T(A)ILS (https://amnesia.boum.org) we want to enable a bridge-only mode (chosen at the boot menu, or at least before Tor starts) which ensures that the Tor network is never directly connected to. We figure some people don't want to disclose that they are using Tor for various reasons.

Hence we'd like to have an option that can be set in torrc that makes Tor only use bridges, but without it being necessary to specify a bridge in torrc -- the user should be able to do that through Vidalia at a later point, and then have Tor bootstrap as soon as a bridge has been set through the control port.

Here follows the discussion on #tor-dev which suggests that a change of the meaning of UseBridges might be the way:

(17:52:11) nickm: It seems like you may also want a "I am using bridges, even though I haven't configured any bridges yet" option (17:52:36) nickm: That seems much closer to what you are trying to achieve than "ReachableAddresses reject :" (17:53:14) nickm: You could even fake it, I bet, with something like Bridge 127.0.0.1:x, where x is an unused port. (17:53:17) anonym: yes, exactly (17:53:31) nickm: that's not a great solution, of course (17:56:30) anonym: a proper "EnforceBridges" or whatever would be best, yes. is that likely to get implemented if I file a feature request? (17:56:43) nickm: EnforceBridges is not really what you mean (17:56:57) nickm: Because Bridge settings are and should be enforced, always (17:57:10) nickm: You want "EnforceTheBridgesIHaventEvenToldYouAboutYet" (17:57:13) nickm: or something (17:57:18) anonym: hence my "or whatever" (17:58:10) nickm: hang on. (17:58:13) nickm: what about UseBridges 1 (17:58:32) nickm: ah. (17:58:53) nickm: if usebridges 1 is set, and you list no bridges, we reject the torrc (17:59:20) Sebastian: The value of the UseBridges config option is kind of debatable (17:59:44) nickm: Sebastian: you mean, if they specify a bridge, UseBridges should automatically turn on? (17:59:56) Sebastian: yes (17:59:56) nickm: or something else? (18:00:15) nickm: if we agreed on that, then this sounds like a fine value for a tristate, with "auto" being the default. (18:00:40) nickm: I don't know if our existing code does the right thing with UseBridges set but Bridges empty; changing this shouldn't be too hard though (18:00:42) anonym: and 1 being what was intended with "EnforceTheBridgesIHaventEvenToldYouAboutYet" (18:00:42) anonym: ? (18:00:52) nickm: anonym: hypothetically yes

Replying to nickm:

In the new check, we give the warning if UseBridges is 0. But previously, we gave it if it was nonzero. Maybe a strcmp mistake?

Err, yeah I screwed up. Will fix it in next patch.

I'm not sure about launching the bridge descriptor fetch from config.c ; the more stuff we launch from configuration parsing, the more spaghettiish our code has gotten historically. We already retry launching downloads every 10 or 60 seconds from main.c; perhaps what we need is a function to tell main that it needs to retry downloads right away.

Do you think this any of these approaches are viable?

Make run_scheduled_events()'s static variable time_to_try_getting_descriptors global, and add a function schedule_imminent_descriptor_refetch() which simply zeros time_to_try_getting_descriptors and hence a bridge descriptor refetch is done within a second or so. schedule_imminent_descriptor_refetch() is then called instead of fetch_bridge_descriptors() in config.c.
Alternatively (different style but equivalent behaviour), if we don't want to make time_to_try_getting_descriptors global (which might be undesirable as it makes it stand out from all the other time_to_* variables), what about adding a global variable forced_descriptor_refetch and change the if-statment in main.c:915 to

  if (time_to_try_getting_descriptors < now ||
      forced_descriptor_refetch) {
    forced_descriptor_refetch = 0;
    /* ... the rest is unchanged ... */
  }

schedule_imminent_descriptor_refetch() would then simply set forced_descriptor_refetch to 1.

The any_bridge_descriptors_known() logic is fragile in depending on have_bridge_descriptors anc choose_random_entry; instead IMO it should probably just iterate through bridge_list and see whether we have a descriptor for any member.

Say I somehow implement the function "routerinfo_t * get_router_by_bridgeinfo()" (or have I missed any other practical function which will do the trick?), which is the opposite of get_configured_bridge_by_routerinfo(), and then change any_bridge_descriptors_known() to the below, would that be ok?

int
any_bridge_descriptors_known(void)
{
  tor_assert(get_options()->EffectiveUseBridges);

  SMARTLIST_FOREACH_BEGIN(bridge_list, bridge_info_t *, bridge)
    {
      if (get_router_by_bridgeinfo())
        return 1;
    }
  SMARTLIST_FOREACH_END(bridge);

  return 0;
}

Trac:
tor-strict_bridges.patch

Makes UseBridges a tristate, Tor can start in an idle state awaiting bridges, and bridge settings are followed more strictly.

New patch.

Replying to anonym:

Replying to nickm:

In the new check, we give the warning if UseBridges? is 0. But previously, we gave it if it was nonzero. Maybe a strcmp mistake?

Fixed.

I'm not sure about launching the bridge descriptor fetch from config.c ; the more stuff we launch from configuration parsing, the more spaghettiish our code has gotten historically. We already retry launching downloads every 10 or 60 seconds from main.c; perhaps what we need is a function to tell main that it needs to retry downloads right away.

Do you think this any of these approaches are viable?

I chose approach 2 from above.

The any_bridge_descriptors_known() logic is fragile in depending on have_bridge_descriptors anc choose_random_entry; instead IMO it should probably just iterate through bridge_list and see whether we have a descriptor for any member.

I instead iterate over entry_guards (since any bridge who's got a descriptor will be put there) as it seemed the easiest way to implement it. Time complexity for any_bridge_descriptors_known() is O(E*B), where E is the number of bridges and B the number of configured bridges, so really it's O(E^2), but that should be ok since E shouldn't ever grow very big, right?

BTW, are previously used bridges ever purged from your entry guards pool? If not, it could be a good idea, I guess.

I've done some pretty frequent bridge switching with ~40 bridges, and I didn't encounter the problem I mentioned before with me previous patch.

In my patch I made it so that Tor will abandon all previous circuits when the bridge settings are changed. This makes sense since Tor then will start/stop using bridges quickly. This is done just like it is done for changed settings for EntryGuards, Exclude*Nodes, etc, i.e.:

    circuit_mark_all_unused_circs();
    circuit_expire_all_dirty_circs();

However, using the above, circuits that handle streams are not closed until their streams are closed, which is especially outstanding with long-liveed streams like SSH/IRC. At least these left-over circuits will not be used for any new streams, but they show up in Tor controllers such as Vidalia and arm, and will most likely confuse users, and I believe it is important to avoid that.

What is the right thing to do here? I guess it all boils down to what the user expects. Does the user expect long-lived connections to continue using old circuits that the user has instructed should not be used any more (and will the general user understand the concepts relating to this)? I personally do not expect that, and would like a more ruthless circuit killer. Maybe something like this (in circlist.c):

/* Closes all circuits. */
void
circuit_kill_all_circs(void)
{
  circuit_t *circ;
  
 for (circ=global_circuitlist; circ; circ = circ->next) {
   if (CIRCUIT_IS_ORIGIN(circ) && !circ->marked_for_close)
     circuit_mark_for_close(circ, END_CIRC_REASON_FINISHED);
 }
}

The question is what general users expect, though. Also, since this new behaviour would kill all streams, any long-lived sessions would be lost. Are users expecting that? Given the context, i.e. changing bridge/EntryGuard/Exclude*Nodes/etc, perhaps it is expectable that Tor will do drastic things?

I think that breaking existing connections is bad enough that we shouldn't do it. If there's a UI issue here, we should look into ways to better communicate the idea that "this is an expired circuit that's getting held around only so long as it takes for the streams using it to close."

Trac:
Milestone: Tor: unspecified to Tor: 0.2.3.x-final

Looking at the patch now.

It sure would be nicer if it were a git branch (or at least multiple patches, one for each topic).

For example, the "only use configured bridges" thing should already be in, from #2511 (moved).

I've split the patch (will attach to this bug asap) and made sure the new patches apply nicely to a recent Tor git. Once I've figured out how, I will make a clone of Tor's master on the Tails git server (http://git.immerda.ch/) with appropriate branching etc for this.

Trac:
tor-01-tristate_usebridges.patch

Parsing for new AUTOBOOL behaviour of UseBridges.

Trac:
tor-02-strict_usebridges.patch

When bridges should be used, allow only configured bridges as entries.

Trac:
tor-03-abandon_old_bridges.patch

Abandon current circuits when changing bridge settings.

Trac:
tor-04-quick_circ_rebuild_with_bridges.patch

Rebuild circuits quickly if they have been abandoned (by changing bridge settings, for instance).

Trac:
tor-05-early_descriptor_refetch_with_bridges.patch

Schedule descriptor refetches immediately after changing bridge settings.

Replying to anonym:

Schedule descriptor refetches immediately after changing bridge settings.

See #3198 (moved) which I think does what you want here.

Replying to anonym:

Abandon current circuits when changing bridge settings.

See #3200 (moved) for a fix.

Replying to anonym:

Rebuild circuits quickly if they have been abandoned (by changing bridge settings, for instance).

What's the actual bug here, how do I trigger it, and what is the behavior you'd expect?

Trac:
Summary: change the meaning of UseBridges to Change UseBridges to prevent any access attempts of public tor network

I think this is very important. Our poor configuration options and the lack of the ability to produce "Bridge Use Only" bundles is endangering people's lives in Syria, Iran, and elsewhere.

It is very unlikely that new Tor users will configure things safely in these situations without prior training... They will just run our default TBBs, hit the firewall, and risk getting arrested for the attempt (especially if they appear to succeed later).

I'm not sure this should wait until 0.2.3.x. It probably shouldn't block 0.2.2.x, but we should definitely backport this if at all possible, given the severity of the situation.

Trac:
Type: enhancement to defect
Priority: minor to major
Component: Tor Bridge to Tor Client

Trac:
Parent: N/A to #3307 (closed)

Trac:
Parent: #3307 (closed) to N/A
Milestone: Tor: 0.2.3.x-final to Tor: 0.2.2.x-final

Hm. This will take some tweaking to be 0.2.2.x-suitable. The 01 patch depends on an 0.2.3.x feature. I can try to do a port unless somebody else is enthusiastic to do it.

04 and 05 are a little confusing to me. anonym or somebody, could you walk me through how they work and why? 05 at the least needs better docs. "Earlier than suggested" is just plain not enough information to let anybody know what the function and variable do.

Also, it'll need a manpage change and a changes entry.

Replying to nickm:

04 and 05 are a little confusing to me. anonym or somebody, could you walk me through how they work and why? 05 at the least needs better docs. "Earlier than suggested" is just plain not enough information to let anybody know what the function and variable do.

Patch 04: Hmm. After some more thought I retract this patch. Please ignore it.

Patch 05: You can ignore this one as #3198 (moved) fixes the issue.

Trac:
0001-Update-man-page-for-new-UseBridges-tristate.patch

Update man page for new UseBridges tristate behaviour.

Replying to anonym:

When bridges should be used, allow only configured bridges as entries.

I thought we did this already -- see e.g. #2511 (moved). Can you describe the remaining bug, symptoms you see, how to reproduce, etc?

Replying to nickm:

Hm. This will take some tweaking to be 0.2.2.x-suitable. The 01 patch depends on an 0.2.3.x feature. I can try to do a port unless somebody else is enthusiastic to do it.

I don't object to something like patch 01 going into 0.2.2 if the patch isn't too complex. I believe nobody is working on it other than anonym with his initial patch and now nickm.

If it's complex, I think it can wait for 0.2.3.x, where it looks easier.

04 and 05 are a little confusing to me. anonym or somebody, could you walk me through how they work and why? 05 at the least needs better docs. "Earlier than suggested" is just plain not enough information to let anybody know what the function and variable do.

I believe patches 02 through 05 are moot now.

Replying to mikeperry:

I think this is very important. Our poor configuration options and the lack of the ability to produce "Bridge Use Only" bundles is endangering people's lives in Syria, Iran, and elsewhere.

It is very unlikely that new Tor users will configure things safely in these situations without prior training... They will just run our default TBBs, hit the firewall, and risk getting arrested for the attempt (especially if they appear to succeed later).

Mike, can you explain why this is best solved inside Tor?

I could imagine a Vidalia patch too, where it chooses not to Start Tor until you've answered its popup about bridges.

Said another way, if only Tor changes, how are you planning to make the user-facing bundles do what you want? And if Vidalia changes, why not change it to do everything you want?

I did a backport and light tidy in branch "bug2355" in my public repo. anonym should have a look to make sure it's sane.

Replying to nickm:

I did a backport and light tidy in branch "bug2355" in my public repo. anonym should have a look to make sure it's sane.

Looks fine overall.

Major points:

6c528a1b7415e should not be in there. A different version of it is already in maint-0.2.2.
f602eab961b2 seems to have snuck in too. I don't yet believe that it does anything good.

Minor points:

In the or.h patch, it says

1 means strictly yes, 0 means stricly no,

I think that should be "1" and "0" rather than bare values. Also, stricly -> strictly.

UseBriges -> UseBridges
The changes entry should say that the default changed too.
For our new CONFIG_TYPE_PORT we say strcasecmp when checking if "auto", but here we say strcmp?

Replying to arma:

Replying to nickm:

I did a backport and light tidy in branch "bug2355" in my public repo. anonym should have a look to make sure it's sane.

Looks fine overall.

Major points:

6c528a1b7415e should not be in there. A different version of it is already in maint-0.2.2.

f602eab961b2 seems to have snuck in too. I don't yet believe that it does anything good.

I'd like to hear anonym's take on these.

Minor points:

In the or.h patch, it says {{{ 1 means strictly yes, 0 means stricly no, }}} I think that should be "1" and "0" rather than bare values. Also, stricly -> strictly.

UseBriges -> UseBridges

The changes entry should say that the default changed too.

Will fix.

For our new CONFIG_TYPE_PORT we say strcasecmp when checking if "auto", but here we say strcmp?

Hm. I think both should be strcmp.

Replying to nickm:

Will fix. Those now have fixup commits in my branch.

Replying to arma:

Replying to anonym:

When bridges should be used, allow only configured bridges as entries.

I thought we did this already -- see e.g. #2511 (moved). Can you describe the remaining bug, symptoms you see, how to reproduce, etc? I had a brief look on your bug2511 branch but I have never looked into that part of the Tor sources (the precise logic of what routerlist effects) so I cannot say anything with reassurance without a deeper look (which I am afraid I won't have time for today). But if your fix guarantees that at any point when Tor must use only bridges (i.e. UseBridges set to 1, or [UseBridges set to auto and some bridges are configured]), only configured bridges will be picked as entries, then it should be ok -- after all, that's the only thing my patch does, although more explicitly.

Replying to nickm:

Replying to arma:

Major points:

6c528a1b7415e should not be in there. A different version of it is already in maint-0.2.2. Yes, in commit 073fed06c458f.

f602eab961b2 seems to have snuck in too. I don't yet believe that it does anything good. If you think your fix for #2511 (moved) already fixes this I trust you.

Replying to anonym:

Replying to arma:

Replying to anonym:

When bridges should be used, allow only configured bridges as entries.

I thought we did this already -- see e.g. #2511 (moved). Can you describe the remaining bug, symptoms you see, how to reproduce, etc? I had a brief look on your bug2511 branch but I have never looked into that part of the Tor sources (the precise logic of what routerlist effects) so I cannot say anything with reassurance without a deeper look (which I am afraid I won't have time for today). But if your fix guarantees that at any point when Tor must use only bridges (i.e. UseBridges set to 1, or [UseBridges set to auto and some bridges are configured]), only configured bridges will be picked as entries, then it should be ok -- after all, that's the only thing my patch does, although more explicitly.

I now believe there is a bug remaining. I just opened #3321 (moved) with more details.

Replying to anonym:

Replying to arma:

f602eab961b2 seems to have snuck in too. I don't yet believe that it does anything good.

That patch actually had two effects. First was to do something like what we should do for #3321 (moved). Second is to make it so none of your bridges are ever marked down when they become unreachable.

Do you still like the second behavior? It seems like a poor idea. Better would be to figure out why we're marking a bridge down when it isn't actually down.

Replying to arma:

Replying to anonym:

Replying to arma:

f602eab961b2 seems to have snuck in too. I don't yet believe that it does anything good.

That patch actually had two effects. First was to do something like what we should do for #3321 (moved). Second is to make it so none of your bridges are ever marked down when they become unreachable.

Do you still like the second behavior? I would have to do some testing and look deeper in the code* to say anything with reassurance, but I think the assmume_reachable=1 stuff in my patch was necessary when switching from a bridge X to a distinct bridge Y. Without assuming reachability for Y it would take ages before its descriptor was fetched and Y is usable. Surely there's a better solution? Or perhaps it's already fixed with #3198 (moved)?

*I'm very sorry, but I'm currently drowning in university work + writing a Tails related grant proposal, so I will not be available for anything more than the occasional comment drawing from what I remember on top of my head for the coming week. I should be back at around the 7th of June if any input is still wanted at that point.

It seems like a poor idea. Better would be to figure out why we're marking a bridge down when it isn't actually down. I agree, that sounds better.

Replying to anonym:

Do you still like the second behavior? I would have to do some testing and look deeper in the code* to say anything with reassurance, but I think the assmume_reachable=1 stuff in my patch was necessary when switching from a bridge X to a distinct bridge Y. Without assuming reachability for Y it would take ages before its descriptor was fetched and Y is usable. Surely there's a better solution? Or perhaps it's already fixed with #3198 (moved)?

I think #3198 (moved) should resolve that issue for you.

So in summary, I believe Nick's patch, minus those two patches from anonym, is now a fine thing to merge.