Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#5044 closed defect (fixed)

Mismatch between dir-spec.txt and routerparse.c

Reported by: karsten Owned by:
Priority: Medium Milestone:
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In #5041 we learned that leaving out contact lines from votes is permitted by the spec, but not by the code. I briefly compared dir-spec.txt and routerparse.c to find more mismatches between spec and code. Here's what I found:

  • protocols lines are specified in dir-spec.txt and written to server descriptors, but they are not parsed in routerparse.c.
  • consensus-method lines are specified as "Exactly once for consensuses" but implemented using the T01 macro in routerparse.c.
  • contact lines are specified as "At most once" for votes and authority sections of consensuses, but are implemented using the T1 and T1N macro in routerparse.c.
  • legacy-key-dir lines are specified as legacy-key in dir-spec.txt.
  • s lines are specified as "At most once" in dir-spec.txt, but are implemented using the T1 macro in routerparse.c.

I didn't prepare a patch, because I didn't know whether to fix spec or code. If someone tells me, I can prepare one.

Child Tickets

Attachments (1)

0001-Fix-mismatches-between-dir-spec.txt-and-routerparse..patch (1.8 KB) - added by karsten 8 years ago.
Patch for dir-spec.txt

Download all attachments as: .zip

Change History (6)

comment:1 in reply to:  description ; Changed 8 years ago by arma

Replying to karsten:

  • protocols lines are specified in dir-spec.txt and written to server descriptors, but they are not parsed in routerparse.c.

I think that's fine. We'll parse them once we care what they say. (And we'll want them to be there for the current versions, once new versions say something different, so we can tell the difference.)

  • consensus-method lines are specified as "Exactly once for consensuses" but implemented using the T01 macro in routerparse.c.

I could go either way here. Looks like a missing value means consensus method 1. We're way past consensus method one now -- we'll never use that. So maybe T1 is slightly better.

  • contact lines are specified as "At most once" for votes and authority sections of consensuses, but are implemented using the T1 and T1N macro in routerparse.c.

I'm fine making these optional. Note that authorities are required to set ContactInfo (see config.c). But we can look it up in their relay if we're confused -- so long as we don't do Sebastian's plan of having authorities not be relays from #3023, that is.

  • legacy-key-dir lines are specified as legacy-key in dir-spec.txt.

And it would appear they are 'legacy-dir-key' in the code? This is a case where we should fix the spec to match the code.

  • s lines are specified as "At most once" in dir-spec.txt, but are implemented using the T1 macro in routerparse.c.

A few months ago I wrote some comment on some trac ticket expressing concern that missing the s line would make parsing harder or riskier in some way. I wonder what that was.

comment:2 in reply to:  1 Changed 8 years ago by karsten

Status: newneeds_review

Replying to arma:

Replying to karsten:

  • protocols lines are specified in dir-spec.txt and written to server descriptors, but they are not parsed in routerparse.c.

I think that's fine. We'll parse them once we care what they say. (And we'll want them to be there for the current versions, once new versions say something different, so we can tell the difference.)

Right. Nothing to change here then.

  • consensus-method lines are specified as "Exactly once for consensuses" but implemented using the T01 macro in routerparse.c.

I could go either way here. Looks like a missing value means consensus method 1. We're way past consensus method one now -- we'll never use that. So maybe T1 is slightly better.

Hmm, now that I think more about it, I think changing the code is problematic. dir-spec.txt says:

   Before generating a consensus, an authority must decide which consensus
   method to use.  To do this, it looks for the highest version number
   supported by more than 2/3 of the authorities voting.  If it supports this
   method, then it uses it.  Otherwise, it falls back to method 1.

I think that means we'll have to fix the spec, not the code.

  • contact lines are specified as "At most once" for votes and authority sections of consensuses, but are implemented using the T1 and T1N macro in routerparse.c.

I'm fine making these optional. Note that authorities are required to set ContactInfo (see config.c). But we can look it up in their relay if we're confused -- so long as we don't do Sebastian's plan of having authorities not be relays from #3023, that is.

Sounds like it's easier (and still correct) to fix the spec here.

  • legacy-key-dir lines are specified as legacy-key in dir-spec.txt.

And it would appear they are 'legacy-dir-key' in the code? This is a case where we should fix the spec to match the code.

Agreed.

  • s lines are specified as "At most once" in dir-spec.txt, but are implemented using the T1 macro in routerparse.c.

A few months ago I wrote some comment on some trac ticket expressing concern that missing the s line would make parsing harder or riskier in some way. I wonder what that was.

So, s lines have always been included in network statuses, AFAIK. If a relay doesn't have any flags, which can be in a bridge status or in a consensus prior to removing non-Running relays, the s line is present, but simply contains no flags.

I think fixing the spec is correct here, too.

I'm attaching a patch with the dir-spec changes.

Changed 8 years ago by karsten

Patch for dir-spec.txt

comment:3 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged; thanks!

comment:4 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:5 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.