Opened 9 years ago

Closed 8 years ago

Last modified 7 years ago

#2355 closed defect (fixed)

Change UseBridges to prevent any access attempts of public tor network

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

Description (last modified by arma)

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

Child Tickets

Attachments (7)

tor-strict_bridges.patch (15.9 KB) - added by anonym 9 years ago.
Makes UseBridges a tristate, Tor can start in an idle state awaiting bridges, and bridge settings are followed more strictly.
tor-01-tristate_usebridges.patch (10.4 KB) - added by anonym 8 years ago.
Parsing for new AUTOBOOL behaviour of UseBridges.
tor-02-strict_usebridges.patch (1.2 KB) - added by anonym 8 years ago.
When bridges should be used, allow only configured bridges as entries.
tor-03-abandon_old_bridges.patch (1.8 KB) - added by anonym 8 years ago.
Abandon current circuits when changing bridge settings.
tor-04-quick_circ_rebuild_with_bridges.patch (575 bytes) - added by anonym 8 years ago.
Rebuild circuits quickly if they have been abandoned (by changing bridge settings, for instance).
tor-05-early_descriptor_refetch_with_bridges.patch (2.0 KB) - added by anonym 8 years ago.
Schedule descriptor refetches immediately after changing bridge settings.
0001-Update-man-page-for-new-UseBridges-tristate.patch (1.4 KB) - added by anonym 8 years ago.
Update man page for new UseBridges tristate behaviour.

Download all attachments as: .zip

Change History (51)

comment:1 Changed 9 years ago by nickm

Milestone: Tor: unspecified
Priority: normalminor

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.

comment:2 in reply to:  1 Changed 9 years ago by anonym

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:

1) Add "ReachableAddresses reject *:*" to torrc
2) Start Tor
3) Let the user add bridges through vidalia

As detailed in bug #2357, 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.

comment:3 Changed 9 years ago by arma

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?

comment:4 in reply to:  3 Changed 9 years ago by anonym

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:

1) 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.

2) 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.

comment:5 Changed 9 years ago by anonym

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:

1) TunnelDirConns set to 0 only works with UseBridges set to 0

2) 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:

1) There are two (related) "FIXME" items detailing one issue -- see the patch.

2) When bridge switching (i.e. repeatedly changing bridges by removing the old ones and adding new ones) Vidalia starts to list one "<Path Empty> 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.

3) 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?

comment:6 Changed 9 years ago by nickm

Status: newneeds_review

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

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:

2) When bridge switching (i.e. repeatedly changing bridges by removing the old ones and adding new ones) Vidalia starts to list one "<Path Empty> 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.

3) 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).

comment:8 Changed 9 years ago by T(A)ILS developers

Cc: amnesia@… added

comment:9 Changed 9 years ago by nickm

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.

comment:10 Changed 9 years ago by arma

Component: Tor ClientTor Bridge
Description: modified (diff)

comment:11 in reply to:  9 ; Changed 9 years ago by 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?

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?

  1. 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.
  1. 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;
}

Changed 9 years ago by anonym

Attachment: tor-strict_bridges.patch added

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

comment:12 in reply to:  11 Changed 9 years ago by anonym

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(E2), 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.

comment:13 Changed 9 years ago by anonym

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?

comment:14 Changed 9 years ago by nickm

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."

comment:15 Changed 8 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.2.3.x-final

comment:16 Changed 8 years ago by arma

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.

comment:17 Changed 8 years ago by anonym

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.

Changed 8 years ago by anonym

Parsing for new AUTOBOOL behaviour of UseBridges.

Changed 8 years ago by anonym

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

Changed 8 years ago by anonym

Abandon current circuits when changing bridge settings.

Changed 8 years ago by anonym

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

Changed 8 years ago by anonym

Schedule descriptor refetches immediately after changing bridge settings.

comment:18 in reply to:  17 Changed 8 years ago by arma

Replying to anonym:

Schedule descriptor refetches immediately after changing bridge settings.

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

comment:19 in reply to:  17 Changed 8 years ago by arma

Replying to anonym:

Abandon current circuits when changing bridge settings.

See #3200 for a fix.

comment:20 in reply to:  17 Changed 8 years ago by arma

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?

comment:21 Changed 8 years ago by mikeperry

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

comment:22 Changed 8 years ago by mikeperry

Component: Tor BridgeTor Client
Priority: minormajor
Type: enhancementdefect

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.

comment:23 Changed 8 years ago by mikeperry

Parent ID: #3307

comment:24 Changed 8 years ago by mikeperry

Milestone: Tor: 0.2.3.x-finalTor: 0.2.2.x-final
Parent ID: #3307

comment:25 Changed 8 years ago by 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.

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.

comment:26 Changed 8 years ago by nickm

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

comment:27 in reply to:  25 Changed 8 years ago by anonym

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 fixes the issue.

Changed 8 years ago by anonym

Update man page for new UseBridges tristate behaviour.

comment:28 in reply to:  17 Changed 8 years ago by 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. Can you describe the remaining bug, symptoms you see, how to reproduce, etc?

comment:29 in reply to:  25 Changed 8 years ago by arma

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.

comment:30 in reply to:  22 Changed 8 years ago by arma

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?

comment:31 Changed 8 years ago by 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.

comment:32 in reply to:  31 ; Changed 8 years ago by 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.

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?

comment:33 in reply to:  32 ; Changed 8 years ago by nickm

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.

comment:34 in reply to:  33 Changed 8 years ago by nickm

Replying to nickm:

Will fix.

Those now have fixup commits in my branch.

comment:35 in reply to:  33 ; Changed 8 years ago by 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. 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 already fixes this I trust you.

comment:36 in reply to:  35 Changed 8 years ago by arma

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. 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 with more details.

comment:37 in reply to:  35 ; Changed 8 years ago by 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. 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.

comment:38 in reply to:  37 ; Changed 8 years ago by anonym

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. 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?

*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.

comment:39 in reply to:  38 Changed 8 years ago by arma

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?

I think #3198 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.

comment:40 Changed 8 years ago by nickm

Okay; revised branch bug2355_v3 in my public repo. Confirm that's what we want?

comment:41 Changed 8 years ago by arma

looks good

comment:42 Changed 8 years ago by nickm

Great; merged.

comment:43 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

comment:44 Changed 7 years ago by nickm

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