Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#3038 closed defect (fixed)

Update dir-spec.txt with microdesc, consensus-flavor info

Reported by: nickm Owned by: nickm
Priority: High Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-auth
Cc: Actual Points:
Parent ID: #4933 Points:
Reviewer: Sponsor:

Description

The microdescriptor and consensus flavor improvements are live in anything running the 0.2.3.0.x master code: they should get into the doc-spec.txt file.

This will involve extracting stuff from proposals 158 and 162 and comparing them against to code to make sure I didn't deviate from the proposals (and if I did, whether I was right to do so).

Child Tickets

Attachments (2)

0001-Start-merging-proposals-158-and-162-into-dir-spec.tx.patch (21.2 KB) - added by karsten 7 years ago.
Patch containing proposals 158 and 162 merged into dir-spec.txt
task-3038-patches.tar (32.5 KB) - added by karsten 7 years ago.
Revised patch containing proposals 158 and 162 merged into dir-spec.txt

Download all attachments as: .zip

Change History (20)

comment:1 Changed 8 years ago by nickm

Owner: set to nickm
Status: newaccepted

comment:2 Changed 8 years ago by nickm

Parent ID: #1748

comment:3 Changed 8 years ago by asn

Parent ID: #4933

comment:4 Changed 7 years ago by karsten

Nick suggested that, as a preliminary step to merging the two proposals into dir-spec.txt, I could compare them to the current code.

I didn't find any obvious discrepancies between spec and code as far as the data formats are concerned. However, I found that /tor/micro/all[.z] and /tor/status-vote/(current|next)/consensus-index[.z] are not implemented as of today. As I understand it, lack of the first URL has performance impacts, because clients need to download all microdescriptors by SHA256 digest. Lack of the second URL might be more problematic, because proposal 162 says "Stage one: authorities begin generating and serving consensus-index files" which are not available.

Changed 7 years ago by karsten

Patch containing proposals 158 and 162 merged into dir-spec.txt

comment:5 Changed 7 years ago by karsten

Status: acceptedneeds_review

I just attached a first patch to dir-spec.txt that contains the relevant parts from proposals 158 and 162 and the or-dev postings referenced from proposal 158. Maybe don't merge this patch just yet, but give me some feedback on whether I added stuff to the right sections and what you think about my comments. I'm happy to work on a new revision next week, unless you want to fix stuff yourself and merge it.

comment:6 Changed 7 years ago by nickm

Status: needs_reviewneeds_revision

If you have time to take another revision on this, great. Otherwise I'll do it.

General comments and notes for stuff we need to fix:

  • It is not a requirement that microdescs be a transform only of the routerdesc; they could also someday include authority-generated info.
  • Clients don't need identity keys; they still have identity fingerprints from the consensus. This lets them correctly make extend cells, and recognize that they've corrected to the right first hop with their entry nodes. I should add something to talk about the security model here.
  • No need to include downside and discussions in spec; that's what proposals are for. Likewise with design suggestions in 4.5.
  • Consensus flavors are not "consensus-like documents": They are consensuses! Regular consensuses have the "ns" flavor. The "microdesc" consensus flavor also exists. It is false to say that their format is as yet unspecified. Must rewrite 3.6.
  • Some of the general language is not really spec-language. A spec is about saying what the system _must_ do in order to be correct; or what it _should_ do to perform well; or so on. Sections 3.6 and 3.6.1 have this problem.
  • Section 3.8 needs to go away; it is not implemented. As does the "an authority further makes the consensus index available at" part later on. As does the part about "Directory caches also fetch the consensus index" later on. We do not merge stuff into the spec if it isn't implemented.

comment:7 in reply to:  6 Changed 7 years ago by karsten

Replying to nickm:

If you have time to take another revision on this, great. Otherwise I'll do it.

Thanks for the review! I'll work on another revision.

comment:8 in reply to:  6 Changed 7 years ago by karsten

Status: needs_revisionneeds_review

Replying to nickm:

  • It is not a requirement that microdescs be a transform only of the routerdesc; they could also someday include authority-generated info.

Right. In fact, they already contain authority-generated info: the exit-policy summary. I changed the first sentence in 3.2.

  • Clients don't need identity keys; they still have identity fingerprints from the consensus. This lets them correctly make extend cells, and recognize that they've corrected to the right first hop with their entry nodes. I should add something to talk about the security model here.

Yes, I think that would be quite useful.

  • No need to include downside and discussions in spec; that's what proposals are for. Likewise with design suggestions in 4.5.

Do you mean my comments in brackets? These mostly contain questions to you when reviewing my patch. Please feel free to remove them if you think they should go away.

Or do you mean other mentions of downsides or design decisions? I tried to reduce those to the absolute minimum. Like, I found it helpful to leave a sentence in that says that creating too many flavors is bad. Please remove anything that you think is too much design.

  • Consensus flavors are not "consensus-like documents": They are consensuses! Regular consensuses have the "ns" flavor. The "microdesc" consensus flavor also exists. It is false to say that their format is as yet unspecified. Must rewrite 3.6.

I changed the phrasing to say that they're variants of the unflavored consensus. I also added a new Section 3.6.1 for the ns consensus flavor.

  • Some of the general language is not really spec-language. A spec is about saying what the system _must_ do in order to be correct; or what it _should_ do to perform well; or so on. Sections 3.6 and 3.6.1 have this problem.

Hmm, I'm not good at spec language. Would you mind fixing the language where it should be more spec-like?

  • Section 3.8 needs to go away; it is not implemented. As does the "an authority further makes the consensus index available at" part later on. As does the part about "Directory caches also fetch the consensus index" later on. We do not merge stuff into the spec if it isn't implemented.

Right. I removed the spec parts mentioning the consensus index in a single patch. Should I create a new Trac ticket for implementing the consensus index and attach the patch there?

What about /tor/micro/all[.z] which is not implemented, either?

I'm attaching a tarball containing three patches:

  • 0001.. is equivalent to the previous patch, except for some Git meta data. Content is the same.
  • 0002.. removes the consensus index parts.
  • 0003.. contains the changes as as mentioned in this comment.

Changed 7 years ago by karsten

Attachment: task-3038-patches.tar added

Revised patch containing proposals 158 and 162 merged into dir-spec.txt

comment:9 Changed 7 years ago by karsten

Pushed the latest patches to branch task-3038 in my shiny new torspec.git repo. (Thanks, Robert and Sebastian.)

comment:10 Changed 7 years ago by rransom

[Should we mention that clients don't learn identity keys anymore
with this approach? Clients only need identity keys for their
entry guards, and in that case they learn the identity key from
the TLS handshake. But clients couldn't check identity keys of
non-entry nodes with the microdescriptor approach anymore, even if
they wanted. -KL]

The microdesc consensus still contains every relay's identity key fingerprint, which (with Tor's current link protocols) is enough for a client or relay to verify that it is opening an OR connection to the right destination relay.

Clients have never been able to check the identity keys of relays which they do not connect to directly; that's why Tor uses ‘onion keys’ to authenticate relays in its circuit-extension handshake protocol.

comment:11 Changed 7 years ago by rransom

Status: needs_reviewneeds_revision

s/artibrary/arbitrary/g

In §3.8 (Consensus index):

"directory-signature" SP algname SP identity SP signing-key-digest

NL signature

[Any number.]

[As "additional-signature" lines in detached signatures, but
without the "flavor" part.]

[Actually, is it a bug that the "flavor" part is missing? -KL]

No; the ‘flavor’ is specified in the ‘document’ line preceding the list of signatures on that document.

comment:12 Changed 7 years ago by rransom

Commit d8bab2a0491dc8dd40e8ed9af96997e841c412e8 (‘Tweak microdesc parts based on Nick's comments.’) should be a ‘fixup’ commit. (See the documentation for the ‘--fixup’ option to ‘git commit’ for what that means.)

comment:13 in reply to:  10 Changed 7 years ago by karsten

Replying to rransom:

[Should we mention that clients don't learn identity keys anymore
with this approach? Clients only need identity keys for their
entry guards, and in that case they learn the identity key from
the TLS handshake. But clients couldn't check identity keys of
non-entry nodes with the microdescriptor approach anymore, even if
they wanted. -KL]

The microdesc consensus still contains every relay's identity key fingerprint, which (with Tor's current link protocols) is enough for a client or relay to verify that it is opening an OR connection to the right destination relay.

Clients have never been able to check the identity keys of relays which they do not connect to directly; that's why Tor uses ‘onion keys’ to authenticate relays in its circuit-extension handshake protocol.

Nick said above that he wants to "add something to talk about the security model here." Please feel free to write something there and remove my comment. Or just delete my comment if it doesn't make sense.

comment:14 in reply to:  11 Changed 7 years ago by karsten

Replying to rransom:

s/artibrary/arbitrary/g

Fixed and pushed to my task-3038 branch, this time committed as fixup.

No; the ‘flavor’ is specified in the ‘document’ line preceding the list of signatures on that document.

I took out Section 3.8, because it's not implemented yet. Your comment should go into the new ticket that I suggested for implementing the consensus index.

comment:15 in reply to:  12 Changed 7 years ago by karsten

Status: needs_revisionneeds_review

Replying to rransom:

Commit d8bab2a0491dc8dd40e8ed9af96997e841c412e8 (‘Tweak microdesc parts based on Nick's comments.’) should be a ‘fixup’ commit. (See the documentation for the ‘--fixup’ option to ‘git commit’ for what that means.)

Makes sense. I don't know how to fix that now without rewriting history of my task-3038 branch. Please feel free to squash the "Tweak..." commit when merging.

Changing to needs_review again.

comment:16 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Looks good enough to me; I'll tweak a little and merge

comment:17 Changed 7 years ago by nickm

Keywords: tor-auth added

comment:18 Changed 7 years ago by nickm

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