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

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

changed milestone to %Tor: 0.4.0.x-final

added 029-no-backport 035-backport 040-backport 041-backport actualpoints::0.2 component::core tor/tor consider-backport-after-0421 ipv6 milestone::Tor: 0.4.0.x-final network-team-roadmap-august owner::teor points::1 priority::medium prop224 resolution::fixed reviewer::asn severity::normal single-onion sponsor::27-must status::closed tor-hs type::defect v3-onion-service-feature-parity version::tor 0.2.9.3-alpha labels

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

Trac:
Description: This makes single onion services connect vias an entry that it can reach when connections fail.

to

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

Moving a bunch of tickets from 033 to 034.

Trac:
Milestone: Tor: 0.3.3.x-final to Tor: 0.3.4.x-final

Trac:
Keywords: N/A deleted, 034-triage-20180328 added

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

Trac:
Keywords: N/A deleted, 034-removed-20180328 added

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

Trac:
Milestone: Tor: 0.3.4.x-final to Tor: unspecified

Trac:
Owner: N/A to neel
Cc: N/A to neel@neelc.org
Status: new to assigned

Trac:
Owner: neel to N/A

Trac:
Status: assigned to new

Trac:
Status: new to assigned
Owner: N/A 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)?

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 (moved) and #23576 (moved).

I suggest you:

1. create a branch from the master branch, and merge the latest branches from #23588 (moved) and #23576 (moved)
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!

4. 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

5. and intro point connection: https://github.com/torproject/tor/blob/fe9f58514349c9d25b48ae29c87d8aaf065d0931/src/feature/rend/rendservice.c#L3031

6. 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

7. 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

8. 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 (moved)).

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,

I merged the two branches for #23588 (moved) and #23576 (moved) 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 (moved) fails make test-network-all also.

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?

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 (moved) for details. I'll probably do the fixes on the rebased branch.

Trac:
Owner: neel to N/A

Trac:
Status: assigned to new

Trac:
Keywords: N/A deleted, v3-onion-service-feature-parity added

nice this will increase reachability and stability

This is a must have feature parity ticket

Trac:
Sponsor: N/A to Sponsor27-must
Points: N/A to 1

Trac:
Milestone: Tor: unspecified to Tor: 0.4.2.x-final

Trac:
Keywords: 034-removed-20180328, 034-triage-20180328 deleted, network-team-roadmap-august added

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.

Trac:
Status: new to assigned
Owner: N/A to teor

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:

• 0.2.9: https://github.com/torproject/tor/pull/1255
  • v2 fix
• 0.3.5: https://github.com/torproject/tor/pull/1256
  • v3 fix
• master: https://github.com/torproject/tor/pull/1257
  • comment 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.

Trac:
Status: assigned to needs_review
Version: N/A to Tor: 0.2.9.3-alpha
Actualpoints: N/A to 0.2
Keywords: N/A deleted, consider-backport-after-0421-alpha, 040-baclport, 029-backport-maybe-not, 041-backport, 035-backport added

Fix summary for actual bug

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

I added the code for #23507 (moved) 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.

Trac:
Reviewer: N/A to asn

Looks great to me!

Trac:
Status: needs_review to merge_ready

Trac:
Keywords: network-team-roadmap-august deleted, network-team-roadmap-august nickm-merge dgoulet-merge added

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.

Trac:
Milestone: Tor: 0.4.2.x-final to Tor: 0.4.0.x-final

Ok, I removed 029 from the backport tags

Trac:
Keywords: 029-backport-maybe-not, network-team-roadmap-august nickm-merge dgoulet-merge, 040-baclport deleted, 029-no-backport, network-team-roadmap-august, 040-backport added

Oops, drop the -alpha from backport versions

Trac:
Keywords: consider-backport-after-0421-alpha deleted, consider-backport-after-0421 added

Trac:
Parent: #23493 (moved) to N/A

Merged to 0.3.5 and later. Merged #32106 (moved), #31807 (moved), #31001 (moved), #23818 (moved), #12399 (moved), and #31372 (moved) together.

Trac:
Status: merge_ready to closed
Resolution: N/A to fixed

closed

changed time estimate to 8h

added 1h 36m of time spent

mentioned in issue #31001 (moved)

mentioned in issue #31372 (moved)

mentioned in issue #31492 (moved)

mentioned in issue #31807 (moved)

mentioned in issue #32106 (moved)

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

mentioned in issue tpo/core/tor#31492 (closed)