Opened 8 years ago

Closed 5 years ago

#5018 closed defect (fixed)

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

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: pt tor-bridge, 024-backport, tbb-needs, tbb-tor-backported-3.6b1
Cc: asn, mk@…, dcf@…, gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by arma)

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.

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

Child Tickets

Attachments (1)

bug5018.patch (6.7 KB) - added by dcf 6 years ago.

Download all attachments as: .zip

Change History (36)

comment:1 Changed 8 years ago by arma

Cc: asn added

comment:2 Changed 8 years ago by arma

Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final

comment:3 Changed 7 years ago by asn

Keywords: pt added

comment:4 Changed 7 years ago by nickm

Keywords: tor-bridge added

comment:5 Changed 7 years ago by nickm

Component: Tor BridgeTor

comment:6 Changed 7 years ago by mk

Cc: mk@… added

comment:7 Changed 7 years ago by arma

Description: modified (diff)

comment:8 Changed 7 years ago by asn

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final

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

comment:9 Changed 7 years ago by asn

Status: newneeds_review

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.

comment:10 Changed 7 years ago by dcf

Cc: dcf@… added

comment:11 Changed 6 years ago by 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:
    /** 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?

comment:12 Changed 6 years ago by dcf

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.

comment:13 in reply to:  11 Changed 6 years ago by asn

Status: needs_reviewneeds_revision

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

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.

comment:14 in reply to:  11 Changed 6 years ago by dcf

Status: needs_revisionneeds_review

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.

Changed 6 years ago by dcf

Attachment: bug5018.patch added

comment:15 Changed 6 years ago by asn

Looks good to me. Thanks!

comment:16 Changed 6 years ago by arma

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

comment:17 Changed 6 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

okay, merged to 0.2.5. Thanks!

comment:18 Changed 6 years ago by mikeperry

Keywords: 024-backport added
Milestone: Tor: 0.2.5.x-finalTor: 0.2.4.x-final
Resolution: fixed
Status: closedreopened

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

Last edited 6 years ago by mikeperry (previous) (diff)

comment:19 Changed 6 years ago by nickm

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.

comment:20 Changed 6 years ago by arma

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.

comment:21 Changed 6 years ago by nickm

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.

comment:22 Changed 6 years ago by dcf

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.

comment:23 Changed 6 years ago by nickm

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.

comment:24 Changed 6 years ago by asn

I like David's idea.

comment:25 in reply to:  22 Changed 6 years ago by dcf

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.

comment:26 Changed 6 years ago by gk

Cc: gk added

comment:27 Changed 6 years ago by mikeperry

Keywords: tbb-needs added

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

comment:28 in reply to:  27 Changed 6 years ago by asn

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.

comment:29 Changed 6 years ago by asn

Status: reopenedneeds_review

comment:30 Changed 6 years ago by nickm

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.

comment:31 Changed 5 years ago by mikeperry

Keywords: tbb-tor-backported-3.6b1 added

comment:33 Changed 5 years ago by nickm

Recommendation; no backport, feels like a feature.

comment:34 Changed 5 years ago by arma

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.

comment:35 Changed 5 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final
Resolution: fixed
Status: needs_reviewclosed

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

Note: See TracTickets for help on using tickets.