Opened 4 years ago

Closed 21 months ago

Last modified 21 months ago

#17882 closed defect (wontfix)

Remove needless *_support_ntor()

Reported by: nickm Owned by:
Priority: Low Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Trivial Keywords: easy tor-client code-removal technical-debt
Cc: Actual Points:
Parent ID: Points: small
Reviewer: Sponsor:

Description

While reviewing #7144, I noticed that circuit_cpath_supports_ntor() is pointless: We no longer allow TAP-only relays on the network, IIUC.

Assuming that I've got that right, we can rip out some code.

Child Tickets

Attachments (1)

0001-Removes-needless-circuit_cpath_supports_ntor.patch (3.2 KB) - added by irl 21 months ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 4 years ago by nickm

(We should do this *after* #7144 is merged, so we don't make a huge conflict)

comment:2 Changed 4 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.???

It is impossible that we will fix all 277 currently open 028 tickets before 028 releases. Time to move some out. This is my first pass through the "new" and "reopened" tickets, looking for things to move to ???.

comment:3 Changed 4 years ago by nickm

Points: small

comment:4 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:5 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:6 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:7 Changed 2 years ago by nickm

Keywords: tor-client code-removal technical-debt added

comment:8 Changed 21 months ago by irl

Status: newneeds_review

In 33da2abd05, directory authorities began rejecting relays that did not publish a valid ntor key. This means that the consensus will only contain relays that do have ntor keys published in their descriptors.

circuit_cpath_supports_ntor is indeed redundant and only duplicating the check that has already been performed by the directory authorities.

There is one other function in the _supports_ntor realm: extend_info_supports_ntor. dir-spec currently does not require ntor in extra info descriptors and so this is not yet removable. For example, v2 hidden services still rely on the TAP handshake.

The above patch removes circuit_cpath_supports_ntor. make test passes and I have tested client functionality for bootstrapping, "normal" circuits, v2 hidden service circuits and v3 hidden service circuits. It also updates a comment in dirserv.c to explain that clients will now assume ntor support, where it previously said clients would check.

comment:9 in reply to:  8 Changed 21 months ago by teor

Resolution: wontfix
Status: needs_reviewclosed

Replying to irl:

In 33da2abd05, directory authorities began rejecting relays that did not publish a valid ntor key. This means that the consensus will only contain relays that do have ntor keys published in their descriptors.

circuit_cpath_supports_ntor is indeed redundant and only duplicating the check that has already been performed by the directory authorities.

This is not an appropriate justification for this change: we expect newer clients to work with older authorities and relays. This is important for private and test tor networks. Clients can't trust the authorities to check ntor support for them.

However, recent client versions ignore descriptors that don't have an ntor key.

But, we still need to check for the edge cases described in onion_populate_cpath().
We will never be able to bootstrap using ntor, so we must keep this function.
However, we can simplify it when we get rid of v2 onion services.

There is one other function in the _supports_ntor realm: extend_info_supports_ntor. dir-spec currently does not require ntor in extra info descriptors and so this is not yet removable. For example, v2 hidden services still rely on the TAP handshake.

This is about extend_info structures, not extra info descriptors.
Also, bootstrapping relies on CREATE_FAST, not ntor.

The above patch removes circuit_cpath_supports_ntor. make test passes and I have tested client functionality for bootstrapping, "normal" circuits, v2 hidden service circuits and v3 hidden service circuits. It also updates a comment in dirserv.c to explain that clients will now assume ntor support, where it previously said clients would check.

The patch removes two important security checks:

  • we must not use TAP or CREATE_FAST for multi-hop paths, unless those paths are onion service paths, and
  • we must not use TAP or CREATE_FAST for single-hop paths, unless we don't have the ntor key for the node.

I'm sorry, but I don't think we should accept this patch or remove this function.

comment:10 Changed 21 months ago by teor

(I opened #24509 because I noticed that circuit_can_use_tap() should exclude v3 onion services, but it doesn't.)

comment:11 Changed 21 months ago by irl

Awesome! Thanks for the detailed review. I primarily took this ticket to gain some context for #13466 and #24047, and this has definitely helped, and we've cleaned up a ticket in the process.

Note: See TracTickets for help on using tickets.