The channel is_client flag is inaccurate

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.

changed milestone to %Tor: 0.3.1.x-final

added actualpoints::0.5 component::core tor/tor milestone::Tor: 0.3.1.x-final owner::teor parent::24898 points::0.5 priority::medium resolution::fixed reviewer::nickm severity::normal status::closed type::defect version::tor 0.2.4.23 labels

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

Trac:
Milestone: Tor: unspecified to Tor: 0.3.1.x-final

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}

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?

Trac:
Status: new to needs_review
Points: N/A to 0.5

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

Trac:
Status: needs_review to needs_revision

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 (moved).

Trac:
Actualpoints: N/A to 0.5
Status: needs_revision to needs_review
Version: N/A to Tor: 0.2.4.23

Setting owner

Trac:
Status: needs_review to assigned
Owner: N/A to teor

Trac:
Status: assigned to needs_review

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.

Trac:
Reviewer: N/A to nickm
Status: needs_review to needs_revision

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 (moved) in 0.3.2 for this.

Trac:
Status: needs_revision to needs_review

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

Thank you! squashed and merged!

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

We will need to backport this with #24898 (moved) to 0.3.0 and earlier. (If we backport #24898 (moved) at all.)

Trac:
Parent: N/A to #24898 (moved)

closed

changed time estimate to 4h

added 4h of time spent

mentioned in issue #21585 (moved)

mentioned in issue #22090 (moved)

mentioned in issue #22136 (moved)

mentioned in issue #22688 (moved)

mentioned in issue #22717 (moved)

mentioned in issue #24898 (moved)

moved to tpo/core/tor#21406 (closed)

mentioned in issue tpo/core/tor#21585 (closed)

mentioned in issue tpo/core/tor#22090 (closed)

mentioned in issue tpo/core/tor#22136 (closed)