Opened 9 years ago

Closed 8 years ago

Last modified 7 years ago

#2841 closed task (implemented)

Implementing the pluggable-transport spec (external proxies)

Reported by: asn Owned by: asn
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-bridge
Cc: nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We should implement the proposals/ideas/xxx-pluggable-transport.txt.

I've *started* doing that here:
https://gitorious.org/mytor/mytor/commits/pluggable

Child Tickets

Change History (19)

comment:1 Changed 9 years ago by nickm

Milestone: Tor: 0.2.3.x-final

comment:2 Changed 9 years ago by nickm

quick notes:

All new functions types and fields will need documentation. Remember to "make check-spaces", and to configure with --enable-gcc-warnings .

bridge_add_from_config() should document that the bridge now owns the transport_protocol_t, I think.

Tor's function style is

return_type
function_name(args)
{
   ...

but in a few places, this patch puts the { on the same line as the args.

By convention, we should almost always have the name for a torrc entry be the same as the name for its field in or_options_t: it cuts down on confusion. Also, we keep
_option_vars in alphabetical order for some reason.

Re-parsing all the transport lines for each bridge seems off. Can we just parse all the transport lines first, then parse the bridge lines?

Spelling in connection_or_finished_connecting: "pluggable", not "pluggalbe".

This makes me worried:

    transport = find_bridge_transport_by_addrport(&addr, port);
    tor_assert(transport);

That can't be right, can it? Back in bridge_add_from_config, we didn't give every bridge a transport.

comment:3 in reply to:  2 Changed 9 years ago by arma

Replying to nickm:

Also, we keep _option_vars in alphabetical order for some reason.

It's so there's a prayer of being able to predict the behavior when you run tor with 'u 1' as a config option.

(Not saying this is an adequate reason; but there it is.)

comment:4 Changed 9 years ago by asn

Status: newneeds_review

Okay, I think I fixed all your comments!

It's probably not the most beautiful code I've ever written but I couldn't find a better way to do the "Re-parsing all the transport lines for each bridge seems off. Can we just parse all the transport lines first, then parse the bridge lines?" part. Sorry!

The current logging domains (everything to warn/notice) are blunt, but they helped me during debugging so I left them like this.

I'll put this in needs_review.

comment:5 Changed 9 years ago by asn

Component: Pluggable transportTor Bridge

I'm moving this to the 'Tor Bridge' component.
Let's leave the 'Pluggable Transport' component to stuff related to transport proxies (obfsproxy, etc.) and transport protocols.

comment:6 Changed 9 years ago by asn

Okay, forget the old branch pluggable and check out my branch bug2841.

comment:7 Changed 9 years ago by asn

Status: needs_reviewassigned

ATM if the pluggable transports proxy is not running when tor boots, all subsequent connections will die silently. I have to code a way for tor to notify the user.
Pulling this out of needs_review for now.

comment:8 Changed 9 years ago by asn

Status: assignedneeds_review

Okay. Fixed. Check out my bug2841 branch.

abe03f49 gives much more specific log output, but the implementation is ugly. It was the reason I asked in #tor-dev if it would be wise to include proxy info in 'connection_t' [1]. Indeed, in a busy relay there will be many 'connection_t' and it's totally not worth it just for this log message; but maybe later on there will be an actual need for this.

[1]: Specifically, I was thinking of attaching a proxy_info_t struct to it -- which will include proxy state/addr/port/type -- and taking away the proxy_state:4.

comment:9 Changed 8 years ago by nickm

First thoughts:

In circuitbuild.c

  • Lots of your doxygen comments don't match Tor's style. Look at how the others are formatted.
  • By convention, our *_free() functions check for NULL and do nothing if called with NULL as an argument.
  • In general, it's not the best idea to call structures "foo_info_t" or "foo_data_t". After all, this is a computer program: everything is information! everything is data! Let's just call it foo_t. (Yes, I know that Tor already violates this in places like bridge_info_t. I think that's a mistake.)
  • It looks like you could use a "transport_get_by_name()" function that transport_add_from_config and match_bridges_with_transports() could use.
  • I don't think it should be an error to have a bridge listed with a corresponding transport configured: it makes the bridge unusable, sure, but we can still use the other bridges.
  • Why give a warning and reject the configuration if we have transports configured without corresponding bridges? That seems acceptable to me: just because I know about a transport doesn't mean I should have to use it.
  • Having bridge_info_t keep a pointer to the transport_(info_)t seems potentially risky if there's a chance we'll free the transport before we free the bridge. Even if that can't happen now, it seems needlessly fragile.
  • The return convention mostly used in Tor is 0 on success, -1 on failure. Not 1 on success, -1 on failure.
  • English possessive "its" is "its"; "it's" is a contraction for "it is". Sorry; I didn't make it up.
  • Successful, not successfull.
  • The check for duplicate names bridge_add_from_config seems very wrong: it is totally okay for two bridges to use the same transport, right?
  • clear_transport_list should probably set transport_list to NULL.

In config.c:

  • In options_act(), it looks like we only call clear_transport_list() if ClientTransportPlugin is set. Consider what this will do if we have some transports configured, and then we remove all the configured transports. It seems that all the old transports will remain configured.
  • Never assert(); always tor_assert().
  • You call match_bridges_with_transports() only if both Bridges and ClientTransportPlugin are set. This means that if the user sets up a bunch of Bridges with transports but sets no ClientTransportPlugins, the code will never notice that the bridges have have their transports missing. ..... (Oh, wait. Later on I see that you check for this case in parse_bridge_line. But that seems redundant: match_bridges_with_transports() already does this test for you.)
  • In the"REALLY ugly" check, how about something like:
      if ((options->Socks4Proxy != NULL) + (options->Socks5Proxy != NULL) +
          (options->HTTPSProxy != NULL) + (options->ClientTransportPlugins != NULL) > 1)
    

?

  • Do we want to actually have ClientTransportPlugin exclusive with all other proxy settings?
  • By convention, we try not to do:
       if (x) {
         a;
       } else
         b;
    
    That is, if one case is bracketed, we should bracket both cases.
  • It looks like the log_debug in parse_bridge_line will log transport_name even if transport_name is NULL; that's not kosher.

In connection.c:

  • Looks like you forgot to run "make check-spaces".
  • In connection_or_connect: we set tor_addr_t values using tor_addr_copy, not =.

comment:10 in reply to:  9 ; Changed 8 years ago by asn

Replying to nickm:

First thoughts:

In circuitbuild.c

  • Lots of your doxygen comments don't match Tor's style. Look at how the others are formatted.

Done! (I think I caught them all)

  • By convention, our *_free() functions check for NULL and do nothing if called with NULL as an argument.

Done!

  • In general, it's not the best idea to call structures "foo_info_t" or "foo_data_t". After all, this is a computer program: everything is information! everything is data! Let's just call it foo_t. (Yes, I know that Tor already violates this in places like bridge_info_t. I think that's a mistake.)

Done!

  • It looks like you could use a "transport_get_by_name()" function that transport_add_from_config and match_bridges_with_transports() could use.

Done!

  • I don't think it should be an error to have a bridge listed with a corresponding transport configured: it makes the bridge unusable, sure, but we can still use the other bridges.
  • Why give a warning and reject the configuration if we have transports configured without corresponding bridges? That seems acceptable to me: just because I know about a transport doesn't mean I should have to use it.
  • Having bridge_info_t keep a pointer to the transport_(info_)t seems potentially risky if there's a chance we'll free the transport before we free the bridge. Even if that can't happen now, it seems needlessly fragile.

Alright I'll handle these three comments together.

I removed the transport_t pointer from the bridge_info_t. I'm now using transport_get_by_name() to get the bridge's transport instead of using bridge->transport.

This change though, essentially makes match_bridges_with_transports() semi-useless. Do you think we should still keep match_bridges_with_transports() around so that it warns the user of configuration anomalies (like the ones you mentioned in those two comments above)?

I'm not sure what the correct course of action is, when the user gives Bridge lines with methods without corresponding ClientTransportPlugin lines (or the opposite.).
I was thinking that it would probably be user misconfiguration (typo or forgetting to add a line) and that's why I was erroring out.

I don't have too strong feelings about this, but there are some small things we should take care off if we don't error out.
For example with the current code, if the user gives a Bridge line with a method without a ClientTransportPlugin line, tor attempts to connect to the Bridge addrport directly thinking that there is no pluggable transport for this bridge.
This happens because find_transport_by_bridge_addrport() returns either the transport or NULL, and would return NULL on the above case. Do you think this is a case we should be handling with something more than a warning?

  • The return convention mostly used in Tor is 0 on success, -1 on failure. Not 1 on success, -1 on failure.

Done!

  • English possessive "its" is "its"; "it's" is a contraction for "it is". Sorry; I didn't make it up.
  • Successful, not successfull.

Your right! Done!

  • The check for duplicate names bridge_add_from_config seems very wrong: it is totally okay for two bridges to use the same transport, right?

Oh my, you are right! Done!

  • clear_transport_list should probably set transport_list to NULL.

What do you mean? clear_transport_list() just initiates transport_list, like clear_bridge_list() does for bridge_list. It doesn't free it, transport_list is a static smartlist just like bridge_list.

In config.c:

  • In options_act(), it looks like we only call clear_transport_list() if ClientTransportPlugin is set. Consider what this will do if we have some transports configured, and then we remove all the configured transports. It seems that all the old transports will remain configured.

Oh you are right! Shouldn't we do the same thing for Bridges? (mark_bridge_list() and sweep_bridge_list() are only called if Bridges is set)

  • Never assert(); always tor_assert().

Done!

  • You call match_bridges_with_transports() only if both Bridges and ClientTransportPlugin are set. This means that if the user sets up a bunch of Bridges with transports but sets no ClientTransportPlugins, the code will never notice that the bridges have have their transports missing. ..... (Oh, wait. Later on I see that you check for this case in parse_bridge_line. But that seems redundant: match_bridges_with_transports() already does this test for you.)

I have removed match_bridges_with_transports() for now.

  • In the"REALLY ugly" check, how about something like:
      if ((options->Socks4Proxy != NULL) + (options->Socks5Proxy != NULL) +
          (options->HTTPSProxy != NULL) + (options->ClientTransportPlugins != NULL) > 1)
    

?

Oh great, thank you!

  • Do we want to actually have ClientTransportPlugin exclusive with all other proxy settings?

I'm not sure. The current code won't work if ClientTransportPlugin is not exclusive with all other proxy settings, and I was considering that a feature.
If you think that it shouldn't be exclusive, we should change that.

  • By convention, we try not to do:
       if (x) {
         a;
       } else
         b;
    
    That is, if one case is bracketed, we should bracket both cases.

Done!

  • It looks like the log_debug in parse_bridge_line will log transport_name even if transport_name is NULL; that's not kosher.

Done!

In connection.c:

  • Looks like you forgot to run "make check-spaces".

Done!

  • In connection_or_connect: we set tor_addr_t values using tor_addr_copy, not =.

Done!

[The above message was written while I was having a terrible headache. If something doesn't make sense; poke me and I shall rewrite it.]

comment:11 in reply to:  10 Changed 8 years ago by rransom

Replying to asn:

Replying to nickm:

First thoughts:

  • I don't think it should be an error to have a bridge listed with a corresponding transport configured: it makes the bridge unusable, sure, but we can still use the other bridges.
  • Why give a warning and reject the configuration if we have transports configured without corresponding bridges? That seems acceptable to me: just because I know about a transport doesn't mean I should have to use it.
  • Having bridge_info_t keep a pointer to the transport_(info_)t seems potentially risky if there's a chance we'll free the transport before we free the bridge. Even if that can't happen now, it seems needlessly fragile.

Alright I'll handle these three comments together.

I removed the transport_t pointer from the bridge_info_t. I'm now using transport_get_by_name() to get the bridge's transport instead of using bridge->transport.

This change though, essentially makes match_bridges_with_transports() semi-useless. Do you think we should still keep match_bridges_with_transports() around so that it warns the user of configuration anomalies (like the ones you mentioned in those two comments above)?

ClientTransportPlugin lines that are not used by any Bridge will be quite normal -- bundles will ship with multiple protocol obfuscators pre-configured in their torrcs, but with no pre-configured bridges.

Bridge lines that specify an unavailable ClientTransportPlugin are bad, and the user should be warned about them.

I'm not sure what the correct course of action is, when the user gives Bridge lines with methods without corresponding ClientTransportPlugin lines (or the opposite.).
I was thinking that it would probably be user misconfiguration (typo or forgetting to add a line) and that's why I was erroring out.

I don't have too strong feelings about this, but there are some small things we should take care off if we don't error out.
For example with the current code, if the user gives a Bridge line with a method without a ClientTransportPlugin line, tor attempts to connect to the Bridge addrport directly thinking that there is no pluggable transport for this bridge.
This happens because find_transport_by_bridge_addrport() returns either the transport or NULL, and would return NULL on the above case. Do you think this is a case we should be handling with something more than a warning?

This is bad. Fix it.

Your right! Done!

s/Your/You're/

In config.c:

  • In options_act(), it looks like we only call clear_transport_list() if ClientTransportPlugin is set. Consider what this will do if we have some transports configured, and then we remove all the configured transports. It seems that all the old transports will remain configured.

Oh you are right! Shouldn't we do the same thing for Bridges? (mark_bridge_list() and sweep_bridge_list() are only called if Bridges is set)

We should. But this doesn't matter for bridges, because we require that at least one Bridge line be set whenever UseBridges is on, and we ignore bridge_list when UseBridges is off. It does matter for transport plugins.

  • Do we want to actually have ClientTransportPlugin exclusive with all other proxy settings?

I'm not sure. The current code won't work if ClientTransportPlugin is not exclusive with all other proxy settings, and I was considering that a feature.
If you think that it shouldn't be exclusive, we should change that.

ClientTransportPlugin shouldn't be exclusive with all other proxy settings, because a user may need to use pluggable transports because his/her/its only Internet access is through a fascist censoring HTTP proxy. But Tor can't tell the transport plugin to use a proxy. Hmm...

Best to make ClientTransportPlugin explicitly exclusive with all other proxy settings for now, but this is a bug that we will need to fix ASAP.

comment:12 in reply to:  10 Changed 8 years ago by nickm

Replying to asn:

Replying to nickm:

  • I don't think it should be an error to have a bridge listed with a corresponding transport configured: it makes the bridge unusable, sure, but we can still use the other bridges.
  • Why give a warning and reject the configuration if we have transports configured without corresponding bridges? That seems acceptable to me: just because I know about a transport doesn't mean I should have to use it.
  • Having bridge_info_t keep a pointer to the transport_(info_)t seems potentially risky if there's a chance we'll free the transport before we free the bridge. Even if that can't happen now, it seems needlessly fragile.

Alright I'll handle these three comments together.

I removed the transport_t pointer from the bridge_info_t. I'm now using transport_get_by_name() to get the bridge's transport instead of using bridge->transport.

Sounds good.

This change though, essentially makes match_bridges_with_transports() semi-useless. Do you think we should still keep match_bridges_with_transports() around so that it warns the user of configuration anomalies (like the ones you mentioned in those two comments above)?

I'm not sure what the correct course of action is, when the user gives Bridge lines with methods without corresponding ClientTransportPlugin lines (or the opposite.).
I was thinking that it would probably be user misconfiguration (typo or forgetting to add a line) and that's why I was erroring out.

I agree with rransom that it's reasonable to have lots of ClientTransportPlugin entries with no corresponding bridge using them. On the other hand, if you have a bridge that you can't use because of a missing ClientTransportPlugin, we should probably warn that we won't be using that bridge.

My rationale for not warning in the first case is that it's a fine idea to configure Tor (or have Tor come preconfigured) with a list of all the transports you know about. You shouldn't have to disable the transports just because you aren't using them.

For a bridge, on the other hand, you probably wouldn't configure it if you didn't want to be able to use it. Having a bridge that you can't use is normal, but it *is* something you'd want to know about.

I don't have too strong feelings about this, but there are some small things we should take care off if we don't error out.
For example with the current code, if the user gives a Bridge line with a method without a ClientTransportPlugin line, tor attempts to connect to the Bridge addrport directly thinking that there is no pluggable transport for this bridge.
This happens because find_transport_by_bridge_addrport() returns either the transport or NULL, and would return NULL on the above case. Do you think this is a case we should be handling with something more than a warning?

Absolutely. We should never connect directly to a bridge that was configured with a transport.

  • clear_transport_list should probably set transport_list to NULL.

What do you mean? clear_transport_list() just initiates transport_list, like clear_bridge_list() does for bridge_list. It doesn't free it, transport_list is a static smartlist just like bridge_list.

Well, what frees the the transport list on exit? Something must: we try to free all our ram on exit so as to make it easier to find and debug leaks.

In config.c:

  • In options_act(), it looks like we only call clear_transport_list() if ClientTransportPlugin is set. Consider what this will do if we have some transports configured, and then we remove all the configured transports. It seems that all the old transports will remain configured.

Oh you are right! Shouldn't we do the same thing for Bridges? (mark_bridge_list() and sweep_bridge_list() are only called if Bridges is set)

I think we should, if that's so. Open a bug?

  • Do we want to actually have ClientTransportPlugin exclusive with all other proxy settings?

I'm not sure. The current code won't work if ClientTransportPlugin is not exclusive with all other proxy settings, and I was considering that a feature.
If you think that it shouldn't be exclusive, we should change that.

If it's harder to implement chained proxies, we can call this an unimplemented feature, and wait to see whether anyone needs it.

It might be that we need to teach the plugins to use proxies, rather than having Tor manage the chaining; I can't figure out how else to implement it in the likeliest case where we have a local transport plugin that needs to go to the internet via a proxy.

comment:13 Changed 8 years ago by asn

Okay, I'm now handling all pluggable transport lines as discussed in this ticket. I'm also making sure that we don't go and connect directly to a bridge.

Please check again!

(I'm also freeing the transport_list.)

comment:14 Changed 8 years ago by asn

Summary: Implementing the pluggable-transport specImplementing the pluggable-transport spec (external proxies)

I'm splitting this ticket.

This ticket is for external proxies and ticket #3472 is for managed proxies.

comment:15 Changed 8 years ago by nickm

I took another look through it, and made some tweaks in branch "bug2841" in my public repo. By my lights, this is ready to squash a bit and merge. It could use review by somebody else, though, or at least by me at a more awake time.

comment:16 Changed 8 years ago by asn

Your "bug2841" tweaks look okay!
I also just tested the branch and it works nicely.

comment:17 Changed 8 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Great; merging time.

(I realize there's more to proposal 180 than was done in 2841; let's open a new ticket or tickets for any of that stuff that never got a ticket.)

comment:18 Changed 7 years ago by nickm

Keywords: tor-bridge added

comment:19 Changed 7 years ago by nickm

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