Relays and bridges are not counting directory requests coming in via IPv6
While testing my #8786 (moved) branch I found that relays and bridges are currently not counting directory requests coming in via IPv6 at all. The reason is the if in the following code snippet from directory_handle_command_get():
struct in_addr in;
tor_addr_t addr;
if (tor_inet_aton((TO_CONN(conn))->address, &in)) {
tor_addr_from_ipv4h(&addr, ntohl(in.s_addr));
geoip_note_client_seen(GEOIP_CLIENT_NETWORKSTATUS,
&addr, NULL,
time(NULL));tor_inet_aton expects an IPv4 address in dotted-quad notation and returns 0 if it's given an IPv6 address.
When digging deeper into Git history, I found that I had changed that code to &TO_CONN(conn)->addr 4 years ago and then again to the code above in 4741aa4 because "Roger notes that address and addr are two different things."
I think this was a mistake and that we can fix this by just reverting 4741aa4. I'll post a branch in a minute that I tested using Chutney's "bridges+ipv6" network (together with teor's #17153 (moved) fix).
Please correct me if we should really use address here instead of addr. In that case we'll probably want to look if address contains an IPv6 address string and handle that separately.
Activity
changed milestone to %Tor: 0.2.8.x-final
Please find branch task-18460 in my public repository.
Trac:
Status: new to needs_reviewWe should be careful that we are checking GeoIP for the remote address, rather than the address of any proxy. I think that 'address' is the remote address, and 'addr' is the proxy, at least in some places in the code.
If that's the case, let's check and then update the comments on the struct definition, because I can never remember which way round it works.
-
Replying to teor:
We should be careful that we are checking GeoIP for the remote address, rather than the address of any proxy. I think that 'address' is the remote address, and 'addr' is the proxy, at least in some places in the code.
If that's the case, let's check and then update the comments on the struct definition, because I can never remember which way round it works.
Oh, okay. Any hints on where to start to find out?
Also, part of what makes me somewhat uncomfortable of using
addressis that it says "FQDN (or IP) of the other end" in the comment. But we really want it to be an IP fortor_inet_aton(), or we're not counting it either. When wouldaddresscontain an FQDN instead of an IP? 
Replying to karsten:
Replying to teor:
We should be careful that we are checking GeoIP for the remote address, rather than the address of any proxy. I think that 'address' is the remote address, and 'addr' is the proxy, at least in some places in the code.
If that's the case, let's check and then update the comments on the struct definition, because I can never remember which way round it works.
Oh, okay. Any hints on where to start to find out?
I would start by looking at everything that sets or modifies conn->address and conn->addr.
One (painful but thorough) way to do that is to rename the field in the header, and look to see where all the errors appear.
Also, part of what makes me somewhat uncomfortable of using
addressis that it says "FQDN (or IP) of the other end" in the comment. But we really want it to be an IP fortor_inet_aton(), or we're not counting it either. When wouldaddresscontain an FQDN instead of an IP?I can't think of a case where it would?
-
Okay, I took the painful route of renaming
addressandaddrinconnection_t(toADDRESSandADDR) and looking for the compile errors appearing from that. I also annotated all places where either of the two fields are set or modified with a/* XXX18460 */comment. I'm pushing my branch as task-18460-annotations-dont-merge to my public repository, but please don't merge that anywhere.I'm counting 16 functions where either or both of the two fields are set or modified, however, I cannot rule out that I forgot some cases. But let's use this as a start.
Let's go through these cases by filtering out all where
addris copied from anotheraddrusingtor_addr_copy()andaddressis set to a string representation of that sameaddrusingtor_dup_addr(). That's the ideal case in the context of this ticket where we could use eitheraddroraddressand get the exact same result:- connection_listener_new()
- connection_exit_connect_dir()
- directory_initiate_command_rend()
- evdns_server_callback()
- connection_ext_or_handle_cmd_useraddr()
Let's move on to the more complicated cases. One of them is where
addressmay be set to a const string like"(Tor_internal)"or"(rendezvous)"rather than a string representation ofaddr. This would make it impossible to useaddress, but we most likely don't care about these addresses anyway:- connection_ap_make_link()
- connection_exit_begin_conn()
- dnsserv_launch_request()
Let's look at the case that teor refers to above, which is where we're parsing a
"Forwarded-For:"HTTP header and writing the parsed address toaddressbut not toaddr. This case makes it difficult to useaddrinstead ofaddress, because the former is not updated but only the latter:- http_set_address_origin()
That leaves us with 7 more places in the code where either
addroraddressis set, which I didn't classify further and which I'm mostly listing here in case somebody else wants to take a look:- connection_handle_listener_read()
- connection_ap_handshake_send_resolve()
- connection_exit_begin_resolve()
- connection_or_init_conn_from_address()
- dns_resolve_impl()
- set_unix_port()
- rend_service_set_connection_addr_port()
My preliminary conclusion for this ticket is that we should stick to
addressand extend the code towards also supporting string representations of IPv6 addresses. I could imagine that we can simply usetor_addr_parse()rather thantor_inet_aton()andtor_addr_from_ipv4h()to convert theaddressto anaddrwhich would support both v4 and v6. I can write a patch and changes file for this and test it in a local Chutney network. But I'd first like to hear whether this makes sense at all. Thanks! -
Hmmm. I think that for the purpose of this patch the approach that you suggest would be fine.
But the status quo above is definitely nasty. I think we should open a ticket to add comments explaining the real story of what's going on above in the code, and that we also open a ticket to define a few accessor functions to provide tor_addr_t and string representations of the final target address, proxy-or-final, or whatever other things we actually need.
-
Hmmm. I think that for the purpose of this patch the approach that you suggest would be fine.
But the status quo above is definitely nasty. I think we should open a ticket to add comments explaining the real story of what's going on above in the code, and that we also open a ticket to define a few accessor functions to provide tor_addr_t and string representations of the final target address, proxy-or-final, or whatever other things we actually need.
Trac:
Status: needs_review to needs_revision -
Okay, I implemented the fix that I suggested above in branch task-18460-2 in my public repository. It works just fine in Chutney networks with IPv4 and IPv6 bridge clients.
I also included a changes file. For the "bugfix on" part, I found 4aca55e where we started counting IPv6 connections in bridge and entry stats, shortly followed by 4741aa4 which partially reverted that change and put back some code that only works for IPv4. I'd say that 4741aa4 is to blame here, so I'm using the first release containing that commit in the "bugfix on" part.
Should we test this patch on an IPv6 bridge before merging it? When looking at extra-info descriptors from the past 72 hours, the following bridges show a non-zero number of IPv6 clients: antirio, starman, MeekGoogle.
Trac:
Status: needs_revision to needs_review -
The two bridges have run with this patch for a few days now.
I'm attaching a graph of stats reported by antirio. The blue and green lines are unique IP addresses, the red line is counted directory requests. There is no direct connection between unique IP addresses and directory requests, though we'd expect the number of directory requests to go up a bit (because those coming in via IPv6 have not been counted previously) while the number of unique IP addresses stays the same. We now have three data points from after the patch, and even though the Mar 28 data points was not entirely clear, I'd say there's a visible upturn in directory requests.
At the same time neither starman nor antirio seem to have problems with this patch.
I'd say this patch is good to be merged.
Replying to nickm:
Hmmm. I think that for the purpose of this patch the approach that you suggest would be fine.
But the status quo above is definitely nasty. I think we should open a ticket to add comments explaining the real story of what's going on above in the code, and that we also open a ticket to define a few accessor functions to provide tor_addr_t and string representations of the final target address, proxy-or-final, or whatever other things we actually need.
For the record, I split off #18720 (moved) (initial comment fix) and #18721 (moved) (accessors) for this.
mentioned in issue #18720 (moved)
