Opened 6 months ago

Closed 3 months ago

#22805 closed defect (implemented)

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 (18)

comment:1 Changed 5 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 3 months 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 3 months ago by nickm

Status: acceptedneeds_review

comment:4 Changed 3 months ago by nickm

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

comment:5 Changed 3 months 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 3 months ago by asn

Reviewer: asn

comment:7 Changed 3 months 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 3 months ago by asn

Status: needs_reviewneeds_revision

comment:9 Changed 3 months ago by nickm

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

comment:10 Changed 3 months ago by nickm

I've started a new bug22805_v2 with aa83959ce5 removed.

comment:11 Changed 3 months 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 3 months 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 3 months 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 months 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 months ago by teor (previous) (diff)

comment:15 Changed 3 months ago by nickm

Status: needs_reviewneeds_revision

comment:16 in reply to:  14 ; Changed 3 months ago by nickm

Status: needs_revisionneeds_review

Replying to teor:

I think we can take out all the code that deals with CREATE_FAST, including the code around cfe6b444d652464b0b6bb18b4a4a24b0cfb0da81

I've updated bug22805_v2 with this change.

and just check for a non-zero identity digest.

Hm; that's a bigger change than we'd been talking about here; I think I should open another ticket to change it in 0.3.3. Would that be okay with you?

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?

So, authentication decisions aren't made as part of the CREATE/CREATE2/CREATE_FAST layer: they all happen as part of the connection layer. Relays always offer authentication, and they don't need a consensus to do so. So if two relays are talking, then in theory they should always do so in an authenticated way.

comment:17 in reply to:  16 Changed 3 months ago by teor

Status: needs_reviewmerge_ready

Replying to nickm:

Replying to teor:

I think we can take out all the code that deals with CREATE_FAST, including the code around cfe6b444d652464b0b6bb18b4a4a24b0cfb0da81

I've updated bug22805_v2 with this change.

Let's get this merged.

and just check for a non-zero identity digest.

Hm; that's a bigger change than we'd been talking about here; I think I should open another ticket to change it in 0.3.3. Would that be okay with you?

Sounds like a good plan.

comment:18 Changed 3 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

merged; made new ticket as #23622. Thanks!

Note: See TracTickets for help on using tickets.