Opened 7 months ago

Last modified 2 days ago

#26698 merge_ready enhancement

Authorities should put a hash of the bandwidth file in their votes

Reported by: teor Owned by:
Priority: Low Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-crypto tor-dirauth tor-bwauth 041-proposed
Cc: Actual Points:
Parent ID: #27047 Points: 0.5
Reviewer: nickm Sponsor:

Description

After #3723 is completed, authorities will list the bandwidth file headers in their votes.
After #21377 is completed, they will make bandwidth files available at a standard URL.

But if we want to cryptographically verify the exact bandwidth file that was used in a vote, we need to put a hash of the file in the vote. Let's do it in an extensible way:

bandwidth-file hash-algo base64(hash(bandwidth-file-content))

Child Tickets

TicketTypeStatusOwnerSummary
#28359enhancementclosedSpecify bandwidth-file-hash in torspec
#29147defectneeds_reviewUpdate dir-spec with the correct Tor version when bandwidth-file-digest is merged

Change History (34)

comment:1 Changed 7 months ago by asn

Keywords: tor-dirauth added

comment:2 Changed 6 months ago by teor

Parent ID: #27047

comment:3 Changed 3 months ago by juga

i wonder if we should add the hash to the bandwidth file name in sbws, similar to what is done with descriptors. Though then we would need to allow giving a directory or a file (to be compatible with older versions) in V3BandwidthsFile, and then adding (or resusing?) code to find the latest file parsing the date in it.

comment:4 in reply to:  3 ; Changed 3 months ago by teor

Replying to juga:

i wonder if we should add the hash to the bandwidth file name in sbws, similar to what is done with descriptors. Though then we would need to allow giving a directory or a file (to be compatible with older versions) in V3BandwidthsFile, and then adding (or resusing?) code to find the latest file parsing the date in it.

We could add this feature if we want, but I think it would make the tor code much more complicated. If you want to add it, let's open a separate ticket.

This ticket is about verifying the bandwidth file that was used in a vote.

comment:5 Changed 3 months ago by juga

Status: newneeds_review

comment:6 in reply to:  4 Changed 3 months ago by juga

Replying to teor:

Replying to juga:

i wonder if we should add the hash to the bandwidth file name in sbws, similar to what is done with descriptors. Though then we would need to allow giving a directory or a file (to be compatible with older versions) in V3BandwidthsFile, and then adding (or resusing?) code to find the latest file parsing the date in it.

We could add this feature if we want, but I think it would make the tor code much more complicated.

i agree

If you want to add it, let's open a separate ticket.

This ticket is about verifying the bandwidth file that was used in a vote.

not doing it so far cause the reason above

comment:7 Changed 3 months ago by teor

If collector is going to use hexadecimal for the bandwidth file hash, we should use hexadecimal in the votes. I asked karsten here:
https://trac.torproject.org/projects/tor/ticket/21378#comment:17

comment:8 in reply to:  7 ; Changed 3 months ago by juga

Replying to teor:

If collector is going to use hexadecimal for the bandwidth file hash, we should use hexadecimal in the votes. I asked karsten here:
https://trac.torproject.org/projects/tor/ticket/21378#comment:17

i asked in IRC why we would encode the hash of a file in base 64 in a vote and i was told that we don't use raw bytes anymore.
Searching for "base64" in dir-spec.txt, gives several results, though i couldn't find a paragraph that would confirm that.

comment:9 in reply to:  8 Changed 3 months ago by teor

Replying to juga:

Replying to teor:

If collector is going to use hexadecimal for the bandwidth file hash, we should use hexadecimal in the votes. I asked karsten here:
https://trac.torproject.org/projects/tor/ticket/21378#comment:17

i asked in IRC why we would encode the hash of a file in base 64 in a vote and i was told that we don't use raw bytes anymore.

Directory documents never contain raw bytes.

Searching for "base64" in dir-spec.txt, gives several results, though i couldn't find a paragraph that would confirm that.

Directory documents only contain printing ASCII characters:
https://gitweb.torproject.org/torspec.git/tree/dir-spec.txt#n193

Although we allow some other text encodings in the contact and platform lines.

comment:10 Changed 3 months ago by teor

We typically use hexadecimal or base64 to encode raw bytes in directory documents.

base64 is shorter, but it doesn't work in file names.

comment:11 Changed 3 months ago by teor

Karsten said that they don't mind.

Since it's only in a vote, we might use hex, so humans can look at the vote, and find the bandwidth file for the vote.

comment:12 Changed 3 months ago by juga

Should i then use hex? (and then change this ticket to revision)

comment:13 Changed 3 months ago by teor

Status: needs_reviewneeds_revision

Yes, let's use hex for the hash.

comment:14 Changed 3 months ago by juga

Keywords: tor-bwauth added
Status: needs_revisionneeds_review

comment:15 Changed 3 months ago by teor

Status: needs_reviewneeds_revision

Based on the conversation in https://trac.torproject.org/projects/tor/ticket/21378#comment:26 , let's use base64-encoded SHA256.

I didn't review the code, but I did review the spec ticket.

comment:16 Changed 3 months ago by juga

Status: needs_revisionneeds_review

comment:17 Changed 3 months ago by asn

Reviewer: mikeperry

comment:18 Changed 3 months ago by teor

Milestone: Tor: unspecifiedTor: 0.4.0.x-final

Putting this in 0.4.0, because it's a feature with code.

Hey mikeperry, do you have time to review this ticket this week?

comment:19 Changed 2 months ago by teor

Reviewer: mikeperryteor

Taking over reviews from Mike, because he's busy with WTF-PAD.

comment:20 Changed 5 weeks ago by teor

Mike, if I don't get to this review on Monday, I'm going to return it to you. Because I'm busy with PrivCount now.

comment:21 Changed 5 weeks ago by nickm

Reviewer: teornickm

Hm. Looking at PR497, I see that it uses sha256. That's good -- sha1 would be a bad choice here.

I've left a short review on the ticket -- it looks mostly good, but there are some points I think could use some changes.

comment:22 Changed 5 weeks ago by nickm

Status: needs_reviewneeds_revision

comment:23 Changed 5 weeks ago by teor

Oh, I also did a GitHub review a few weeks ago, but for some reason I never submitted it. I hope there's not too much overlap with Nick's review, or any changes since I did my review.

comment:24 Changed 4 weeks ago by juga

Status: needs_revisionneeds_review

Nickm and teor reviews are not overlapping (except for the 255 typo).
I made lot of fixups because i moved the hash calculation from the new function to dirserv_read_measured_bandwidths and i realized about more issues on the way.
I explain each fixup in the PR.
If you prefer to review a squashed version, see also the comment in the PR.

comment:25 Changed 4 weeks ago by teor

Keywords: 041-proposed added
Milestone: Tor: 0.4.0.x-finalTor: 0.4.1.x-final
Points: 0.5

The 0.4.0 code freeze is on, so this ticket should move to 0.4.1.
The points estimate should be the remaining review time.

comment:26 Changed 4 weeks ago by teor

Status: needs_reviewneeds_revision

I did a review on the pull request.

I noticed some bugs that we need to fix before we merge.
I also found some things that we could fix after we merge, so I opened child tickets for them.

comment:27 in reply to:  26 Changed 4 weeks ago by juga

Status: needs_revisionneeds_review

Replying to teor:

I noticed some bugs that we need to fix before we merge.

See new fixups and comments, not sure i'm calculating the digest correctly.

comment:28 Changed 4 weeks ago by teor

Status: needs_reviewneeds_revision

I found out why your hashes weren't matching: see the pull request.

Please be careful with sizeof() and strlen()!

comment:29 Changed 3 weeks ago by juga

Status: needs_revisionneeds_review

ok, more fixups

comment:30 Changed 4 days ago by nickm

Status: needs_reviewneeds_revision

This looks good to me now! It gives me conflicts when I try to squash it, though. Once there's a squashed branch, please put it into a PR and put the ticket into merge_ready?

comment:31 Changed 3 days ago by juga

Status: needs_revisionmerge_ready

I squashed it in https://github.com/torproject/tor/pull/699.

I also had to rebase it, because i created the branch when appveyor was failing with openssl (see comment https://github.com/torproject/tor/pull/497#discussion_r248581721).
I tried to rebase it to the commit in which i think the openssl was fixed, but then travis was failing cause not being able to write cargo.lock, so i just rebased to current master where CI doesn't fail.

comment:32 Changed 3 days ago by nickm

Okay -- the commits look like what I read before, and chutney passes on the results. Merged to master!

comment:33 Changed 3 days ago by nickm

(I don't know whether the child tickets here should be closed or unparented -- please do whatever is appropriate to them, and then close this ticket? Thanks!)

comment:34 Changed 2 days ago by juga

will update the spec, then close

Note: See TracTickets for help on using tickets.