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

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

changed milestone to %Tor: 0.3.2.x-final

added actualpoints::.3 component::core tor/tor milestone::Tor: 0.3.2.x-final owner::nickm parent::24898 points::1 priority::medium resolution::implemented review-group-23 reviewer::asn security-review severity::normal status::closed technical-debt type::defect labels

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.

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

Trac:
Actualpoints: N/A to .3
Owner: N/A to nickm
Status: new to accepted

Trac:
Status: accepted to needs_review

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

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

Trac:
Keywords: N/A deleted, review-group-23 added

Trac:
Reviewer: N/A to 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 (moved). 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.

Trac:
Status: needs_review to needs_revision

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

I've started a new bug22805_v2 with aa83959ce5 removed.

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.

Trac:
Status: needs_revision to needs_review

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.

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

What do you think, teor?

Trac:
Cc: N/A to 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?

Trac:
Status: needs_review to needs_revision

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.

Trac:
Status: needs_revision to needs_review

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.

Trac:
Status: needs_review to merge_ready

merged; made new ticket as #23622 (moved). Thanks!

Trac:
Status: merge_ready to closed
Resolution: N/A to implemented

We might also want to check these changes in #24898 (moved)

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

Remove useless CC

Trac:
Cc: teor to N/A

closed

changed time estimate to 8h

added 2h 24m of time spent

mentioned in issue #23622 (moved)

mentioned in issue #24898 (moved)

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

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