Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#18460 closed defect (fixed)

Relays and bridges are not counting directory requests coming in via IPv6

Reported by: karsten Owned by:
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: 0.2.8.1-alpha
Severity: Normal Keywords: tor-relay tor-bridge 026-backport 027-backport TorCoreTeam201603
Cc: teor, arma Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

While testing my #8786 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 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.

Child Tickets

Attachments (1)

antirio.png (49.7 KB) - added by karsten 4 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 4 years ago by karsten

Status: newneeds_review

Please find branch task-18460 in my public repository.

comment:2 Changed 4 years ago by 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.

comment:3 Changed 4 years ago by nickm

Keywords: 026-backport 027-backport added
Milestone: Tor: 0.2.6.x-finalTor: 0.2.8.x-final

comment:4 in reply to:  2 ; Changed 4 years ago by 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?

Also, part of what makes me somewhat uncomfortable of using address is that it says "FQDN (or IP) of the other end" in the comment. But we really want it to be an IP for tor_inet_aton(), or we're not counting it either. When would address contain an FQDN instead of an IP?

comment:5 in reply to:  4 Changed 4 years ago by nickm

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 address is that it says "FQDN (or IP) of the other end" in the comment. But we really want it to be an IP for tor_inet_aton(), or we're not counting it either. When would address contain an FQDN instead of an IP?

I can't think of a case where it would?

comment:6 Changed 4 years ago by karsten

Okay, I took the painful route of renaming address and addr in connection_t (to ADDRESS and ADDR) 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 addr is copied from another addr using tor_addr_copy() and address is set to a string representation of that same addr using tor_dup_addr(). That's the ideal case in the context of this ticket where we could use either addr or address and 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 address may be set to a const string like "(Tor_internal)" or "(rendezvous)" rather than a string representation of addr. This would make it impossible to use address, 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 to address but not to addr. This case makes it difficult to use addr instead of address, 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 addr or address is 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 address and extend the code towards also supporting string representations of IPv6 addresses. I could imagine that we can simply use tor_addr_parse() rather than tor_inet_aton() and tor_addr_from_ipv4h() to convert the address to an addr which 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!

comment:7 Changed 4 years ago by 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.

comment:8 Changed 4 years ago by nickm

Status: needs_reviewneeds_revision

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.

comment:9 Changed 4 years ago by karsten

Status: needs_revisionneeds_review

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.

comment:10 Changed 4 years ago by nickm

Keywords: TorCoreTeam201603 added

We should at least see if we can look over this patch for 0.2.8 in March, though deferring might be needed.

comment:11 Changed 4 years ago by karsten

I asked two people, one via private mail and one via the tor-relays@ mailing list, to test this patch on their IPv6 bridge. Ideally, we will have feedback in 48 hours from now how their bridges liked this patch and what statistics they reported.

comment:12 Changed 4 years ago by karsten

This patch is now running on antirio for 1 day 9 hours and on starman for 22 hours. Neither of them has exploded, and antirio reported plausible statistics. I'd want to wait another 24 hours or so to see if we fixed the problem. But at least it looks like we didn't introduce a new one.

Changed 4 years ago by karsten

Attachment: antirio.png added

comment:13 Changed 4 years ago by karsten

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.


comment:14 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

I'm convinced. Merging to 0.2.8!

comment:15 in reply to:  8 Changed 4 years ago by teor

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 (initial comment fix) and #18721 (accessors) for this.

Note: See TracTickets for help on using tickets.