don't start ClientTransportPlugin proxies until we have a bridge that wants them

changed milestone to %Tor: 0.2.5.x-final

added 024-backport component::core tor/tor milestone::Tor: 0.2.5.x-final priority::medium pt resolution::fixed status::closed tbb-needs tbb-tor-backported-3.6b1 tor-bridge type::defect labels

Trac:
Cc: N/A to asn

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

Trac:
Keywords: N/A deleted, pt added

Trac:
Keywords: pt deleted, pt tor-bridge added

Trac:
Component: Tor Bridge to Tor

Trac:
Username: mk
Cc: asn to asn, mk@dee.su

Trac:
Description: Eventually we want to ship the standard Vidalia bundle with a set of supported client transports. We don't want to launch all of the possible transports when Tor starts, because most users won't ever configure a bridge line that asks for a given transport.

So the client managed transports should launch only when we are configured to use a bridge that needs them.

And maybe they could be closed after we no longer have any bridges that need them. That could certainly wait and be considered a "later feature" though.

Do we foresee any client transports that would want to start earlier than when a bridge shows up that wants to use them? I guess that ties into #5010 (moved).

Another option would be for Vidalia to paw through your bridge lines and know to setconf a given ClientTransportPlugin line when it sees a certain type of bridge line. Putting the smarts in Vidalia is probably a poor choice though.

[I'm not sure which component this should be in -- either a Tor one or the pluggable transport one. Picking the one Nick is more likely to see, for now.]

[I'm also not sure which milestone to use. We can push it back to 0.2.4 if it's too much like a feature. That would mean that our "normal clients using pluggable transports" bundles will want to use 0.2.4.]

to

Eventually we want to ship the standard Tor Browser Bundle with a set of supported client transports. We don't want to launch all of the possible transports when Tor starts, because most users won't ever configure a bridge line that asks for a given transport.

So the client managed transports should launch only when we are configured to use a bridge that needs them.

And maybe they could be closed after we no longer have any bridges that need them. That could certainly wait and be considered a "later feature" though.

Do we foresee any client transports that would want to start earlier than when a bridge shows up that wants to use them? I guess that ties into #5010 (moved).

Another option would be for Vidalia to paw through your bridge lines and know to setconf a given ClientTransportPlugin line when it sees a certain type of bridge line. Putting the smarts in Vidalia is probably a poor choice though.

[I'm not sure which component this should be in -- either a Tor one or the pluggable transport one. Picking the one Nick is more likely to see, for now.]

[I'm also not sure which milestone to use. We can push it back to 0.2.4 if it's too much like a feature. That would mean that our "normal clients using pluggable transports" bundles will want to use 0.2.4.]

This is a useful feature, indeed.

Implementing this is not trivial, since it will have to change some of the logic that parses Bridge lines and ClientTransportPlugin lines. I think I will move this to 0.2.5.x, since it's a small feature and it's too late for 0.2.4.x.

(This is another feature that does not go well with #3725 (moved).)

Trac:
Milestone: Tor: 0.2.4.x-final to Tor: 0.2.5.x-final

Please see branch bug5018 in https://git.torproject.org/user/asn/tor.git.

My approach was to check whether the transport in ClientTransportPlugin lines was already referenced by a Bridge line. If yes, we launch the managed proxy. Otherwise, we ignore it.

Trac:
Status: new to needs_review

Trac:
Cc: asn, mk@dee.su to asn, mk@dee.su, dcf@torproject.org

Initial review:

The function comment for parse_client_transport_line() needs to change too, yes?
I think your transport_is_needed() is simpler as just:

/** Return True if we have a bridge that uses a transport with name
 *  <b>transport_name</b>. */
int
transport_is_needed(const char *transport_name)
{
  if (!bridge_list)
    return 0;

  SMARTLIST_FOREACH_BEGIN(bridge_list, const bridge_info_t *, bridge) {
    if (bridge->transport_name &&
        !strcmp(bridge->transport_name, transport_name))
      return 1;
  } SMARTLIST_FOREACH_END(bridge);

  return 0;
}

and that makes me wonder about whether the comparison should be case-sensitive or insensitive. (How is this handled elsewhere in the code? Looks like case-sensitive?)

This code has no plans for shutting down transports that no longer have bridges asking for them, yes? I think that's ok for a first go, but want to point it out in case people want to work on that later.
Man -- my code above looks almost exactly like the transport_get_by_name() function, doesn't it?

Linking comment:14:ticket:7153 and comment:18:ticket:7153 for the sake of reference. Some transports (flash proxy) would ideally start with no configured bridges, and add bridges to the configuration as they are discovered dynamically. In such a case, we would want a flag to tell the transport to start despite having no bridges configured.

I say, don't let this consideration delay the resolution of this ticket. We can continue using a phony bridge address, as we have been doing.

Replying to arma:

Initial review:

The function comment for parse_client_transport_line() needs to change too, yes?

True. Need to fix.

I think your transport_is_needed() is simpler as just: {{{ /** Return True if we have a bridge that uses a transport with name

transport_name. */ int transport_is_needed(const char *transport_name) { if (!bridge_list) return 0;

SMARTLIST_FOREACH_BEGIN(bridge_list, const bridge_info_t *, bridge) { if (bridge->transport_name && !strcmp(bridge->transport_name, transport_name)) return 1; } SMARTLIST_FOREACH_END(bridge);

return 0; } }}} and that makes me wonder about whether the comparison should be case-sensitive or insensitive. (How is this handled elsewhere in the code? Looks like case-sensitive?)

Also true. My original function was indeed very suboptimal.

This code has no plans for shutting down transports that no longer have bridges asking for them, yes? I think that's ok for a first go, but want to point it out in case people want to work on that later.

Right. This should probably be a new trac ticket.

Man -- my code above looks almost exactly like the transport_get_by_name() function, doesn't it?

Yep, even though these two functions cannot be merged in an obvious manner, we should look into this.

Moving this to needs_revision.

Trac:
Status: needs_review to needs_revision

Replying to arma:

Initial review:

The function comment for parse_client_transport_line() needs to change too, yes?

I think your transport_is_needed() is simpler as just:

Here's a patch implementing these suggestions on top of George's bug5018 branch.

Trac:
Status: needs_revision to needs_review

Trac:
bug5018.patch

Looks good to me. Thanks!

Looks good to me too. I'll let Nick play the git game?

okay, merged to 0.2.5. Thanks!

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

For TBB 3.5, I want to be able to ship all of the pluggable transports with the default bundle so that users can enter PT bridge lines into Tor Launcher and have them work out of the box.

However, I do not want the pluggable transport code to execute at all if users are not using pluggable transports. The most straightforward way to accomplish this for 0.2.4.x would be to backport this patch. Otherwise, we need custom configuration (and potentially restart) logic in Tor Launcher for 0.2.4.x but not 0.2.5.x, which is a situation I would like to avoid (and seems more risky than built-in Tor support for this behavior).

Trac:
Milestone: Tor: 0.2.5.x-final to Tor: 0.2.4.x-final
Status: closed to reopened
Keywords: N/A deleted, 024-backport added
Resolution: fixed to N/A

The thing to merge would, I believe, be my "bug5018" branch, which was against 0.2.4 in the first place.

This looks like a safe backport to me, but I'd like signoff from arma too. I don't think it's necessary to delay calling 0.2.4 stable for this, either.

Backport sounds fine to me. The main question is if we try to sneak it into 0.2.4.19, which I'm hoping to release rsn (like todayish), or wait for 0.2.4.20, which could be a while if there's nothing else urgently requiring a new release.

If we're super duper sure it will work and break nothing, sneaking it into 0.2.4.18 is the right answer.

I say you should put out the release today, then we should backport, and we should be a little more aggressive about 0.2.4 releases than we were during the 0.2.3 series.

The purpose of backporting this patch for TBB 3.5, is so that we can ship pluggable transport executables, with ClientTransportPlugin lines in torrc-default, but avoid having them run until the user configures a Bridge line using them.

I think that people are currently using the pluggable transports bundle for two reasons: 1) because it comes with the executables they need, and 2) because it comes with a list of preconfigured default bridges (which apparently get a traffic, despite that they are trivial to block). I'm afraid of leaving current PT bundle users out in the cold as they upgrade to a TBB 3.5 that has their executables but not their default bridges.

What do you think about the following idea? We alter the patch from this ticket so that a ClientTransportPlugin is started only if there is a bridge that needs it and UseBridges is true. Then we ship a torrc-defaults file that has all the ClientTransportPlugin lines uncommented, and all the Bridge lines uncommented, but has UseBridges set to 0. That way, users who don't need PTs don't see any change. Those who do need PTs will have the list of default bridges populated for them when they click the "set up bridges" button. The PT users will only have an extra click compared to what they have to do now--as opposed to additionally having to go to bridges.torproject.org, or copy-pasting the commented default bridges from torrc-defaults.

That seems like okay design from my pov, if other TBB/PT people agree that it makes sense to them. But inasmuch as it requires changes to the patches here, it would probably need a bit more time to percolate in 0.2.5 for a bit to make sure it isn't breaking anything, and a bit more thorough testing, than it would otherwise.

I like David's idea.

Replying to dcf:

What do you think about the following idea? We alter the patch from this ticket so that a ClientTransportPlugin is started only if there is a bridge that needs it and UseBridges is true. Then we ship a torrc-defaults file that has all the ClientTransportPlugin lines uncommented, and all the Bridge lines uncommented, but has UseBridges set to 0.

I higher-level alternative to this idea is #10418 (moved).

Trac:
Cc: asn, mk@dee.su, dcf@torproject.org to asn, mk@dee.su, dcf@torproject.org, gk

This patch currently emits a log_warn in the case that a ClientTransportPlugin is defined without any bridges. That should be a notice, since it is now common behavior for Tor Launcher, and is causing it to display a warning indication in its UI (which is confusing, because nothing is actually wrong).

Trac:
Keywords: N/A deleted, tbb-needs added

Replying to mikeperry:

This patch currently emits a log_warn in the case that a ClientTransportPlugin is defined without any bridges. That should be a notice, since it is now common behavior for Tor Launcher, and is causing it to display a warning indication in its UI (which is confusing, because nothing is actually wrong).

See branch bug5018_notice in https://git.torproject.org/user/asn/tor.git for the trivial patch that changes that log_warn to a log_notice.

Trac:
Status: reopened to needs_review

I've merged that to master and cherry-picked it to "bug5018" for backport.

Is this working well enough in 0.2.5.2-alpha that we should backport it now?

dcf/asn -- is there a new ticket for your preferred approach here? I'm going to call the current implementation a completion of this ticket I think.

FYI: This was backported to TBB 3.6-beta-1 (https://trac.torproject.org/projects/tor/attachment/ticket/5018/bug5018.patch).

Trac:
Keywords: N/A deleted, tbb-tor-backported-3.6b1 added

Sorry, that link should be: https://gitweb.torproject.org/builders/tor-browser-bundle.git/blob/HEAD:/gitian/patches/bug5018.patch

Recommendation; no backport, feels like a feature.

It's kind of a shame that TBB is maintaining a separate list of patches for their 0.2.4.x releases, but unless we think we can get rid of that practice entirely, backporting this one isn't going to help anything. So, fine with no backport.

Okay, no backport to 0.2.4 for these, for stability reasons.

Trac:
Milestone: Tor: 0.2.4.x-final to Tor: 0.2.5.x-final
Status: needs_review to closed
Resolution: N/A to fixed

closed

mentioned in issue #7153 (moved)

mentioned in issue #9444 (closed)

mentioned in issue #10418 (moved)

mentioned in issue #10538 (moved)

mentioned in issue #11658 (closed)

mentioned in issue #13124 (moved)

moved to tpo/core/tor#5018 (closed)

don't start ClientTransportPlugin proxies until we have a bridge that wants them

Child items 0

Activity