Skip to content

Ticket33072 043 02 #1804

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

Open
wants to merge 7 commits into
base: maint-0.4.3
Choose a base branch
from

Conversation

dgoulet-tor
Copy link
Contributor

No description provided.

This also adds the support for the option meaning, if set, every uncompressed
directory requests will be served by the dirauth with a 503 error code.

Signed-off-by: David Goulet <[email protected]>
@coveralls
Copy link

coveralls commented Mar 17, 2020

Pull Request Test Coverage Report for Build 8673

  • 10 of 11 (90.91%) changed or added relevant lines in 3 files are covered.
  • 1281 unchanged lines in 11 files lost coverage.
  • Overall coverage increased (+0.1%) to 63.602%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/feature/dircache/dircache.c 4 5 80.0%
Files with Coverage Reduction New Missed Lines %
src/core/or/sendme.c 3 73.02%
src/feature/dirparse/parsecommon.c 5 87.67%
src/lib/crypt_ops/crypto_rsa.c 7 91.63%
src/lib/crypt_ops/crypto_rsa_openssl.c 10 91.7%
src/core/or/circuitpadding.c 14 92.24%
src/feature/relay/relay_config.c 46 74.44%
src/feature/hs/hs_service.c 59 74.87%
src/feature/dirauth/process_descs.c 84 42.57%
src/feature/hs/hs_descriptor.c 103 88.88%
src/feature/dirauth/dirvote.c 337 64.81%
Totals Coverage Status
Change from base Build 8398: 0.1%
Covered Lines: 50220
Relevant Lines: 78960

💛 - Coveralls

doc/tor.1.txt Outdated

HIDDEN SERVICE OPTIONS
----------------------
== HIDDEN SERVICE OPTIONS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change doesn't seem to belong in this pull request?

@@ -0,0 +1,3 @@
o Minor bugfix (directory authority):
- Always allow compressed directory requests. Fixes bug 33072; bugfix on
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the changes file mention the new AuthDirRejectUncompressedRequests option?

return true;
}
if (c_method != NO_METHOD) {
/* Always answer compressed request. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure we always want to allow compressed requests?

We could just fall through to the other code, which might reject some requests, if the write bucket is low.

If an authority is getting a lot of compressed requests, we should prioritise voting over answering compressed requests.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some binary files in this PR, please remove.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the 0.4.2 and master code got out of sync with 0.4.3.
And the function comments need to be updated in every branch.

Comment on lines +3280 to +3284
* If we are a directory authority, false is returned (indicating that we
* should answer the request) if one of these conditions is met:
* - Connection is from a known relay (address is looked up).
* - AuthDirRejectRequestsUnderLoad is set to 0.
* - Compression is being used for the request (looking at c_method).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment doesn't match what the code does.

@@ -0,0 +1,4 @@
o Minor bugfix (directory authority):
- Always allow compressed directory requests and introduce option
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, the changes file is out of sync as well.

We let AuthDirRejectRequestsUnderLoad and the bandwidth buckets decide about compressed requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants