Add IPv6 Support to is_local_addr()

changed milestone to %Tor: 0.4.4.x-final

added component::core tor/tor ipv6 milestone::Tor: 0.4.4.x-final outreachy-ipv6 owner::kimimaro points::1 priority::medium prop312 reporter::kimimaro reviewer::teor severity::normal sponsor::55-can status::needs-revision type::enhancement labels

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?

Trac:
Username: kimimaro

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?

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

Trac:
Sponsor: N/A to Sponsor55-can
Milestone: N/A to Tor: 0.4.4.x-final
Description: " 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). See also the next section, which uses IPv6 /64 for sybils." Source: https://gitweb.torproject.org/torspec.git/tree/proposals/312-relay-auto-ipv6-addr.txt#n1099

to

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
Keywords: outreachy-ipv6 ipv6 deleted, outreachy-ipv6 ipv6 prop312 added
Points: N/A to 1

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

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.

Trac:
Username: kimimaro

"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

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.

Trac:
Username: kimimaro

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.

Trac:
Cc: N/A to neel
Owner: N/A to neel
Status: new to assigned

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.

Trac:
Owner: neel to kimimaro

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.

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

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 :-)

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

Oops, the pull request is: https://github.com/torproject/tor/pull/1825

I suggested a way we could write tests at: https://github.com/torproject/tor/pull/1825#issuecomment-602005587

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

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

Trac:
Status: assigned to needs_review

Trac:
Reviewer: N/A to teor

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?

Trac:
Status: needs_review to needs_revision

How are the unit tests going here?

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.

Trac:
Username: kimimaro

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()?

changed time estimate to 8h

moved to tpo/core/tor#33618 (closed)

Add IPv6 Support to is_local_addr()

Child items 0

Activity