David Goulet authored 4 years ago and Nick Mathewson committed 4 years ago

Handle the EOF situation for a metrics connection. Furthermore, if we failed
to fetch the data from the inbuf properly, mark the socket as closed because
the caller, connection_process_inbuf(), assumes that we did so on error.

Fixes #40257

Signed-off-by: David Goulet <dgoulet@torproject.org>

In two instances we must look at this flag:

1. When we build the descriptor so the IPv6 is NOT added to the descriptor in
   case we judge that we need to omit the address but still publish.

2. When we are deciding if the descriptor is publishable. This flags tells us
   that the IPv6 was not found reachable but we should still publish.

Fixes #40279

Signed-off-by: David Goulet <dgoulet@torproject.org>

Signed-off-by: David Goulet <dgoulet@torproject.org>

Any lookup now will be certain and not probabilistic as the bloomfilter.

Closes #40269

Signed-off-by: David Goulet <dgoulet@torproject.org>

This is to minimize false positive and thus deny reentry to Exit connections
that were in reality not re-entering. Helps with overall UX.

Signed-off-by: David Goulet <dgoulet@torproject.org>

Any lookup now will be certain and not probabilistic as the bloomfilter.

Closes #40269

Signed-off-by: David Goulet <dgoulet@torproject.org>

This is to minimize false positive and thus deny reentry to Exit connections
that were in reality not re-entering. Helps with overall UX.

Signed-off-by: David Goulet <dgoulet@torproject.org>

In order to deny re-entry in the network, we now keep a bloomfilter of relay
ORPort + address and authorities ORPort + address and DirPort + address
combinations.

So when an Exit stream is handled, we deny anything connecting back into the
network on the ORPorts for relays and on the ORPort+DirPort for the
authorities.

Related to #2667

Signed-off-by: David Goulet <dgoulet@torproject.org>

George Kadianakis authored 4 years ago and David Goulet committed 4 years ago

Signed-off-by: David Goulet <dgoulet@torproject.org>

In order to deny re-entry in the network, we now keep a bloomfilter of relay
ORPort + address and authorities ORPort + address and DirPort + address
combinations.

So when an Exit stream is handled, we deny anything connecting back into the
network on the ORPorts for relays and on the ORPort+DirPort for the
authorities.

Related to #2667

Signed-off-by: David Goulet <dgoulet@torproject.org>

David Goulet authored 4 years ago and Nick Mathewson committed 4 years ago

Everytime we try to discover an address we want to publish, emit a log notice
if we are unable to find it even though an ORPort was configured for it.

Because the function can be called quite often, we rate limit that notice to
every hour so it gets annoying just enough so the operator fixes that.

Related to #40254

Signed-off-by: David Goulet <dgoulet@torproject.org>

David Goulet authored 4 years ago and Nick Mathewson committed 4 years ago

We would before do an address discovery and then a lookup in the cache if not
found which is now simplified by calling relay_find_addr_to_publish() directly
which does all those combined.

Furthermore, by doing so, we won't trigger an address discovery every minute
if we have no ORPort configured for the family.

Fixes #40254

Signed-off-by: David Goulet <dgoulet@torproject.org>

David Goulet authored 4 years ago and Nick Mathewson committed 4 years ago

Now that relay_find_addr_to_publish() checks if we actually have an ORPort, we
can simplify the descriptor building phase for IPv6.

This also avoid triggering an IPv6 discovery if the IPv4 can't be found in the
first place.

Related to #40254

Signed-off-by: David Goulet <dgoulet@torproject.org>

David Goulet authored 4 years ago and Nick Mathewson committed 4 years ago

In other words, if we don't have an ORPort configured for a specific family
(IPv4/v6), we don't bother doing address discovery.

Related to #40254

Signed-off-by: David Goulet <dgoulet@torproject.org>

Roger Dingledine authored 4 years ago and David Goulet committed 4 years ago

Give more visibility to directory authority operators during the consensus
voting process.

Closes ticket 40245.

Roger Dingledine authored 4 years ago and David Goulet committed 4 years ago

This step happens after we make each consensus flavor, and before we
worry about sigs or anything. That way if Tor crashes, or if we fail to
get enough sigs, we still have a chance to know what consensus we wanted
to make.

In addition to simplifying callsites a little, this function gives
correct behavior for bridges without a configured transport.

Don't pick the bridge as the guard or launch descriptor fetch if no transport
is found.

Fixes #40106

Signed-off-by: David Goulet <dgoulet@torproject.org>

This patch limits the number of items in the consensus diff cache to 64
on the Windows platform. Hopefully, this will allow us to investigate a
smarter fix while avoiding the situation reported in tor#24857 where
Windows relay operators report Tor using 100% CPU.

See: tor#24857

We used to actually discard ORPorts that were the same port and same family
but they could have different address.

Instead, we need to keep all different ORPorts so we can bind a listener on
each of them. We will publish only one of these in our descriptor though.

Related to #40246

Signed-off-by: David Goulet <dgoulet@torproject.org>

This reverts commit d07f17f6.

We don't want to consider an entire routable IPv6 network as sybil if more
than 2 relays happen to be on it. For path selection it is very important but
not for selecting relays in the consensus.

Fixes #40243

We can end up trying to find our address from an authority while we don't have
yet its descriptor.

In this case, don't BUG() and just come back later.

Closes #40231

Signed-off-by: David Goulet <dgoulet@torproject.org>

In case building the descriptor would fail, we could still flag that we did in
fact publish the descriptors leading to no more attempt at publishing it which
in turn makes the relay silent for some hours and not try to rebuild the
descriptor later.

This has been spotted with #40231 because the operator used a localhost
address for the ORPort and "AssumeReachable 1" leading to this code path where
the descriptor failed to build but all conditions to "can I publish" were met.

Related to #40231

Signed-off-by: David Goulet <dgoulet@torproject.org>

This one should work on GCC _and_ on Clang.  The previous version
made Clang happier by not having unreachable "fallthrough"
statements, but made GCC sad because GCC didn't think that the
unconditional failures were really unconditional, and therefore
_wanted_ a FALLTHROUGH.

This patch adds a FALLTHROUGH_UNLESS_ALL_BUGS_ARE_FATAL macro that
seems to please both GCC and Clang in this case: ordinarily it is a
FALLTHROUGH, but when ALL_BUGS_ARE_FATAL is defined, it's an
abort().

Fixes bug 40241 again.  Bugfix on earlier fix for 40241, which was
merged into maint-0.3.5 and forward, and released in 0.4.5.3-rc.

Fixes bug #40234; bugfix on 0.3.2.5-alpha.

Some days before this commit, the network experienced a DDoS on the directory
authorities that prevented them to generate a consensus for more than 5 hours
straight.

That in turn entirely disabled onion service v3, client and service side, due
to the subsystem requiring a live consensus to function properly.

We know require a reasonably live consensus which means that the HSv3
subsystem will to its job for using the best consensus tor can find. If the
entire network is using an old consensus, than this should be alright.

If the service happens to use a live consensus while a client is not, it
should still work because the client will use the current SRV it sees which
might be the previous SRV for the service for which it still publish
descriptors for.

If the service is using an old one and somehow can't get a new one while
clients are on a new one, then reachability issues might arise. However, this
is a situation we already have at the moment since the service will simply not
work if it doesn't have a live consensus while a client has one.

Fixes #40237

Signed-off-by: David Goulet <dgoulet@torproject.org>

Thanks to proposal 315 / ticket #30132, more fields are now
required in these documents.  But ancient Tors that try to upload
obsolete documents were causing the authorities to log warnings
about missing fields, and to do so very spammily.

We now detect the missing fields before tokenizing, and log at
debug.  This is a bit of ugliness, but it's probably a safer choice
than making _all_ unparseable-desc warnings into debug-level logs.

I'm looking at identity-ed25519 in extrainfos and proto in
routerdescs because they were (I believe) the latest-added fields in
Tor's history: any Tor that lacks them will also lack the other
newly required fields.

Fixes bug #40238; bugfix on 0.4.5.1-alpha.

We're getting "fallback annotation annotation in unreachable code"
warnings when we build with ALL_BUGS_ARE_FATAL. This patch fixes
that.

Fixes bug 40241.  Bugfix on 0.3.5.4-alpha.

Karsten Loesing authored 4 years ago and Nick Mathewson committed 4 years ago

The previous parser only considered stats files _starting_ with the
timestamp tag, not stats files having the timestamp tag in a later
position. While this applies to all current stats files, a future
stats file might look differently. Better to fix the function now than
be surprised in another 9 years from now.

This commit also adds a test case for such future stats, and it fixes
stats file paths in newly added unit tests.

David Goulet authored 4 years ago and Nick Mathewson committed 4 years ago

It turns out that 9 years ago, we stopped appending data into stats file and
rather overwrite everytime we have new stats (see commit
a6a127c8)

The load_stats_file() function was still thinking that we could have the same
line many times in the file which turns out to be false since 9 years ago.
However, that did not cause problem until IPv6 connection stats came along
which introduced a new line in conn-stats: "ipv6-conn-bi-direct ...".

Before, that file contained a single line starting with the tag
"conn-bi-direct".  That very tag appears also in the IPv6 tag (see above) so
the load_stats_file() function would consider that the IPv6 line as the last
tag to be appeneded to the file and fail to report the line above (for IPv4).
It would actually truncate the IPv6 line and report it (removing the "ipv6-"
part).

In other words, "conn-bi-direct" was not reported and instead
"ipv6-conn-bi-direct" was used without the "ipv6-" part.

This commit refactors the entire function so that now it looks for a
"timestamp tag" to validate and then if everything is fine, returns the entire
content of the file. The refactor simplifies the function, adds logging in
case of failures and modernize it in terms of coding standard.

Unit tests are also added that makes sure the loaded content matches the
entire file if timestamp validation passes.

Fixes #40226

Signed-off-by: David Goulet <dgoulet@torproject.org>

David Goulet authored 4 years ago and Alexander Hansen Færøy committed 4 years ago

When making sure we have a Bridge line with a ClientTransportPlugin, we
now check in the managed proxy list and so we can catch any missing
ClientTransportPlugin for a Bridge line.

Fixes #40106

Signed-off-by: David Goulet <dgoulet@torproject.org>

Fixes #40205

Signed-off-by: David Goulet <dgoulet@torproject.org>

Fixes #25528

Signed-off-by: David Goulet <dgoulet@torproject.org>

The function in charge of removing duplicate ORPorts from our configured ports
was skipping all non ORPorts port but only for the outer loop thus resulting
in comparing an ORPort with a non-ORPort which lead to problems.

For example, tor configured with the following would fail:

  ORPort auto
  DirPort auto

Both end up being the same configuration except that one is a OR listener and
one is a Dir listener. Thus because of the missing check in the inner loop,
they looked exactly the same and thus one is removed.

Fixes #40195

Signed-off-by: David Goulet <dgoulet@torproject.org>

David Goulet authored 4 years ago and Nick Mathewson committed 4 years ago

First, this commit moves the launch_dummy_circuit_as_needed() function into
relay_find_addr.c and renames it to relay_addr_learn_from_dirauth(). This is
an attempt to centralize anything relate with address discovery in the right
module.

Second, when building a descriptor and we fail to discover our address,
immediately launch a dummy circuit to an authority in an attempt to learn our
descriptor.

It is still only done every 20 minutes even though the descriptor build is
done every minute. We ought to avoid load on the authority and if we can't
learn in the first place our address from them, chances are more things are
wrong.

Related to #40071

Signed-off-by: David Goulet <dgoulet@torproject.org>

David Goulet authored 4 years ago and Nick Mathewson committed 4 years ago

Regularly, tor looks if its IP has changed. It does the entire auto discovery
process again. However, it is possible that it does not find anything.

Instead of thinking the IP changed to an unknown address, look at our cache
and see if that value has changed.

The reason for this is because if tor gets its address as a suggestion from a
directory authority, it is because the auto discovery failed and thus that
address should be consider for the IP change check.

Related to #40071

Signed-off-by: David Goulet <dgoulet@torproject.org>

David Goulet authored 4 years ago and Nick Mathewson committed 4 years ago

Tor now can learn its address from a NETINFO cell coming from an authority.
Thus, instead from launching a dummy descriptor fetch to learn the address
from the directory response (unauthenticated), we simply now launch a one-hop
testing circuit.

Related to #40071

Signed-off-by: David Goulet <dgoulet@torproject.org>

Commit c3a0f757 added this feature for ORPort
that we ignore any port that is not the family of our default address when
parsing the port. So if port_parse_config() was called with an IPv4 default
address, all IPv6 address would be ignored.

That makes sense for ORPort since we call twice port_parse_config() for
0.0.0.0 and [::] but for the rest of the ports, it is not good since a
perfectly valid configuration can be:

  SocksPort 9050
  SocksPort [::1]:9050

Any non-ORPort only binds by default to an IPv4 except the ORPort that binds
to both IPv4 and IPv6 by default.

The fix here is to always parse all ports within port_parse_config() and then,
specifically for ORPort, remove the duplicates or superseding ones. The
warning is only emitted when a port supersedes another.

A unit tests is added to make sure SocksPort of different family always exists
together.

Fixes #40183

Signed-off-by: David Goulet <dgoulet@torproject.org>

Samanta Navarro authored 4 years ago and David Goulet committed 4 years ago

Typos found with codespell.

Please keep in mind that this should have impact on actual code
and must be carefully evaluated:

src/core/or/lttng_circuit.inc
-    ctf_enum_value("CONTROLER", CIRCUIT_PURPOSE_CONTROLLER)
+    ctf_enum_value("CONTROLLER", CIRCUIT_PURPOSE_CONTROLLER)