Opened 18 months ago

Closed 12 days 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 18 months 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 17 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 15 months ago by dgoulet

Owner: set to dgoulet
Status: newassigned

comment:4 Changed 14 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 12 months ago by nickm

Keywords: 034-triage-20180328 added

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

Keywords: fast-fix added

comment:12 Changed 8 months ago by asn

Reviewer: asn

comment:13 Changed 7 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 7 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 7 months ago by asn

Status: needs_reviewmerge_ready

LGTM for 036!

comment:16 Changed 7 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 7 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 7 months ago by teor

Status: merge_readyneeds_revision

comment:19 Changed 7 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 7 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 5 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 2 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 8 weeks 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 weeks 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 weeks 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 weeks 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 weeks ago by dgoulet

Reviewer: asndgoulet

comment:28 Changed 5 weeks 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 2 weeks ago by teor

Parent ID: #23493#23588

comment:30 Changed 2 weeks 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 2 weeks ago by dgoulet

Keywords: asn-merge added
Status: needs_reviewmerge_ready

lgtm!

comment:32 Changed 13 days 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 12 days 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 12 days ago by asn

Keywords: asn-merge nickm-merge removed

removing merge keywords since it got merged.

comment:35 Changed 12 days ago by teor

Resolution: fixed
Status: merge_readyclosed

Thanks! This is exciting, almost there!

Note: See TracTickets for help on using tickets.