Opened 14 months ago

Last modified 21 hours ago

#23588 needs_review enhancement

Write fascist_firewall_choose_address_ls() and use it in hs_get_extend_info_from_lspecs()

Reported by: teor Owned by: neel
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: prop224, tor-hs, single-onion, ipv6, 034-triage-20180328, 034-removed-20180328
Cc: neel@…, nickm Actual Points:
Parent ID: #23493 Points: 1
Reviewer: teor Sponsor:

Description

Currently, the address choice logic is:

  • if we have an IPv6 address and can reach the ls IPv6 address, and prefer IPv6, use it
  • if we have an IPv4 address and can reach the ls IPv4 address, use it

But it needs to be:

  • if we have both addresses and can reach both, then use whatever we prefer
  • if we have one address and can reach it, use it

This doesn't matter until clients put IPv6 addresses in the link specifier.

Child Tickets

TicketTypeStatusOwnerSummary
#24193enhancementcloseddgouletMake v3 single onion services parse and use IPv6 introduce link specifiers
#27086defectassignedneelWrite unit tests for fascist_firewall_choose_address_ls() and hs_get_extend_info_from_lspecs()

Change History (65)

comment:1 Changed 14 months ago by teor

These tickets aren't urgent, and they involve major refactoring.
Deferring to 0.3.3

comment:2 Changed 13 months ago by teor

In my commit 20b0e9e07d in my branch bug23820_032 (#23820), I ripped out IPv6 support in hs_get_extend_info_from_lspecs(). We'll need to revert that commit, and then fix the issues in the function.

Remove buggy IPv6 support from hs_get_extend_info_from_lspecs()

The previous version of this function has the following issues:
* it doesn't choose between IPv4 and IPv6 addresses correctly, and
* it doesn't fall back to a 3-hop path when the address for a direct
  connection is unreachable.
But we can't fix these things in a bugfix release.

Instead, treat IPv6 addresses like any other unrecognised link specifier
and ignore them. If there is no IPv4 address, return NULL.

comment:3 Changed 10 months ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final

Defer a small handful of non-enhancement "new" tickets to 0.3.4

comment:4 Changed 8 months ago by nickm

Keywords: 034-triage-20180328 added

comment:5 Changed 8 months ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:6 Changed 8 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: unspecified

These tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if time permits.

comment:7 Changed 6 months ago by neel

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

comment:8 Changed 4 months ago by neel

In ed25519_cert.h, link_specifier_t/link_specifier_st looks like this:

struct link_specifier_st {
  uint8_t ls_type;
  uint8_t ls_len;
  uint32_t un_ipv4_addr;
  uint16_t un_ipv4_port;
  uint8_t un_ipv6_addr[16];
  uint16_t un_ipv6_port;
  uint8_t un_legacy_id[20];
  uint8_t un_ed25519_id[32];
  TRUNNEL_DYNARRAY_HEAD(, uint8_t) un_unrecognized;
  uint8_t trunnel_error_code_;
};

But the fascist_firewall_choose_address_* family of functions defines DirPorts in the function, like this:

static void
fascist_firewall_choose_address_base(const tor_addr_t *ipv4_addr,
                                     uint16_t ipv4_orport,
                                     uint16_t ipv4_dirport,
                                     const tor_addr_t *ipv6_addr,
                                     uint16_t ipv6_orport,
                                     uint16_t ipv6_dirport,
                                     firewall_connection_t fw_connection,
                                     int pref_only,
                                     int pref_ipv6,
                                     tor_addr_port_t* ap)

Should I then assume the ipv4_dirport and ipv6_dirport is 0?

comment:9 in reply to:  8 Changed 4 months ago by teor

Replying to neel:

In ed25519_cert.h, link_specifier_t/link_specifier_st looks like this:

struct link_specifier_st {
  uint8_t ls_type;
  uint8_t ls_len;
  uint32_t un_ipv4_addr;
  uint16_t un_ipv4_port;
  uint8_t un_ipv6_addr[16];
  uint16_t un_ipv6_port;
  uint8_t un_legacy_id[20];
  uint8_t un_ed25519_id[32];
  TRUNNEL_DYNARRAY_HEAD(, uint8_t) un_unrecognized;
  uint8_t trunnel_error_code_;
};

But the fascist_firewall_choose_address_* family of functions defines DirPorts in the function, like this:

static void
fascist_firewall_choose_address_base(const tor_addr_t *ipv4_addr,
                                     uint16_t ipv4_orport,
                                     uint16_t ipv4_dirport,
                                     const tor_addr_t *ipv6_addr,
                                     uint16_t ipv6_orport,
                                     uint16_t ipv6_dirport,
                                     firewall_connection_t fw_connection,
                                     int pref_only,
                                     int pref_ipv6,
                                     tor_addr_port_t* ap)

Should I then assume the ipv4_dirport and ipv6_dirport is 0?

Yes, link specifiers are only defined for ORPorts.
Also, #24732 will remove the IPv6 DirPort code, because it's unused.

comment:11 in reply to:  10 Changed 4 months ago by teor

Milestone: Tor: unspecifiedTor: 0.3.5.x-final
Status: assignedneeds_revision

Replying to neel:

My PR is here: https://github.com/torproject/tor/pull/252

Thanks for this patch!

The unit tests fail in the new function:
https://travis-ci.org/torproject/tor/jobs/409667005#L3572

Please add new commits to the branch to make them pass.

You can run the unit tests before you push your branch using:

make check

You might be able to see the CI status on:
https://github.com/torproject/tor/pull/252
https://travis-ci.org/torproject/tor
https://ci.appveyor.com/project/torproject/tor

If you can't, let us know, and we'll work out how to get you access.

comment:12 Changed 4 months ago by teor

The Appveyor Windows CI gives a good error:

bash.exe : In file included from ../src/core/or/or.h:51:0,
At line:2 char:5
+     & $commandPath $args 2>&1
+     ~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (In file include...e/or/or.h:51:0,:String) [], RemoteException
    + FullyQualifiedErrorId : NativeCommandError
 
                 from ../src/feature/hs/hs_common.c:14:
../src/feature/hs/hs_common.c: In function 'hs_get_extend_info_from_lspecs':
../src/lib/net/address.h:308:49: error: null pointer dereference [-Werror=null-dereference]
         tor_addr_port_is_valid(&(ap)->addr, (ap)->port, (for_listening))
                                                 ^
../src/lib/net/address.h:305:29: note: in definition of macro 'tor_addr_port_is_valid'
          tor_port_is_valid((port), (for_listening)))
                             ^~~~
../src/feature/hs/hs_common.c:1720:8: note: in expansion of macro 'tor_addr_port_is_valid_ap'
   if (!tor_addr_port_is_valid_ap(ap, 0) || !have_legacy_id) {
        ^~~~~~~~~~~~~~~~~~~~~~~~~
../src/feature/hs/hs_common.c:1729:64: error: null pointer dereference [-Werror=null-dereference]
              fascist_firewall_allows_address_addr(&ap->addr, ap->port,
                                                              ~~^~~~~~
In file included from ../src/lib/crypt_ops/crypto_rsa.h:21:0,
                 from ../src/lib/crypt_ops/crypto.h:20,
                 from ../src/core/or/or.h:30,
                 from ../src/feature/hs/hs_common.c:14:
../src/feature/hs/hs_common.c:1748:48: error: null pointer dereference [-Werror=null-dereference]
            "it: %s:%u", fmt_addr(&ap->addr), ap->port);
                                              ~~^~~
../src/lib/log/log.h:250:51: note: in definition of macro 'log_fn'
   log_fn_(severity, domain, __FUNCTION__, args, ##__VA_ARGS__)
                                                   ^~~~~~~~~~~
../src/feature/hs/hs_common.c:1755:50: error: null pointer dereference [-Werror=null-dereference]
                          onion_key, &ap->addr, ap->port);
                                                ~~^~~~~~
cc1.exe: all warnings being treated as errors
make: *** [Makefile:8173: src/feature/hs/hs_common.o] Error 1
make: *** Waiting for unfinished jobs....

https://ci.appveyor.com/project/torproject/tor/build/job/7isiro2d4mryqtr0#L1825

comment:13 Changed 4 months ago by neel

I have fixed these issues. The same PR is used for this new patch: https://github.com/torproject/tor/pull/252

comment:14 Changed 4 months ago by teor

Reviewer: teor
Status: needs_revisionneeds_review

Appveyor builds on the latest master are failing due to #26986, so I merged the fix to b23588 and pushed to https://github.com/teor2345/tor.git

I'll review your branch once the CI succeeds.

comment:15 Changed 4 months ago by teor

Status: needs_reviewneeds_revision

Hi Neel, thanks for this pull request.

I did a review on the pull request on github:
https://github.com/torproject/tor/pull/252

comment:16 Changed 4 months ago by neel

Last edited 4 months ago by neel (previous) (diff)

comment:17 Changed 4 months ago by teor

Please don't open a new pull request for every branch. New pull requests don't have the comments from old pull requests.

Instead, add new commits to the existing branch, then push that branch. Then the pull request will update with the new commits.

comment:18 Changed 4 months ago by neel

Thank you for the advice. I will follow this for future patches.

However, could you still review the patch even with the "new" pull request?

comment:19 in reply to:  18 Changed 4 months ago by teor

Replying to neel:

Thank you for the advice. I will follow this for future patches.

However, could you still review the patch even with the "new" pull request?

Yes, of course. It might take me a few days.

I'll also try to push your new branch to the old pull request, so I can see my comments.

comment:20 Changed 4 months ago by teor

Hi, the code looks good, but a few comments need updating.

Reminder: please add extra commits to b23588a. When you push b23588a to your github, the pull request will update automatically.

comment:21 Changed 4 months ago by neel

I have updated my comments and pushed it to b23588a.

comment:22 Changed 3 months ago by teor

Thanks!

When I was reviewing your comments, I noticed that you deleted some code that we need to keep. Please fix with extra commits on that branch.

comment:23 Changed 3 months ago by neel

I have updated my code and used the same branch b23588a.

comment:24 Changed 3 months ago by teor

Thanks!

I added a commit to your branch with some more comment fixes based on:
https://github.com/torproject/tor/pull/256#discussion_r208425879
https://github.com/torproject/tor/pull/256#pullrequestreview-144234103
And I fixed another outdated function comment.

I also ran make test-network-all on the branch, using the chutney fix from #27067.

And I got the following errors:

$ make test-network-all
mkdir -p ./test_network_log
ping6 ::1 or ping ::1 succeeded, running IPv6 flavors: bridges+ipv6-min ipv6-exit-min hs-v23-ipv6-md single-onion-ipv6-md.
tor-stable found, running mixed flavors: mixed+hs-v23.
FAIL: basic-min
FAIL: bridges-min
PASS: hs-v2-min
PASS: hs-v3-min
Detail: chutney/tools/warnings.sh /Users/base/chutney/net/nodes.1533723794
Warning: tor_addr_is_null: Bug: Called with unknown address family 214 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 1
Warning: tor_addr_is_null: Bug: Called with unknown address family 32 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 1
PASS: single-onion-v23
Detail: chutney/tools/warnings.sh /Users/base/chutney/net/nodes.1533723833
Warning: tor_addr_is_null: Bug: Called with unknown address family 214 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 1
Warning: tor_addr_is_null: Bug: Called with unknown address family 32 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 1
PASS: bridges+ipv6-min
PASS: ipv6-exit-min
FAIL: hs-v23-ipv6-md
Detail: chutney/tools/warnings.sh /Users/base/chutney/net/nodes.1533723949
Warning: Bug:     0   tor                                 0x000000010bdb0a2c log_backtrace_impl + 76 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     1   tor                                 0x000000010bda1177 tor_bug_occurred_ + 503 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     10  tor                                 0x000000010b83fc17 do_main_loop + 2423 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     11  tor                                 0x000000010b8445fb tor_run_main + 779 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     12  tor                                 0x000000010b9a54a1 tor_main + 161 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     13  tor                                 0x000000010b7a039b main + 27 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     14  libdyld.dylib                       0x00007fff54fbe015 start + 1 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     2   tor                                 0x000000010bac2f6e hs_client_dir_info_changed + 206 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     3   tor                                 0x000000010bb16b9c networkstatus_set_current_consensus + 9740 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     4   tor                                 0x000000010ba59e11 connection_dir_client_reached_eof + 20913 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     5   tor                                 0x000000010ba54a1f connection_dir_reached_eof + 143 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     6   tor                                 0x000000010b8210d4 connection_handle_read + 11668 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     7   tor                                 0x000000010b836938 conn_read_callback + 88 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     8   libevent-2.1.6.dylib                0x000000010c7839c2 event_process_active_single_queue + 1057 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     9   libevent-2.1.6.dylib                0x000000010c780cb3 event_base_loop + 1074 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug: Non-fatal assertion !(status == HS_CLIENT_FETCH_HAVE_DESC) failed in retry_all_socks_conn_waiting_for_desc at ../src/feature/hs/hs_client.c:275. Stack trace: (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Every introduction point for service ggdz6i6fshpuszndyqu373r7vzo2kltoofydyie2mtpv4rketkixxgad is unusable or we can't extend to it. We can't connect. Number: 7
Warning: Not enough info to open a circuit to a rendezvous point for hidden service ggdz6i6fshpuszndyqu373r7vzo2kltoofydyie2mtpv4rketkixxgad. Number: 1
Warning: tor_addr_is_null: Bug: Called with unknown address family 214 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 43
Warning: tor_addr_is_null: Bug: Called with unknown address family 32 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: tor_bug_occurred_: Bug: ../src/feature/hs/hs_client.c:275: retry_all_socks_conn_waiting_for_desc: Non-fatal assertion !(status == HS_CLIENT_FETCH_HAVE_DESC) failed. (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
PASS: single-onion-ipv6-md
PASS: mixed+hs-v23
Log and result files are available in ./test_network_log.
make: *** [test-network-all] Error 1
Exit 2

But Tor master passes make test-network-all.

I think that these errors happen because you're using tor_addr_t's without initialising them. See my comments on the pull request.

comment:25 Changed 3 months ago by teor

I think that these errors happen because you're using tor_addr_t's without initialising them.

If fascist_firewall_choose_address_ls() and hs_get_extend_info_from_lspecs() had unit tests, we would have discovered these bugs sooner.

Would you like to write unit tests?
If not, someone will write tests eventually.

comment:26 Changed 3 months ago by neel

I have pushed new commits.

I do not want to write a unit test (for now at least).

comment:27 in reply to:  26 Changed 3 months ago by teor

Replying to neel:

I have pushed new commits.

I do not want to write a unit test (for now at least).

Ok, I opened #27086 for the unit tests.

comment:28 Changed 3 months ago by teor

I ran make test-network-all on this branch, using the latest chutney master. I am seeing some tests pass, but some failures from this branch. I also saw some HS failures (#24610) and bridge failures (#27080).

So I am not sure if the code in https://github.com/torproject/tor/pull/256 works. It also doesn't have any unit tests. (All the other fascist_firewall_choose_address*() functions have unit tests.)

We need to be careful about our address parsing code, because it is a source of security and anonymity issues.

I don't think we can merge this branch until the bugs are fixed:

$ make test-network-all
mkdir -p ./test_network_log
ping6 ::1 or ping ::1 succeeded, running IPv6 flavors: bridges+ipv6-min ipv6-exit-min hs-v23-ipv6-md single-onion-ipv6-md.
tor-stable found, running mixed flavors: mixed+hs-v23.
PASS: basic-min
FAIL: bridges-min
PASS: hs-v2-min
PASS: hs-v3-min
PASS: single-onion-v23
FAIL: bridges+ipv6-min
FAIL: ipv6-exit-min
FAIL: hs-v23-ipv6-md
Detail: chutney/tools/warnings.sh /Users/base/chutney/net/nodes.1533778119
Warning: Bug:     0   tor                                 0x0000000100c53a2c log_backtrace_impl + 76 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     1   tor                                 0x0000000100c44177 tor_bug_occurred_ + 503 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     10  tor                                 0x00000001006e2c57 do_main_loop + 2423 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     11  tor                                 0x00000001006e763b tor_run_main + 779 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     12  tor                                 0x0000000100848451 tor_main + 161 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     13  tor                                 0x00000001006433db main + 27 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     14  libdyld.dylib                       0x00007fff54fbe015 start + 1 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     2   tor                                 0x0000000100965f1e hs_client_dir_info_changed + 206 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     3   tor                                 0x00000001009b9b9c networkstatus_set_current_consensus + 9740 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     4   tor                                 0x00000001008fcdc1 connection_dir_client_reached_eof + 20913 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     5   tor                                 0x00000001008f79cf connection_dir_reached_eof + 143 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     6   tor                                 0x00000001006c4114 connection_handle_read + 11668 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     7   tor                                 0x00000001006d9978 conn_read_callback + 88 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     8   libevent-2.1.6.dylib                0x00000001016269c2 event_process_active_single_queue + 1057 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     9   libevent-2.1.6.dylib                0x0000000101623cb3 event_base_loop + 1074 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug: Non-fatal assertion !(status == HS_CLIENT_FETCH_HAVE_DESC) failed in retry_all_socks_conn_waiting_for_desc at ../src/feature/hs/hs_client.c:275. Stack trace: (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Every introduction point for service p2qpw3p725i6dspcw5yxy7zltrqulp4dnspfza3quiasro3rgx2yabid is unusable or we can't extend to it. We can't connect. Number: 8
Warning: Not enough info to open a circuit to a rendezvous point for hidden service p2qpw3p725i6dspcw5yxy7zltrqulp4dnspfza3quiasro3rgx2yabid. Number: 1
Warning: tor_bug_occurred_: Bug: ../src/feature/hs/hs_client.c:275: retry_all_socks_conn_waiting_for_desc: Non-fatal assertion !(status == HS_CLIENT_FETCH_HAVE_DESC) failed. (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
PASS: single-onion-ipv6-md
PASS: mixed+hs-v23
Detail: chutney/tools/warnings.sh /Users/base/chutney/net/nodes.1533778259
Warning: Unable to add signatures to consensus: Mismatched digest. Number: 20
Log and result files are available in ./test_network_log.

Edit: typo

Last edited 3 months ago by teor (previous) (diff)

comment:29 Changed 3 months ago by neel

Surprisingly the code did not crash on my laptop (FreeBSD 12), but did on my desktop (FreeBSD 12 again) and on a VM (Debian 9).

I figured out how to get fix the bug, however, with a catch: instead of using fascist_firewall_choose_address_base() to choose the address in fascist_firewall_choose_address_ls(), I do it manually with:

  if (have_v6 && (pref_ipv6 || !have_v4)) {
    tor_addr_copy(&ap->addr, &addr_v6);
    ap->port = port_v6;
  } else if (have_v4 && (!pref_ipv6 || !have_v6)) {
    tor_addr_copy(&ap->addr, &addr_v4);
    ap->port = port_v4;
  } 

Would this be okay?

This code goes in place of fascist_firewall_choose_address_base() in fascist_firewall_choose_address_ls(). This passes make test-network-all.

These changes are now pushed. A copy old branch (before the change) is here: https://github.com/neelchauhan/tor/tree/b23588a_old

UPDATE: Make comment more descriptive and announce the push.

Last edited 3 months ago by neel (previous) (diff)

comment:30 Changed 3 months ago by teor

We try not to duplicate code. I think your new patch is much simpler than fascist_firewall_choose_address_base(), because it doesn't actually choose addresses using the firewall rules on the client.

Here's what I suggest:

  • If fascist_firewall_choose_address_base() doesn't work, then we should fix it.
  • If fascist_firewall_choose_address_ls() is calling fascist_firewall_choose_address_base() wrong, then we should fix the call.

Also, I don't understand what you mean by:

We compare
addresses manually here as fascist_firewall_choose_address_base()
doesn't always return addresses given from lspecs.

Can you explain what behaviour you need from fascist_firewall_choose_address_base(), and what is happening instead?

I think you might be expecting fascist_firewall_choose_address_base() to always return a valid address. But sometimes, there won't be any reachable addresses. If there aren't any reachable addresses, we need the 3-hop fallback code, which hasn't been written yet.

But localhost (127.0.0.1 and ::1) should be reachable from the clients in the chutney networks. So I'm not sure why we're seeing this bug in chutney.

Maybe some unit tests could help track it down.

comment:31 in reply to:  30 ; Changed 3 months ago by teor

Replying to teor:

I think you might be expecting fascist_firewall_choose_address_base() to always return a valid address. But sometimes, there won't be any reachable addresses. If there aren't any reachable addresses, we need the 3-hop fallback code, which hasn't been written yet.

You can log a BUG() warning and fail the connection when there's no reachable address. We can write the 3-hop fallback code later in #23818.

comment:32 in reply to:  31 Changed 3 months ago by neel

Replying to teor:

Replying to teor:

I think you might be expecting fascist_firewall_choose_address_base() to always return a valid address. But sometimes, there won't be any reachable addresses. If there aren't any reachable addresses, we need the 3-hop fallback code, which hasn't been written yet.

You can log a BUG() warning and fail the connection when there's no reachable address. We can write the 3-hop fallback code later in #23818.

I plan to do this after having reverted my address choosing code and switched back to fascist_firewall_choose_address_base().

I do have a question: where should I put the BUG() warning when there's no reachable address?

UPDATE: Fix spelling.

Last edited 3 months ago by neel (previous) (diff)

comment:33 Changed 3 months ago by teor

I do have a question: where should I put the BUG() warning when there's no reachable address?

hs_get_extend_info_from_lspecs() is a good place to check if the address and port returned by fascist_firewall_choose_address_ls() are valid. (You can use tor_addr_port_is_valid() to check ap.)

In future, when we make other functions call hs_get_extend_info_from_lspecs(), they might want to do different checks.

comment:34 Changed 3 months ago by neel

I tried using BUG() here:

neel@kat:~/tor/tor % git diff
diff --git a/src/feature/hs/hs_common.c b/src/feature/hs/hs_common.c
index df43d5cde..35e5d9ac1 100644
--- a/src/feature/hs/hs_common.c
+++ b/src/feature/hs/hs_common.c
@@ -1724,7 +1724,7 @@ hs_get_extend_info_from_lspecs(const smartlist_t *lspecs,
   fascist_firewall_choose_address_ls(lspecs, 0, &ap, direct_conn);
 
   /* Legacy ID is mandatory, and we require an IP address. */
-  if (!tor_addr_port_is_valid_ap(&ap, 0) || !have_legacy_id) {
+  if (BUG(!tor_addr_port_is_valid_ap(&ap, 0)) || !have_legacy_id) {
     /* If we're missing the legacy ID or the IP address, return NULL. */
     goto done;
   }
neel@kat:~/tor/tor %

I compiled the code and ran make test-network-all and got this:

neel@kat:~/tor/tor % make test-network-all                                      
mkdir -p ./test_network_log                                                     
ping6 ::1 or ping ::1 succeeded, running IPv6 flavors: bridges+ipv6-min ipv6-exi
t-min hs-v23-ipv6-md  single-onion-ipv6-md.                                     
tor-stable not found, skipping mixed flavors: mixed+hs-v23.                     
SKIP: mixed+hs-v23                                                              
PASS: basic-min                                                                 
FAIL: bridges-min                                                               
PASS: hs-v2-min                                                                 
PASS: hs-v3-min                                                                 
PASS: single-onion-v23                                                          
FAIL: bridges+ipv6-min                                                          
PASS: ipv6-exit-min                                                             
FAIL: hs-v23-ipv6-md                                                            
Detail: chutney/tools/warnings.sh /usr/home/neel/tor/chutney//net/nodes.1534250$
96                                                                              
Warning: Bug:     0x107a055 <_start+0xa5> at /usr/home/neel/tor/tor/src/app/tor 
(on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 29                          
Warning: Bug:     0x107a249 <main+0x19> at /usr/home/neel/tor/tor/src/app/tor ($
n Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 29
Warning: Bug:     0x107a3ac <tor_main+0x4c> at /usr/home/neel/tor/tor/src/app/t$
r (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 29
Warning: Bug:     0x107a6b4 <connection_add_impl+0x1f4> at /usr/home/neel/tor/to
r/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 5             
Warning: Bug:     0x107d1e1 <do_main_loop+0x421> at /usr/home/neel/tor/tor/src/a
pp/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 29                   
Warning: Bug:     0x107ef90 <tor_run_main+0xe0> at /usr/home/neel/tor/tor/src/ap
p/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 29                    
Warning: Bug:     0x10a580b <connection_ap_handshake_attach_circuit+0x46b> at /u
sr/home/neel/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Num
ber: 24                                                                         
Warning: Bug:     0x10a6168 <connection_ap_handshake_attach_circuit+0xdc8> at /u
sr/home/neel/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Num
ber: 24                                                                         
Warning: Bug:     0x10a799f <command_process_cell+0x72f> at /usr/home/neel/tor/t
or/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 1            
Warning: Bug:     0x10ab0ea <connection_ap_attach_pending+0xda> at /usr/home/nee
l/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 24    
Warning: Bug:     0x10b0c72 <connection_or_process_inbuf+0x1b2> at /usr/home/nee
l/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 1     
Warning: Bug:     0x10c0059 <circuit_receive_relay_cell+0x349> at /usr/home/neel
/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 1
Warning: Bug:     0x10c05c2 <circuit_receive_relay_cell+0x8b2> at /usr/home/neel
/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 1      
Warning: Bug:     0x10f4b3e <connection_dir_reached_eof+0x2e> at /usr/home/neel/
tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 4       
Warning: Bug:     0x10f6ce8 <connection_dir_reached_eof+0x21d8> at /usr/home/nee
l/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 4     
Warning: Bug:     0x110a125 <hs_circ_handle_introduce2+0x155> at /usr/home/neel/
tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 1       
Warning: Bug:     0x110e0a1 <hs_client_get_random_intro_from_edge+0x211> at /usr
/home/neel/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Numbe
r: 24                                                                           
Warning: Bug:     0x110ed8e <hs_client_dir_info_changed+0x5e> at /usr/home/neel/
tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 4       
Warning: Bug:     0x1111cfc <hs_get_extend_info_from_lspecs+0x21c> at /usr/home/
neel/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 25 
Warning: Bug:     0x1118cac <hs_service_receive_introduce2+0xbc> at /usr/home/ne
el/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 1    
Warning: Bug:     0x11222b4 <networkstatus_set_current_consensus+0xe04> at /usr/
home/neel/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number
: 4                                                                             
Warning: Bug:     0x116081d <rend_process_relay_cell+0x18d> at /usr/home/neel/to
r/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 1 
Warning: Bug:     0x11a1ce3 <connection_handle_read+0x8a3> at /usr/home/neel/tor
/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 1          
Warning: Bug:     0x11a1f97 <connection_handle_read+0xb57> at /usr/home/neel/tor
/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 4          
Warning: Bug:     0x120fcf1 <tor_bug_occurred_+0x111> at /usr/home/neel/tor/tor/
src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 29              
Warning: Bug:     0x121370c <log_backtrace_impl+0x4c> at /usr/home/neel/tor/tor/
src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 29              
Warning: Bug:     0x801b54f4e <event_base_loop+0x50e> at /usr/local/lib/libevent
-2.1.so.6 (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 29                
Warning: Bug:     0x801b58da0 <event_base_assert_ok_nolock_+0x9d0> at /usr/local
/lib/libevent-2.1.so.6 (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 24   
Warning: Bug:     0x801b5906f <event_base_assert_ok_nolock_+0xc9f> at /usr/local
/lib/libevent-2.1.so.6 (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 5    
Warning: Bug: Non-fatal assertion !(!tor_addr_port_is_valid_ap(&ap, 0)) failed i
n hs_get_extend_info_from_lspecs at src/feature/hs/hs_common.c:1727. Stack trace
: (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 25                        
Warning: Bug: Non-fatal assertion !(status == HS_CLIENT_FETCH_HAVE_DESC) failed 
in retry_all_socks_conn_waiting_for_desc at src/feature/hs/hs_client.c:275. Stac
k trace: (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 4
Warning: Didn't recognize cell, but circ stops here! Closing circ. Number: 1    
Warning: Every introduction point for service cuq3slyq45o3smrd4chykbhklcltdfbg63
xopsezrwrxrfi3hkbtznad is unusable or we can't extend to it. We can't connect. N
umber: 8                                                                        
Warning: Not enough info to open a circuit to a rendezvous point for hidden serv
ice cuq3slyq45o3smrd4chykbhklcltdfbg63xopsezrwrxrfi3hkbtznad. Number: 1         
Warning: tor_bug_occurred_: Bug: src/feature/hs/hs_client.c:275: retry_all_socks
_conn_waiting_for_desc: Non-fatal assertion !(status == HS_CLIENT_FETCH_HAVE_DES
C) failed. (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 4                
Warning: tor_bug_occurred_: Bug: src/feature/hs/hs_common.c:1727: hs_get_extend_
info_from_lspecs: Non-fatal assertion !(!tor_addr_port_is_valid_ap(&ap, 0)) fail
ed. (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 25                      
PASS: single-onion-ipv6-md                                                      
Log and result files are available in ./test_network_log.                       
*** Error code 1                                                                
                                                                                
Stop.                                                                           
make: stopped in /usr/home/neel/tor/tor                                         
neel@kat:~/tor/tor %

This error is very similar to what you got. Surprisingly, I did not get this error on two laptops from different brands (I tested FreeBSD on both Dell and HP), so I am testing it on a server where the tests are more accurate.

Is the above code sample wrong (it hasn't been committed to git)? Can I fix it here, or should I do #23818 first, then update my branch for this patch with the latest Tor code, and then finish the patch?

comment:35 Changed 3 months ago by neel

Also, when this code crashes, does it mean the connection wants to fall back to a 3-hop circuit but can't (and therefore crashes)?

comment:36 Changed 3 months ago by neel

I think I know what the bug for hs-v23-ipv6-md is: some addresses are being marked as invalid as they don't make it through fascist_firewall_allows_address_ap() (I believe the main Tor codebase does not make addresses go though this now, nor does my old "simplified" address choosing code that got abandoned).

A solution can be to add a flag like verify_addr to fascist_firewall_choose_address_impl(), fascist_firewall_choose_address(), and fascist_firewall_choose_address_base() that determines whether we should verify the addresses or not, and set this flag to 0 in fascist_firewall_choose_address_ls()'s call to fascist_firewall_choose_address_base() and 1 in other calls.

In my solution, we end up choosing addresses by replacing code in fascist_firewall_choose_address_impl() from something like this:

   if (fascist_firewall_allows_address_ap(a, fw_connection, pref_only,
                                          pref_ipv6)) {

to this:

   if (fascist_firewall_allows_address_ap(a, fw_connection, pref_only,
                                          pref_ipv6) || !verify_addr) {

Would this be okay? I know this may not be a good solution, but it may help fix this bug we are talking about (and is similar to my previous address choosing code which worked, but with the fascist_firewall_allows_address_* family of functions).

I also attempted to do a three-hop fallback, and merged it with this codebase (not committed at all), but it still crashed with the same error described. It is possible that my three-hop fallback is broken.

Last edited 3 months ago by neel (previous) (diff)

comment:37 in reply to:  36 Changed 3 months ago by neel

Well, the "solution" above did not end up working (not committed, however). It still crashed.

Last edited 3 months ago by neel (previous) (diff)

comment:38 Changed 3 months ago by neel

I have a solution which should work (I tested and it does not crash), but it requires fixing #23818 first. I will upload a patch/PR for #23818 hopefully by the end of the day.

comment:39 in reply to:  38 Changed 3 months ago by neel

Replying to neel:

I have a solution which should work (I tested and it does not crash), but it requires fixing #23818 first. I will upload a patch/PR for #23818 hopefully by the end of the day.

Never mind, this patch crashes also.

comment:40 Changed 3 months ago by neel

Owner: neel deleted
Status: needs_revisionassigned

I'll just give up on this bug. It is crashing too much.

comment:41 Changed 3 months ago by neel

Status: assignednew

comment:42 Changed 3 months ago by neel

Owner: set to neel
Status: newassigned

comment:43 Changed 3 months ago by neel

Status: assignedneeds_revision

comment:44 in reply to:  36 Changed 3 months ago by teor

Replying to neel:

I think I know what the bug for hs-v23-ipv6-md is: some addresses are being marked as invalid as they don't make it through fascist_firewall_allows_address_ap() (I believe the main Tor codebase does not make addresses go though this now, nor does my old "simplified" address choosing code that got abandoned).

This issue sounds like a bug in fascist_firewall_allows_address_ap(), or a bug in how we're using fascist_firewall_allows_address_ap().

(The v2 onion service code uses fascist_firewall_allows_address_ap() and it works.)

comment:45 Changed 3 months ago by neel

I looked closer and after inserting log_warn statements containing the IP address, I learned that while ap->addr gets set in fascist_firewall_choose_address_base(), if ap->addr can be an unknown address (probably AF_UNSPEC and ap->port is 0.

For instance, if I use this code on my branch:

diff --git a/src/core/or/policies.c b/src/core/or/policies.c
index e9e2cb4a5..a9a98ecc3 100644
--- a/src/core/or/policies.c
+++ b/src/core/or/policies.c
@@ -877,6 +877,9 @@ fascist_firewall_choose_address_base(const tor_addr_t *ipv4_addr,
   if (result) {
     tor_addr_copy(&ap->addr, &result->addr);
     ap->port = result->port;
+    char *out_addr = tor_addr_to_str_dup(&ap->addr);
+    log_warn(LD_BUG, "%s %d\n", out_addr, ap->port);
+    tor_free(out_addr);
   }
 }
 
@@ -1049,6 +1052,10 @@ fascist_firewall_choose_address_ls(const smartlist_t *lspecs,
                                        FIREWALL_OR_CONNECTION,
                                        pref_only, pref_ipv6,
                                        ap);
+
+  char *out_addr = tor_addr_to_str_dup(&ap->addr);
+  log_warn(LD_BUG, "%s %d\n", out_addr, ap->port);
+  tor_free(out_addr);
 }
 
 /** Like fascist_firewall_choose_address_base(), but takes <b>node</b>, and

and running make test-network-all gave me this for hs-v23-ipv6-md:

FAIL: hs-v23-ipv6-md
Detail: chutney/tools/warnings.sh /usr/home/neel/code/tor/chutney//net/nodes.1534465377
Warning: Bug:     0x111f299 <main+0x19> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x111f43c <tor_main+0x4c> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x111f744 <connection_add_impl+0x1f4> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x1122271 <do_main_loop+0x421> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x1124028 <tor_run_main+0xe8> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x1153037 <connection_handle_read+0xb57> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x11bdb1e <connection_dir_reached_eof+0x2e> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x11bfcc8 <connection_dir_reached_eof+0x21d8> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x11daade <hs_client_dir_info_changed+0x5e> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x11efb34 <networkstatus_set_current_consensus+0xe04> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x12b51b1 <tor_bug_occurred_+0x111> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x12b8bcc <log_backtrace_impl+0x4c> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x80139076f <event_base_loop+0x51f> at /usr/local/lib/libevent-2.1.so.6 (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x80139481d <event_base_assert_ok_nolock_+0xc8d> at /usr/local/lib/libevent-2.1.so.6 (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug: Non-fatal assertion !(status == HS_CLIENT_FETCH_HAVE_DESC) failed in retry_all_socks_conn_waiting_for_desc at src/feature/hs/hs_client.c:275. Stack trace: (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Every introduction point for service 6zlc2ni5rciibhqtva3354stgd6rywukmhb6cnevps3rdnucozisshyd is unusable or we can't extend to it. We can't connect. Number: 7
Warning: Not enough info to open a circuit to a rendezvous point for hidden service 6zlc2ni5rciibhqtva3354stgd6rywukmhb6cnevps3rdnucozisshyd. Number: 1
Warning: fascist_firewall_choose_address_base: Bug: 127.0.0.1 5000 Number: 70
Warning: fascist_firewall_choose_address_base: Bug: 127.0.0.1 5001 Number: 77
Warning: fascist_firewall_choose_address_base: Bug: 127.0.0.1 5002 Number: 6
Warning: fascist_firewall_choose_address_base: Bug: 127.0.0.1 5003 Number: 10
Warning: fascist_firewall_choose_address_base: Bug: 127.0.0.1 7000 Number: 93
Warning: fascist_firewall_choose_address_base: Bug: 127.0.0.1 7001 Number: 102
Warning: fascist_firewall_choose_address_base: Bug: ::1 5000 Number: 95
Warning: fascist_firewall_choose_address_base: Bug: ::1 5001 Number: 51
Warning: fascist_firewall_choose_address_base: Bug: ::1 5002 Number: 7
Warning: fascist_firewall_choose_address_base: Bug: ::1 5003 Number: 4
Warning: fascist_firewall_choose_address_base: Bug: ::1 5004 Number: 21
Warning: fascist_firewall_choose_address_ls: Bug: 127.0.0.1 5001 Number: 1
Warning: fascist_firewall_choose_address_ls: Bug: <unknown address type> 0 Number: 22
Warning: tor_bug_occurred_: Bug: src/feature/hs/hs_client.c:275: retry_all_socks_conn_waiting_for_desc: Non-fatal assertion !(status == HS_CLIENT_FETCH_HAVE_DESC) failed. (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3

I think the address referred to by this line:

Warning: fascist_firewall_choose_address_ls: Bug: <unknown address type> 0 Number: 22

is the one which crashes Tor.

comment:46 Changed 3 months ago by teor

Yes, "0" is the numeric value for AF_UNSPEC. fascist_firewall_allows_address_ap() returns an AF_UNSPEC address when neither address is reachable.

That's a bug in the new code for this ticket, because each client should have at least one reachable address.

In the hs-ipv6-md network, chutney configures:

  • an IPv4-only client
  • an IPv6-only client
  • an IPv6-only onion service
  • a few IPv4/IPv6 relays

https://gitweb.torproject.org/chutney.git/tree/networks/hs-ipv6-md

Please check the addresses that you're getting out of the lspecs. Maybe the parsing is wrong.

comment:47 Changed 3 months ago by neel

The problematic input to fascist_firewall_choose_address_ls() is the one which despite having a valid IPv4 address does not pass fascist_firewall_allows_address_ap().

I found a solution which I have tested to work: Add a flag to fascist_firewall_choose_address_impl(), fascist_firewall_choose_address(), and fascist_firewall_choose_address_base() called check_fw where we use fascist_firewall_allows_address_ap() if check_fw is 1, or just tor_addr_port_is_valid_ap() if check_fw is 0.

All callers to fascist_firewall_choose_address_base() except for fascist_firewall_choose_address_ls() will set check_fw to 1. The reason why fascist_firewall_choose_address_ls() uses 0 for check_fw is to prevent the regression we have in make test-network-all.

It may not be a perfect solution (or it may), but it works while keeping our fascist_firewall_choose_address_* family of functions and not duplicating code.

The same PR is used (https://github.com/torproject/tor/pull/256).

comment:48 in reply to:  46 Changed 3 months ago by teor

No, I'm sorry, we need to check address reachability on clients. We can't disable reachability checks to work around other bugs in the code.

Here is one bug:

  • When direct_conn is false, hs_get_extend_info_from_lspecs() calls fascist_firewall_choose_address_ls() on the IPv4 address.

Here is a fix:

  • When direct_conn is false, hs_get_extend_info_from_lspecs() accepts any IPv4 address.

Here's why that works:

  • An IPv6-only client can't connect to IPv4, but the relay at the end of its circuit should be able to extend to any IPv4 address.

Replying to teor:

Yes, "0" is the numeric value for AF_UNSPEC. fascist_firewall_allows_address_ap() returns an AF_UNSPEC address when neither address is reachable.

That's a bug in the new code for this ticket, because each client should have at least one reachable address.

In the hs-ipv6-md network, chutney configures:

  • an IPv4-only client
  • an IPv6-only client
  • an IPv6-only onion service
  • a few IPv4/IPv6 relays

https://gitweb.torproject.org/chutney.git/tree/networks/hs-ipv6-md

Please check the addresses that you're getting out of the lspecs. Maybe the parsing is wrong.

comment:49 Changed 3 months ago by teor

Thanks for the update.
We need some minor comment changes, and then I think we're good to go.

Does it pass "make test-network-all"?

comment:50 Changed 3 months ago by neel

I have updated the comment.

Also, it passes make test-network-all.

comment:51 Changed 3 months ago by teor

(Well, you're doing better than me. The master branch fails make test-network-all right now.)

comment:52 Changed 3 months ago by teor

Cc: nickm added
Status: needs_revisionmerge_ready

I'd like to defer this patch until #27080 is fixed, or until 0.3.6. It's not an essential feature, and changes to this code have been risky in the past.

nickm, what do you think?

(If we do want it in 0.3.5, let's make sure test-network-all passes on a few machines.)

I am also concerned about merging this feature without:

  • unit tests (#27086),
  • the 3-hop fallback for single onion services (#23818), and
  • the corresponding service-side IPv6 code (#23576 and #24193).

We shouldn't hold it back forever, but if we wait until the next release, some of these related tasks should get done in that release.

Edit: #24193 is implemented by this ticket.

Last edited 3 months ago by teor (previous) (diff)

comment:53 Changed 3 months ago by teor

Milestone: Tor: 0.3.5.x-finalTor: 0.3.6.x-final
Type: defectenhancement

nickm has agreed to defer this feature until 0.3.6.

comment:54 Changed 3 weeks ago by dgoulet

@teor, should we still merge this. I'm a bit confused about the fact that we have concerns but still merge it? :S...

comment:55 Changed 3 weeks ago by dgoulet

Status: merge_readyneeds_information

comment:56 in reply to:  54 Changed 2 weeks ago by teor

Status: needs_informationneeds_revision

Replying to dgoulet:

@teor, should we still merge this. I'm a bit confused about the fact that we have concerns but still merge it? :S...

I would like this new code to have unit tests (#27086) before we merge it.

The other related tickets would be nice to have, but they're optional.

comment:57 Changed 2 weeks ago by neel

I decided that I will add the unit tests as well and do #27086.

comment:58 Changed 2 weeks ago by teor

Thanks neel!
Let us know if you need some help.

comment:59 Changed 2 weeks ago by neel

Status: needs_revisionneeds_review

I have added a test. The same PR (https://github.com/torproject/tor/pull/256) is used here.

On my system (FreeBSD 11.2 on HP EliteBook 1040 G3), the test passes. There is a merge as the last commit, but this is because of a merge conflict in the #includes of src/test/test_policy.c and CI would not build without it.

comment:60 Changed 2 weeks ago by teor

Status: needs_reviewneeds_revision

Thanks!

I think this is good test code, but we can make the link specifier code a bit more reliable by using memcpy(), and setting and getting lengths. See the pull request comments for details.

comment:61 Changed 2 weeks ago by neel

Status: needs_revisionneeds_review

I have added the requested changes and have pushed them. Same PR.

comment:62 Changed 2 weeks ago by teor

Status: needs_reviewneeds_revision

The tests for fascist_firewall_choose_address_ls() look good to me.

It looks like we still need tests for hs_get_extend_info_from_lspecs(), then we're done.

comment:63 Changed 2 weeks ago by teor

hs_get_extend_info_from_lspecs() already has good coverage:
https://coveralls.io/builds/19903451/source?filename=src/feature/hs/hs_common.c#L1677

But the direct_conn line needs to be tested, because it changed in this branch:
https://coveralls.io/builds/19903451/source?filename=src/feature/hs/hs_common.c#L1740

Thanks!

comment:64 Changed 2 weeks ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

comment:65 Changed 21 hours ago by neel

Status: needs_revisionneeds_review

I have added a hs_get_extend_info_from_lspecs() test. I am reusing the ls and mock configuration structs used to test fascist_firewall_choose_address_ls().

Sorry for the delay, I was really busy in the past two weeks.

Note: See TracTickets for help on using tickets.