Opened 5 months ago

Closed 8 weeks ago

#21406 closed defect (fixed)

The channel is_client flag is inaccurate

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version: Tor: 0.2.4.23
Severity: Normal Keywords:
Cc: Actual Points: 0.5
Parent ID: Points: 0.5
Reviewer: nickm Sponsor:

Description

The channel_t is_client flag is inaccurate: relays set it when the other end uses a CREATE_FAST cell, but usecreatefast is set to 0 in the consensus.

This means that the only time CREATE_FAST is used is when a relay gets an extend request *without* an ntor key, and the purpose of the circuit is *not* one of the hidden service purposes where TAP is allowed.

See should_use_create_fast_for_circuit() for details.

Child Tickets

Change History (11)

comment:1 Changed 5 months ago by teor

In fact, the only time we use CREATE_FAST consistently is when bootstrapping.
See #21407.

comment:2 Changed 5 months ago by nickm

  • Milestone changed from Tor: unspecified to Tor: 0.3.1.x-final

comment:3 Changed 4 months ago by teor

We could replace this with a check if the remote end:

  • connected to us: chan->conn->handshake_state->started_here
  • has neither a RSA or ed25519 identity key: chan->conn->handshake_state->{authenticated_rsa_peer_id,authenticated_ed25519_peer_id}

comment:4 Changed 4 months ago by teor

  • Points set to 0.5
  • Status changed from new to needs_review

See my branch connection-with-client which fixes the channel flag. Should this be equivalent to the connection flag? Should we set one, and then make both accessors use it?

comment:5 Changed 4 months ago by nickm

  • Status changed from needs_review to needs_revision

This code looks sensible. IMO it makes sense for there to be only one flag, and have the accessors both look at it.

comment:6 Changed 4 months ago by teor

  • Actual Points set to 0.5
  • Status changed from needs_revision to needs_review
  • Version set to Tor: 0.2.4.23

Please see my branch connection-with-client-v2.

The flag in or_connection_t was unused (there wasn't even an accessor), so I removed it with a note about how to get the channel value.

I added a comment noting that CREATE_FAST is a legacy mechanism for checking for clients.

We can create an accessor for this flag from an or_connection_t when we need it. (I suspect that the traffic padding code and custom measurement code might be interested in this flag from an or_connection.)

I also opened another ticket to check for places where we're identifying clients by their absence from the consensus: #21585.

comment:7 Changed 4 months ago by nickm

  • Owner set to teor
  • Status changed from needs_review to assigned

Setting owner

comment:8 Changed 4 months ago by nickm

  • Status changed from assigned to needs_review

comment:9 follow-up: Changed 4 months ago by nickm

  • Reviewer set to nickm
  • Status changed from needs_review to needs_revision

I think the change in connection_or_check_valid_tls_handshake() may be wrong: This is about the certificate received in the TLS handshake, not the certificate received in the CERTS cell during the v3 Tor handshake. But in the v3 handshake, nobody provides a client certificate during TLS negotiation.

You can test this yourself by adding tor_assert(!has_cert || started_here), and running a test network. (Don't do this in real life, since it would crash whenever somebody tried running an ancient server and/or sending you a client TLS certificate by mistake.)

The other changes look okay to me. I would like to rename "channel_mark_client" to "channel_mark_as_client" and "channel_is_client" to "channel_comes_from_client" or something, but that's another ticket.

comment:10 in reply to: ↑ 9 Changed 2 months ago by teor

  • Status changed from needs_revision to needs_review

Replying to nickm:

I think the change in connection_or_check_valid_tls_handshake() may be wrong: This is about the certificate received in the TLS handshake, not the certificate received in the CERTS cell during the v3 Tor handshake. But in the v3 handshake, nobody provides a client certificate during TLS negotiation.

I pushed a fixup to connection-with-client-v2 that reverts the connection_or_check_valid_tls_handshake() change.

The other changes look okay to me. I would like to rename "channel_mark_client" to "channel_mark_as_client" and "channel_is_client" to "channel_comes_from_client" or something, but that's another ticket.

I opened #22090 in 0.3.2 for this.

comment:11 Changed 8 weeks ago by nickm

  • Resolution set to fixed
  • Status changed from needs_review to closed

I pushed a fixup to connection-with-client-v2 that reverts the connection_or_check_valid_tls_handshake() change.

Thank you! squashed and merged!

Note: See TracTickets for help on using tickets.