- 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.
This allows us to use this with an item builder type which doesn't
impl Default. (Obviously this only makes sense for items which aren't
actually builders.)
It is Quite Vexing that we have to use [ ] rather than the < > around
the generics, particularly given that we are also using [ ] to signal
"this is arrayish".
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
Although these do not appear in the config, it does have a builder.
It seems sensible to get rid of this ad-hoc list manipulation site,
and replace it with our standard list builder API.
define_list_builder_helper requires that the builder element type be
Deserialize. Currently GuardUsageRestriction is a transparent, public
enum, so we aren't really exposing anything.
We could introduce GuardUsageRestrictionBuilder now, but
since it's not in the config and thereofore only in the public API of
the lower crates, we can definitely put that off.
This type was returned by the public DownloadSchedule::builder
function. But the only thing that seems to have noticed that the type
name itself wasn't exported, was rustdoc. Hmmm.
The new API is (roughly) as discussed in
https://gitlab.torproject.org/tpo/core/arti/-/issues/451
This is quite a large commit and it is not convenient to split it up.
It contains the following changes:
* Redo the list builder and accessor macros implemnetation,
including docs and tests.
* Change uses of define_list_config_builder. In each case:
- Move the docs about the default value to the containing field.
- Remove the other docs (which were just recapitulations, and
are now not needed since the ListBuilder is no longer public).
- Rewmove or replace `pub` in the define_list_builder_helper call,
so that the builder is no longer public.
- Change the main macro call site to use define_list_builder_helper.
- Add a call to define_list_builder_accessors.
* Make the module `list_builder` pub so that we have somewhere to
put the overview documentation.
* Consequential changes:
- Change `outer.inner().replace(X)` to `outer.set_inner(X)`
- Consequential changes to imports (`use` statements).
Previously this field was differently named to its serde and to its
accessors. We are about to introduce a macro_rules macro which will
provide list accessors and we don't want that macro to have a field
renaming feature.
So stop renaming the field.
Document that this can contain either a string for expansion, or a
literal PathBuf not for expansion.
Rename the `from_path` method to `new_literal`: a very important
difference is whether it gets expanded - less important than the Rust
type. Also, now it takes `Into<PathBuf>`, which avoids a needless
clone.
(We don't change the API in `arti-client` because
`&tempfile::Tempdir()` doesn't implement `Into<PathBuf>`, so
`arti-client` has to have some new `as_ref` calls.)
Provide accessors `as_unexpanded_str` and `as_literal_path`. The
deserialisation already makes this part of the stable API,l so not
pvoding accessors seems just obstructive. They are useful for tests,
too.
Add tests for the new entrypoints, and for deserialisation of both
variants from TOML (via config, or directly) and JSON.
We introduce LiteralPath struct, so that a literal path deserialises
from
some_path = { literal: "actual path string" }
This makes the deserialisation unambiguous.
arti uses this. Somehow this seems to be enabled by some other thing
in the crate graph, but I found that adding a similar dependency to
another crate resulted in a `config` which doesn't compile.
This helps make it possible to use `SecureDir` (name pending) even
when we want to disable permissions checks. Otherwise, optional
permission checking would require users of this crate to maintain
separate code paths for the "check" and "don't check" cases.
This required a bit of poking through the `users` crate, to mess
with the user and group dbs. The original goal was to "trust the
group with the same name as us", but it turned into a bit of a
production, since:
* We want to take our own name from $USER, assuming that matches
our uid. (Otherwise we want to ask getpwuid_r().)
* We only want to trust the group if we are actually a member of
that group.
* We want to cache this information.
* We want to test this code.
Previously we would temporarily put self.resolved into an invalid
state by adding a path component that might be a symlink. With this
change, we create a new temporary path object (using Cow to avoid
unnecessary allocations) and only conditionally replace
self.resolved.
The only way to get a SecureDir is by having checked a directory.
Once you have one, it encourages you to open and create files and
directories with the right permissions, and checks them for you.
This crate is meant to solve #315 by giving a way to make sure that
a file or directory is only accessible by trusted users. I've tried
to explain carefully (in comments and documentation) what this crate
is doing and why, under the assumption that it will someday be read
by another person like me who does _not_ live and breathe unix file
permissions. The crate is still missing some key features, noted in
the TODO section.
It differs from the first version of the crate by taking a more
principled approach to directory checking: it emulates the path
lookup process (reading symlinks and all) one path change at a time,
thus ensuring that we check every directory which could enable
an untrusted user to get to our target file, _or_ which could
enable them to get to any symlink that would get them to the target
file.
The API is also slightly different: It separates the `Mistrust`
object (where you configure what you do or do not trust) from the
`Verifier` (where you set up a check that you want to perform on a
single object). Verifiers are set up to be a bit ephemeral,
so that it is hard to accidentally declare that _every_ object
is meant to be readable when you only mean that _some_ objects
may be readable.
The `[patch]` approach causes the tree not to build when used as a
dependency, unless the `[patch]` is replicated into the depending
project.
Instead, replace our `derive_builer =` dependencies with a reference
to a specific git commit:
perl -i~ -pe 'next unless m/^derive_builder/; s#"(0\.11\.2)"#{ version = "$1", git = "https://github.com/ijackson/rust-derive-builder", rev = "ba0c1a5311bd9f93ddf5f5b8ec2a5f6f03b22fbe" }#' crates/*/Cargo.toml
Note that the commitid has changed. This is because derive_builder is
in fact a workspace of 4 crates. 3 of them are of interest to arti
itself (the 4th exists only for testing). So the same "add git
revision" treatment had to be done to the `derive_builder` and
`derive_builder_macro` crates. Each dependency edge involves a new
commit in the derive_builder workspace, since we can't create a git
commit containing its own commitid. (We want to use commits, rather
than a branch, so that what we are depending on is actually properly
defined, and not subject to the whims of my personal github
namespace.)
There are no actual code changes in derive_builder.
The arti crate itself is only used in benchmark and testing crate.
I think this sentence does not belong here.
Also extend retry-error description (from Architecture.md).
Now that we require Rust 1.56, we can upgrade to AES 0.8. This
forces us to have some slight API changes.
We require cipher 0.4.1, not cipher 0.4.0, since 0.4.0 has
compatibility issues with Rust 1.56.
eta
Nick Mathewson
Ian Jackson
Ian Jackson
trinity-1686a
Samanta Navarro