Opened 2 years ago

Last modified 5 weeks ago

#23818 merge_ready defect

Make v2 and v3 single onion services retry all failed intro and rend connections with a 3-hop path

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version: Tor: 0.2.9.3-alpha
Severity: Normal Keywords: 029-no-backport, 035-backport, 040-backport, 041-backport, v3-onion-service-feature-parity, prop224, tor-hs, single-onion, ipv6, network-team-roadmap-august, consider-backport-after-0421
Cc: neel@… Actual Points: 0.2
Parent ID: Points: 1
Reviewer: asn Sponsor: Sponsor27-must

Description (last modified by teor)

This makes a single onion service connect via an entry that it can reach when connections fail.

Child Tickets

TicketStatusOwnerSummaryComponent
#23507closedteorHandle unreachable addresses on v3 single onion services by using a 3 hop pathCore Tor/Tor
#31492closedteorUpdate hsv3 spec for unreachable/failed single onion retries using 3 hopsCore Tor/Tor

Change History (32)

comment:1 Changed 2 years ago by teor

Description: modified (diff)

I removed partial support for this feature in commit b4aa8fc3d9 in my bug23820_032 branch (#23820), because it was buggy. We'll need to revert that commit and build on it for 0.3.3.

comment:2 Changed 21 months ago by dgoulet

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

Moving a bunch of tickets from 033 to 034.

comment:3 Changed 19 months ago by nickm

Keywords: 034-triage-20180328 added

comment:4 Changed 19 months ago by nickm

Keywords: 034-removed-20180328 added

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

comment:5 Changed 18 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:6 Changed 14 months ago by neel

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

comment:7 Changed 14 months ago by neel

Owner: neel deleted

comment:8 Changed 14 months ago by neel

Status: assignednew

comment:9 Changed 14 months ago by neel

Owner: set to neel
Status: newassigned

comment:10 Changed 14 months ago by neel

I am interested in this patch.

Right now, I am looking at commit b4aa8fc3d9 (that you referred to in Comment 1) and saw that in pick_intro_point(), you replaced this line

info = extend_info_from_node(node, direct_conn);

with this:

info = extend_info_from_node(node, 0);

Should I revert this to the former line (the one with direct_conn)?

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

Replying to neel:

I am interested in this patch.

Right now, I am looking at commit b4aa8fc3d9 (that you referred to in Comment 1) and saw that in pick_intro_point(), you replaced this line

info = extend_info_from_node(node, direct_conn);

with this:

info = extend_info_from_node(node, 0);

Should I revert this to the former line (the one with direct_conn)?

We re-wrote a lot of this code in #23588 and #23576.

I suggest you:

  1. create a branch from the master branch, and merge the latest branches from #23588 and #23576
  2. test that the merged branch passes "make test-network-all", including all the IPv6 tests
  3. test that the merged branch fails the single-onion-v23-ipv6-md network using "chutney/tools/test-network.sh --flavour single-onion-v23-ipv6-md"

Please paste the test results into a ticket, to confirm that you're running the correct tests.

Then you can start writing a patch for this ticket based on those branches:

As you are writing the patch, you might find some of this removed code useful:
https://github.com/torproject/tor/commit/b4aa8fc3d918cc3aea375985c44abd086f91ae7a#diff-b2d25162bf60cfbeb456effc403d3282L1540
https://github.com/torproject/tor/commit/b4aa8fc3d918cc3aea375985c44abd086f91ae7a#diff-b2d25162bf60cfbeb456effc403d3282L1557

But remember, that old code is buggy and incomplete!

  1. write code that matches the v2 single onion service behaviour for intro point selection:

https://github.com/torproject/tor/blob/fe9f58514349c9d25b48ae29c87d8aaf065d0931/src/feature/rend/rendservice.c#L4133

  1. and intro point connection:

https://github.com/torproject/tor/blob/fe9f58514349c9d25b48ae29c87d8aaf065d0931/src/feature/rend/rendservice.c#L3031

  1. write code that matches the v2 single onion service behaviour for rend point connection:

https://github.com/torproject/tor/blob/fe9f58514349c9d25b48ae29c87d8aaf065d0931/src/feature/rend/rendservice.c#L2089

  1. write a comment that explains the single onion service behaviour for rend point retries (which just use a standard 3-hop connection):

https://github.com/torproject/tor/blob/422abd4fa3c2c9c10f4e7f83eced7416785e89c4/src/feature/rend/rendservice.c#L2994

  1. when you have implemented the 3-hop fallback feature, your code will pass on the single-onion-v23-ipv6-md network, and we can make that network part of the standard test suite (#27251).

I suggest that you open child tickets for steps 1-3, 4-5, and 6-8. Otherwise, this ticket will become very large and hard to read,

comment:12 Changed 14 months ago by neel

I merged the two branches for #23588 and #23576 here: https://github.com/neelchauhan/tor/tree/v3_sos

However, make test-network-all tests involving v3 onion services fail:

neel@megora:~/Code/Tor/tor % 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 not found, skipping mixed flavors: mixed+hs-v23.
SKIP: mixed+hs-v23
PASS: basic-min
Detail: chutney/tools/warnings.sh /usr/home/neel/Code/Tor/chutney/net/nodes.1535244496
Warning: Rejected vote from 127.0.0.1 ("Duplicate discarded"). Number: 1
PASS: bridges-min
PASS: hs-v2-min
FAIL: hs-v3-min
FAIL: single-onion-v23
Detail: chutney/tools/warnings.sh /usr/home/neel/Code/Tor/chutney/net/nodes.1535244735
Warning: Rejected vote from 127.0.0.1 ("Duplicate discarded"). Number: 2
PASS: bridges+ipv6-min
PASS: ipv6-exit-min
FAIL: hs-v23-ipv6-md
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/Code/Tor/tor
neel@megora:~/Code/Tor/tor % 

It seems teor's branch for #23576 fails make test-network-all also.

comment:13 Changed 14 months ago by neel

For Steps 1-3 in Comment 11, after the buggy code gets fixed, should I make the branch based on teor's bug23576-rebased or just bug23576?

comment:14 in reply to:  13 Changed 14 months ago by teor

Replying to neel:

For Steps 1-3 in Comment 11, after the buggy code gets fixed, should I make the branch based on teor's bug23576-rebased or just bug23576?

Check #23576 for details. I'll probably do the fixes on the rebased branch.

comment:15 Changed 8 months ago by neel

Owner: neel deleted

comment:16 Changed 8 months ago by neel

Status: assignednew

comment:17 Changed 7 months ago by teor

Keywords: v3-onion-service-feature-parity added

comment:18 Changed 6 months ago by cypherpunks

nice this will increase reachability and stability

comment:19 Changed 3 months ago by teor

Points: 1
Sponsor: Sponsor27-must

This is a must have feature parity ticket

comment:20 Changed 3 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.4.2.x-final

comment:21 Changed 3 months ago by gaba

Keywords: network-team-roadmap-august added; 034-triage-20180328 034-removed-20180328 removed

comment:22 Changed 8 weeks ago by gaba

Owner: set to teor
Status: newassigned

Teor: this is something you were going to work on in August, right? I have that in my notes. Please, change and make a comment if not.

comment:23 Changed 8 weeks ago by teor

Actual Points: 0.2
Keywords: consider-backport-after-0421-alpha 029-backport-maybe-not 035-backport 040-baclport 041-backport added
Status: assignedneeds_review
Version: Tor: 0.2.9.3-alpha

It turns out that there is actually an identical bug in v2 and v3 onion services, from the original v2 implementation in 0.2.9.3-alpha.

Here are the PRs for the fix:

I don't know how far we should backport this fix. It's a reachability fix. I think we should backport to 0.3.5, I'm not sure if we should worry about 0.2.9.

comment:24 Changed 8 weeks ago by teor

Summary: Make v3 single onion services retry failed connections with a 3-hop pathMake v2 and v3 single onion services retry all failed intro and rend connections with a 3-hop path

Fix summary for actual bug

comment:25 Changed 8 weeks ago by teor

I added the code for #23507 to the 0.3.5 branch, and merged it forward to master. They both changed the same lines in one function, so there would have been conflicts otherwise.

comment:26 Changed 7 weeks ago by dgoulet

Reviewer: asn

comment:27 Changed 7 weeks ago by asn

Status: needs_reviewmerge_ready

Looks great to me!

comment:28 Changed 7 weeks ago by dgoulet

Keywords: nickm-merge dgoulet-merge added

comment:29 Changed 7 weeks ago by dgoulet

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

Merged PR 1255 and 1256 in 041.
Merged 041 forward into master.
Merged PR 1257 in master.

I believe we should backport down to 035. Leaving 029 like this is fine imo. Moving this to 040 Milestone.

comment:30 Changed 7 weeks ago by teor

Keywords: 029-no-backport 040-backport added; 029-backport-maybe-not 040-baclport nickm-merge dgoulet-merge removed

Ok, I removed 029 from the backport tags

comment:31 Changed 7 weeks ago by teor

Keywords: consider-backport-after-0421 added; consider-backport-after-0421-alpha removed

Oops, drop the -alpha from backport versions

comment:32 Changed 5 weeks ago by teor

Parent ID: #23493
Note: See TracTickets for help on using tickets.