Opened 13 months ago

Last modified 2 months ago

#23576 needs_revision defect

Make service_intro_point_new() take a node instead of an extend_info

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.3.6.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
Cc: neel@… Actual Points:
Parent ID: #23493 Points: 1
Reviewer: asn 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
#26992needs_revisionteorAdd intro point IPv6 address to service descriptorsCore Tor/Tor

Change History (20)

comment:1 Changed 13 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 12 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 10 months ago by dgoulet

Owner: set to dgoulet
Status: newassigned

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

Keywords: 034-triage-20180328 added

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

Keywords: fast-fix added

comment:12 Changed 3 months ago by asn

Reviewer: asn

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

Status: needs_reviewmerge_ready

LGTM for 036!

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

Status: merge_readyneeds_revision

comment:19 Changed 2 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 2 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.

Note: See TracTickets for help on using tickets.