Opened 2 months ago

Last modified 5 weeks ago

#33618 needs_revision enhancement

Add IPv6 Support to is_local_addr()

Reported by: kimimaro Owned by: kimimaro
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: outreachy-ipv6 ipv6 prop312
Cc: neel Actual Points:
Parent ID: Points: 1
Reviewer: teor Sponsor: Sponsor55-can

Description (last modified by teor)

We propose this optional change, to improve the accuracy of IPv6 address
detection from directory documents.

Directory servers use is_local_addr() to detect if the requesting tor
instance is on the same local network. If it is, the directory server does
not include the X-Your-Address-Is HTTP header in directory documents.

Currently, is_local_addr() checks for:

  • an internal IPv4 or IPv6 address, or
  • the same IPv4 /24 as the directory server.

We propose also checking for:

  • the same IPv6 /48 as the directory server.

We choose /48 because it is typically the smallest network in the global
IPv6 routing tables, and it was previously the recommended per-customer
network block. (See [RFC 6177: IPv6 End Site Address Assignment].)

Tor currently uses:

  • IPv4 /8 and IPv6 /16 for port summaries,
  • IPv4 /16 and IPv6 /32 for path selection (avoiding relays in the same network block).

Source: https://gitweb.torproject.org/torspec.git/tree/proposals/312-relay-auto-ipv6-addr.txt#n1099

Child Tickets

Change History (23)

comment:1 Changed 2 months ago by kimimaro

teor said that we should use tor_addr_compare_masked(). Here is the definition:

int
tor_addr_compare_masked(const tor_addr_t *addr1, const tor_addr_t *addr2,
                        maskbits_t mbits, tor_addr_comparison_t how)

I assume we check the first 48 bits (so that's the third argument), and use CMP_EXACT (fourth argument).

is_local_addr() already takes in an address pointer, which gives us the address we are checking. Now we need the IPv6 address of the local system (that would be the second argument). How exactly can I get that?

comment:2 Changed 2 months ago by nickm

Hmmm. It looks like we don't currently have an equivalent to last_resolved_address for IPv6. It's usually set by resolve_my_address(), but that one doesn't have an IPv6 equivalent either.

I suppose we could generalize resolve_my_address() to take a address family, but that function is already pretty complex.

Another option is to declare an IPv6 equivalent for last_resolved_address, but don't actually have anything set it yet. Then in a separate ticket, we can write the code that sets it.

Any thoughts here, teor?

comment:3 Changed 2 months ago by teor

Description: modified (diff)
Keywords: prop312 added
Milestone: Tor: 0.4.4.x-final
Points: 1
Sponsor: Sponsor55-can

I just fixed the ticket description formatting so it's more readable, and added some standard ticket fields.

comment:4 in reply to:  2 Changed 2 months ago by teor

Replying to nickm:

Hmmm. It looks like we don't currently have an equivalent to last_resolved_address for IPv6. It's usually set by resolve_my_address(), but that one doesn't have an IPv6 equivalent either.

I suppose we could generalize resolve_my_address() to take a address family, but that function is already pretty complex.

Another option is to declare an IPv6 equivalent for last_resolved_address, but don't actually have anything set it yet. Then in a separate ticket, we can write the code that sets it.

Once we finish implementing the essential parts of proposal 312, last_resolved_address and resolve_my_address() will support IPv6.

Any thoughts here, teor?

In the meantime, we can use the published IPv6 address of the relay, router_get_my_routerinfo()->ipv6_addr.

Before using router_get_my_routerinfo()->ipv6_addr, we should check:

  • router_get_my_routerinfo() is not NULL
  • tor_addr_is_valid(router_get_my_routerinfo()->ipv6_addr) is true

comment:5 Changed 2 months ago by kimimaro

Current code changes on commit afbf854ee1ef17622748ec4878e8929df8c9dc0c:

// includes added
#include "feature/relay/router.h"

/** Return true iff <b>addr</b> is judged to be on the same network as us, or
 * on a private network.
 */
MOCK_IMPL(int,
is_local_addr, (const tor_addr_t *addr))
{
  /* Check for an internal IPv4 or IPv6 address */
  if (tor_addr_is_internal(addr, 0))
    return 1;

  /* Check whether ip is on the same /24 as we are. */
  if (get_options()->EnforceDistinctSubnets == 0)
    return 0;
  if (tor_addr_family(addr) == AF_INET) {
    uint32_t ip = tor_addr_to_ipv4h(addr);

    /* It's possible that this next check will hit before the first time
     * resolve_my_address actually succeeds.  (For clients, it is likely that
     * resolve_my_address will never be called at all).  In those cases,
     * last_resolved_addr will be 0, and so checking to see whether ip is on
     * the same /24 as last_resolved_addr will be the same as checking whether
     * it was on net 0, which is already done by tor_addr_is_internal.
     */
    if ((last_resolved_addr & (uint32_t)0xffffff00ul)
        == (ip & (uint32_t)0xffffff00ul))
      return 1;
  }

  /* Check for the same IPv6 /48 as the directory server */
  if (tor_addr_family(addr) == AF_INET6) {
    if (router_get_my_routerinfo() && tor_addr_is_valid(router_get_my_routerinfo()->ipv6_addr, true)) {
      return tor_addr_compare_masked(addr->addr, router_get_my_routerinfo()->ipv6_addr, 48, CMP_EXACT);
    }

  }
  return 0;
}

Here is the error I am getting:

tor ) make test-network
$CHUTNEY_PATH was not set.
Assuming test-network.sh will find ./../chutney
  CC       src/app/config/src_core_libtor_app_testing_a-config.o
src/app/config/config.c: In function ‘is_local_addr__real’:
src/app/config/config.c:3024:83: error: dereferencing pointer to incomplete type ‘routerinfo_t {aka const struct routerinfo_t}’
 _addr_is_valid(router_get_my_routerinfo()->ipv6_addr, true)) {
                                          ^~
src/app/config/config.c:3025:38: error: incompatible type for argument 1 of ‘tor_addr_compare_masked’
       return tor_addr_compare_masked(addr->addr, router_get_my_routerinfo()->ipv6_addr, 48, CMP_EXACT);
                                      ^~~~
In file included from ./src/core/or/or.h:53:0,
                 from src/app/config/config.c:65:
./src/lib/net/address.h:249:5: note: expected ‘const tor_addr_t * {aka const struct tor_addr_t *}’ but argument is of type ‘const union <anonymous>’
 int tor_addr_compare_masked(const tor_addr_t *addr1, const tor_addr_t *addr2,
     ^~~~~~~~~~~~~~~~~~~~~~~
Makefile:13398: recipe for target 'src/app/config/src_core_libtor_app_testing_a-config.o' failed
make[1]: *** [src/app/config/src_core_libtor_app_testing_a-config.o] Error 1
Makefile:21570: recipe for target 'test-network' failed
make: *** [test-network] Error 2

Looks like the only declaration I have found for router_get_my_routerinfo was this code in router.h:

MOCK_DECL(const routerinfo_t *, router_get_my_routerinfo, (void));
MOCK_DECL(const routerinfo_t *, router_get_my_routerinfo_with_err,(int *err));

I'm not sure what to do next.

comment:6 Changed 2 months ago by nickm

"dereferencing pointer to incomplete type" means that there isn't a definition for the structure in scope. you might need to add an #include for the header that defines routerinfo_t. That's in a header called feature/nodelist/routerinfo_st.h

comment:7 Changed 2 months ago by kimimaro

current code changes in config.c

// includes
#include "feature/relay/router.h"
#include "feature/nodelist/routerinfo_st.h"

// modified function
/** Return true iff <b>addr</b> is judged to be on the same network as us, or
 * on a private network.
 */
MOCK_IMPL(int,
is_local_addr, (const tor_addr_t *addr))
{
  /* Check for an internal IPv4 or IPv6 address */
  if (tor_addr_is_internal(addr, 0))
    return 1;

  /* Check whether ip is on the same /24 as we are. */
  if (get_options()->EnforceDistinctSubnets == 0)
    return 0;
  if (tor_addr_family(addr) == AF_INET) {
    uint32_t ip = tor_addr_to_ipv4h(addr);

    /* It's possible that this next check will hit before the first time
     * resolve_my_address actually succeeds.  (For clients, it is likely that
     * resolve_my_address will never be called at all).  In those cases,
     * last_resolved_addr will be 0, and so checking to see whether ip is on
     * the same /24 as last_resolved_addr will be the same as checking whether
     * it was on net 0, which is already done by tor_addr_is_internal.
     */
    if ((last_resolved_addr & (uint32_t)0xffffff00ul)
        == (ip & (uint32_t)0xffffff00ul))
      return 1;
  }

  /* Check for the same IPv6 /48 as the directory server */
  if (tor_addr_family(addr) == AF_INET6) {
    if (router_get_my_routerinfo() && tor_addr_is_valid(&(router_get_my_routerinfo()->ipv6_addr), true)) {
      return tor_addr_compare_masked(addr, &(router_get_my_routerinfo()->ipv6_addr), 48, CMP_EXACT);
    }

  }
  return 0;
}

The error is quite interesting now:

tor ) make test-network
$CHUTNEY_PATH was not set.
Assuming test-network.sh will find ./../chutney
Running IPv4 flavors: bridges+hs-v23.
ping6 ::1 or ping ::1 succeeded, running IPv6 flavors: single-onion-v23-ipv6-md.
FAIL: bridges+hs-v23
cat: '/home/seneca/Documents/outreachy/tor/../chutney/net/nodes/*/info.log': No such file or directory
FAIL: single-onion-v23-ipv6-md
cat: '/home/seneca/Documents/outreachy/tor/../chutney/net/nodes/*/info.log': No such file or directory
Log and result files are available in ./test_network_log.
Makefile:21723: recipe for target 'test-network-results' failed
make[1]: *** [test-network-results] Error 1
Makefile:21570: recipe for target 'test-network' failed
make: *** [test-network] Error 2

tor ) wc -l test_network_log/*
   670 test_network_log/bridges+hs-v23.log
     4 test_network_log/bridges+hs-v23.trs
   599 test_network_log/single-onion-v23-ipv6-md.log
     4 test_network_log/single-onion-v23-ipv6-md.trs
  1277 total

Please tell me if you want me to attach the logs.

Last edited 2 months ago by kimimaro (previous) (diff)

comment:8 Changed 2 months ago by MrSquanchee

If I may help, I can see that tor_addr_is_valid(const tor_addr_t *addr, int for_listening) does not check for ipv6 addresses here: https://github.com/torproject/tor/blob/686494f0f71b9235399b8241aba3e0c2fcb03ea1/src/lib/net/address.c#L831.
You can check the addresses validity manually.

int tor_addr_ipv6_is_valid(const tor_addr_t *addr){
    if (! addr->sa_family_t == AF_INET6)
        return 0;
    const uint32_t *a32 = tor_addr_to_in6_addr32(addr);
    iph6[0] = ntohl(a32[0]);
    iph6[1] = ntohl(a32[1]);
    iph6[2] = ntohl(a32[2]);
    iph6[3] = ntohl(a32[3]);
    if (!iph6[0] && !iph6[1] && !iph6[2] && !iph6[3]) /* :: */
        return 0;
    return 1;
}

Hope this helps.

comment:9 Changed 2 months ago by neel

Cc: neel added
Owner: set to neel
Status: newassigned

comment:10 Changed 2 months ago by teor

Owner: changed from neel to kimimaro

Hi Neel, please check if other people are working on tickets before you assign them to yourself. (And please only assign yourself tickets if you're actively working on them.)

kimimaro is working on this ticket right now.

comment:11 in reply to:  8 Changed 2 months ago by teor

Replying to MrSquanchee:

If I may help, I can see that tor_addr_is_valid(const tor_addr_t *addr, int for_listening) does not check for ipv6 addresses here: https://github.com/torproject/tor/blob/686494f0f71b9235399b8241aba3e0c2fcb03ea1/src/lib/net/address.c#L831.

That's true, but we should open another ticket to fix the issue with tor_addr_is_valid(). Because that issue is not relevant to this ticket.

Here's why:

It doesn't make any sense to compare an all-zeroes IPv6 address to see if it's on the same network as another address. So we should pass "false" for "for_listening".

And all-zeroes addresses never reach this code, because they are excluded by tor_addr_is_internal() at the start of the function.

comment:12 Changed 2 months ago by teor

I opened #33679 for that bug in tor_addr_is_valid(). Good catch!

comment:13 in reply to:  7 Changed 2 months ago by teor

Replying to kimimaro:

current code changes in config.c

The error is quite interesting now:

tor ) make test-network
$CHUTNEY_PATH was not set.
Assuming test-network.sh will find ./../chutney
Running IPv4 flavors: bridges+hs-v23.
ping6 ::1 or ping ::1 succeeded, running IPv6 flavors: single-onion-v23-ipv6-md.
FAIL: bridges+hs-v23
cat: '/home/seneca/Documents/outreachy/tor/../chutney/net/nodes/*/info.log': No such file or directory
FAIL: single-onion-v23-ipv6-md
cat: '/home/seneca/Documents/outreachy/tor/../chutney/net/nodes/*/info.log': No such file or directory
Log and result files are available in ./test_network_log.
Makefile:21723: recipe for target 'test-network-results' failed
make[1]: *** [test-network-results] Error 1
Makefile:21570: recipe for target 'test-network' failed
make: *** [test-network] Error 2

tor ) wc -l test_network_log/*
   670 test_network_log/bridges+hs-v23.log
     4 test_network_log/bridges+hs-v23.trs
   599 test_network_log/single-onion-v23-ipv6-md.log
     4 test_network_log/single-onion-v23-ipv6-md.trs
  1277 total

Please tell me if you want me to attach the logs.

Yes, we need to see the detailed logs to help you.

It looks like tor is crashing, but I can't see why that's happening.

Can you submit your code as a git pull request to:
https://github.com/torproject/tor

Then we can be sure we are all seeing the same code. And we also have continuous integration (CI) set up to test the code. So we will all see the same logs. And the same code reviews :-)

comment:14 Changed 2 months ago by teor

Thanks for the pull request, I made some comments asking for changes.

comment:15 Changed 2 months ago by teor

comment:16 Changed 2 months ago by teor

comment:17 Changed 2 months ago by teor

I reviewed the changes, and gave some examples of similar tests,

comment:18 Changed 2 months ago by teor

Status: assignedneeds_review

Hi, just letting you know that I've seen your changes, and I'll try to review them over the next day or so.

comment:19 Changed 2 months ago by dgoulet

Reviewer: teor

comment:20 Changed 2 months ago by teor

Status: needs_reviewneeds_revision

There were some bugs in the new code, that were a bit tricky to fix.

So I made a new pull request, and added some changes:
https://github.com/torproject/tor/pull/1849#issuecomment-606973738

I also fixed some bugs in our old tests, that were making it harder to write this code.

Can you try writing unit tests now?

comment:21 Changed 7 weeks ago by teor

How are the unit tests going here?

comment:22 Changed 6 weeks ago by kimimaro

Here's the main guidelines I am using with regards to writing the unit tests: https://github.com/torproject/tor/pull/1825#issuecomment-604905534

I have a question. Is this what I'm supposed to write in test_config.c? This is just a prototype of course.

static void
test_config_is_local_addr_impl(void *arg)
{
  (void)arg;
  const routerinfo_t *my_routerinfo = router_get_my_routerinfo();
  const tor_addr_t *my_ipv6_addr = NULL;

  if (my_routerinfo) {
    my_ipv6_addr = &my_routerinfo->ipv6_addr;
  }

  // add test cases here like https://github.com/torproject/tor/blob/384a771fccb0b0ab8b95122815b33ef26bf042e8/src/test/test_addr.c#L345

  done:
    // free memory
}

// add this function into config_tests_t[]

I did read tinytest's API, in case you are wondering.

Last edited 6 weeks ago by kimimaro (previous) (diff)

comment:23 Changed 5 weeks ago by nickm

Hm, that looks like it could be a start, but there's going to be a problem to the extent that router_get_my_routerinfo() might not actually work from the unit tests: it requires that there actually exists a routerinfo, which clients don't have. Maybe there is a better way to get one or more ipv6 addresses to test with -- how about tor_addr_parse()?

Note: See TracTickets for help on using tickets.