Opened 9 months ago

Closed 5 months ago

#33222 closed enhancement (implemented)

Prop 311: 4.2. Implement IPv6 ORPort Reachability Self-Tests

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: ipv6, prop311
Cc: Actual Points: 4
Parent ID: #33221 Points: 6
Reviewer: nickm Sponsor: Sponsor55-must

Description (last modified by teor)

4.2. Checking IPv6 ORPort Reachability

We propose that testing relays (and bridges) select some IPv6 extend-capable
relays for their reachability circuits, and include their own IPv4 and IPv6
ORPorts in the final extend cells on those circuits.

The final extending relay will extend to the testing relay:

  • using an existing authenticated connection to the testing relay (which may be over IPv4 or IPv6), or
  • over a new connection via the IPv4 or IPv6 ORPort in the extend cell.

The testing relay will confirm that test circuits can extend to both its
IPv4 and IPv6 ORPorts.

4.2.1. Selecting the Final Extending Relay

IPv6 ORPort reachability checks require an IPv6 extend-capable relay as
the second-last hop of reachability circuits. (The testing relay is the
last hop.)

IPv6-extend capable relays must have:

  • Relay subprotocol version 3 (or later), and
  • an IPv6 ORPort.

(See section 5.1 for the definition of Relay subprotocol version 3.)

The other relays in the path do not require any particular protocol
versions.

4.2.2. Extending from the Second-Last Hop

IPv6 ORPort reachability circuits should put the IPv4 and IPv6 ORPorts in
the extend cell for the final extend in reachability circuits.

Supplying both ORPorts makes these extend cells indistinguishable from
future client extend cells.

If reachability succeeds, the testing relay (or bridge) will accept the
final extend on one of its ORPorts, selected at random by the extending
relay (see section 3.2.1).

4.2.3. Separate IPv4 and IPv6 Reachability Flags

Testing relays (and bridges) will record reachability separately for IPv4
and IPv6 ORPorts, based on the ORPort that the test circuit was received on.

Here is a reliable way to do reachability self-tests for each ORPort:

  1. Check for create cells on inbound ORPort connections from other relays

Check for a cell on any IPv4 and any IPv6 ORPort. (We can't know which listener(s) correspond to the advertised ORPorts, particularly when using port forwarding.) Make sure the cell was received on an inbound OR connection, that is authenticated to another relay.

  1. Check for created cells from testing circuits on outbound OR connections

Check for a returned created cell on our IPv4 and IPv6 self-test circuits. Make sure those circuits were on outbound OR connections.

By combining these tests, we confirm that we can:

  • reach our own ORPorts with testing circuits
  • send and receive cells via inbound OR connections from other relays to our own ORPorts
  • send and receive cells via outbound OR connections to other relays' ORPorts

This isn't a perfect test, there are a few false positives:

  • relays with multiple IPv4 or IPv6 ORPorts, where only some ports are reachable:
    • (this configuration is uncommon, but multiple ORPorts are supported)
    • the final extend cell contains the advertised IPv6 address of the self-testing relay
    • if the extending relay already has a connection to an old but working ORPort, it may use that connection instead
    • so these tests can pass, even when the advertised ORPort is unreachable
  • relays whose keys have been copied from another relay in the consensus, for similar reasons
    • this configuration is rare and unsupported

From proposal 311, section 4.2:
https://gitweb.torproject.org/torspec.git/tree/proposals/311-relay-ipv6-reachability.txt#n283

Child Tickets

TicketTypeStatusOwnerSummary
#33227enhancementclosedteorProp 311: 5.1. Update Tor Spec Relay Subprotocols

Change History (23)

comment:1 Changed 9 months ago by teor

Description: modified (diff)

comment:2 Changed 9 months ago by teor

Type: defectenhancement

comment:3 Changed 9 months ago by teor

Sponsor: Sponsor55-must

comment:4 Changed 6 months ago by teor

Description: modified (diff)

Update the end of the description with a robust self-test design.

Here are some additional notes:

Once we validate the created cell, we have confirmed that the final remote relay has our private keys.

If our relay was set up using a copy of another relay's keys, then we might have connected to that relay. We could test that the create cells we receive match create cells that we have actually sent. But that seems unnecessary, because duplicate keys are rare. (And authorities enforce key uniqueness in the consensus.)

At best, it would provide a warning for operators who have accidentally duplicated their keys. (Operators can override these checks using AssumeReachable.)

comment:5 Changed 6 months ago by teor

Summary: Prop 311: 4.2. Checking IPv6 ORPort ReachabilityProp 311: 4.2. Implement IPv6 ORPort Reachability Self-Tests

comment:6 Changed 6 months ago by teor

We can start work on this ticket now.

We can start working on:

  • circuit-building, and
  • separate IPv4 and IPv6 reachability flags.

We can open another ticket for the Relay protocol version checks,
or move that work into the protocol version ticket #33226.

comment:7 Changed 6 months ago by teor

Actual Points: 1.1
Reviewer: nickm

Here's an initial PR, to show that the IPv6 extend and reachability code can work:

It sends IPv4-only and IPv6-only extends on reachability circuits, to any third hop.

Here's what we need to do before we merge:

  • 4.2.1 Selecting the Final Extending Relay (this ticket)
    • 5. Declare Support for Subprotocol Version "Relay=3" (#33226)
  • unit tests
  • changes files

Then we can iterate on the remaining work.

I'd like an initial review on this code. But it's not blocking anything for me. I have a big list of tasks I still need to do :-)

comment:8 Changed 6 months ago by nickm

Done with initial review; see comments on github.

comment:9 Changed 6 months ago by teor

Description: modified (diff)

Notes about some false positives in the reachability checks.

comment:10 Changed 6 months ago by teor

Description: modified (diff)

Say "port forwarding" for clarity

comment:11 in reply to:  7 ; Changed 6 months ago by teor

Thanks for the review!

I might be a bit busy next week with non-work tasks, but I'll try to do at least one revision.

Replying to teor:

It sends IPv4-only and IPv6-only extends on reachability circuits, to any third hop.

Here's what we need to do before we merge:

  • 4.2.1 Selecting the Final Extending Relay (this ticket)
    • 5. Declare Support for Subprotocol Version "Relay=3" (#33226)
  • unit tests
  • changes files

Then we can iterate on the remaining work.

My next priority is to:

  • Separate tor's IPv4 and IPv6 reachability flags (#34067)

So we know that we're actually getting IPv6 connections. (I'm pretty sure we are now, but we don't want to accidentally break them.)

comment:12 in reply to:  11 Changed 6 months ago by teor

Status: assignedneeds_review

I've done another revision.

This branch is not ready for merge yet, but the basic design and functionality should be there. I still have to:

Replying to teor:

  • 4.2.1 Selecting the Final Extending Relay (this ticket)
    • 5. Declare Support for Subprotocol Version "Relay=3" (#33226)
  • unit tests
  • changes files

Then I think we can safely merge.

Since this PR is getting big, I'd like to leave any other significant changes for a later PR and ticket:

Replying to teor:

My next priority is to:

  • Separate tor's IPv4 and IPv6 reachability flags (#34067)

So we know that we're actually getting IPv6 connections. (I'm pretty sure we are now, but we don't want to accidentally break them.)

The extend_info changes in #34069, and dual-stack EXTEND2 cells aren't a high priority. The code will work either way.

Last edited 6 months ago by teor (previous) (diff)

comment:13 Changed 6 months ago by teor

Description: modified (diff)

Spec change: let's check for inbound create cells from other relays (rather than relays or clients)

comment:14 Changed 6 months ago by nickm

All the changes on github, up through 492c512, look fine to me.

I think we could delay #33226 till after this merges if you want to, but we can't delay the unit tests (and integration testing).

comment:15 in reply to:  14 Changed 6 months ago by teor

Replying to nickm:

All the changes on github, up through 492c512, look fine to me.

I think we could delay #33226 till after this merges if you want to, but we can't delay the unit tests (and integration testing).

I'd like to declare Relay=3 so this new code can check for it. Otherwise it will send IPv6 extends to a bunch of older relays that don't handle them. That's not harmful in any way, but it may be confusing.

And then unit tests :-)

comment:16 Changed 6 months ago by teor

Actual Points: 1.13

I've updated the PR with the changes from #33226, and some refactoring:

It took a bit longer than I thought, I forgot how tricky node filters are. (I needed to change the protocol versions, circuit-level flags, and node-level flags.)

That's all the features, but it still needs unit tests and changes file.

Chutney already does integration tests:

  • using tor's make test-network-all locally, and
  • via Travis CI on macOS (dual-stack) and Linux (IPv4 only).

comment:17 Changed 6 months ago by nickm

I've left comments up through 04b6b7b.

Last edited 6 months ago by nickm (previous) (diff)

comment:18 Changed 6 months ago by teor

I just pushed a final refactor that cleans up a bunch of duplicate code. But I think it's best that we deal with the latest refactor commits in another ticket. (They shouldn't hold up merging this feature.) So I opened #34200 for the refactor, and split the branch.

The (shorter) PR still needs unit tests and changes files.

The stem tests will fail due to:

Reverting to the most recent stem release, or stem commit 13e401d, should resolve this issue.

comment:19 Changed 5 months ago by teor

Actual Points: 34

I have pushed some extra tests and changes files:

This branch also fixes the following Rust protover bugs in master:

  • #34248: add HSIntro=5 in Rust
  • #34251: fix error handling in Rust protover is supported

I've left those tickets open, because we need to backport those changes.

Here are the features and functions that don't have unit tests:

  • launch IPv6 ORPort self-test circuits
    • fmt_addr32_port()
    • fmt_af_family() - refactoring
    • fmt_addr_family() - refactoring
    • router_get_orport()
    • extend_info_from_router()
    • router_do_orport_reachability_checks()
    • router_do_dirport_reachability_checks() - minor modifications and refactoring
    • router_do_reachability_checks() - minor modifications and refactoring
    • inform_testing_reachability() - minor modifications and refactoring
  • when launching IPv6 ORPort self-test circuits, make sure that the last hop supports IPv6 extends
    • node_supports_initiating_ipv6_extends()
    • node_supports_accepting_ipv6_extends()
    • router_add_running_nodes_to_smartlist() - added IPv6 extend check

These tests should be much easier to write on top of the refactor in #34200.

This code runs in chutney's IPv6 networks, which are part of Tor's CI. But chutney doesn't fully test IPv6 reachability yet. Chutney will test more of this code, when we split tor's IPv4 and IPv6 reachability flags in #34067.

comment:20 Changed 5 months ago by nickm

I don't see any remaining blockers to merge this. The only thing I need to do first is make 100% sure I understand what is changing, so I can maintain and extend it going forward.

comment:21 Changed 5 months ago by nickm

This is merged to master with #34200; now cleaning up child tickets.

comment:22 Changed 5 months ago by nickm

Milestone: Tor: 0.4.4.x-finalTor: 0.4.5.x-final

comment:23 Changed 5 months ago by nickm

Resolution: implemented
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.