Opened 3 months ago

Last modified 3 days ago

#22805 needs_review defect

Remove or_circuit_t.is_first_hop, because it's not accurate any more

Reported by: teor Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: technical-debt, security-review, review-group-23
Cc: teor Actual Points: .3
Parent ID: Points: 1
Reviewer: asn Sponsor:

Description

Tor relays set or_circuit_t.is_first_hop when a client uses CREATE_FAST.
But recent tor clients only use CREATE_FAST when they don't know a relay's onion key.

So we need to replace this with channel_is_client().

Child Tickets

TicketStatusOwnerSummaryComponent
#21585closedCheck code that uses consensus membership to find clientsCore Tor/Tor
#22136closedMake entry relays expire idle client and bridge connectionsCore Tor/Tor

Change History (14)

comment:1 Changed 2 months ago by teor

Also, relays use CREATE_FAST if they make an OR connection without a consensus (do they ever actually do this? Clients use CREATE_FAST when connecting to fallback directories' ORPorts, but relays use the DirPort).

In any case, we don't want to flag CREATE_FAST users as clients any more.

comment:2 Changed 2 weeks ago by nickm

Actual Points: .3
Owner: set to nickm
Status: newaccepted

My branch bug22805 now fixes the issues here that I could identify.

comment:3 Changed 2 weeks ago by nickm

Status: acceptedneeds_review

comment:4 Changed 2 weeks ago by nickm

note to self: does this break test-network-all? Try it before merging.

comment:5 Changed 2 weeks ago by nickm

Keywords: review-group-23 added

Put 0.3.2 needs_review and merge_ready tickets into review-group-23.

comment:6 Changed 9 days ago by asn

Reviewer: asn

comment:7 Changed 7 days ago by asn

Hello. I'm not very experienced with this channel stuff, so I might be wrong but here are my review comments:

  • In comment:1, teor said we don't want to flag CREATE_FAST users as clients any more.. However, I see that in command_process_create_cell() we unconditionally call channel_mark_client(chan); if we receive a CREATE_FAST cell. It also contains a comment to that effect, which might be outdated given what teor said above. Does it also need a connection_or_digest_is_known_relay(chan->identity_digest) like the call above it? Or perhaps it's ok as is?
  • The comment in circuit_expire_old_circuits_serverside() is now outdated since it still mentions create_fast as an identifier.
  • The branch contains aa83959ce5 on top which seems to be related to the merged #21585. It seemed unrelated to this ticket so I didn't review it. Let me know if I should take a look please.

FWIW, running test-full succeeds with the branch.

Marking this as needs_revision just for the minor comment fix above.

comment:8 Changed 6 days ago by asn

Status: needs_reviewneeds_revision

comment:9 Changed 6 days ago by nickm

oh dear; aa83959ce5 should be its own ticket. Opened #23533 for that.

comment:10 Changed 6 days ago by nickm

I've started a new bug22805_v2 with aa83959ce5 removed.

comment:11 Changed 6 days ago by nickm

Status: needs_revisionneeds_review

I've tried to address your issues mentioned above in bug22805_v2. I'm not 100% sure that this is the best solution, but it does seem like an improvement.

comment:12 Changed 5 days ago by asn

282696e7b9 is an interesting change. Do we care about false positives and are they possible? Could we accidentally mark a channel as client if connection_or_digest_is_known_relay() returns false because we have a consensus desynch (too old/new consensus)?

I'm mainly concerned about channel_get_for_extend() skipping is_client channels, so if false positives are possible we should think if we care here.

comment:13 Changed 5 days ago by nickm

Cc: teor added

Hm. That's true, maybe we should just remove that blob of code entirely.

What do you think, teor?

comment:14 Changed 3 days ago by teor

I think we can take out all the code that deals with CREATE_FAST, including the code around cfe6b444d652464b0b6bb18b4a4a24b0cfb0da81, and just check for a non-zero identity digest.

If a connecting peer has a zero identity digest, it's a client/bridge, if it doesn't, it's a relay. (A listening peer is always a relay. Interestingly, bridges look like relays to clients, but look like clients to public relays.)

If a connecting peer uses CREATE_FAST, it might be an old client, or a bootstrapping client, or a bootstrapping relay (on 0.2.9 and later).

But I'm unsure what happens after the initial circuit, when a bootstrapping relay A uses CREATE_FAST to B.

Does A authenticate to the listening relay B once A has a consensus?

Or, if A has authenticated B, but B never authenticated A:

  • does A discard its early connection to B?
  • does A use its early connections for client extends to B, but B doesn't use that connection for client extends to A?
Last edited 3 days ago by teor (previous) (diff)
Note: See TracTickets for help on using tickets.