Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Authorities expose bandwidth file used in votes #21

Closed
wants to merge 7 commits into from

Conversation

Labels
None yet
Projects
None yet
3 participants
@juga0
Copy link
Contributor

@juga0 juga0 commented Jul 9, 2018

In a similar ways as authorities expose the votes for the next
consensus.
This way it can be archived by CollectTor

In a similar ways as authorities expose the votes for the next
consensus.
This way it can be archived by CollectTor
Copy link
Contributor

@teor2345 teor2345 left a comment

Hi, please move the extra text, and make it more detailed.

Think about how you are going to implement the spec. Don't make it too hard to implement!

Please also check with metrics to see what they need for collector.

dir-spec.txt Outdated
@@ -1588,6 +1588,14 @@
and sending it in an HTTP POST request to each other authority at the URL
http://<hostname>/tor/post/vote

If an authority has used a Bandwidth List file to generate this vote it
Copy link
Contributor

@teor2345 teor2345 Jul 9, 2018

Please don't put the bandwidth file URL in the middle of the vote section. It makes the rest of the vote section confusing.

You can put it before section 3.4.1., or in its own subsection 3.4.3.

Copy link
Contributor Author

@juga0 juga0 Jul 9, 2018

i better move all the paragraph (not just the URL), right?

Copy link
Contributor

@teor2345 teor2345 Jul 9, 2018

Yes, please move all the new content somewhere else.

dir-spec.txt Outdated
http://<hostname>/tor/status-vote/next/bandwidth.z

If an authority makes this file available, its headers MUST correspond to
the ones in the "bandwidth-file-headers" item in the vote document.
Copy link
Contributor

@teor2345 teor2345 Jul 9, 2018

Don't say "it's headers MUST correspond to the ones in the…vote document", because that's confusing.
It could mean that tor reads the headers from the vote, which is not what you want.
Instead, say "it MUST be the file used to create the vote".

The bandwidth generator can overwrite the bandwidth file at any time.
How are you going to store the old bandwidth file so the authority can serve it with the old vote?

You could say:
"If an authority makes this file available, it MAY be the bandwidth file used to create the vote, or the bandwidth file currently on disk."

Which vote document?
"the vote document available at http:///tor/status-vote/next/authority.z"?

Please also reference the bandwidth file spec, so people know the format of the file.

Copy link
Contributor

@teor2345 teor2345 Jul 9, 2018

Or you can say:
"If an authority makes this file available, it MUST be the bandwidth file used to create the vote."

Then tor MUST make a copy of the file before closing it.
(Tor and sbws will need to be careful of race conditions when writing the file.)

Copy link
Contributor

@teor2345 teor2345 Jul 9, 2018

Please also say the version of Tor that introduced this feature. Looks like it will be 0.3.5.

Copy link
Contributor

@teor2345 teor2345 Jul 9, 2018

I opened a sbws ticket to avoid the race condition:
https://trac.torproject.org/projects/tor/ticket/26692

Copy link
Contributor Author

@juga0 juga0 Jul 9, 2018

The bandwidth generator can overwrite the bandwidth file at any time.

Torflow archives the file renaming it with a timestamp and putting it in a dir, but scp inside cron.sh sends the file without timestamp.
This is an easy change so that it sends the file with the timestamp.
In the DirAuth there should be a cron or ssh command that create a link to the latest.
If Torflow runs in the same system as the DirAuth then it could just read the link locally.

sbws already archives files with timestamp, then creates a link to the latest.

How are you going to store the old bandwidth file so the authority can serve it with the old vote?

i don't understand this question

"If an authority makes this file available, it MAY be the bandwidth file used to create the vote, or the bandwidth file currently on disk."

I like more the idea of trying to ensure that the file used to create the vote is the one being served.

"If an authority makes this file available, it MUST be the bandwidth file used to create the vote."

Hmm, maybe it's not needed at all to say this sentence, it's basically the same as the 1st one.

Then tor MUST make a copy of the file before closing it.

This would be a solution, while is not done in the same path where the generator puts the file.

(Tor and sbws will need to be careful of race conditions when writing the file.)

Hmm, Tor doesn't need to write to that file, only read, right?.

@tomrittervg
Copy link
Contributor

@tomrittervg tomrittervg commented Jul 9, 2018

See Also: https://marc.info/?l=tor-dev&m=151605509930409&w=2

tor-dev thread "Proposal: Expose raw bwauth votes"

@teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jul 9, 2018

@tomrittervg, it looks like we never added your proposal to:
https://gitweb.torproject.org/torspec.git/tree/proposals

@juga0, please add the proposal in another commit in this branch.

@teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jul 9, 2018

Hmm, we should update the proposal so it matches the spec change.

@juga0
Copy link
Contributor Author

@juga0 juga0 commented Jul 9, 2018

Now the name of this branch doesn't match the new ticket for the spec https://trac.torproject.org/projects/tor/ticket/26694.
Maybe i can rename the branch when it's ready (what would imply to close this PR and open other or just get the other merged)

@juga0
Copy link
Contributor Author

@juga0 juga0 commented Jul 9, 2018

@tomrittervg thanks for pointing at the proposal, didn't know it exists.
@teor2345:

  1. should we make proposals for any new changes to the spec?.
  2. if we do the changes directly in the spec (we could make a commit in this branch with the parts in the proposal that apply), does still makes sense to add the proposal to proposals?.
    Both:
  • part of the url in that proposal is now, should we then use now instead of next?.
  • should i bring this discussion to that threat in tor-dev ml?

@teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jul 9, 2018

The bandwidth generator can overwrite the bandwidth file at any time.

Torflow archives the file renaming it with a timestamp and putting it in a dir,

The archived file might be written atomically, if cp writes atomically:
https://gitweb.torproject.org/torflow.git/tree/NetworkScanners/BwAuthority/cron.sh#n19

But the file that Tor reads is not written atomically:
https://gitweb.torproject.org/torflow.git/tree/NetworkScanners/BwAuthority/aggregate.py#n878

but scp inside cron.sh sends the file without timestamp.
This is an easy change so that it sends the file with the timestamp.
In the DirAuth there should be a cron or ssh command that create a link to the latest.
If Torflow runs in the same system as the DirAuth then it could just read the link locally.

We are not fixing bugs in torflow. We will never get sbws done if we stop to fix bugs in torflow.

We can't guarantee that authority operators will implement file transfers atomically. Instead, we should remind authority operators not to schedule bandwidth file generation or transfers during voting times:
https://trac.torproject.org/projects/tor/ticket/26702

sbws already archives files with timestamp, then creates a link to the latest.

Ok, that is another way to implement atomic reads and writes. I closed trac ticket #26692.

How are you going to store the old bandwidth file so the authority can serve it with the old vote?

i don't understand this question

Does metrics need to download the bandwidth file that matches the vote?
I think they do.

How can they reliably download the bandwidth file that matches the vote, if the bandwidth generator can overwrite the bandwidth file at any time?

Tor must copy the file.

Then tor MUST make a copy of the file before closing it.

This would be a solution, while is not done in the same path where the generator puts the file.

I don't understand what you mean by "while is not done in the same path where the generator puts the file."

(Tor and sbws will need to be careful of race conditions when writing the file.)

Hmm, Tor doesn't need to write to that file, only read, right?.

Yes. See my answer above about trac ticket #26702.

"If an authority makes this file available, it MUST be the bandwidth file used to create the vote."

Hmm, maybe it's not needed at all to say this sentence, it's basically the same as the 1st one.

The first sentence in you patch talks about files and votes with matching headers.
It is confusing and ambiguous.
Saying "the bandwidth file used to create the vote at http:///tor/status-vote/next/authority.z" is more precise.

Now the name of this branch doesn't match the new ticket for the spec https://trac.torproject.org/projects/tor/ticket/26694.
Maybe i can rename the branch when it's ready (what would imply to close this PR and open other or just get the other merged)

Please work on the current branch and pull request. When the branch is ready to merge, put the branch name on the trac ticket again.

  1. should we make proposals for any new changes to the spec?

No, only large changes.

  1. if we do the changes directly in the spec (we could make a commit in this branch with the parts in the proposal that apply), does still makes sense to add the proposal to proposals?.

Yes, because the proposal has more information than the spec change.

  1. part of the url in that proposal is now, should we then use now instead of next?

No, please update the proposal to match the spec.

There is a standard format for dir-spec URLs:

  • next is for the unpublished vote and associated documents
  • current is for the published vote and associated documents

We should match this format.

In your dir-spec patch, please modify the "current" section as well:
https://gitweb.torproject.org/torspec.git/tree/dir-spec.txt#n3329

should i bring this discussion to that threat in tor-dev ml?

No, you should update the proposal, get it reviewed, then send it to tor-dev.

Here is a list of the other changes I asked for, so I don't need to search all the comments:

Please also reference the bandwidth file spec, so people know the format of the file.

Please also say the version of Tor that introduced this feature. Looks like it will be 0.3.5.

Please move all the new content somewhere else.

@juga0
Copy link
Contributor Author

@juga0 juga0 commented Jul 10, 2018

I think i've made all the changes suggested in dir-spec.
I've added tomrittervg's proposal.
I've made some changes in it to match terminology.
Probably i should also replace the term bwauth and speed. Though i would prefer to don't spend much time on it.
Should the section Specification match exactly in both documents?.
Should i add the last 2 paragraphs to dir-spec since it's useful information and the last one also helps with #26702?

@teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jul 16, 2018

Thanks for your changes.

I made some extra changes in my torspec repository, and put them in a different pull request:
#26

You can review there, or push my commits to your branch, and review here.
I don't mind.

  1. part of the url in that proposal is now, should we then use now instead of next?

No, please update the proposal to match the spec.

There is a standard format for dir-spec URLs:

  • next is for the unpublished vote and associated documents
  • current is for the published vote and associated documents
    We should match this format.

In your dir-spec patch, please modify the "current" section as well:
https://gitweb.torproject.org/torspec.git/tree/dir-spec.txt#n3329

I added bandwidth.z to the current section.

should i bring this discussion to that threat in tor-dev ml?

No, you should update the proposal, get it reviewed, then send it to tor-dev.

I made some edits to the proposal, please review.
If you're happy with it, we can send it to tor-dev.

Probably i should also replace the term bwauth and speed. Though i would prefer to don't spend much time on it.

I made this change.

Should the section Specification match exactly in both documents?.

They don't need to match. Proposals often have extra detail, or leave details to the spec.

Should i add the last 2 paragraphs to dir-spec since it's useful information and the last one also helps with #26702?

No, I solved #26702 with a tor man page update.
See https://trac.torproject.org/projects/tor/ticket/26702#comment:5

@teor2345
Copy link
Contributor

@teor2345 teor2345 commented Feb 25, 2019

This was obsoleted by #26.

@teor2345 teor2345 closed this Feb 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment