[tor-bugs] #2841 [Tor Bridge]: Implementing the pluggable-transport spec

[Tor Bug Tracker & Wiki]

#2841: Implementing the pluggable-transport spec
------------------------+---------------------------------------------------
 Reporter:  asn         |          Owner:  asn               
     Type:  task        |         Status:  needs_review      
 Priority:  normal      |      Milestone:  Tor: 0.2.3.x-final
Component:  Tor Bridge  |        Version:                    
 Keywords:              |         Parent:                    
   Points:              |   Actualpoints:                    
------------------------+---------------------------------------------------

Comment(by asn):

 Replying to [comment:9 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.]

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/2841#comment:10>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online