Opened 3 years ago

Closed 17 months ago

#20287 closed defect (fixed)

Perform another review of CollecTor's file protocol and fix any remaining differences to the code

Reported by: iwakeh Owned by: iwakeh
Priority: High Milestone:
Component: Metrics/Website Version:
Severity: Normal Keywords: metrics-2018
Cc: metrics-team Actual Points:
Parent ID: #20234 Points:
Reviewer: karsten Sponsor:

Description

It seems that section 4.3.3 of the protocol doesn't coincide with CollecTor's code.

The section reads:

4.3.3
   'votes' contains files named

   year DASH month DASH day DASH hour DASH minute DASH second
   DASH VOTE DASH fingerprint DASH digest

   Where VOTE is the string "vote" and all time related
   values are derived from the valid-after dates. 'fingerprint'
   is the fingerprint of the authority and 'digest' is the SHA1
   digest of the authority's medium term signing key.

But the code for creating the digest calculates a digest for the
descriptor bytes from the start of the vote to the 'directory-signature ' (incl.).
(cf. here).

    ...
            String ascii = new String(data, "US-ASCII");
            String startToken = "network-status-version ";
            String sigToken = "directory-signature ";
            int start = ascii.indexOf(startToken);
            int sig = ascii.indexOf(sigToken);
            if (start >= 0 && sig >= 0 && sig > start) {
              sig += sigToken.length();
              byte[] forDigest = new byte[sig - start];
              System.arraycopy(data, start, forDigest, 0, sig - start);
              String digest = DigestUtils.shaHex(forDigest).toUpperCase();
              if (this.aw != null) {
                this.aw.storeVote(data, validAfter, dirSource, digest,
                    serverDescriptorDigests);
      ...

Which is correct?

Child Tickets

Change History (18)

comment:1 Changed 3 years ago by karsten

Status: newneeds_information

The code is correct, I overlooked that difference when reviewing the protocol draft.

I guess for votes in the recent/ folder this doesn't matter much, because we're thinking about merging them all into a single file. And just in case we decide against that, we can still document what exactly we're doing and possibly even why.

For votes in the tarballs it matters a bit, because those will stay in separate files. There it makes sense to include a descriptor digest, because just in case an authority publishes more than one vote, we might want to keep those different copies and not overwrite the first with the second.

Does that answer the question?

comment:2 Changed 3 years ago by iwakeh

Thanks! Yes, it does answer the question. I assumed the code is the way to go, but wanted a confirmation. The question was asked with regard to both 'recent' and tar-structure.

I need these clarifications to provide useful tests for the sync-code. That's why this is on set to 'high'.

comment:3 Changed 3 years ago by iwakeh

Owner: set to iwakeh
Status: needs_informationassigned

comment:4 Changed 3 years ago by iwakeh

Milestone: CollecTor 1.1.0CollecTor 1.2.0

comment:5 Changed 2 years ago by karsten

Status: assignedneeds_information

What's left to do here? And is this something that should go into the next release next week, or can it wait until end of the month or even longer?

comment:6 Changed 2 years ago by iwakeh

Milestone: CollecTor 1.2.0CollecTor 1.3.0
Status: needs_informationaccepted

Needs some more time, but should be decided for the next milestone.

comment:7 Changed 2 years ago by iwakeh

Adapt description to current setup (i.e., source code), also adapt torperf/onionperf path descriptions.

comment:8 Changed 2 years ago by iwakeh

Milestone: CollecTor 1.3.0CollecTor 1.4.0

Move to next milestone to limit 1.3.0 to the webstats functionality.

comment:9 Changed 2 years ago by iwakeh

Component: Metrics/CollecTorMetrics/Website
Milestone: CollecTor 1.4.0
Parent ID: #20234

Moving this to metrics-web as this is a documentation issue.

Also consider the point raised by Tom in metrics-team ml.
Quote:

I think that document is wrong.

year DASH month DASH day DASH hour DASH minute DASH second
DASH VOTE DASH fingerprint DASH digest

Where VOTE is the string "vote" and all time related
values are derived from the valid-after dates. 'fingerprint'
is the fingerprint of the authority and 'digest' is the SHA1
digest of the authority's medium term signing key.

Specifically: 'digest' is the SHA1 digest of the authority's medium term signing key.

It's not the medium term signing key, it's the digest of the vote document.

This should be fixed before publishing the protocol on metrics-web. -> Setting parent id to #20234.

comment:10 Changed 2 years ago by karsten

Summary: digest computation for names of vote files in CollecTor's file protocol and code differsPerform another review of CollecTor's file protocol and fix any remaining differences to the code

Make the summary more generic, as there seems to be more than just one remaining issue with this document.

comment:11 Changed 23 months ago by karsten

Keywords: metrics-2018 added

comment:12 Changed 23 months ago by karsten

Keywords: metrics-2017 added; metrics-2018 removed

comment:13 Changed 20 months ago by iwakeh

Keywords: metrics-2018 added; metrics-2017 removed

Will be completed in 2018.

comment:14 Changed 19 months ago by iwakeh

Owner: changed from iwakeh to metrics-team
Status: acceptedassigned

Move to metrics-team as these are not worked on by me during the next week.

comment:15 Changed 18 months ago by iwakeh

Owner: changed from metrics-team to iwakeh
Status: assignedaccepted

comment:16 Changed 18 months ago by iwakeh

Status: acceptedneeds_review

Seems the repeatedly interrupted work on this ticket was cause for some confusion.

The torperf/onionperf issue was already cleared before the first merge to master, i.e., nothing left to do about the topic mentioned in comment:7, cf. collector.git.

The topic from comment:9 and the initial description mention are the same issue, which is patched here.

I couldn't identify any other issues.

Please review.

comment:17 Changed 17 months ago by karsten

Cc: metrics-team added
Reviewer: karsten

Adding to my review queue.

comment:18 Changed 17 months ago by karsten

Resolution: fixed
Status: needs_reviewclosed

Patch looks good. Merged to master.

I guess that resolves this issue? Closing. Please re-open if something remains. Thanks!

Note: See TracTickets for help on using tickets.