Opened 15 months ago

Closed 4 months ago

#20895 closed task (fixed)

Split node_supports_ed25519_link_authentication into two or three separate functions

Reported by: nickm Owned by: nickm
Priority: Low Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: refactor, review-group-24
Cc: Actual Points:
Parent ID: #15054 Points: 2
Reviewer: dgoulet Sponsor:

Description

As of our #15056 code to implement the circuit-side part of prop220, we have a function, node_supports_ed25519_link_authentication, which isn't quite right.

Sometimes, when we use it, we mean, "If we try to connect to this node, should we expect that we will authenticate its ed25519 identity?"

Sometimes, we mean "If we try to make a connection through some random node to this node, authenticating with its ed25519 identity, will that work?"

And sometimes we mean "I'm thinking of asking _that_ node to extend a circuit to _this_ node. Should I tell it about _this_ node's Ed25519 identity, or would it take it the wrong way?"

I wrote a patch here in response to dgoulet's review of my #15056 branch, but on reflection, it isn't right. I'll attach it, but it's a bad start, and it's too complex, and maybe you should ignore it?

Child Tickets

Attachments (1)

newticket (8.3 KB) - added by nickm 15 months ago.
Patch!

Download all attachments as: .zip

Change History (19)

Changed 15 months ago by nickm

Attachment: newticket added

Patch!

comment:1 Changed 15 months ago by nickm

Parent ID: #15056#15054

comment:2 Changed 14 months ago by nickm

Points: 2

comment:3 Changed 13 months ago by nickm

Type: defecttask

comment:4 Changed 13 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:5 Changed 13 months ago by dgoulet

Keywords: refactor added
Milestone: Tor: 0.3.0.x-finalTor: 0.3.1.x-final

comment:6 Changed 12 months ago by nickm

Priority: MediumLow

Lower priority on some of my assigned tickets

comment:7 Changed 10 months ago by nickm

Keywords: 031-reach added

comment:8 Changed 10 months ago by nickm

Milestone: Tor: 0.3.1.x-finalTor: 0.3.2.x-final

I don't think these are happening in 0.3.1

comment:9 Changed 9 months ago by nickm

Keywords: 031-reach removed

comment:10 Changed 5 months ago by nickm

Sometimes, when we use it, we mean, "If we try to connect to this node, should we expect that we will authenticate its ed25519 identity?"

This is what is currently implemented by node_supports_ed25519_link_authentication().

It's what we want for the instances currently in connection_or.c and dirserv.c, and one of the two instances in circuitbuild.c.

The logic here is that we know which link authentication versions we support, so we know that if we try to use an ed25519 link authentication mechanism, it has to be one of the ones we support.

Sometimes, we mean "If we try to make a connection through some random node to this node, authenticating with its ed25519 identity, will that work?"

This seems to be what we want for one of the two instances in circuitbuild.c, and the instance in hs_service.c.

The real question in this case is, "should we tell people to use the ed25519 id for node X when it supports some link authentication mechanism we don't even recognize, if it doesn't support linkauth=3"?

I think "yes". I'll write a patch.

And sometimes we mean "I'm thinking of asking _that_ node to extend a circuit to _this_ node. Should I tell it about _this_ node's Ed25519 identity, or would it take it the wrong way?"

This invokes a concept we don't currently have: the idea that two well-behaved supported relays might not be able to talk to one another. We aren't planning to change this soon, and if we do, it will take more machinery, so I suggest we ignore it.

comment:11 Changed 5 months ago by nickm

Status: acceptedneeds_review

I've tried to take the future-proofing approach above in my branch ticket20895. To support non-clique topologies better is out of scope here.

comment:12 Changed 5 months ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewneeds_revision

I'm not sure I fully understand here the use of protocol_list_supports_protocol_or_later() which goes like this:

+      if (range->high >= version) {
+        contains = 1;
+        goto found;

So let's say I have a relay with LinkAuth 2,4 which deliberately not list "3", this "or later" function will return true for return protocol_list_supports_protocol_or_later(protos, PRT_LINKAUTH, 3); but it should really not actually say that it supports LinkAuth 3

Can we really imply that if a relay supports 4 then it has to support 3 ? What if we disabled it or made it obsolete or consensus put if off?

comment:13 Changed 5 months ago by nickm

So, my rationale is that if we want to know "does this support 3", then we'll be looking at protocol_list_supports_protocol. That already exists.

I'm not claiming that if a relay supports 4 it must support 3: I'm claiming that if it supports 4, it is okay to include an ed25519 key in its link specifiers.

comment:14 Changed 5 months ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: 0.3.3.x-final
Status: needs_revisionneeds_review

comment:15 Changed 5 months ago by nickm

(Deferred to 0.3.3, since any problems here are likely to be subtle)

comment:16 Changed 4 months ago by nickm

Keywords: review-group-24 added

review-group-24 is now open.

comment:17 Changed 4 months ago by dgoulet

Status: needs_reviewmerge_ready

lgtm;

comment:18 Changed 4 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

merged to master. thanks for the review!

Note: See TracTickets for help on using tickets.