Opened 4 years ago

Closed 3 years ago

#17435 closed defect (fixed)

Patch dir-spec with the shared randomness info

Reported by: asn Owned by: asn
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: TorCoreTeam201606, tor-spec, tor-hs, review-group-3
Cc: atagar Actual Points:
Parent ID: #16943 Points: 1
Reviewer: Sponsor: SponsorR-can

Description

We have proposal250 specifying the shared randomness feature, but that's not enough.

We should extract all the directory-related info from prop250, and patch dir-spec.txt to specify all the changes in vote documents and consensuses.

Child Tickets

Change History (21)

comment:1 Changed 4 years ago by nickm

Milestone: Tor: 0.2.8.x-final

comment:2 Changed 4 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final

Throw most 0.2.8 "NEW" tickets into 0.2.9. I expect that many of them will subsequently get triaged out.

comment:3 Changed 4 years ago by asn

I made the required changes to dir-spec.txt. You can find it in my branch prop250_dirspec:
https://gitweb.torproject.org/user/asn/torspec.git/log/?h=prop250_dirspec

When we decide this is the final design, I will merge to torspec.git .

comment:4 Changed 4 years ago by atagar

Hi asn. Great start but I'd appreciate if you could document the format of the line attributes. For example, you don't indicate what characters are valid in 'AlgName', 'Identity', etc. See the params line below it for an example...

    "params" SP [Parameters] NL

        [At most once]

        Parameter ::= Keyword '=' Int32
        Int32 ::= A decimal integer between -2147483648 and 2147483647.
        Parameters ::= Parameter | Parameters SP Parameter

comment:5 in reply to:  4 Changed 4 years ago by asn

Replying to atagar:

Hi asn. Great start but I'd appreciate if you could document the format of the line attributes. For example, you don't indicate what characters are valid in 'AlgName', 'Identity', etc. See the params line below it for an example...

You are right.

Please pull my branch again and check the latest fixup commit. Is that better for you? :)

comment:6 Changed 4 years ago by atagar

Looks good to me, thanks asn! When this becomes live please shoot me an example of these in actual descriptors (for unit tests) and I'd be happy to add it to Stem.

Just a couple minor nitpicks left...

AlgName ::= Any number of ASCII characters

I suspect this can be pinned down a bit more. How about the following?

AlgName ::= 1*(ALPHA / DIGIT / "_" / "-")

Identity ::= 40 hexadecimal characters ([A-Fa-f0-9])

Actually, if you want to use 'Fingerprint' that's already defined...

https://gitweb.torproject.org/torspec.git/tree/control-spec.txt#n206

comment:7 Changed 4 years ago by isabela

Sponsor: SponsorR-can

comment:8 Changed 4 years ago by nickm

Points: small/medium

comment:9 Changed 4 years ago by dgoulet

Cc: dgoulet removed
Keywords: tor-hs added
Owner: set to asn
Points: small/mediumsmall
Status: newassigned

comment:10 Changed 4 years ago by dgoulet

Status: assignedneeds_revision

comment:11 in reply to:  6 Changed 4 years ago by asn

Status: needs_revisionneeds_review

Replying to atagar:

Looks good to me, thanks asn! When this becomes live please shoot me an example of these in actual descriptors (for unit tests) and I'd be happy to add it to Stem.

Just a couple minor nitpicks left...

AlgName ::= Any number of ASCII characters

I suspect this can be pinned down a bit more. How about the following?

AlgName ::= 1*(ALPHA / DIGIT / "_" / "-")

Identity ::= 40 hexadecimal characters ([A-Fa-f0-9])

Actually, if you want to use 'Fingerprint' that's already defined...

https://gitweb.torproject.org/torspec.git/tree/control-spec.txt#n206

I addressed both issues you raised above and pushed a fixup commit in my prop250_dirspec.

(FWIW, we don't actually use the dollar sign in fingerprints, so I didn't put it there.)

comment:12 Changed 4 years ago by arma

AlgName is the hash algorithm that is used

What are some expected values of AlgName here? For example, what is the default value that the new code is going to use? Usually we'll want to specify at least one value that must be supported.

Identity is the authority's SHA1 identity fingerprint

I bet you mean the authority's SHA1 v3 identity fingerprint? If I wanted to be really confused I might think you meant the authority's SHA1 relay identity fingerprint.

"shared-rand-commit"

You say "any number" for votes. Are there constraints on what is allowed to show up in a set of these lines in a given vote? For example, I would expect that maybe there must not be more than one shared-rand-commit line with the same identity fingerprint in a single vote?

Commit is the encoded commitment value in base64

Are there commitment values that are unacceptable? What's the format of it?

And here's where it gets exciting: what is the process of how authorities should take in these shared-rand-commit lines, and then compute the shared-rand-current-value line for their consensus? This needs to be specified, or somebody trying to build a directory authority from this spec won't be able to do it.

Similarly, how do authorities generate the "shared-rand-previous-value" values? I guess they're taken out of some previous consensuses or other past state? What should a dir auth do to generate a shared-rand-previous-value that will conform to spec and be what clients expect?

Min: 1. Max: <Total number of dirauths>. Default: <Total number of dirauths>.

What does "max" mean in dir-spec? I worry that it means that a value higher than max means that the consensus or vote is not conforming to the spec, i.e. that it's an invalid consensus or vote. And if different people have different views on how many dirauths there are, I could totally imagine somebody being surprised by a high number here. Maybe we just want Max as INT32_MAX?

comment:13 Changed 4 years ago by nickm

Keywords: review-group-1 added

comment:14 Changed 4 years ago by asn

Status: needs_reviewneeds_revision

comment:15 Changed 4 years ago by isabela

Points: small1

comment:16 Changed 4 years ago by nickm

Keywords: review-group-2 added; review-group-1 removed

Everything in review-group-1 has had at least 1 review.

comment:17 Changed 4 years ago by asn

Keywords: TorCoreTeam201606 added

comment:18 in reply to:  12 Changed 3 years ago by asn

Status: needs_revisionneeds_review

Replying to arma:

AlgName is the hash algorithm that is used

What are some expected values of AlgName here? For example, what is the default value that the new code is going to use? Usually we'll want to specify at least one value that must be supported.

Identity is the authority's SHA1 identity fingerprint

I bet you mean the authority's SHA1 v3 identity fingerprint? If I wanted to be really confused I might think you meant the authority's SHA1 relay identity fingerprint.

"shared-rand-commit"

You say "any number" for votes. Are there constraints on what is allowed to show up in a set of these lines in a given vote? For example, I would expect that maybe there must not be more than one shared-rand-commit line with the same identity fingerprint in a single vote?

Commit is the encoded commitment value in base64

Are there commitment values that are unacceptable? What's the format of it?

And here's where it gets exciting: what is the process of how authorities should take in these shared-rand-commit lines, and then compute the shared-rand-current-value line for their consensus? This needs to be specified, or somebody trying to build a directory authority from this spec won't be able to do it.

Similarly, how do authorities generate the "shared-rand-previous-value" values? I guess they're taken out of some previous consensuses or other past state? What should a dir auth do to generate a shared-rand-previous-value that will conform to spec and be what clients expect?

Min: 1. Max: <Total number of dirauths>. Default: <Total number of dirauths>.

What does "max" mean in dir-spec? I worry that it means that a value higher than max means that the consensus or vote is not conforming to the spec, i.e. that it's an invalid consensus or vote. And if different people have different views on how many dirauths there are, I could totally imagine somebody being surprised by a high number here. Maybe we just want Max as INT32_MAX?

OK I worked on the comments above, and pushed the changes in my prop250_dirspec branch.

FWIW, I ended up not explaining the whole logic behind the computation of shared-rand-current-value as was requested. Instead I just linked to the right sections of proposal 250. This makes dir-spec.txt less self-contained which is bad, however explaining all that logic would basically mean copying prop250 in dir-spec.txt, and I didn't know how to do this without messing up dir-spec even more. e.g. just pasting the SRV computation formula would not be very useful without specifying which commits are considered active and how commits are validated. Let me know if you think there is a better middle ground here.

comment:19 Changed 3 years ago by nickm

Status: needs_reviewmerge_ready

I am going to call this merge-ready, unless somebody objects. This should get merged when the prop250 implementation gets merged.

comment:20 Changed 3 years ago by nickm

Keywords: review-group-3 added; review-group-2 removed

These have all had at least one review during review-group-2.

comment:21 Changed 3 years ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

#16943 has been merged. This is now upstream in torspec.

Note: See TracTickets for help on using tickets.