Opened 17 months ago
Last modified 3 weeks 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: | teor |
---|---|---|---|
Priority: | Medium | Milestone: | Tor: unspecified |
Component: | Core Tor/Tor | Version: | Tor: 0.3.2.1-alpha |
Severity: | Normal | Keywords: | prop224, tor-hs, single-onion, ipv6, 034-triage-20180328, 034-removed-20180328, 040-unreached-20190109, 041-proposed |
Cc: | neel@…, nickm | Actual Points: | 0.5 |
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
Ticket | Type | Status | Owner | Summary |
---|---|---|---|---|
#24193 | enhancement | closed | dgoulet | Make v3 single onion services parse and use IPv6 introduce link specifiers |
#27086 | defect | closed | neel | Write unit tests for fascist_firewall_choose_address_ls() and hs_get_extend_info_from_lspecs() |
#29237 | defect | new | Restore IPv6 intro points in the HS client tests |
Change History (76)
comment:1 Changed 17 months ago by
comment:2 Changed 16 months ago by
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 13 months ago by
Milestone: | Tor: 0.3.3.x-final → Tor: 0.3.4.x-final |
---|
Defer a small handful of non-enhancement "new" tickets to 0.3.4
comment:4 Changed 11 months ago by
Keywords: | 034-triage-20180328 added |
---|
comment:5 Changed 11 months ago by
Keywords: | 034-removed-20180328 added |
---|
Per our triage process, these tickets are pending removal from 0.3.4.
comment:6 Changed 11 months ago by
Milestone: | Tor: 0.3.4.x-final → Tor: 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 9 months ago by
Cc: | neel@… added |
---|---|
Owner: | set to neel |
Status: | new → assigned |
comment:8 follow-up: 9 Changed 7 months ago by
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 Changed 7 months ago by
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
andipv6_dirport
is0
?
Yes, link specifiers are only defined for ORPorts.
Also, #24732 will remove the IPv6 DirPort code, because it's unused.
comment:10 follow-up: 11 Changed 7 months ago by
My PR is here: https://github.com/torproject/tor/pull/252
comment:11 Changed 7 months ago by
Milestone: | Tor: unspecified → Tor: 0.3.5.x-final |
---|---|
Status: | assigned → needs_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 7 months ago by
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 7 months ago by
I have fixed these issues. The same PR is used for this new patch: https://github.com/torproject/tor/pull/252
comment:14 Changed 7 months ago by
Reviewer: | → teor |
---|---|
Status: | needs_revision → needs_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 7 months ago by
Status: | needs_review → needs_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 7 months ago by
New PR is (here)https://github.com/torproject/tor/pull/256.
UPDATE: Fix link.
comment:17 Changed 7 months ago by
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 follow-up: 19 Changed 7 months ago by
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 Changed 7 months ago by
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 7 months ago by
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:22 Changed 7 months ago by
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:24 Changed 7 months ago by
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 7 months ago by
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 follow-up: 27 Changed 7 months ago by
I have pushed new commits.
I do not want to write a unit test (for now at least).
comment:27 Changed 7 months ago by
comment:28 Changed 7 months ago by
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
comment:29 Changed 6 months ago by
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.
comment:30 follow-up: 31 Changed 6 months ago by
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 follow-up: 32 Changed 6 months ago by
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 Changed 6 months ago by
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.
comment:33 Changed 6 months ago by
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 6 months ago by
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 6 months ago by
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 follow-ups: 37 44 Changed 6 months ago by
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.
comment:37 Changed 6 months ago by
Well, the "solution" above did not end up working (not committed, however). It still crashed.
comment:38 follow-up: 39 Changed 6 months ago by
comment:39 Changed 6 months ago by
comment:40 Changed 6 months ago by
Owner: | neel deleted |
---|---|
Status: | needs_revision → assigned |
I'll just give up on this bug. It is crashing too much.
comment:41 Changed 6 months ago by
Status: | assigned → new |
---|
comment:42 Changed 6 months ago by
Owner: | set to neel |
---|---|
Status: | new → assigned |
comment:43 Changed 6 months ago by
Status: | assigned → needs_revision |
---|
comment:44 Changed 6 months ago by
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 throughfascist_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 6 months ago by
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 follow-up: 48 Changed 6 months ago by
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 6 months ago by
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 Changed 6 months ago by
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 6 months ago by
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 6 months ago by
I have updated the comment.
Also, it passes make test-network-all
.
comment:51 Changed 6 months ago by
(Well, you're doing better than me. The master branch fails make test-network-all right now.)
comment:52 Changed 6 months ago by
Cc: | nickm added |
---|---|
Status: | needs_revision → merge_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.
comment:53 Changed 6 months ago by
Milestone: | Tor: 0.3.5.x-final → Tor: 0.3.6.x-final |
---|---|
Type: | defect → enhancement |
nickm has agreed to defer this feature until 0.3.6.
comment:54 follow-up: 56 Changed 4 months ago by
@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 4 months ago by
Status: | merge_ready → needs_information |
---|
comment:56 Changed 4 months ago by
Status: | needs_information → needs_revision |
---|
comment:59 Changed 4 months ago by
Status: | needs_revision → needs_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 #include
s of src/test/test_policy.c
and CI would not build without it.
comment:60 Changed 4 months ago by
Status: | needs_review → needs_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 4 months ago by
Status: | needs_revision → needs_review |
---|
I have added the requested changes and have pushed them. Same PR.
comment:62 Changed 4 months ago by
Status: | needs_review → needs_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 4 months ago by
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 3 months ago by
Milestone: | Tor: 0.3.6.x-final → Tor: 0.4.0.x-final |
---|
Tor 0.3.6.x has been renamed to 0.4.0.x.
comment:65 Changed 3 months ago by
Status: | needs_revision → needs_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.
comment:66 Changed 3 months ago by
Status: | needs_review → needs_revision |
---|
We all get busy at times.
Thanks for these tests.
It looks like we're missing a few tests for each function.
I commented on the pull request.
comment:67 Changed 3 months ago by
Status: | needs_revision → needs_review |
---|
I have (hopefully) added the tests requested. Can you review it and push it if it's okay?
comment:68 Changed 3 months ago by
Status: | needs_review → needs_revision |
---|
We just need a few tweaks to the log messages: if the remote side sends bad data, it's a protocol warning, not a bug warning. (This was my mistake in my last review.)
Maybe we should also log warnings when we don't have a legacy ID or a reachable IP address?
- a missing legacy ID is a protocol warning
- an unreachable IP address is an info-level warning (not a bug and not a protocol warning, because it can happen without any side doing the wrong thing)
comment:69 Changed 3 months ago by
Status: | needs_revision → needs_review |
---|
I have added the needed changes to fascist_firewall_choose_address_ls()
.
I have also added the new warnings to hs_get_extend_info_from_lspecs()
and added a test for the missing legacy ID as the last commit. The unreachable IP address warning is untested for now.
Setting as needs review.
comment:70 Changed 3 months ago by
Status: | needs_review → needs_revision |
---|
Thanks, just two more tweaks left.
This branch will need a merge or a rebase before we merge it into master.
I'm happy to do the merge in a new pull request, once we get the logging sorted.
comment:71 Changed 3 months ago by
Status: | needs_revision → needs_review |
---|
I have made and pushed the changes. Setting as needs review.
comment:72 Changed 3 months ago by
Owner: | changed from neel to teor |
---|---|
Status: | needs_review → assigned |
Ok, I think we're good here. I'll do the rebase and squash once I have my dev environment working again.
comment:73 Changed 2 months ago by
Status: | assigned → merge_ready |
---|
I rebased and squashed neel's branch in:
https://github.com/torproject/tor/pull/583
I also edited the changes file to say what the patch does (not how it is implemented).
Thanks neel!
comment:74 Changed 2 months ago by
Status: | merge_ready → needs_revision |
---|
This branch passes all our current chutney tests.
It also passes the IPv6 v3 onion service test in chutney. So maybe the test is wrong, or this branch is choosing IPv4 addresses?
comment:75 Changed 6 weeks ago by
Keywords: | 040-unreached-20190109 041-proposed added |
---|---|
Milestone: | Tor: 0.4.0.x-final → Tor: unspecified |
These IPv6 tickets won't make it into 0.4.0, let's think about doing them in 0.4.1.
(They're not on the roadmap, so we'll need to decide if diagnosing the underlying issues is worth the time.)
comment:76 Changed 3 weeks ago by
Actual Points: | → 0.5 |
---|---|
Status: | needs_revision → needs_review |
Version: | → Tor: 0.3.2.1-alpha |
These tickets aren't urgent, and they involve major refactoring.
Deferring to 0.3.3