[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #3786 [Tor Relay]: Make clients and bridges use their IPv6 address
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs