[tor-bugs] #3786 [Tor Relay]: Make clients and bridges use their IPv6 address

Sun Nov 20 02:47:31 UTC 2011

#3786: Make clients and bridges use their IPv6 address
-----------------------+----------------------------------------------------
 Reporter:  ln5        |          Owner:  ln5               
     Type:  task       |         Status:  needs_review      
 Priority:  normal     |      Milestone:  Tor: 0.2.3.x-final
Component:  Tor Relay  |        Version:                    
 Keywords:             |         Parent:  #3563             
   Points:             |   Actualpoints:                    
-----------------------+----------------------------------------------------

Comment(by nickm):

 Initial notes on skimming:

 '''I love how you are doing this as a bunch of little commits!'''

 This is going to be easier to review than I'm used to.

 6a92ca076c11b3: Decapitalize 'V' in IPV*Only

 Trivially correct; doesn't actually matter for strcasecmp, but that's how
 we specced it, I believe.

 5b302fb679c91:

 Hm. The idea of a single "alt" addr feels like it's going to hurt us.
 Maybe instead we want the idea of a "best IPv6 address for connecting" or
 "best address for connecting" or something?

 Also, the documentation for functions that can return -1 on error should
 say so, and maybe why they would do that.

 Our style for doxygen comments is to have a * at the start of each line of
 documentation.

 node_does_prefer_ipv6 would seem to crash if it ever gets a node without a
 routerinfo.  Also, node_ipv6_is_preferred might be a better name?  We're
 not asking whether the node prefers ipv6; we're asking whether we prefer
 using ipv6 with that node.

 I wonder if it makes sense to have all of these prim/alt/pref functions to
 return both an ORPort and an addr ?   (returning the or_port via a pointer
 argument, I'd guess.)

 Mutable fields belong in node_t more than in routerinfo_t.  The ones that
 are there are legacy; I'd prefer to have prefer_ipv6 there if possible.

 Also, I think we need a separate bit for whether we think the node is
 running at either address.  Right now we use node->is_running for whether
 we think the node is up.   I think we might need one bit per address;
 otherwise, it's hard to have the notion "I can connect at this address but
 not that one."

 20e4621fb6fc976a793f06: Add helper functions

 We can do this if we want to be pedantic, and I have no objection to being
 pedantic, but there are a bunch of places where we already assume that
 AF_INET==PF_INET, and AF_INET6==PF_INET6.  I have never run into an OS
 where this wasn't true, but if you think it's going to be a problem
 anywhere you can think of, we should probably open a ticket for it.

 d380fce501595c17a0: Use correct address and protocol family

 Looks good

 1b9c82fd6d1206d9:

 Looks good.

 As a style thing, we're completely willing in Tor to say "if (foo)" where
 you say "if (foo != NULL)", but we accept either.

 0151ae4bec9bafe435: Make get_primary_or_port() consider IPv6

 I don't know how you ''could'' use this; when you call
 get_primary_or_port(), how do you know which port you got unless you know
 what address it goes with?

 And for the current use, I think it's wrong.  The function
 get_primary_or_port() is called only by router_get_advertised_or_port,
 which uses it to decide which port to use in the "router ..." line of a
 router descriptor -- and that's the IPv4 port.

 [FWIW, there will need to be a fix in router_get_advertised_or_port ; when
 it does connection_get_by_type(CONN_TYPE_OR_LISTENER), it will need to
 make sure it gets an IPv4 listener.  Right now it doesn't.]
 [Please ignore that last comment if you fixed it later.]

 34a44559fa3ae1d2d5fb2: Make rewrite_node_address_for_bridge() match IPv6
 addresses too.

 Looks mostly okay.  (Quibble: your brace and indentation style looks a
 little off.  We use K&R, with 2-space indents.  I see some one-space
 indents here {or are they tabs? can't tell}, and an "} \n else {" when "}
 else {" would be more usual for us.)

 I am not sure how I feel about replacing ri->address.  (What do we
 actually use ri->address for anyway?)

 0289f47ade0258c: Don't use the IPv4 address if there's an IPv6 address and
 IPv6 is preferred

 This seems mostly plausible but a little problematic.  I wonder if
 prefer_ipv6() is enough for us?   It seem that in the medium term, we want
 not only the idea of "I support IPv4 and IPv6, but prefer (IPv6/IPv4)" but
 also the idea of "I only support IPv4" and "I only support IPv6".   [Oh,
 never mind!  It looks like you removed it later.]

 Also, extend_info_from_router() and node_get_addr() are called in too many
 cases to make this change for all the users of the function.  Sometimes,
 when you're making an extend_info(), it's so you can connect to a node
 directly.  Sometimes, it's so you can tell another node on your circuit to
 connect there (I think).  The same applies to nodes, I believe.

 The change to node_get_prim_addr seems wrong, I think.  It makes sense to
 make that change for a node's ''preferred'' address, but I thought a a
 node's ''primary'' address was supposed to be IPv4 automatically.

 3a812331514e25479: Allow extending to IPv4 and IPv6 addresses

 We don't want this patch yet, I think.  To extend a circuit is (usually,
 assuming that we're being terminologically precise) to tell the current
 last hop of the circuit to add another hop at the end of the circuit.
 Right now, we can't do that: current Tor nodes don't support EXTEND
 requests to an IPv6 address.

 So right now, I think we only want to explicitly allow IPv6 addresses for
 the _first_ hop of the circuit.  I believe that's the first main branch of
 circuit_send_next_onion_skin().

 ef7a9f327b982dce795a:

  This messes more with 0151ae4bec9bafe435 ; the same issues apply.

 b11e2ddb01eebfb:

   Looks fine.

 33d3730cdac6a8c175:

   Do you want prim_addr or pref_addr here?  I thought pref_addr was the
 one that we wanted to connect to, and prim_addr was the "primary",
 "official" ipv4 address.

 bc6c988482d77575b582: Remove the need for a "prefer ipv6" option.

   Using pref_addr_port is still problematic in extend_info_from_router:
 see 0289f47ade0258c. We use extend_info_from_router not only for
 connecting directly to the router, but also for telling other routers whom
 to connect to when they extend.

 b90ff192dae076d02d3fa: Parse IPv6 addresses in descriptors

   Looks fine

 51226c36e720bad6: Indicate preferred IP family for a bridge based on
 configured bridges

   Maybe better to stick this flag node_t; but not a disaster if you can't
 move it by the end of the month.

   What happens if the client lists both the v4 and the v6 address for a
 bridge?

 b6d741abc547a4bdd:

   The whole pack of changes to "get_primary_or_port()" are still
 problematic; see above.

 several documentaion-only commits:

    Looks fine.

 4cb534e23caecb14: Handle bridges with IPv6-only orport better:

     Looks ok.

 Wow, that's a pile of comments!  Please let me know about any of these
 you've got questions about, and I'll try to answer.

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