From e0f59e8e959be80ed6fa393d8091489bb70a5c08 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 30 Jan 2020 09:40:55 -0500 Subject: [PATCH 1/2] dirauth: Always answer compressed directory requests Fixes #33072 Signed-off-by: David Goulet --- changes/ticket33072 | 4 ++++ src/core/mainloop/connection.c | 14 +++++++++++--- src/core/mainloop/connection.h | 3 ++- src/feature/dircache/dircache.c | 15 ++++++++++----- src/test/test_bwmgt.c | 20 ++++++++++++-------- 5 files changed, 39 insertions(+), 17 deletions(-) create mode 100644 changes/ticket33072 diff --git a/changes/ticket33072 b/changes/ticket33072 new file mode 100644 index 00000000000..ad427bd0493 --- /dev/null +++ b/changes/ticket33072 @@ -0,0 +1,4 @@ + o Minor bugfix (directory authority): + - Always allow compressed directory requests and introduce option + AuthDirRejectUncompressedRequests to control that behavior. Fixes bug + 33072; bugfix on 0.1.2.5-alpha. diff --git a/src/core/mainloop/connection.c b/src/core/mainloop/connection.c index 50cd3810a46..0d4bed01488 100644 --- a/src/core/mainloop/connection.c +++ b/src/core/mainloop/connection.c @@ -3190,8 +3190,11 @@ connection_bucket_write_limit(connection_t *conn, time_t now) * shouldn't send attempt bytes of low-priority directory stuff * out to conn. * - * If we are a directory authority, always answer dir requests thus true is - * always returned. + * 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). * * Note: There are a lot of parameters we could use here: * - global_relayed_write_bucket. Low is bad. @@ -3206,7 +3209,8 @@ connection_bucket_write_limit(connection_t *conn, time_t now) * that's harder to quantify and harder to keep track of. */ bool -connection_dir_is_global_write_low(const connection_t *conn, size_t attempt) +connection_dir_is_global_write_low(const connection_t *conn, size_t attempt, + const compress_method_t c_method) { size_t smaller_bucket = MIN(token_bucket_rw_get_write(&global_bucket), @@ -3214,6 +3218,10 @@ connection_dir_is_global_write_low(const connection_t *conn, size_t attempt) /* Special case for authorities (directory only). */ if (authdir_mode_v3(get_options())) { + if (c_method != NO_METHOD) { + /* Always answer compressed request. */ + return false; + } /* Are we configured to possibly reject requests under load? */ if (!get_options()->AuthDirRejectRequestsUnderLoad) { /* Answer request no matter what. */ diff --git a/src/core/mainloop/connection.h b/src/core/mainloop/connection.h index 668c7400426..a3a1352d7ac 100644 --- a/src/core/mainloop/connection.h +++ b/src/core/mainloop/connection.h @@ -197,7 +197,8 @@ void connection_mark_all_noncontrol_connections(void); ssize_t connection_bucket_write_limit(struct connection_t *conn, time_t now); bool connection_dir_is_global_write_low(const struct connection_t *conn, - size_t attempt); + size_t attempt, + const compress_method_t c_method); void connection_bucket_init(void); void connection_bucket_adjust(const or_options_t *options); void connection_bucket_refill_all(time_t now, diff --git a/src/feature/dircache/dircache.c b/src/feature/dircache/dircache.c index 59cdcc5e022..19768d1f085 100644 --- a/src/feature/dircache/dircache.c +++ b/src/feature/dircache/dircache.c @@ -951,7 +951,8 @@ handle_get_current_consensus(dir_connection_t *conn, goto done; } - if (connection_dir_is_global_write_low(TO_CONN(conn), size_guess)) { + if (connection_dir_is_global_write_low(TO_CONN(conn), size_guess, + compress_method)) { log_debug(LD_DIRSERV, "Client asked for network status lists, but we've been " "writing too many bytes lately. Sending 503 Dir busy."); @@ -1060,7 +1061,8 @@ handle_get_status_vote(dir_connection_t *conn, const get_handler_args_t *args) } }); - if (connection_dir_is_global_write_low(TO_CONN(conn), estimated_len)) { + if (connection_dir_is_global_write_low(TO_CONN(conn), estimated_len, + compress_method)) { write_short_http_response(conn, 503, "Directory busy, try again later"); goto vote_done; } @@ -1119,7 +1121,8 @@ handle_get_microdesc(dir_connection_t *conn, const get_handler_args_t *args) write_short_http_response(conn, 404, "Not found"); goto done; } - if (connection_dir_is_global_write_low(TO_CONN(conn), size_guess)) { + if (connection_dir_is_global_write_low(TO_CONN(conn), size_guess, + compress_method)) { log_info(LD_DIRSERV, "Client asked for server descriptors, but we've been " "writing too many bytes lately. Sending 503 Dir busy."); @@ -1217,7 +1220,8 @@ handle_get_descriptor(dir_connection_t *conn, const get_handler_args_t *args) msg = "Not found"; write_short_http_response(conn, 404, msg); } else { - if (connection_dir_is_global_write_low(TO_CONN(conn), size_guess)) { + if (connection_dir_is_global_write_low(TO_CONN(conn), size_guess, + compress_method)) { log_info(LD_DIRSERV, "Client asked for server descriptors, but we've been " "writing too many bytes lately. Sending 503 Dir busy."); @@ -1314,7 +1318,8 @@ handle_get_keys(dir_connection_t *conn, const get_handler_args_t *args) len += c->cache_info.signed_descriptor_len); if (connection_dir_is_global_write_low(TO_CONN(conn), - compress_method != NO_METHOD ? len/2 : len)) { + compress_method != NO_METHOD ? len/2 : len, + compress_method)) { write_short_http_response(conn, 503, "Directory busy, try again later"); goto keys_done; } diff --git a/src/test/test_bwmgt.c b/src/test/test_bwmgt.c index e6f028ed745..aede39d84ef 100644 --- a/src/test/test_bwmgt.c +++ b/src/test/test_bwmgt.c @@ -359,7 +359,7 @@ test_bwmgt_dir_conn_global_write_low(void *arg) * that our limit is _not_ low. */ addr_family = tor_addr_parse(&conn->addr, "1.1.1.1"); tt_int_op(addr_family, OP_EQ, AF_INET); - ret = connection_dir_is_global_write_low(conn, INT_MAX); + ret = connection_dir_is_global_write_low(conn, INT_MAX, NO_METHOD); tt_int_op(ret, OP_EQ, 0); /* Now, we will reject requests under load so try again a non authority non @@ -369,32 +369,36 @@ test_bwmgt_dir_conn_global_write_low(void *arg) addr_family = tor_addr_parse(&conn->addr, "1.1.1.1"); tt_int_op(addr_family, OP_EQ, AF_INET); - ret = connection_dir_is_global_write_low(conn, INT_MAX); + ret = connection_dir_is_global_write_low(conn, INT_MAX, NO_METHOD); tt_int_op(ret, OP_EQ, 1); /* Now, lets try with a connection address from moria1. It should always * pass even though our limit is too low. */ addr_family = tor_addr_parse(&conn->addr, "128.31.0.39"); tt_int_op(addr_family, OP_EQ, AF_INET); - ret = connection_dir_is_global_write_low(conn, INT_MAX); + ret = connection_dir_is_global_write_low(conn, INT_MAX, NO_METHOD); tt_int_op(ret, OP_EQ, 0); /* IPv6 testing of gabelmoo. */ addr_family = tor_addr_parse(&conn->addr, "[2001:638:a000:4140::ffff:189]"); tt_int_op(addr_family, OP_EQ, AF_INET6); - ret = connection_dir_is_global_write_low(conn, INT_MAX); + ret = connection_dir_is_global_write_low(conn, INT_MAX, NO_METHOD); tt_int_op(ret, OP_EQ, 0); /* Lets retry with a known relay address. It should pass. Possible due to * our consensus setting above. */ memcpy(&conn->addr, &relay_addr, sizeof(tor_addr_t)); - ret = connection_dir_is_global_write_low(conn, INT_MAX); + ret = connection_dir_is_global_write_low(conn, INT_MAX, NO_METHOD); tt_int_op(ret, OP_EQ, 0); /* Lets retry with a random IP that is not an authority nor a relay. */ addr_family = tor_addr_parse(&conn->addr, "1.2.3.4"); tt_int_op(addr_family, OP_EQ, AF_INET); - ret = connection_dir_is_global_write_low(conn, INT_MAX); + ret = connection_dir_is_global_write_low(conn, INT_MAX, NO_METHOD); + tt_int_op(ret, OP_EQ, 0); + + /* Compress method should always let us through. */ + ret = connection_dir_is_global_write_low(conn, INT_MAX, ZLIB_METHOD); tt_int_op(ret, OP_EQ, 0); /* Finally, just make sure it still denies an IP if we are _not_ a v3 @@ -402,13 +406,13 @@ test_bwmgt_dir_conn_global_write_low(void *arg) mock_options.V3AuthoritativeDir = 0; addr_family = tor_addr_parse(&conn->addr, "1.2.3.4"); tt_int_op(addr_family, OP_EQ, AF_INET); - ret = connection_dir_is_global_write_low(conn, INT_MAX); + ret = connection_dir_is_global_write_low(conn, INT_MAX, NO_METHOD); tt_int_op(ret, OP_EQ, 1); /* Random IPv6 should not be allowed. */ addr_family = tor_addr_parse(&conn->addr, "[CAFE::ACAB]"); tt_int_op(addr_family, OP_EQ, AF_INET6); - ret = connection_dir_is_global_write_low(conn, INT_MAX); + ret = connection_dir_is_global_write_low(conn, INT_MAX, NO_METHOD); tt_int_op(ret, OP_EQ, 1); done: From fb7f4e137f70587fa8d184c40b218b1203d6de68 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 8 Apr 2020 09:03:28 -0400 Subject: [PATCH 2/2] dirauth: Add option AuthDirRejectUncompressedRequests 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 --- doc/tor.1.txt | 4 ++++ scripts/maint/practracker/exceptions.txt | 2 +- src/app/config/config.c | 1 + src/app/config/or_options_st.h | 4 ++++ src/core/mainloop/connection.c | 6 ++++++ src/core/mainloop/connection.h | 2 ++ src/test/test_bwmgt.c | 7 +++++++ 7 files changed, 25 insertions(+), 1 deletion(-) diff --git a/doc/tor.1.txt b/doc/tor.1.txt index c7c41e78419..7f05e15ab96 100644 --- a/doc/tor.1.txt +++ b/doc/tor.1.txt @@ -2931,6 +2931,10 @@ on the public Tor network. bandwidth pressure (reaching the configured limit if any). Relays will always tried to be answered even if this is on. (Default: 1) +[[AuthDirRejectUncompressedRequests]] **AuthDirRejectUncompressedRequests** **0**|**1**:: + If set, the directory authority will reject every uncompressed directory + requests that is requests not ending with ".z" and lacking + "Accept-Encoding". (Default: 1) HIDDEN SERVICE OPTIONS ---------------------- diff --git a/scripts/maint/practracker/exceptions.txt b/scripts/maint/practracker/exceptions.txt index 7b15b37f8c5..a8ca61c2360 100644 --- a/scripts/maint/practracker/exceptions.txt +++ b/scripts/maint/practracker/exceptions.txt @@ -56,7 +56,7 @@ problem dependency-violation /src/core/crypto/onion_crypto.c 5 problem dependency-violation /src/core/crypto/onion_fast.c 1 problem dependency-violation /src/core/crypto/onion_tap.c 3 problem dependency-violation /src/core/crypto/relay_crypto.c 9 -problem file-size /src/core/mainloop/connection.c 5569 +problem file-size /src/core/mainloop/connection.c 5694 problem include-count /src/core/mainloop/connection.c 62 problem function-size /src/core/mainloop/connection.c:connection_free_minimal() 185 problem function-size /src/core/mainloop/connection.c:connection_listener_new() 324 diff --git a/src/app/config/config.c b/src/app/config/config.c index 8c635ab848b..b4fa96ee238 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -672,6 +672,7 @@ static const config_var_t option_vars_[] = { V(User, STRING, NULL), OBSOLETE("UserspaceIOCPBuffers"), V(AuthDirRejectRequestsUnderLoad, BOOL, "1"), + V(AuthDirRejectUncompressedRequests, BOOL, "1"), V(AuthDirSharedRandomness, BOOL, "1"), V(AuthDirTestEd25519LinkKeys, BOOL, "1"), OBSOLETE("V1AuthoritativeDirectory"), diff --git a/src/app/config/or_options_st.h b/src/app/config/or_options_st.h index e6be797017a..3932588232d 100644 --- a/src/app/config/or_options_st.h +++ b/src/app/config/or_options_st.h @@ -1015,6 +1015,10 @@ struct or_options_t { * regardless of bandwidth pressure or not. */ int AuthDirRejectRequestsUnderLoad; + /** Bool (default: 1) Reject uncompressed directory requests. A 503 error + * code is returned. */ + int AuthDirRejectUncompressedRequests; + /** Bool (default: 1): Switch for the shared random protocol. Only * relevant to a directory authority. If off, the authority won't * participate in the protocol. If on (default), a flag is added to the diff --git a/src/core/mainloop/connection.c b/src/core/mainloop/connection.c index 0d4bed01488..8bb3da3f29c 100644 --- a/src/core/mainloop/connection.c +++ b/src/core/mainloop/connection.c @@ -3218,6 +3218,12 @@ connection_dir_is_global_write_low(const connection_t *conn, size_t attempt, /* Special case for authorities (directory only). */ if (authdir_mode_v3(get_options())) { + /* If this requests is uncompressed and we are configured to reject those, + * indicate that have reached the limit thus deny answering. */ + if (c_method == NO_METHOD && + get_options()->AuthDirRejectUncompressedRequests) { + return true; + } if (c_method != NO_METHOD) { /* Always answer compressed request. */ return false; diff --git a/src/core/mainloop/connection.h b/src/core/mainloop/connection.h index a3a1352d7ac..b5f67c37dec 100644 --- a/src/core/mainloop/connection.h +++ b/src/core/mainloop/connection.h @@ -12,6 +12,8 @@ #ifndef TOR_CONNECTION_H #define TOR_CONNECTION_H +#include "lib/compress/compress.h" + listener_connection_t *TO_LISTENER_CONN(connection_t *); struct buf_t; diff --git a/src/test/test_bwmgt.c b/src/test/test_bwmgt.c index aede39d84ef..97062ef915e 100644 --- a/src/test/test_bwmgt.c +++ b/src/test/test_bwmgt.c @@ -336,6 +336,8 @@ test_bwmgt_dir_conn_global_write_low(void *arg) mock_options.AuthoritativeDir = 1; mock_options.V3AuthoritativeDir = 1; mock_options.UseDefaultFallbackDirs = 0; + mock_options.AuthDirRejectUncompressedRequests = 0; + mock_options.AuthDirRejectRequestsUnderLoad = 0; /* This will set our global bucket to 1 byte and thus we will hit the * banwdith limit in our test. */ @@ -401,6 +403,11 @@ test_bwmgt_dir_conn_global_write_low(void *arg) ret = connection_dir_is_global_write_low(conn, INT_MAX, ZLIB_METHOD); tt_int_op(ret, OP_EQ, 0); + /* Lets configure ourselves to reject uncompressed requests. */ + mock_options.AuthDirRejectUncompressedRequests = 1; + ret = connection_dir_is_global_write_low(conn, INT_MAX, NO_METHOD); + tt_int_op(ret, OP_EQ, 1); + /* Finally, just make sure it still denies an IP if we are _not_ a v3 * directory authority. */ mock_options.V3AuthoritativeDir = 0;