Just as `in_same_family` is a member of Relay, so the function for
getting all the real family members of a relay should belong in the
same crate.
This change also removes the `family()` accessor: it gives the _claimed_ family rather
than the _acknlowedged_ family, and is therefore a bit dangerous.
There's still a hole in this logic; I've noted it in the Limitations
section. If we get a microdescriptor for a relay in between creating
and using the guard restriction, it might be omitted from the family
list.
There's not much reason to use a HashSet here, since we're just
going over the whole list.
This reverts commit 16e8489abb and does a little more
refactoring.
The redundant method was a `to_owned` that probably shouldn't have
been called that. It was only used in one place.
The tests should get tor-linkspec's line coverage up above 90%.
This was a relic of the old, now-unused "caret_enum!" macro.
Removing it gets caret's coverage to 100%.
Yes, technically this is a semver breaker on caret.
For this one I just wrote some "are things completely broken" tests
for the rand_compat wrappers. These won't detect subtle biases in
the RNGs! They'll only let you know if the wrappers have screwed up
in some way that always sets a given bit to 1 or 0.
This is mostly a finger exercise, and an experiment in "what does
grcov consider to be coverage". Here's what I've found out...
* In grcov's eyes, most #[derive(Foo)] lines count as containing code;
but calling any one derived function counts as calling those lines.
* Unlike with tarpaulin, it is actually possible to reach 100% grcov
line coverage. (Tarpaulin likes to pick "}" lines and tell you that
you never reached them; or sometimes it picks expression
statements that have the effect of a return, and tells you that
they're unreached. Even with these tests, tarpaulin claims that
the line coverage of tor-units is only 97.3%.)
* In rust, it may be a bit hopeless trying to get high function
coverage. Even though we've hit every line of the tor-units crate,
the function coverage from its own tests is only 9.38% (55.41%
from other crates). I think this is probably due to derived
functions, or maybe due to generics getting instantiated?
I've got no idea; the denominator for the function coverage
lines fluctuates oddly.
The sane_defaults() call is now the same as you get from a default
builder: by convention, we just call that method Default::default().
The with_directories() constructor makes more sense as a constructor
for the TorClientConfigBuilder than for TorClientConfig.
This test seems unreliable on CI: we've got to disable them for now
so that we have a working CI system. The CI failure is #238; the
ticket to repair them is #244.
The new CircMgr::build_circuits_preemptively function actually causes
preemptive circuits to be built; it gets called from arti-client, like
the other daemon tasks the CircMgr has.
This commit implements the "metabuilder" pattern and the "builder
reconstruction" pattern for the ArtiConfig type.
I'm not 100% that this will be necessary, but it will certainly help
with testing.
I'm still not 100% sure this is the right move: should we encourage
app developers to always pick their own directories? Or should we
make it easy for them to use, well, `sane_defaults`?
This patch takes the second approach.
Rust nightly claims that Vec might get its own retain_mut method,
which would potentially conflict with the extension method we've
grabbed from the retain_mut crate. To solve this, we're calling the
method explicitly.
In preparation for making Arti build circuits preemptively, this commit
introduces `TargetCircUsage::Preemptive`, a circuit usage that works
somewhat differently from other ones: it requires at least 2 circuits to
exist that can exit the port it contains in order for an existing
circuit to match against it (path-spec.txt § 2.1.1); if that's not the
case, that usage will require building new circuits (in order that we
build enough to have 2 available).
This required refactoring how circuit reuse worked; now,
`CircList::find_open` uses the new `AbstractSpec::find_supported` trait
method, which we customize to implement the above check in the case of
`Preemptive` circuit usages. To make that work, `OpenEntry` now takes
two type parameters (the spec and circuit types), instead of taking a
builder type parameter and using its associated types. (We also got rid
of type constraints on that struct, yay!)
A WIP implementation of a preemptive circuit predictor that implements
path-spec.txt § 2.1.1 is also included, but this will require additional
effort to wire it up with the `CircMgr` properly.
I traced the problem here to the fact that sometimes "rx" in this
test would be dropped before the test was done. When "rx" is
dropped, the channel reactor shuts down, which in turn kills off the
circuit reactor.
This bug may exist in other cases in these tests. This patch may
fix one case of #238.
This implements a basic typed event broadcast mechanism, as described in
arti#230: consumers of the new `tor-events` crate can emit `TorEvent`
events, which others can consume via the `TorEventReceiver`.
Under the hood, the crate uses the `async-broadcast`
(https://github.com/smol-rs/async-broadcast) crate, and a
`futures::mpsc::UnboundedSender` for the event emitters; these are glued
together in the `EventReactor`, which must be run in a background thread
for things to work. (This is done so event sending is always cheap and
non-blocking, since `async-broadcast` senders don't have this
functionality.)
Additionally, the `TorEventKind` type is used to implement selective
event reception / emission: receivers can subscribe to certain event
types (and in fact start out receiving nothing), which filters the set
of events they receive. Having no subscribers for a given event type
means it won't even be emitted in the first place, making things more
efficient.
This patch makes sure that for every* config type we have, the defaults
you get from a Builder match those you get from Serde, and that both
match the value that you get from arti_defaults.toml. Later down the
line I'll be adding some tests to keep these in sync.
* StorageConfig still has no defaults of its own, since we aren't so
sure we want other applications to use Arti's directories by default.
Since these shell-variables are hardwired to use org.torproject.Arti as
the program name, it isn't appropriate to call them "app-specific".
If we someday reinstate APP_FOO, it should be based on a user-provided
application name.
In order to handle explicitly specified path buffers directly, we now
let CfgPath be either a string (that gets expanded) or a PathBuf
(that doesn't).
This simplifies TorClientConfig::with_directories()
Now every section that the two configuration objects share has the
same type and name. This should help us in documenting our configuration
in a way that doesn't confuse people.
There is still lots of API work to go.
To do this at all neatly, I had to split out `tor-config` from
`arti-config` again, and putting the lower level stuff (paths,
builder errors) into tor-config. I also changed our use of
derive_builder to always use a common error type, to avoid
error type proliferation.
The `bad_extend_*` failures were caused by bad test code in
`bad_extend_test_impl` that used `futures::join!`; this meant that the
reactor could receive the `Extended2` cell before it actually got the
`ExtendNtor` request, which caused it to get (quite rightly) confused
and close the circuit. Spawning a background thread which has a short
delay before sending the `Extended2` cell seems to have alleviated this
problem.
`new_circ_create_failure` is similar; I think the reactor was getting
dropped before it had a chance to flush out its `CreateFast` cell
properly, because it had already gotten the result back (since the test
code sends it indiscriminately). This was "fixed" in much the same
manner as the other test: making it wait a bit before sending the result
cell back.
There seem to be other tests that use `futures::join!` (like
`begindir`?), and use similarly erroneous patterns; I haven't gotten any
to fail reliably enough to be able to debug them, though.
This field isn't used in modern Tor, and has never been used in
Arti. If tor!489 is merged, then it will no longer contain a useful
value in future consensuses. We shouldn't store it, or else
somebody else will get the smart idea of using it for something.
This commit breaks API compatibility for tor-netdoc with the
`build_docs` feature enabled. I haven't entered that into the
semver_status.md file, since we already have a pending tor-netdoc
API breaker in !129.
Nothing in arti currently uses this document type. Eventually it
will be useful for relays and for bridge clients.
I've left the "SHA1 digest of a router descriptor" type available
unconditinoally, however, since it does get used in a few places.
Part of #125.
We don't currently need a couple of the key manipulation features
that we have, since we aren't yet doing relays or onion service
clients.
Part of #125
Previously, the reactor would use an `UnboundedSender` to send things to
the `RawCellStream`, in order that the reactor wouldn't block if you
failed to read from the latter. This is bad, though, since it means
people can just run us out of memory by sending lots of things.
To fix this, we make the new `StreamReader` type (which does the reading
parts from `RawCellStream`) keep track of the stream's receive window
and issue SENDMEs once *it* has consumed enough data to require it, thus
meaning that we shouldn't get sent enough data to fill the channel
between reactor and `StreamReader` (and, if we do, that's someone trying
to flood us, and we abort the circuit).
As hinted to above, the `RawCellStream` was removed and its reading
functionalities replaced by `StreamReader`; its writing functionalities
are handled by `StreamTarget` anyway, so we just give out one of those
for the write end. This now means we don't need any mutexes!
note: this commit introduces a known issue, arti#230
Rather like e8e9699c3c ("Get rid of
tor-proto's ChannelImpl, and use the reactor more instead"), this
admittedly rather large commit refactors the way circuits in `tor-proto`
work, centralising all of the logic in one large nonblocking reactor
which other things send messages into and out of, instead of having a
bunch of `-Impl` types that are protected by mutexes.
Congestion control becomes a lot simpler with this refactor, since the
reactor can manage both stream- and circuit-level congestion control
unilaterally without having to share this information with consumers,
meaning we can get rid of some locks.
The way streams work also changes, in order to facilitate better
handling of backpressure / fairness between streams: each stream now has
a set of channels to send and receive messages over, instead of sending
relay cells directly onto the channel (now, the reactor pulls messages
off each stream in each map, and tries to avoid doing so if it won't be
able to forward them yet).
Additionally, a lot of "close this circuit / stream" messages aren't
required any more, since that state is simply indicated by one end of a
channel going away. This should make cleanup a lot less brittle.
Getting all of this to work involved writing a fair deal of intricate
nonblocking code in Reactor::run_once that tries very hard to be mindful
of making backpressure work correctly (and congestion control); the old
code could get away with having tasks .await on things, but the new
reactor can't really do this (as it'd lock the reactor up), so has to do
everything in a nonblocking manner.
It requires tracing-subscriber 0.2, which is a lower version than we
want, and which causes trouble with our minimal-versions CI test.
There is a pending issue to fix this; we can reinstate tracing-test
once it is merged: https://github.com/dbrgn/tracing-test/pull/11
This patch makes the rust-nightly CI task fail if it detects any
dbg!(), println!(), or eprintln!() calls in production code.
Because of clippy limitations, it may also gripe about calls to
these macros in our tests. The preferred workarounds are to either
instead. Both are acceptable.
We're doing this check in CI rather than unconditionally with clippy
directives, since we often want to have these calls in our code
temporarily while we're developing. Some day we might want this
test to go into a pre-push hook.
This patch also adds #![allow()] directives for println!() and
eprintln!() in the arti crate. Since that one isn't a library, it's
okay for it to speak to stdout/stderr.
Closes#218.
Previously it was either all-locked or all-not-locked. Now you can
simulate having the same shared storage opened by multiple managers,
only one of which has the lock.
@nickm pointed out that refactoring tor_proto::channel's Reactor to do
sending as well meant that it could only send or receive, but not both,
simultaneously, which was bad!
To fix this, rewrite Reactor::run_once to use a handcrafted future (with
futures::future::poll_fn) that can handle the logic required to push
items onto the sink asynchronously (i.e. checking that it can be written
to before trying to do that, and then flushing it).
This also means we don't use select_biased! any more, and just handroll
that logic ourselves; as a small bonus, we can now process all 3 kinds
of message in one run_once() call, instead of having to do only one of
them.
Instead of awkwardly sharing the internals of a `tor-proto` `Channel`
between the reactor task and any other tasks, move most of the internals
into the reactor and have other tasks communicate with the reactor via
message-passing to allocate circuits and send cells.
This makes a lot of things simple, and has convenient properties like
not needing to wrap the `Channel` in an `Arc` (though some places in the
code still do this for now).
A lot of test code required tweaking in order to deal with the refactor;
in fact, fixing the tests probably took longer than writing the mainline
code (!). Importantly, we now use `tokio`'s `tokio::test` annotation
instead of `async_test`, so that we can run things in the background
(which is required to have reactors running for the circuit tests).
This is an instance of #205, and also kind of #217.
We define "coming back online" as happening when a guard attempt
succeeds, if that attempt that was launched when we seemed to be
offline.
We define "seeming to be offline" as having all of our primary
guards marked unreachable, and having received no incoming network
traffic in a while.
Closes#216.
We need this for the circuit timeout estimator (#57). It needs to
know "how recently have we got some incoming traffic", so that it
can tell whether a circuit has truly timed out, or whether the
entire network is down.
I'm implementing this with coarsetime, since we need to update these
in response to every single incoming cell, and we need the timestamp
operation to be _fast_.
(This reinstates an earlier commit, f30b2280, which I reverted
because we didn't need it at the time.)
Closes#179.
This is based on @eta's patches for !118 and !119: Since we already
have an unbounded channel, we don't need to use an elaborate mess of
one-shot senders. We can just use the unbounded_send() method,
which also lets us enqueue a message without having to await.
Closes#219.
Basically the same thing as 371437d338
("Refactor tor_proto::channel::Reactor to use an UnboundedSender"), but
for tor_proto::circuit's Reactor instead.
(part of arti#217)
There wasn't any good reason for tor-proto's channel reactor to use a
shedload of oneshot channels instead of just an mpsc UnboundedSender,
and the whole `CtrlResult` thing made even less sense.
Straighten this code out by replacing all of that machinery with a
simple UnboundedSender, instead.
(part of arti#218)
Most of the structs in `arti-client` have example code now, to give a
clearer idea of how they're used.
Annoyingly, a lot of the types exposed in `arti-client` are actually
re-exports, which makes documentation a bit harder: example code that
references other parts of `arti-client` can't actually be run as a
doctest, since the crate it's in is a dependency of `arti-client`.
We might be able to fix this in future by doing the documentation in
`arti-client` itself, but rustdoc seems to have some weird behaviours
there that need to be investigated first (for example, it seems to merge
the re-export and original documentation, and also put the re-export
documentation on the `impl` block for some reason).
For now, though, this commit just writes the docs from the point of view
of an `arti-client` consumer, removing notes specific to the crate in
which they're defined. It's not ideal, but at least the end user
experience is decent.
Instead of putting a fully qualified name in the text, in most cases
we should just use the short name of the type or function we're
referring to.
In other words, instead of saying [`crate::module::Foo`], we should
typically say [`Foo`](crate::module::Foo).
This overhauls the top-level `arti-client` documentation significantly:
- the "Using arti-client" section walks the user through all of the
necessary steps to initiate a Torified TCP connection, and then
provides a code example
- this example is also available as `examples/readme.rs`; it's not run
as a doctest, since it involves connecting to Tor
- a "More advanced usage" subheading provides information about stream
isolation (and can potentially be used for other interesting
features once we get them).
- a new "Multiple runtime support" section was added to explain the
purpose and usage of the `tor-rtcompat` crate
- the section on design and privacy considerations was removed; this is
probably okay to keep in a README, but users of the crate aren't going
to be interested in this (at least I don't think)
(also, the doc comment for `arti_client::Error` was fixed to make actual
sense)
This test uses a consensus that I've copied from
tor-netdoc/testdata. I would include it directly, but I think that
will cause trouble when it comes time to run "cargo package".
The previous version of this test used the old, racy version of
wait_for (see #149). The new version is refactored so that
simulated time is only allowed to advance after each step is done,
so that we can actually be sure that each step in the process will
happen as it should.
In order to get the time-advances to proceed properly, and avoid
polluting state between tests, I've had to introduce some machinery
to encode the proper amount for time to advance. It isn't something
I'd want to use for a whole bunch of tests, but for just one set,
it's fine.
These tests now pass reliably for me.
I wonder if a discrete-event-simulation approach (hello, Shadow)
would let us write tests like these to our hearts' content?
This switches out `arti`'s argument-parsing library with `clap`, which
is a lot more featureful (and very widely used within the Rust
ecosystem). We also now use a lot of `clap`'s features to improve the
CLI experience:
- The CLI now expects a subcommand (currently, either "help", or "proxy"
for the existing SOCKS proxy behaviour). This should let us add
additional non-SOCKS-proxy features to arti in future.
- `clap` supports default values determined at runtime, so the way the
default config file is loaded was changed: now, we determine the
OS-specific path for said file before invoking `clap`, so the help
command can show it properly.
- The behaviour of `tor_config` was also changed; now, one simply
specifies a list of configuration files to load, together with
whether they're required.
- That function also way overused generics; this has been fixed.
- Instead of using the ARTI_LOG environment variable to configure
logging, one now uses the `-l, --log-level` CLI option.
(The intent is for this option to be more discoverable by users.)
- The `proxy` subcommand allows the user to override the SOCKS port used
on the CLI without editing the config file.
Nick Mathewson
Neel Chauhan
eta
dagon
Daniel Eades
Trinity Pointard
Dimitris Apostolou
Yuan Lyu