Opened 7 months ago

Closed 5 months ago

#33029 closed defect (implemented)

dir-auth: Dir auths should resume sending 503's but never to relays or other dir auths

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-dirauth 043-must 042-backport consider-backport-after-0433
Cc: armadev Actual Points: 1
Parent ID: #33018 Points: 0.4
Reviewer: nickm, armadev Sponsor:

Description

This is a child ticket from #33018.

The idea here is that even if we hit the global write limit (bw), we should not return 503 code but rather answer another directory authority.

Dirauth _must_ be able to talk to each other at all time regardless of the bandwidth state.

Setting 043 milestone because this should be considered a bug and could even be considered for backport since dirauth are suffering from this at the moment.

Child Tickets

Change History (41)

comment:1 Changed 7 months ago by dgoulet

Status: assignedneeds_review

Branch: ticket33029_043_01
PR: https://github.com/torproject/tor/pull/1681

comment:2 Changed 7 months ago by dgoulet

Points: 0.20.4

comment:3 Changed 7 months ago by arma

Looks good! A small issue:

  • "is the one of a configured directory" -> "is a configured directory"

and a bigger issue:

  • "so it might get a 503 code and thus fail the upload of its brand new descriptor" -- I don't think you can get a 503 in response to a post attempt. That is, we only check global_write_bucket_low() in five cases:
    • handle_get_current_consensus(), in response to a vanilla or microdesc consensus request
    • handle_get_status_vote(), for when somebody is asking for our current or most recent vote [that one's fun because only dir auths serve votes, and previously dir auths would never decide to reply with a 503]
    • handle_get_microdesc(), when somebody is asking for individual microdescs
    • handle_get_descriptor(), same as above but for vanilla descriptors
    • handle_get_keys(), when somebody is asking for authority certificates


So the "To clarify further the situation:" paragraph in the commit comment needs to change. I think the problematic scenario is that relays try to fetch new consensus and descriptor documents from authorities, because directory_fetches_from_authorities(), but the authorities give them a 503 and then they don't have a proper cached version to give out when clients come asking, and then clients don't get their network view and it all falls apart.

That's why this patch here should be ok for one or two authorities to run, but not more, until we also do the "whitelist relays" piece of it.

comment:4 in reply to:  3 Changed 7 months ago by arma

Summary: dir-auth: Never send a 503 directory request code to another directory authoritydir-auth: Dir auths should resume sending 503's but never to relays or other dir auths

Replying to arma:

That's why this patch here should be ok for one or two authorities to run, but not more, until we also do the "whitelist relays" piece of it.

Oh! I see that it does include this clause. So we should rename this ticket. I tried a new name here.

comment:5 Changed 7 months ago by arma

Another optimization we could do here: the whole priority thing is no longer used, since all five callers send in "priority 2". So we could either rip it out, or we could re-purpose it to use new kinds of priorities, like microdesc vs vanilla.

Slight preference for keeping it in, and then using it for item (6) in #33018 ("handle vanilla vs microdesc flavors differently").

comment:6 Changed 7 months ago by teor

Status: needs_reviewneeds_revision

Please check for authority IPv6 addresses. I'm just about to make relays use IPv6 to authorities as part of sponsor 55, so we need an IPv6 check when we whitelist relays.

There's a few subtle address issues in this patch, which we should document:

  • configured vs consensus addresses
  • inbound vs outbound addresses

See the PR for details.

comment:7 Changed 7 months ago by nickm

Keywords: 043-should? added

comment:8 Changed 6 months ago by arma

I have been running this patch on moria1 lately, with an additional patch where I send a 503 response to vanilla-flavored consensus fetches, or old-style descriptor fetches, if they're not from a dir auth or relay address, even if I otherwise have enough bandwidth to answer them.

With both patches in place, moria's outbound traffic has gone from 200-500mbit/s down to 10-40mbit/s.

Here are some stats from a one hour period (1400 to 1500 EST):


Dir auth requests

I whitelisted 13 dirport connections from dir auths during this time:

Jan 24 14:18:32.445 [notice] Prioritizing dir auth response
Jan 24 14:31:28.374 [notice] Prioritizing dir auth response
Jan 24 14:50:03.921 [notice] Prioritizing dir auth response
Jan 24 14:50:04.452 [notice] Prioritizing dir auth response
Jan 24 14:50:04.836 [notice] Prioritizing dir auth response
Jan 24 14:50:05.016 [notice] Prioritizing dir auth response
Jan 24 14:50:05.261 [notice] Prioritizing dir auth response
Jan 24 14:50:05.458 [notice] Prioritizing dir auth response
Jan 24 14:50:05.575 [notice] Prioritizing dir auth response
Jan 24 14:50:05.697 [notice] Prioritizing dir auth response
Jan 24 14:50:05.808 [notice] Prioritizing dir auth response
Jan 24 14:50:07.510 [notice] Prioritizing dir auth response
Jan 24 14:50:09.915 [notice] Prioritizing dir auth response

Looking through the logs, these are all for /extra/d or server/d, i.e. old-style descriptors. Most of them occur a little bit after :50, which makes sense because that would be when those other dir auths discovered new descriptors from the vote I just sent them.


Relay requests

I whitelisted 847 dirport requests from relay addresses during this period. Of these, 763 of them happened in the first half of the period (:00 through :30), which makes sense because fetch-extra-early tries to get a cached copy of the consensus in place before clients start asking for it. Accounting for a bit of time skew, 830 of the 847 happened between :00 and :33.

Spot-checking these fetches, every single one of them that I looked at was a fetch for /micro/d, i.e. fetching a new microdescriptor. That's weird! I would have thought many of them would be fetching a microdesc-flavored consensus. I wonder if I am failing to log those, or if the process of answering them bypasses global_write_bucket_low().


Non-relay requests

I sent back 110818 "503" responses during this hour (i.e. averaging over thirty "503" responses per second).

Of those, 105452 were for "network status lists":
Jan 24 14:00:00.038 [info] handle_get_current_consensus(): Client asked for network status lists, but we've been writing too many bytes lately. Sending 503 Dir busy.
which I think is almost entirely requests to

And the remaining 5366 were for "server descriptors":
Jan 24 14:00:02.387 [info] handle_get_descriptor(): Client asked for server descriptors, but we've been writing too many bytes lately. Sending 503 Dir busy.

comment:9 in reply to:  6 Changed 6 months ago by arma

Replying to teor:

Please check for authority IPv6 addresses. I'm just about to make relays use IPv6 to authorities as part of sponsor 55, so we need an IPv6 check when we whitelist relays.

Thanks, teor, this is a great point.

I actually think I have a better plan that will accomplish your goal and the rest of these goals better: let's make sure the dir auth addresses (all of them) are added to the bloom filter that nodelist_probably_contains_address() checks, and then just only check that and we're done. That is, the logic should be: "if we're a dir auth, always answer every question from other relays."

It looks like the ipv6 address for relays gets added properly in node_add_to_address_set():

    if (!tor_addr_is_null(&node->rs->ipv6_addr))
      address_set_add(the_nodelist->node_addrs, &node->rs->ipv6_addr);
[...]
    if (!tor_addr_is_null(&node->ri->ipv6_addr))
      address_set_add(the_nodelist->node_addrs, &node->ri->ipv6_addr);
[...]
    if (!tor_addr_is_null(&node->md->ipv6_addr))
      address_set_add(the_nodelist->node_addrs, &node->md->ipv6_addr);

So the change we would want to make is in or near nodelist_set_consensus(), where right after we call

  /* Now add all the nodes we have to the address set. */
  SMARTLIST_FOREACH_BEGIN(the_nodelist->nodes, node_t *, node) {
    node_add_to_address_set(node);
  } SMARTLIST_FOREACH_END(node);

we call some similar thing that loops through trusted_dir_servers and calls address_set_add() and/or address_set_add_ipv4h() on each known dir auth address. That way we get the dir auth addresses in the consensus because they are relays, and we get the configured addresses (if different) with this new code.

And then the minor worry in dgoulet's code about "if this shows up in the profile, we can move to have an address set instead" gets resolved too because it is just one thing we are checking.

And as a tiny bonus, we handle dir auth addresses as though they are relays from the perspective of the DoS module, which is probably a thing we should have done from the beginning there anyway.

Do you buy all of that, dgoulet and teor?

comment:10 in reply to:  8 Changed 6 months ago by arma

Replying to arma:

Here are some stats from a one hour period (1400 to 1500 EST):

These stats are not right. I am doing another experiment and will report new numbers soonish.

comment:11 Changed 6 months ago by dgoulet

And then the minor worry in dgoulet's code about "if this shows up in the profile, we can move to have an address set instead" gets resolved too because it is just one thing we are checking.

The reason I did _not_ go with the dirauth + bloomfilter idea is because when we ask "is this address a dirauth?", we should NOT get a probabilistic answer which is what the address_set does unfortunately.

comment:12 Changed 6 months ago by dgoulet

Status: needs_revisionneeds_review

Published a new version of the code on the PR.

It considers IPv6 but as teor mentions, only the trusted configured authorities, not from the consensus. For dirauth, that is fine for now since they all configure the other dirauths in their torrc.

Also, this can backfire if a dirauth outbound address is different from its inbound DirPort address.

comment:13 in reply to:  11 ; Changed 6 months ago by nickm

Replying to dgoulet:

And then the minor worry in dgoulet's code about "if this shows up in the profile, we can move to have an address set instead" gets resolved too because it is just one thing we are checking.

The reason I did _not_ go with the dirauth + bloomfilter idea is because when we ask "is this address a dirauth?", we should NOT get a probabilistic answer which is what the address_set does unfortunately.

Are you sure here? We never get false negatives; only false positives. When we're asking "is address a dirauth" it's not so bad if we accidentally allow a tiny fraction of non-dirauths; only if we accidentally block a dirauth.

comment:14 in reply to:  13 Changed 6 months ago by dgoulet

Are you sure here? We never get false negatives; only false positives. When we're asking "is address a dirauth" it's not so bad if we accidentally allow a tiny fraction of non-dirauths; only if we accidentally block a dirauth.

Ah true!!! I always forgot it is the other way around. Ok then, we should simply use the address set for which we'll get the IPv6 for free it seems.

comment:15 Changed 6 months ago by dgoulet

Ok, I've re-engineered quite a bit the branch after all the feedbacks. I've created a new branch thus a new PR:

PR: https://github.com/torproject/tor/pull/1691
Branch: ticket33029_043_02

Now the approach is simplified. HOWEVER, because this branch only uses the nodelist address set, the authority will fail to recognize its fellow authorities as long as it doesn't have a consensus. I think that is fine but it might not be if anyone can think of a reason why. I can see that the authority is starting and gets bombarded already but doesn't have a consensus?

If this is indeed an issue, we'll have to fallback to testing the trusted dir list directly.

comment:16 Changed 6 months ago by dgoulet

Reviewer: nickm, teor, armadev

comment:17 Changed 6 months ago by arma

In commit 823006f5, in the commit message, s/seperate/separate/ and s/addresse/address/

If somebody shows me how to submit this as a comment on the commit in github, I will do that. :)

[Edit: ok, there is a way to submit a comment on the whole PR. I did that. If you know a way to submit a comment on a commit, I am still hoping to learn that. :)

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

comment:18 Changed 6 months ago by arma

Thanks, dgoulet, the new (fixed up, squashed) branch looks great.

And while I'm here, here is a minimal patch for Tor stables that I think will do everything we need. We might choose to do something else for an official backport, but in the meantime I'll offer this one to dir auth operators who want to patch but don't want to move to git master and don't want to examine six commits themselves.

index 6094f33e4d..b0687dc2fd 100644
--- a/src/core/mainloop/connection.c
+++ b/src/core/mainloop/connection.c
@@ -3212,7 +3212,8 @@ global_write_bucket_low(connection_t *conn, size_t attempt, int priority)
     MIN(token_bucket_rw_get_write(&global_bucket),
         token_bucket_rw_get_write(&global_relayed_bucket));
   if (authdir_mode(get_options()) && priority>1)
-    return 0; /* there's always room to answer v2 if we're an auth dir */
+    if (nodelist_probably_contains_address(&conn->addr))
+      return 0; /* always answer relays if we're an auth dir */
 
   if (!connection_is_rate_limited(conn))
     return 0; /* local conns don't get limited */

comment:19 in reply to:  15 ; Changed 6 months ago by teor

Status: needs_reviewneeds_revision

I think this patch looks good, I made a small suggestion that removes a few duplicate checks.

What about the bridge authority?
Does it need to allow bridges to post descriptors and get directory documents?
If so, we need a version of nodelist_probably_contains_address() that contains bridge addresses.

Replying to dgoulet:

Now the approach is simplified. HOWEVER, because this branch only uses the nodelist address set, the authority will fail to recognize its fellow authorities as long as it doesn't have a consensus. I think that is fine but it might not be if anyone can think of a reason why. I can see that the authority is starting and gets bombarded already but doesn't have a consensus?

If this is indeed an issue, we'll have to fallback to testing the trusted dir list directly.

If a directory authority doesn't have a consensus, then it can't serve any consensuses. And it will probably refuse to serve most other directory documents. So I think we're fine here.

comment:20 in reply to:  19 ; Changed 6 months ago by arma

Replying to teor:

What about the bridge authority?
Does it need to allow bridges to post descriptors

None of these functions are about how to respond to directory_handle_command_post(), so posting descriptors should be fine and unaffected, both from bridge relays and from other kinds of relays.

and get directory documents?

I don't think the bridge authority is any different from any other dir cache, with respect to publishing the consensus. The fact that it doesn't have the Guard flag will mean it is mostly ignored by clients.

In an alternate universe where we'd finished building UpdateBridgesFromAuthority, then we'd want to make sure that it is willing to serve bridge descriptors to people who know the right fingerprint for them. But we don't live in that universe so I don't think it's an issue. And also that design is about responding to clients -- who might be routing their requests via Tor, in which case they'll get prioritized because the requests come from relays, and if not, they would be random looking clients anyway.

Though! This does make me realize: our changed behavior here applies to begindir requests over the ORPort too, right? So we are now willing to give 503 responses to clients when we're under load. Maybe that's a feature, since "under load" means we'd be answering them poorly anyway. It definitely makes me more interested in the "several tiers of responses (like 'orport or dirport', 'micro flavored consensus or vanilla flavored consensus', 'requests compression or doesn't') where we reserve some of our bandwidth and don't spend it on the lowest tier" sort of design. But that's something we can save for #33018.

comment:21 in reply to:  20 Changed 6 months ago by arma

Replying to arma:

I don't think the bridge authority is any different from any other dir cache, with respect to publishing the consensus.

I think by that logic, we should change

   /* Special case for authorities (directory and bridge). */
   if (authdir_mode(get_options())) {

to

   /* Special case for v3 directory authorities. */
   if (authdir_mode_v3(get_options())) {

That is, the bridge authority should feel free to send 503's in response to directory fetches, just like any other relay.

comment:22 Changed 6 months ago by dgoulet

Status: needs_revisionneeds_review

Fixed pushed for the _v3... I agree, I originally made it v3 only and then changed for unknown reasons.

comment:23 Changed 6 months ago by teor

Reviewer: nickm, teor, armadevnickm, armadev

comment:24 Changed 6 months ago by teor

I've been tracking these changes, they look good to me.

comment:25 Changed 6 months ago by nickm

For mysterious reasons, github isn't letting me commetn right now, so I'm going to have to do the comments here, old-school.

On dirlist: Add configured trusted dir to the nodelist address set:

  • We should rename dirlist_add_trusted_addresses(). It makes it sound like the addresses themselves are trusted, but we don't actually trust the addresses: they are just the addresses of trusted dirauths. We don't want somebody else down the line think that these addresses are automatically trusted.

Otherwise this looks correct to me!

comment:26 Changed 6 months ago by nickm

Status: needs_reviewneeds_revision

comment:27 Changed 6 months ago by dgoulet

Status: needs_revisionneeds_review

Name fixed in commit 14c8b6628964ebc8

comment:28 Changed 6 months ago by nickm

lgtm. Now we only need to decide on maybe backport/no backport, and see whether a backport has to happen.

comment:29 Changed 6 months ago by nickm

Status: needs_reviewneeds_revision

Putting this on needs_revision for now, because of the possibility of needing an 042 backport. If we decide not to backport, then it can go into merge_ready IMO, especially if it has had testing on a dirauth.

comment:30 Changed 6 months ago by dgoulet

Actual Points: 1
Keywords: 043-must 042-backport added; 043-should? removed
Status: needs_revisionneeds_review

Discussion happened on the dir-auth list. Backport was asked for by some members and so here it is. I've only gone to 0.4.2 since all dirauth are on 042 except one at this time and they should upgrade.

0.4.2

Branch: ticket33029_042_01
PR: https://github.com/torproject/tor/pull/1718

I have created a new 043 branch that merges forward the above with a commit that fixes the conflicts (not straightforward due to dirauth config refactor between 042 and 043):

0.4.3

Branch: ticket33029_043_03
PR: https://github.com/torproject/tor/pull/1719

comment:31 Changed 6 months ago by dgoulet

FYI: longclaw is currently testing the 042 branch.

comment:32 Changed 6 months ago by nickm

Cool! FWIW I'd like testing with both branches before we consider the merge.

comment:33 Changed 6 months ago by nickm

I've read the code again, and it looks good to me. Once there's testing, let's merge.

comment:34 Changed 6 months ago by arma

I'm running the 043 branch on moria1 for the past few days and nothing has exploded.

(The overload seems to have gone away to some extent; I haven't explored in detail yet.)

comment:35 Changed 6 months ago by nickm

(Have we gotten anybody to test 0.4.2? How is that going?)

comment:36 Changed 6 months ago by dgoulet

Status: needs_reviewmerge_ready

I know moria1 (043) is happy with it. It loads has decreased but unsure if due to the patch helping or DoS stopping.

As for 042, longclaw is telling me that everything is well. And bandwidth stabilized also.

I believe this is a go! so we can get to finally move on with #33072.

comment:37 Changed 6 months ago by nickm

Milestone: Tor: 0.4.3.x-finalTor: 0.4.2.x-final

pffew!

Merging to 0.4.3, marking for 0.4.2 backport. I believe we should expedite that 042 backport since this is a dirauth dos issue, and since this has already received a bunch of testing.

comment:38 Changed 5 months ago by teor

Keywords: consider-backport-after-0433 added

Nick, I'm not sure exactly what timeframe you had in mind here.

Do you want to merge it before or after 0.4.3.3-alpha?

comment:39 Changed 5 months ago by nickm

I _think_ this should be safe to merge before 0.4.3 alpha, given how much testing it has had.

comment:40 Changed 5 months ago by nickm

Merging this to 0.4.2 now; it should help network stability.

comment:41 Changed 5 months ago by nickm

Resolution: implemented
Status: merge_readyclosed
Note: See TracTickets for help on using tickets.