Opened 2 years ago

Closed 5 months ago

#23576 closed defect (fixed)

Make service_intro_point_new() take a node instead of an extend_info

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.1.x-final
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, fast-fix, 040-unreached-20190109, 041-proposed
Cc: neel@… Actual Points: 2
Parent ID: #23588 Points: 1
Reviewer: dgoulet Sponsor:

Description

service_intro_point_new() and hs_desc_link_specifier_new() need to take a node_t, so they can fill it in with IPv4 and IPv6 addresses.

Child Tickets

TicketStatusOwnerSummaryComponent
#22781closedteorhs: Unify link specifier API/ABICore Tor/Tor
#23759closedteorRefactor common code out of setup_introduce1_data and intro point functionsCore Tor/Tor
#26992closedteorAdd intro point IPv6 address to service descriptorsCore Tor/Tor
#29243closedteorFix minor bugs in the HSv3 unit testsCore Tor/Tor

Change History (35)

comment:1 Changed 2 years ago by teor

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

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

comment:2 Changed 22 months ago by teor

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

    Remove buggy IPv6 support from pick_intro_point() and service_intro_point_new()
    
    The previous version of these functions had the following issues:
    * they can't supply both the IPv4 and IPv6 addresses in link specifiers,
    * they try to fall back to a 3-hop path when the address for a direct
      connection is unreachable, but this isn't supported by
      launch_rendezvous_point_circuit(), so it fails.
    But we can't fix these things in a bugfix release.
    
    Instead, always put IPv4 addresses in link specifiers.
    And if a v3 single onion service can't reach any intro points, fail.

comment:3 Changed 20 months ago by dgoulet

Owner: set to dgoulet
Status: newassigned

comment:4 Changed 19 months ago by dgoulet

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

Move 033 ticket I own to 034

comment:5 Changed 17 months ago by nickm

Keywords: 034-triage-20180328 added

comment:6 Changed 17 months ago by nickm

Keywords: 034-removed-20180328 added

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

comment:7 Changed 17 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:8 Changed 13 months ago by teor

Milestone: Tor: unspecifiedTor: 0.3.5.x-final
Owner: changed from dgoulet to teor
Version: Tor: 0.3.2.1-alpha

We can implement this ticket easily by replacing the link specifier part of service_intro_point_new() with get_lspecs_from_node().

This refactor completes part of #23759.

comment:9 Changed 13 months ago by teor

Status: assignedneeds_review

See my branch bug23576 on https://github.com/teor2345/tor.git

This refactor also puts intro IPv6 link specifiers in onion service descriptors (#26992).

It passes make check and make test-network-all on my machine, but will probably fail Appveyor CI due to #26986.

comment:10 Changed 13 months ago by teor

Keywords: fast-fix added

comment:12 Changed 13 months ago by asn

Reviewer: asn

comment:13 Changed 12 months ago by asn

Status: needs_reviewneeds_revision

This looks good and I like the code simplification!

I left a few nitpicks on the PR that you might want to handle.

Also, should we try to rebase this so that we can see the appveyor green checkmark (now that #26986 got fixed)?

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

Milestone: Tor: 0.3.5.x-finalTor: 0.3.6.x-final
Status: needs_revisionneeds_review

Replying to asn:

This looks good and I like the code simplification!

I left a few nitpicks on the PR that you might want to handle.

See the PR for my changes.

Also, should we try to rebase this so that we can see the appveyor green checkmark (now that #26986 got fixed)?

See my branch bug23576-rebased:

I'd like to defer the merge to early in 0.3.6, for the same reasons as:
https://trac.torproject.org/projects/tor/ticket/23588#comment:52

We're still missing #23818, and when we test in #27251, we might find we're missing other things as well. I'd like to have a full release to implement and test them.

comment:15 Changed 12 months ago by asn

Status: needs_reviewmerge_ready

LGTM for 036!

comment:16 Changed 12 months ago by neel

Cc: neel@… added

I think this branch fails make test-network-all:

neel@megora:~/Code/Tor/back/teor2345-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.1535248896
Warning: Rejected vote from 127.0.0.1 ("Duplicate discarded"). Number: 1
PASS: bridges-min
PASS: hs-v2-min
FAIL: hs-v3-min
Detail: chutney/tools/warnings.sh /usr/home/neel/Code/Tor/chutney/net/nodes.1535249030
Warning: Rejected vote from 127.0.0.1 ("Duplicate discarded"). Number: 1
FAIL: single-onion-v23
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/back/teor2345-tor
neel@megora:~/Code/Tor/back/teor2345-tor

Is this branch failing because of missing code (that should get implemented), bugs in the code, or just my computer?

comment:17 Changed 12 months ago by teor

It's possible that this code is buggy. Since it's supposed to be a refactor, there shouldn't be any missing code.

But we've also fixed some issues in tor and chutney over the last few days.

Please try again with:

  • the latest version of chutney master, and
  • bug23576-rebased merged into tor master

If it still fails, I'll take a look at it some time next week.

comment:18 Changed 12 months ago by teor

Status: merge_readyneeds_revision

comment:19 Changed 12 months ago by neel

Using this new merged branch also fails on both FreeBSD 12-ALPHA3 and Fedora 28 (the latter on LiveUSB).

comment:20 Changed 12 months ago by teor

On macOS 10.13 with tor bug23576-rebased and chutney 0ccad53 (or later) I see:

$ make test-network-all
 cd . && /bin/sh ./config.status Makefile depfiles
config.status: creating Makefile
config.status: executing depfiles commands
mkdir -p ./test_network_log
ping6 ::1 and ping ::1 failed, skipping 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: bridges+ipv6-min
SKIP: ipv6-exit-min
SKIP: hs-v23-ipv6-md
SKIP: single-onion-ipv6-md
SKIP: mixed+hs-v23
PASS: basic-min
PASS: bridges-min
PASS: hs-v2-min
FAIL: hs-v3-min
FAIL: single-onion-v23
Log and result files are available in ./test_network_log.
make: *** [test-network-all] Error 1
Exit 2

When I merge bug23576-rebased into master to pick up the latest fixes, I see similar output. I'll have a look into it later this week.

comment:21 Changed 10 months 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:22 Changed 7 months ago by teor

Keywords: 040-unreached-20190109 041-proposed added
Milestone: Tor: 0.4.0.x-finalTor: 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:23 Changed 7 months ago by teor

Actual Points: 2

I rebased this branch on master, and dgoulet helped me find some bugs in it:
https://github.com/torproject/tor/pull/673

I still need to tidy it up, add changes files, and find tickets for the new changes. But it seems to work well enough to run it on CI.

comment:24 Changed 7 months ago by teor

The CI has two spurious errors:

circuitpadding/circuitpadding_circuitsetup_machine: [forking] 
  FAIL src/test/test_circuitpadding.c:1880: assert(n_client_cells OP_EQ 2): 3 vs 2
  [circuitpadding_circuitsetup_machine FAILED]

https://travis-ci.org/torproject/tor/jobs/486721893#L5750

error: failed to write /home/travis/build/torproject/tor/tor-0.4.0.1-alpha-dev/src/rust/Cargo.lock

Caused by:
  failed to open: /home/travis/build/torproject/tor/tor-0.4.0.1-alpha-dev/src/rust/Cargo.lock

Caused by:
  Permission denied (os error 13)

https://travis-ci.org/torproject/tor/jobs/486721901#L3634

comment:25 in reply to:  24 Changed 7 months ago by teor

Replying to teor:

The CI has two spurious errors:

circuitpadding/circuitpadding_circuitsetup_machine: [forking] 
  FAIL src/test/test_circuitpadding.c:1880: assert(n_client_cells OP_EQ 2): 3 vs 2
  [circuitpadding_circuitsetup_machine FAILED]

https://travis-ci.org/torproject/tor/jobs/486721893#L5750

Reopened #29122.

error: failed to write /home/travis/build/torproject/tor/tor-0.4.0.1-alpha-dev/src/rust/Cargo.lock

Caused by:
  failed to open: /home/travis/build/torproject/tor/tor-0.4.0.1-alpha-dev/src/rust/Cargo.lock

Caused by:
  Permission denied (os error 13)

https://travis-ci.org/torproject/tor/jobs/486721901#L3634

Opened #29244.

comment:26 Changed 7 months ago by teor

Status: needs_revisionneeds_review

Ok, the branch is ready with changes files and a cleaned-up series of commits:
https://github.com/torproject/tor/pull/673

The branch fixes all the child tickets of this ticket. See the commit messages for details.

asn, if you don't want to review this branch, dgoulet reviewed the first two commits yesterday at the hackfest. There are 2 new commits that dgoulet has not seen: they remove hs_desc_link_specifier_t, and fix the unit tests.

comment:27 Changed 6 months ago by dgoulet

Reviewer: asndgoulet

comment:28 Changed 6 months ago by dgoulet

Status: needs_reviewneeds_revision

Couple comments. Nothing crazy. Once that is fixed, feel free to put it in merge_ready

Thanks!

comment:29 Changed 6 months ago by teor

Parent ID: #23493#23588

comment:30 Changed 6 months ago by teor

Status: needs_revisionneeds_review

I found some more NULLs to check, and some more code to delete, so I'm putting it back in needs_review.

comment:31 Changed 6 months ago by dgoulet

Keywords: asn-merge added
Status: needs_reviewmerge_ready

lgtm!

comment:32 Changed 5 months ago by teor

Keywords: nickm-merge added
Milestone: Tor: unspecifiedTor: 0.4.1.x-final

This is a feature, it belongs in master.

comment:33 Changed 5 months ago by nickm

Squashed and merged to master.

Please do whatever is appropriate for #29243 and #22781, and then close this ticket? Thanks!

comment:34 Changed 5 months ago by asn

Keywords: asn-merge nickm-merge removed

removing merge keywords since it got merged.

comment:35 Changed 5 months ago by teor

Resolution: fixed
Status: merge_readyclosed

Thanks! This is exciting, almost there!

Note: See TracTickets for help on using tickets.