Opened 16 months ago
Last modified 4 months ago
#27190 needs_revision defect
disparate duplicate subproto handling in protover
Reported by: | cyberpunks | Owned by: | |
---|---|---|---|
Priority: | Medium | Milestone: | Tor: unspecified |
Component: | Core Tor/Tor | Version: | |
Severity: | Normal | Keywords: | rust, 035-deferred-20190115, 041-proposed, 033-unreached-backport |
Cc: | nickm | Actual Points: | |
Parent ID: | Points: | ||
Reviewer: | Sponsor: |
Description
protover.c
treats Link=1 Link=1
and Link=1
identically, allowing duplicate entries without complaint, though it does explicitly check for duplicates to avoid double-counting it as two votes for the same version.
protover.c
also treats Link=1 Link=2
and Link=1-2
the same, while the rust implementation of protover treats Link=1 Link=2
as if it were Link=2
.
Child Tickets
Ticket | Status | Owner | Summary | Component |
---|---|---|---|---|
#27722 | new | rust protover doesn't canonicalize adjacent and overlapping ranges | Core Tor/Tor |
Change History (13)
comment:1 Changed 16 months ago by
comment:2 follow-up: 4 Changed 16 months ago by
Keywords: | 033-backport 034-backport added |
---|
No, duplicate subprotocol entries are allowed by the spec:
https://gitweb.torproject.org/torspec.git/tree/dir-spec.txt#n780
The rust code is wrong, and should be fixed.
comment:3 Changed 16 months ago by
Milestone: | → Tor: unspecified |
---|
comment:4 follow-up: 5 Changed 16 months ago by
Replying to teor:
No, duplicate subprotocol entries are allowed by the spec:
That spec seems ambiguous and could be clarified in either direction.
If it's clarified to forbid duplicates--it already says the entries should be sorted alphabetically--it would become simpler to also verify that there aren't any overlapping ranges, which the spec says shouldn't happen but the C implementation doesn't check for right now.
Also, make test-network-all
passes on branch protodupes1, at least.
comment:5 Changed 16 months ago by
Replying to cyberpunks:
Replying to teor:
No, duplicate subprotocol entries are allowed by the spec:
That spec seems ambiguous and could be clarified in either direction.
If it's clarified to forbid duplicates--it already says the entries should be sorted alphabetically--it would become simpler to also verify that there aren't any overlapping ranges, which the spec says shouldn't happen but the C implementation doesn't check for right now.
"must" is an absolute requirement, but "should" is a recommendation:
SHOULD This word, or the adjective "RECOMMENDED", mean that there
may exist valid reasons in particular circumstances to ignore a
particular item, but the full implications must be understood and
carefully weighed before choosing a different course.
https://gitweb.torproject.org/torspec.git/tree/dir-spec.txt#n19
https://tools.ietf.org/html/rfc2119
Therefore, Tor's parser can tolerate duplicates, unsorted ranges, and overlapping ranges. I think that it's worth accepting them, to support a broad range of relay implementations. As a trivial example, it's worth accepting both "Foo=1,2" and "Foo=1-2".
But all directory authority implementations MUST generate identical consensuses:
https://gitweb.torproject.org/torspec.git/tree/dir-spec.txt#n1614
Therefore:
- all directory authority parser implementations MUST interpret edge cases as the same protocol versions, and
- all directory authority serialisation implementations MUST generate exactly the same text for the same protocol versions.
So I think we should fix the Rust to match the C.
We can also open another ticket to clarify the spec, but I'm not sure exactly what changes to make. We don't try to specify the exact format of a consensus in the spec - we only provide enough detail to parse a consensus.
Also,
make test-network-all
passes on branch protodupes1, at least.
Unfortunately, those tests don't test all the edge cases. And apparently neither do our unit tests. So we should add more unit tests.
comment:6 Changed 16 months ago by
Status: | new → needs_revision |
---|
comment:7 Changed 16 months ago by
Cc: | nickm added |
---|
Since my last comment raises some spec issues, I'm cc'ing nickm.
comment:8 Changed 16 months ago by
Milestone: | Tor: unspecified → Tor: 0.3.5.x-final |
---|
comment:9 Changed 11 months ago by
Keywords: | 035-deferred-20190115 041-proposed added |
---|---|
Milestone: | Tor: 0.3.5.x-final → Tor: unspecified |
Marking a number of 0.3.5 tickets as possible, maybe even a good idea, for later. Possibly backportable, some of them. But not currently things to do as part of 0.3.5 stabilization.
comment:10 Changed 9 months ago by
Keywords: | 033-backport removed |
---|
These open, non-merge_ready tickets can not get backported to 0.3.3, because 0.3.3 is now unsupported.
comment:11 Changed 9 months ago by
Keywords: | 033-backport-unreached added |
---|
Hmm, I guess they should still get 033-backport-unreached
comment:12 Changed 6 months ago by
Keywords: | 034-backport removed |
---|
Removing 034-backport from all open tickets: 034 has reached EOL.
comment:13 Changed 4 months ago by
Keywords: | 033-unreached-backport added; 033-backport-unreached removed |
---|
Fix 033-unreached-backport spelling.
The C code already has to check for duplicates to prevent double-voting. Could both C and Rust be changed to consider it an error to have duplicate subprotocol entries?