In reviewing !553 I noticed that the empty digest list error had to be
handled in two places. I filed #492 about the duplication.
In fact it turns out to have been quadruplication.
The new code also avoids cloning the underlying objects, instead
sorting a Vec of references.
This will make it much more convenient for code that only wants one of
these traits (or a subset of them). This is a good thing to support
because it will allow us to use a ZST in places that do not need an
actual async runtime handle (typically, the runtime handle is needed
only for spawn).
(This is slightly different from recovering from errors in the
middle of a list of mds, since in this case we _can't_ advance to the
next md.)
Also, note that a given branch is probably not reachable.
After changes to the prop339, the domain name in an Address can only be
255 bytes max and can NOT contain nul byte(s).
Unit tests had to be modified to accept this change:
- Centralise msg_ip_address
- Add currently-passing tests for address length
- Test counted address length longer than type wants
Related to #463
Signed-off-by: David Goulet <dgoulet@torproject.org>
Some of our existing code optionally takes a netdir from the
caller. When it doesn't give us one, use the netdir from the
installed NetDirProvider.
(Possibly someday we should remove the NetDir arguments
entirely. I'm deferring that because there are only two APIs
affected, and because making this change would force us to rewrite a
pretty large mess of unit tests.)
Previously it was the job of a task in CircMgr to do this; but we're
going to want to give GuardMgr full access to the latest NetDir for
this, and for other code-simplification reasons.
With this change I'm deprecating a couple of functions in
tor-circmgr. It's no longer necessary for us to have an artificial
external way for you to feed new NetDirs to a circmgr. (I could
just remove them, but I want practice deprecating.)
Our own code is the only stuff that consumes NetDirProvider, and all
the code that consumes it wants it to be Send and Sync.
Making this change avoids our having to define a new function to
upcast Arc<dyn Foo> to Arc<dyn NetDirProvider + Send + Sync>.
Unlike "cancel" and "fire", "suspend" and "resume" don't change any
pending timers or events: they just prevent execution of those
events for a while, and let them resume later on.
This uses some apparently-standard trickery to implement a function
that lets us upcast from Arc<dyn Subtrait> to Arc<dyn Supertrait>.
I considered as alternatives `as_dyn_trait` and `cast_dyn_object`.
Both were nice, but generated a far larger interface than this.
The randomized tests in this crate take a lot of iterations to
converge, so they default to using a deterministic PRNG seed with
few iterations and higher tolerance, and they only randomize the
tests (with more iterations and tighter tolerances) when you
explicitly opt in to randomization.
(If you specify a seed explicitly, you're doing that to reproduce a
randomized case, so we use the same behavior.)
This only affects uses of thread_rng(), and affects them all more or
less indiscriminately. One test does not work with
ARTI_TEST_PRNG=deterministic; the next commit will fix it.
The new `testing_rng()` function is meant as a replacement for
thread_rng() for use in unit tests. By default, it uses a randomly
seeded RNG, but prints the seed before the test so that you can
reproduce any failures that occur. You can override this via the
environment to use a previous seed, or by using a deterministic
seed for all your tests.
Backend for #486.
To me, "Incorrect permissions on file or directory /path: g=w o=w"
implies that the current permissions on /path are 022.
Change the message to "Incorrect permissions: /path is
u=rwx,g=rwx,o=rwx; need g-w,o-w", which is closer to chmod syntax and is
more useful in non-interactive environments such as CI and support.
I was looking to see if we could delete this altogether but I
discovered that on lib.rs the description does not say it's a removed
crate, in the summaries.
Fix this.
Now the validated configuration will never be `Some(0)`, even if that
is what was written in the config file. The arti CLI parser can still
produce this, so we don't touch the code that actually uses this.
(Without the canonicalisation the default builder produces `None` for
the `dns_port`, but the example would produce `Some(0)`, which is
semantically identical but fails the test.)
See https://gitlab.torproject.org/tpo/core/arti/-/issues/488 for some
background.
Canonicalise the `logging.journald` setting in the validated
configuration. Now it will never be `Some("")`, even if that is what
was written in the config file.
This allows us to write `journald = ""` in the example configuration.
(Without the canonicalisation the default builder produces `None` and
the example would produce `Some("")`, which are semantically identical
but fail the test.)
See https://gitlab.torproject.org/tpo/core/arti/-/issues/488 for some
background.
This fixes a unit test failure in weird environments (like some
containers) where the current effective GID is not included in the
list of current groups.
Closes#487.
Bug reported by @sjm217.
Cargo publish (and probably nobody else!) builds this crate with no
features enabled. When you do that, you get a warning about an
unused `use std::io`.
Fixing that.
The compiler can apparently tell that we aren't generating compile
errors! Also that we're not yet using a feature provided here.
(New lint in currently Nightly.)
This commit was made by reverting the previous commit, then
re-running the script I used to generate it. In theory there should
be no semantic changes: only changes due to improved formatting from
cargo edit.
I followed the following procedure to make these changes:
* I used maint/changed_crates to find out which crates had changed
since 0.3.0.
* I used grep and maint/list_crates to sort those crates in
topological (dependency) order.
* I looked through semver_status to find which crates were listed as
having semver-relevant changes (new APIs and breaking changes).
* I scanned through the git logs of the crates with no
semver-relevant changes listed to confirm that, indeed, they had
no changes. For those crates, I incremented their patch-level
version _without_ changing the version that other crates depend on.
* I scanned through the git logs of the crates with no
semver-relevant changes listed to confirm that, indeed, they had
no obvious breaking changes.
* I treated all crates that depend on `arti` and/or `arti-client` as
having breaking changes.
* I identified crates that depend on crates that have changed, even
if they have not changed themselves, and identified them as having
a non-breaking change.
* For all of the crates, I used `cargo set-version -p $CRATE --bump
$STATUS` (where `STATUS` is `patch` or `minor`) to update the
versions, and the depended-upon versions.
These crates had only clippy fixes that do not affect their
behavior:
tor-bytes
tor-cell
tor-events
tor-linkspec
tor-netdir
tor-socksproto
This crate only had the cargo-husky dependency removed, which
does not affect compatibility:
tor-llcrypto
Since these changes have no compatibility effects, it is not
necessary to bump the versions of these crates which other crates
depend on.
It turns out that the `toml` crate can't handle OsString, since
`toml` doesn't support serialize_newtype_variant, and the `serde`
crate tries to serialize OsString using that method.
In this commit we document that limitation, and test that we can at
least round-trip through json.
Found by inspecting test coverage.
Now that the relevant functions now report changed/not-changed
status via a boolean out-parameter (see !527), there's no reason to
have a separate NoChanged error case.
Closes#484.
This gets rid of `#[serde(flatten)]` which prevents serde_ignored (and
other kinds of introspection) from working properly.
The price is now that the toplevel has to deal with two configuration
objects.
The Resolvable trait is overkill right now, but is going to do More
Things in a moment. In particular, we need the impl on tuples, so
that the whole config can be processed in one go.
We are going to need this for some generic code which is going to
appear shortly. Having it produced by impl_standard_builder seems
best. But that does mean being able to disable it, so extra stuff in
the macro. Nothing uses this trait yet.
ConfigResolveError is not used now either, but will be in a moment.
We are going to reorganise ArtiConfig to not contain a
TorClientConfig. This test case's calls to bld.tor() will all need to
change. Do this in advance to make that future commit more readable.
I don't understand why this isn't tripping all the time. Maybe
because this is in a macro. Anyway, I am going to add a new
invocation of this macro from within a test where, empirically, it
trips.
This change requires a little refactoring of TorClientBuilder: now,
instead of enabling or disabling mistrust, it enables or disables
the decision to _override_ the mistrust in the config.
We support all of the following (in TOML notation):
```
user = "rose" # by name
user = 413 # by ID
user = false # no user
user = ":current" # A 'special' user.
user = { name: "rose" }
user = { id: 413 }
user = { special: ":none" }
user = { special: ":current" }
```
Previously in !511 I had introduced a bug where, if there was an
error more serious than "no change", that error would keep us from
noticing that we had no change, and we'd loop until the safety
counter ran out. Then we'd panic.
This commit fixes the bug by reintroducing the `changed` boolean --
this time as an outparam for the add_from_* methods.
Fixes#482.
this change adds unit tests for the public methods of StreamPrefs. although
these are mostly "setter" style functions, the tests confirm the basic
expectations and operation.
When I added these tests, they didn't find any bugs in my own
implementation, but I did find a bug in futures::future::unfold.
See the in-code comment.
This is a general-purpose implementation of the ad-hoc approach
currently taken in (eg) crates/tor-proto/src/channel/reactor.rs,
with an API intended to defned against the more obvious mistakes.
This allows us to separate the two concerns: the channel reactor can
focus on handling channel cells and control messages and is over 2.5x
shorter.
The complexity of the manual sink implementation, and the machinery
needed to avoid having to suspend while holding an item, are dealt
with separately. That separate implemenation now has proper
documentation. (Tests are in the nest commit to avoid this one being
even more unwieldy.)
We use `extend` to define this as an extension trait. A competitor is
`ext` but in my personal projects I have found `extend` slightly
better.
We no longer have separate return paths for recoverable and fatal
errors; instead, they are merged, and distinguished based on
recovery actions.
Since it is now possible for download() to give an error that should
_not_ destroy the previous state, it takes `&mut Box<dyn DirState>`.
This change unfortunately means that we can no longer call `state =
state.advance()`, but instead have to do some mem::swap junk with
poisoned values. Any better solution would be a good thing.
Additionally, the reset() and advance() methods can no longer fail.
There is still a separate return path for reset-triggering errors;
I'm about to fix that.
Previously, we did this in `advance()`, but that wasn't so great: it
meant that we could fail in the advance() code, whereas the calls to
`advance()` treated errors as fatal.
This treats failed verification as a blocking error that requires a
reset.
Fixes one aspect of #439.
Previously DocSource would tell you whether the document was from
a local store or a cache server, but it wouldn't tell you _which_
server it came from.
This change required adding DocSource as an argument to
DirState::add_from_download.
Generally, change the paths that mention the crate name to go via a
module-level "use".
This involves adding tor-config as a direct dependency for a few
crates.
This crate no longer has any reason to exist. All its remaining
functionality is generic enough to go into tor-config.
In this commit, we move the contents of lib.rs into a new file in
tor-config. It contains:
* Code motion
* The minimal "mod" and "use" changes
* The minimal doc comment
* A new a compat alias for ConfigurationSources.
The compat alias is there because various crates currently speak of
arti_config::ConfigurationSources and it is most convenient to fix
them up after the type is available in tor_config.
This is a benchmarking tool, and fs-mistrust doesn't like the
permissions in our CI. The env var ARTI_FS_DISABLE_PERMISSION_CHECKS
is (of course) specific to arti. Maybe it should be honoured here,
or this should be done via the config files.
But disabling this is fine for now.
Test the Deserialize impl of every config struct.
This detects bugs like the one fixed in !502.
The macro now becomes more complex because it needs to take options.
Right now this tt-munching option parser is overkill, but this
leave space for further options in the future.
This was anomalous, in that it contains &'static str, rather than a
proper nested error (eg a config::ConfigError, maybe).
But in fact it tursn out it is now not constructed. The last
construction site was removed a long time ago in
Use derive_builder for Authority and FallbackDir.
I have Plans for this macro. In particular:
* I have a wip branch which tests that the Builder can be
deserialised from an empty config (ie, that config reading
of a config with a blank section for this item works).
* I think we should autogenerate $Config::builder(),
and promote that, rather than $ConfigBuilder::default().
This macro could do that.
This macro is kind of derive-y. Also it has a test in it, and failing
to call it could allow bugs to exist, as well as missing bits of API.
Putting it next to the structs makes it easy to see that it's actually
been called.
We expect that a user may copy this file and uses it as a starting
point for their own configuration.
When they do that, we don't want them to freeze the default config in
time. Instead, we can expect them to uncomment settings they wish to
change. Then when they upgrade arti, *other* settings will get the
new defaults, which I think is right.
This is redundant, because the defaults have to be supplied by the
config builders (usually via builder default attributes).
That this is actually done and correct is tested by the
`default_config()` test case in arti/src/cfg.rs.
Discovered by a test case in my local tree. The test case was
macro-generated by an extension of impl_standard_builder (which
macro istself currently awaiting review, arti!499)
Have also sent an MR to update the upstream docs
https://github.com/jean-airoldie/humantime-serde/pull/8
The type of ret.map_err(codec_err_to_chan)? is (). ISTM that
writing `let () = ` makes it clear that there is nothing there,
but the lint forbids this.
This lint is warn by default and trips here for me on current nightly.
It seems wrong to me. We should be able to make it clear to the
reader that there is nothing here - note how this differs from the
lines below where Ready contains msg. A let () binding is a good way
to do that.
I think the lint allow ought to be added everywhere, but that doesn't
seem easy right now - see this issue about maint/add_warning:
https://gitlab.torproject.org/tpo/core/arti/-/issues/469
With this API we can now stop consensus download attempts early if
any consensus that the directory cache gave us would be necessarily
too far in the future or in the past.
This saves wasted bandwidth for clients with skewed clocks.
Closes#466.
If we're happy with a directory from 3 days ago, we should say
"if-modified-since 3 days ago".
This patch is larger than I'd like, since I had to add &DirMgrConfig
as an argument to the functions that make a consensus request.
Closes#467.
Since we want to be willing to use older consensuses, we don't
necessarily want to reset a download just because the consensus is
expired.
This new behavior isn't ideal either; I've added a TODO that relates
to #433.
Related of #412
This new section describes how much variance we accept when it comes
to expired and not-yet-valid directory documents. (Currently, the
only ones where this matters for are consensus documents and
authority certificates.) A document that is invalid by no more than
these tolerances is not _live_, but it can still be used.
These tolerances serve two purposes:
* First, they allow clients to run with a little more clock skew
than they would tolerate otherwise.
* Second, they allow clients to survive the situation where the
authorities are unable to reach a consensus for a day or two.
Compare with Tor's REASONABLY_LIVE_TIME and NETWORKSTATUS_ALLOW_SKEW
constants; also compare with proposal 212.
Closes#412.
These generic arguments weren't consistent. It doesn't make sense ot
insist on getting a borrowed type and then cloning it. So tidy things
up in the direction of taking owned values, which is what
ConfigurationSources actually needs.
(My personal preference would be &dyn to avoid monomorphisation code
bloat but that was controversial last time I proposed it somewhere.)
- arti#445 highlighted the lack of good documentation around Arti's
multiple runtime support, as well as it being difficult to determine
what runtime was actually in use.
- Improve the documentation to solve the first problem.
- To solve the second problem, make Runtime require Debug (which is
arguably a good idea anyway, since it makes them easier to embed in
things), and print out the current runtime's Debug information when
arti is invoked with `--version`.
- (It also prints out other Cargo features, too!)
fixes arti#445
A build script reaching into your .git/hooks/ and modifying them
nonconsensually was a bit of a horrifying concept, and also made it hard
to build arti with the feature disabled. Remove this crate, and replace
it with manual instructions on how to install the hooks in
CONTRIBUTING.md.
- Some FIXMEs got removed or amended.
- AddMicrodescs now yields a mutable reference, so we can use .drain()
and reuse the allocation.
- Some panics were downgraded to debug_asserts.
- We don't want to inadvertently replace our netdir with one that's
actually older, so detect and error on this condition.
- Also, print a debug line when we get a new netdir without enough
guards.
- (An unrelated TODO was also added.)
- Taking a previous netdir directly and keeping it around before we need
it is a bit of a waste of memory, and also doesn't mesh well with how
SharedMutArc works.
- To remedy this, introduce a new trait `PreviousNetDir` and have the
state machines take that instead. (I was a bit tempted to just pass in
the SharedMutArc directly. Maybe I should've done that.)
- GetMicrodescsState now uses the NetDirChange API to propagate netdir
changes, instead of modifying the netdir directly.
- PendingNetDir was refactored in order to support this use case.
- As a result, the netdir-related methods in WriteNetDir can be removed,
leaving only the DirFilter for now.
- add_from_cache() no longer takes a store, because nothing uses it.
- (bodge: apply_netdir_changes() was put in a few places missed
previously)
- The new DirState::get_netdir_change() API lets the state machine
export a NetDirChange: a request to either replace the current netdir,
or add microdescs to it.
- bootstrap.rs now consumes this new API, even though nothing implements
it yet.
- This will let us implement GetMicrodescsState without having to
directly mutate the netdir. The calling code also handles checking the
netdir against the circmgr for sufficiency, and updating the consensus
metadata in the store, meaning the revised GetMicrodescsState will not
have to perform these tasks.
- The additional parameters passed to GetConsensusState are now passed
through all the states, and used as well.
- WriteNetDir doesn't have a now() or config() method any more, since
the states now get this from the runtime or the config parameters.
- This required modifying the tests to make a mocked runtime and custom
config directly, instead of using DirRcv for this purpose.
- Additionally, because we don't have to upgrade a weak reference for
DirState::dl_config(), that function no longer wraps its return value
in Result.
- (A bunch of the FIXMEs from the previous commit that introduced the
additional parameters have now been rectified as a result.)
Previously, CompoundRuntime would use the default implementations of
SleepProvider::now() and ::wallclock(), instead of using its wrapped
SleepProvider. This mildly embarrassing omission has been rectified.
- GetConsensusState::new now takes a set of parameters matching what it
actually needs, instead of just taking a writedir. (It still *does*
take a writedir, and indeed still uses it for basically everything,
but that will eventually go away.)
- Its call sites were updated.
- Some tests now need to take a runtime, and got indented a lot as a
result.
- Resetting was made non-functional, because we need to thread through
the parameters passed to GetConsensusState to all of the other
states, too. This will happen in a later commit.
- Given that this is effectively an implementation detail, it doesn't
really make sense to have it be in the crate root...
- (also, we're going to change it a bunch now)
- fetch_single now takes what it needs, instead of an Arc<DirMgr<R>>.
- This required refactoring the CANNED_RESPONSE mechanism, given the
test would otherwise fail due to not having a CircMgr to pass to
fetch_single.
- query_into_requests is now called make_requests_for_documents, and
does the &[DocId] -> DocQuery conversion internally instead.
- DirMgr::make_consensus_request and DirMgr::query_into_requests are now
gone. The tests use the new functions, as does fetch_multiple.
- There's no good reason these functions needed to be part of the
dirmgr, apart from needing a runtime and a store.
- However, we can just add those as arguments and copy them over. This
commit does that.
- Function renamed & docs tidied up a bit
- Function signature now takes what it needs (immutable &dyn Store
instead of mutex, slice instead of Vec) and nothing more
- DocQuery::load_documents_into was also renamed
DocQuery::load_from_store_into and given similar treatment
Annoyingly, Rust doesn't automatically generate this sort of `impl` for
you, and I'd like to reduce the usage of Mutex<DynStore> everywhere else
in favour of either &dyn Store or &mut dyn Store.
(This is for two reasons: firstly, we might have a Store implementation
that doesn't use a mutex as above, or similar refactors; secondly,
passing the raw trait object reference lets us encode mutability into
the function signature, which I believe is quite valuable.)
Relay nicknames are always between 1 and 19 characters long, and
they're always ASCII: That means that storing them in a [u8;19] will
always be possible, and always use less resources than storing them
in a String.
Fortunately, the tinystr crate already helps us with this kind of
thing.
If the target directory itself is unreadable by untrusted users,
then its contents can't be read[*] by them regardless of their
permissions. If the target directory _is_ readable, then _it_ will
be rejected if we are forbidding readable objects. (And if we
aren't we don't care if the contents are readable.)
A similar argument would apply to writable objects within an
unreadable target directory. We're not making that argument, since
such contents are likelier to be a mistake.
[*] Unless they're hard-linked; see comments in "Limitations"
section.
I'm doing this per discussion, so that we can have it be part of the
TorConfig later on, and not break stuff as we change the Mistrust
API to have a builder.
This change, unfortunately, results in a little more internal
complexity and duplicated code in arti and arti-client. I've marked
those points with TODOs.
This is derived from the environment, not the configuration file: We
might not want to trust the configuration file until we've decided
whether we like its permissions.
This includes:
* a CachedDir::join method.
* functions to read and write from provided filenames in a
CachedDir.
* a method to tell whether a fs-mistrust error is about bad file
permissions, or failure to inspect file permissions or some other
kind of IO problem.
For reference, the git source for this crate (and the others in its
workspace) currently lives in my personal github account (ijackson).
If this fork turns out to be long-lived and gains features and/or
users, it would be good to move it to a gitlab somewhere.
I have granted Nick crate ownership on the crates.io system.
Here we add a config option to disable safe logging, and ensure that
safe logging is disabled when we are formatting an error message on
exit (since we assume it's safe to write sensitive info to stderr.)
This is a rough first-cut of an API that I think might help us with
keeping limited categories of sensitive information out of our logs.
I'll refine it based on experiences with using it.
* Builders additionally derive: Debug, Serialize, Deserialize.
* Validated structs no longer derive: Serialize, Deserialize
and all related attributes deleted.
* As a consequence, all the `#[serde(deny_unknown_fields)]`
are gone. That means that right now unknown fields are totally
ignored. This is good for compatibility but poor for useability.
Doing something better here is arti#417, in progress.
* As a consequence, delete tor_dirmgr::retry::default_parallelism.
(The default value was already duplicated into a builder attr.)
And drop the ad-hoc orport() method. This brings FallbackDir's
orports field in line with our list builder API.
The general semver note in "configuation" seems to cover most of this.
Ian Jackson
Nick Mathewson
Ian Jackson
trinity-1686a
David Goulet
Dimitris Apostolou
Alex Xu (Hello71)
Neel Chauhan
trinity-1686a
michael mccune
Orhun Parmaksız
eta
Jim Newsome