Opened 23 months ago

Last modified 36 hours ago

#21377 needs_revision enhancement

DirAuths should expose bwauth bandwidth files

Reported by: tom Owned by: juga
Priority: High Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-dirauth, metrics, tor-bwauth, 035-removed-20180711, 040-roadmap-proposed
Cc: starlight@… Actual Points:
Parent ID: #25925 Points:
Reviewer: ahf Sponsor:

Description

Currently, DirAuths that vote on bwauth files do not expose the raw voting file they used. This data would be good to archive for debugging and transparency purposes.

Child Tickets

TicketTypeStatusOwnerSummary
#26694enhancementreopenedjugadir-spec: DirAuths should expose bwauth bandwidth files
#28815defectnewRefactor similar compression buffer code in dircache.c
#28816defectnewLog a bug for uncompressed data on a compressed connection

Change History (44)

comment:1 Changed 23 months ago by tom

The ticket blocks archiving the data in collector, #21378

comment:2 Changed 23 months ago by asn

Keywords: tor-dirauth metrics added
Milestone: Tor: unspecified

comment:3 Changed 18 months ago by nickm

Priority: LowHigh

comment:4 Changed 6 months ago by juga

This is now fixed in sbws, though is still not being use by any DirAuth.

It is not planned to add this to Torflow.

comment:5 Changed 6 months ago by juga

Keywords: tor-bwauth added

comment:6 Changed 6 months ago by juga

Cc: teor added

comment:7 in reply to:  4 Changed 6 months ago by teor

Summary: DirAuths should expose bwauth votesDirAuths should expose bwauth bandwidth files

Replying to juga:

This is now fixed in sbws, though is still not being use by any DirAuth.

It is not planned to add this to Torflow.

This ticket is about archiving exposing the entire v3bw file, not just the headers.

Can you explain how this is fixed in sbws?

Last edited 6 months ago by teor (previous) (diff)

comment:8 Changed 6 months ago by juga

My mistake, didn't re-read the title of the ticket.
No, sbws does not fix this.

comment:9 Changed 5 months ago by juga

Changes to dir-spec commented https://trac.torproject.org/projects/tor/ticket/21378#comment:7 implemented in my branch.
Please let me know if i should create a different ticket for the spec.

Edit: correct url to the comment

Last edited 5 months ago by juga (previous) (diff)

comment:10 Changed 5 months ago by juga

Status: newneeds_review

comment:11 in reply to:  9 Changed 5 months ago by teor

Milestone: Tor: unspecifiedTor: 0.3.5.x-final
Status: needs_reviewneeds_revision

Replying to juga:

Changes to dir-spec commented https://trac.torproject.org/projects/tor/ticket/21378#comment:7 implemented in my branch.

I think the pull request is:
https://github.com/torproject/torspec/pull/21

In future, please link to the branch or pull request, so I know which one you want me to review.

Please let me know if i should create a different ticket for the spec.

Yes, please open a different ticket, otherwise this ticket will close when we merge the spec.

Please see my comments on the pull request.

comment:12 Changed 5 months ago by juga

Created #26694 for the spec changes.

comment:13 Changed 5 months ago by teor

Parent ID: #25925

comment:14 Changed 5 months ago by juga

Owner: set to juga
Status: needs_revisionaccepted

Changing status since the ticket for the spec is #26694

comment:15 Changed 5 months ago by nickm

Keywords: 035-removed-20180711 added
Milestone: Tor: 0.3.5.x-finalTor: unspecified

These tickets are being triaged out of 0.3.5. The ones marked "035-roadmap-proposed" may return.

comment:16 Changed 5 months ago by teor

Keywords: 035-roadmap-proposed added

We would like to implement this ticket in 0.3.5, but we want a working bandwidth scanner first.

comment:17 Changed 5 months ago by teor

The refactor in #26797 will involve storing a copy of the bandwidth file in RAM or on disk, so let's do it first.

comment:18 Changed 7 weeks ago by teor

Keywords: 036-roadmap-proposed added; 035-roadmap-proposed removed

0.3.5 is closed to new features, moving to 0.3.6 proposed.

comment:19 Changed 7 weeks ago by juga

Cc: teor removed
Status: acceptedneeds_review

https://github.com/torproject/tor/pull/468

I'm not sure i've implemented in the right way, maybe it should be call from handle_get_next_vote, or somehow check that the bandwidth file that used in the vote is the same being read from options->V3BandwidthsFile.
Maybe the hash of the file should be stored somewhere?.
Should some maximum size be check?.
Would be the timeline of the file working as expected?.

comment:20 Changed 6 weeks ago by teor

Note for the review assigner:

ahf wants to review tor bandwidth authority patches.

Note for the reviewer:

Let's get a simple version of this patch working first. The simplest version sends the bandwidth file that is on disk right now.

Then we can follow up with:

  • #27047: authorities should keep recent consensuses, votes, and bandwidth files
  • #26797: read the file once per vote, and use the cached file for votes and bandwidth file requests
  • #26698: put a hash of the bandwidth file in the vote

comment:21 Changed 6 weeks ago by teor

Status: needs_reviewneeds_revision

All the CI on this branch failed.

juga, I'm not sure if you get emails from appveyor or travis when builds fail? Or if you're in the #tor-ci channel?

comment:22 in reply to:  21 Changed 6 weeks ago by juga

Replying to teor:

juga, I'm not sure if you get emails from appveyor or travis when builds fail? Or if you're in the #tor-ci channel?

yes i do. I just was not going to work on it at the moment i saw it, but i should have removed "need_review".

comment:23 Changed 6 weeks ago by juga

Status: needs_revisionneeds_review

Fixed CI

comment:24 Changed 6 weeks ago by dgoulet

Status: needs_reviewneeds_revision

comment:25 Changed 6 weeks ago by juga

Status: needs_revisionneeds_review

New commits

comment:26 Changed 5 weeks ago by starlight

Cc: starlight@… added

comment:27 Changed 5 weeks ago by teor

Keywords: 040-roadmap-proposed added; 036-roadmap-proposed removed

0.3.6 is now 0.4.0: changing roadmap keywords

comment:28 Changed 5 weeks ago by teor

Milestone: Tor: unspecifiedTor: 0.4.0.x-final

This is a new feature with code: it belongs in the next feature release.

comment:29 Changed 5 weeks ago by asn

Reviewer: ahf

comment:30 Changed 4 weeks ago by ahf

Status: needs_reviewneeds_revision

Left some minor stylistic comments on the PR, but code looks overall very good.

comment:31 Changed 4 weeks ago by juga

Status: needs_revisionneeds_review

comment:32 Changed 3 weeks ago by ahf

Status: needs_reviewmerge_ready

LGTM now.

comment:33 Changed 2 weeks ago by nickm

Status: merge_readyneeds_revision

This looks solid, but I have some requests and questions:

  • Could we please have a changes file?
  • Should we be passing "len" to write_http_response_header when compression is in use? I'm not sure what is correct here. Maybe we're supposed to send the compressed length if compression is in use?

comment:34 in reply to:  33 ; Changed 2 weeks ago by teor

Replying to nickm:

This looks solid, but I have some requests and questions:

  • Could we please have a changes file?
  • Should we be passing "len" to write_http_response_header when compression is in use? I'm not sure what is correct here.

If it is present, Content-Length must be the length of the encoded data.

Maybe we're supposed to send the compressed length if compression is in use?

When we deliver compressed replies in other code, we don't provide Content-Length:
https://github.com/torproject/tor/blob/8020d6fb05d9477e77c6ca554dc1288873f6115c/src/feature/dircache/dircache.c#L1293

comment:35 in reply to:  34 ; Changed 2 weeks ago by juga

Replying to teor:

  • Should we be passing "len" to write_http_response_header when compression is in use? I'm not sure what is correct here.

If it is present, Content-Length must be the length of the encoded data.

Maybe we're supposed to send the compressed length if compression is in use?

When we deliver compressed replies in other code, we don't provide Content-Length:
https://github.com/torproject/tor/blob/8020d6fb05d9477e77c6ca554dc1288873f6115c/src/feature/dircache/dircache.c#L1293

I verified that there's not Content-Length in the header doing that.

If i call detect_compression_method, as test_dir_handle_get_status_vote_current_consensus_ns is doing: https://github.com/torproject/tor/blob/8020d6fb05d9477e77c6ca554dc1288873f6115c/src/test/test_dir_handle_get.c#L1875, it will say the compression method is UNKNOWN_METHOD, instead of ZLIB_METHOD. What i'm doing wroing?

comment:36 in reply to:  35 ; Changed 2 weeks ago by teor

Replying to juga:

Replying to teor:

  • Should we be passing "len" to write_http_response_header when compression is in use? I'm not sure what is correct here.

If it is present, Content-Length must be the length of the encoded data.

Maybe we're supposed to send the compressed length if compression is in use?

When we deliver compressed replies in other code, we don't provide Content-Length:
https://github.com/torproject/tor/blob/8020d6fb05d9477e77c6ca554dc1288873f6115c/src/feature/dircache/dircache.c#L1293

I verified that there's not Content-Length in the header doing that.

I don't understand what you mean.
If you pass a zero or positive length, then the code sets Content-Length.
I added a suggested fix on the pull request.

If i call detect_compression_method, as test_dir_handle_get_status_vote_current_consensus_ns is doing: https://github.com/torproject/tor/blob/8020d6fb05d9477e77c6ca554dc1288873f6115c/src/test/test_dir_handle_get.c#L1875, it will say the compression method is UNKNOWN_METHOD, instead of ZLIB_METHOD. What i'm doing wroing?

I don't understand what you mean here.
If you're calling the new code in the test, you have to add headers to the request.
Otherwise, the new code should return NO_METHOD.

I don't understand how you are getting UNKNOWN_METHOD.
Please show me the code you are using, and the logs of the results.

comment:37 in reply to:  36 ; Changed 2 weeks ago by juga

Replying to teor:

If i call detect_compression_method, as test_dir_handle_get_status_vote_current_consensus_ns is doing: https://github.com/torproject/tor/blob/8020d6fb05d9477e77c6ca554dc1288873f6115c/src/test/test_dir_handle_get.c#L1875, it will say the compression method is UNKNOWN_METHOD, instead of ZLIB_METHOD. What i'm doing wroing?

I don't understand what you mean here.
If you're calling the new code in the test, you have to add headers to the request.
Otherwise, the new code should return NO_METHOD.

I don't understand how you are getting UNKNOWN_METHOD.
Please show me the code you are using, and the logs of the results.

https://github.com/torproject/tor/commit/b03091842bc4590e11e3ac026daae8ed6d8f7554

comment:38 in reply to:  37 Changed 2 weeks ago by teor

Replying to juga:

Replying to teor:

If i call detect_compression_method, as test_dir_handle_get_status_vote_current_consensus_ns is doing: https://github.com/torproject/tor/blob/8020d6fb05d9477e77c6ca554dc1288873f6115c/src/test/test_dir_handle_get.c#L1875, it will say the compression method is UNKNOWN_METHOD, instead of ZLIB_METHOD. What i'm doing wroing?

I don't understand what you mean here.
If you're calling the new code in the test, you have to add headers to the request.
Otherwise, the new code should return NO_METHOD.

I don't understand how you are getting UNKNOWN_METHOD.
Please show me the code you are using, and the logs of the results.

https://github.com/torproject/tor/commit/b03091842bc4590e11e3ac026daae8ed6d8f7554

I can't find the logs of the results. Can you put them in a pastebin?

comment:40 Changed 2 weeks ago by juga

I put them in code comments. Created also a pastebin with logs only for that test and log level debug: https://paste.debian.net/1054163/

comment:41 in reply to:  40 Changed 8 days ago by juga

Replying to juga:

I put them in code comments. Created also a pastebin with logs only for that test and log level debug: https://paste.debian.net/1054163/

Now in https://paste.debian.net/hidden/23b3c78e/

comment:42 Changed 7 days ago by teor

It looks like you're setting the compression state, then adding the data uncompressed.

You didn't open a pull request, so I commented on the commit:
https://github.com/torproject/tor/commit/b03091842bc4590e11e3ac026daae8ed6d8f7554#r31629291
https://github.com/torproject/tor/commit/b03091842bc4590e11e3ac026daae8ed6d8f7554#r31629096

I also opened:

  • #28815 to clean up the copied code,
  • #28816 to log a bug warning when uncompressed data is added to a compressed connection

You don't have to do these tickets: they are not on the roadmap.

comment:43 in reply to:  42 ; Changed 6 days ago by juga

Replying to teor:

It looks like you're setting the compression state, then adding the data uncompressed.

You didn't open a pull request, so I commented on the commit:
https://github.com/torproject/tor/commit/b03091842bc4590e11e3ac026daae8ed6d8f7554#r31629291
https://github.com/torproject/tor/commit/b03091842bc4590e11e3ac026daae8ed6d8f7554#r31629096

I didn't create a PR, cause i knew that code fails, but wanted to ask about it.
i've left new comments after setting the compression state.

comment:44 in reply to:  43 Changed 36 hours ago by teor

Replying to juga:

Replying to teor:

It looks like you're setting the compression state, then adding the data uncompressed.

You didn't open a pull request, so I commented on the commit:
https://github.com/torproject/tor/commit/b03091842bc4590e11e3ac026daae8ed6d8f7554#r31629291
https://github.com/torproject/tor/commit/b03091842bc4590e11e3ac026daae8ed6d8f7554#r31629096

I didn't create a PR, cause i knew that code fails, but wanted to ask about it.

Please open pull requests for code questions and CI, even if the code doesn't work yet.

When you open a pull request:

  • comments are easier to make and easier to find
  • new commits get handled better
  • the CI is done on a merge with master, so any bugs fixed in master are fixed in the CI

If you don't want a pull request merged, put the ticket in needs_review, and say that the code doesn't work.

Some people also put "WIP" or "work in progress" in the pull request title:
https://stackoverflow.com/a/39741877

i've left new comments after setting the compression state.

I'm not sure how to find the new code, or the CI for that code.

I tried looking at your branches, but they haven't changed:
https://github.com/juga0/tor/branches

When you ask questions, please open a pull request.
(Or link to a branch, or a commit. But a pull request is better.)

Note: See TracTickets for help on using tickets.