This introduces the new API functions to Store.
But currently they are all no-ops.
So all this machinery doesn't actually do anything.
Also, it changes the API to the mockable downloader, to allow it to
support if-modified-since. So this isn't used either. But it is more
convenient to do this all at once in BridgeDescManager, since care
needs to be taken about the intraction between if-modified-since and
the persistent cache.
We use the one in the dirmgr. That means that our constructor now has
to take a dirmgr. And, the dirmgr must have a circmgr.
This is all rather odd, TBH.
Add .. to the binding of the return values from setup, as
future-proofing.
The tests now need to provide a Store too. Make the sqlite::new_empty
function pub(crate) so we can use it.
We must retain the _db_tmp_dir, since when it goes away the tmp
directory is deleted and the db goes readonly.
* Centralise applying the schema updates, in a closure
* Make the schema update SQL texts be in an array so we can loop
* Make the version update statement generic, not cloned-and-hacked
This will make it possible to add another schema version without
error-prone conditions etc.
Also replace Arg::takes_value with Arg::action to configure flags
that don't take parameters vs options that do.
This makes it easy to upgrade to clap 4.
Tested deprecations with:
cargo check --workspace --all-features --features clap/deprecated
This removes the last cargo audit override (for the unmaintained
ansi_term).
Don't mark options as required when they have default values:
see <https://github.com/clap-rs/clap/pull/3793>.
The RetryDelays are being triggered for the 2nd time here, so their
timeouts can be longer. We must bump the sleep to make sure we don't
have a flaky test.
Prior to the previous commit, set_bridges would malfunction if there
were bridges which where (i) in current (ii) in queued or running
(iii) in the new bridge set.
This test failed then and passes now.
This was fundamentally confused and irregular. Now it is more
regular: it does the same things to all the elements of Tracked,
and a simple filtering on current.
This fixes a bug, for which I'm about to add a test case.
0.99.[012] have a bug https://github.com/JelteF/derive_more/issues/114
which makes the Deref derive for bridgedesc::StateGuard not work
and therefore breaks minimal-versions CI.
It seems simpler to require the newer version everywhere.
Previously we always set `dir_info_missing` to `false` for new
guards, since new guards could only be taken from ones that were
present in the NetDir. But for bridges, we don't download their
info until _after_ we have chosen them as guards.
In this and the upcoming commits I'll be changing how guards related
to `NetDir` and to `Relay`. Previously, a guard could only come
from (or be updated from) a `Relay` in a `NetDir`. Soon it will be
able to be built from a bridge as well.
To do this, I'm defining a `Universe` trait (name negotiable) that
represents a set of things that may be guards. I'm going to
continue extending its functionality until there are no more
methods in guard.rs or sample.rs that take `NetDir`.
This commit removes most of the usage of `NetDir` and `Relay` in
`guard.rs`.
The new `ipc` module inside `tor-ptmgr` implements the Pluggable
Transport Specification version 1 (`pt-spec.txt`,
https://gitlab.torproject.org/tpo/core/torspec/-/blob/main/pt-spec.txt).
This enables module users to spawn pluggable transport binaries inside a
child process asynchronously, and receive structured information about
how to connect to the transports provided by said binaries.
Internally, this is structured as a pure set of serialisers and
deserialisers for the protocol in the specification (in the form of
environment variables, and the PT's stdout), a wrapper to run the PT
binary and enable accessing its output asynchronously, and a user-facing
wrapper that handles ensuring all the requested transports launched
properly.
The included `run-pt` example is an exceedingly minimal wrapper program
that was useful in testing. More tests can and should be added in a
further MR.
closes arti#394; part of arti#69
This is necessary for the (somewhat undesirable) lookup_ids function
to return an ID that the dirmgr can actually use to report successes
and failures.
As noted, lookup_ids will create problems down the road when we
implement relays. We should refactor it out before then.
This required a number of changes, which I've tried to document.
I've taken a conservative approach to modification, and I'm not
using any of the by_*_mut() functions (yet). For cases which
potentially modify the whole set, I'm using into_values() and
collect() to ensure that it's re-indexed correctly, even though the
identities don't change.
I introduce some "TODO pt-client" comments here which I will resolve
in the next commit(s).
The explicit list of variant names, that needs to be kept in sync, and
is a test failure semver break hazard, is now gone.
All the necessary code is now generated automatically, and cannot be
wrong.
I want this because I find myself wanting to add a second
implementation of FlagEvent, for another type.
1. Import it, as we do elsewhere, and use that import.
2. Use thiserror to generate the Display impl, rather than
derive_more, as we do for errors elsewhere.
(tor_error still needs derive_more::Display for ErrorKind.)
Previously we could only use Relay for this case, which won't work
any more: a Bridge is not a `tor_netdir::Relay`. Instead we allow
the GuardMgr to give us something that knows how to convert itself
into an OwnedCircTarget.
This change required a far amount of follow-on revisions and
refactoring, but it should all be internal to the path-building
logic.
Now it contains either an `OwnedChanTarget` or an `OwnedCircTarget`,
which will let `GuardMgr` return bridges that can be used to make
circuits.
As part of this change, it was necessary to revise some
address-modification functions that applied to filters and
`OwnedChanTarget`. Now they do the smart thing, and remove only the
address that are in the `ChanMethod`. This means that the addresses
from HasAddrs are still accurate about which addresses the relay
"has".
This commit improves the overflow protection of one call to
Vec::write_u16(), by replacing the cast conversion from self.sig.len()
with a call to u16::try_from(), like it is already done in the rest of
the accompanying function.
This commit adds a `debug_assert!` macro into the `new_unchecked()`
function of the Data cell. Beside this, it also fixes a misleading
comment regarding that limit.
We could in the future provide a version of abs_retry_time which took
an &dyn closure if that turns out to be wanted.
I think this isn't a semver break because trait implementors are
allowed to relax bounds.
The most important part of this commit is to make sure that each
`FirstHopId` includes the `GuardSetSelector` from which the guard
was selected. Doing this lets us be certain that when we report
that a guard has succeeded or failed, we're reporting it in the
right context.
Additionally, this commit uses strum to make an iterator over the
samples, so that we can make sure that our "for each sample" code is
robust against future changes, and we don't miss the bridge sample.
This commit changes the way how a vector with a known sized gets
allocated, by using Vec::with_capacity() instead of Vec::new().
It will eventually avoid an allocation of more memory than required.
This commit renames the for_client and for_relay functions to
from_client and from_relay respectively, in order to indicate their
origin, as the term "for" is more likely to indicate a destination,
which is not true in that situation.
This commit renames the fixed_len! macro to fixed_len_handshake!, in
order to indicate, that this macro is only suited for cells with
commands related to handshaking.
The `PtTarget` type and its contents (`TransportName`,
`PtTargetAddr`, `PtSettings`) are now unconditionally compiled and
exposed. This will allow us to serialize and deserialize them in
our guard-state files even when we have been built without explicit
PT support.
The `pt-client` feature controls whether `TransportName` is a
variant of `TransportId`, and whether `PtTarget` is a variant of
`ChanMethod`: this in turn means that we'll still have simpler
binary code and smaller structures when we're building without PT
support (which is what we wanted when we initially made these types
conditional).
This lets us write functions which can either take an existing
owned OwnedChanTarget, or copy out of some other kind of ChanTarget
passed by reference.
Now keyed by Arc<BridgeConfig>, and the values can be errors.
Currently there is no implementation so there can't be any errors,
but the error enum will become nonempty.
This is a compromise between C tor (never escapes = signs) and the
spec (calls for escaping all = signs). In C tor there is no way to
actually construct a key with an = sign in it, so this difference
should be undetectable.
This code is _not_ conditional, since we want to support running
with a proxy even if we don't support pluggable transports.
We may eventually want to refactor this into a new create.
Now, instead of duplicate checks in various cases, we simply go
through the loop one last time.
This allows us to simplify some of our other logic around here.
This is necessary so that we can look up channels (open and pending)
by all of the Ids that we know about them.
The operations needed here are pretty complex: to get them right,
I've replaced most of the accessors on the inner `ChannelMap` with a
function that holds the lock while another `FnOnce` is called. This
still gets us the invariant that we can't accidentally await while
holding the lock on the `ChannelMap`.
I've removed the tests for the accessors that are no longer there.
There are some subtleties here. Now that we have more than one kind
of Id, it's possible to have a partial match. I've tried to explain
all these cases in the comments.
}
Even though channels are practically changeable, they use locks
internally so that you don't need a `&mut Channel` to send or
receive traffic. It makes sense for reparameterizing the channel to
also use a &self reference.
I'll need this so that I can store channels in an `ByRelayIds<>`
set, and still invoke their reparameterize methods.
This is mostly a testing only change for now too, but soon I'll use
it to deal with the fact that we need to know the IDs to actually
build a channel at all.
The `ByRelayIds` type doesn't have a type equivalent to
`hash_map::Entry`, since it's a set type rather than a map
type. Therefore, the only plausible way to do entry mutation will
be to remove the old entry and insert a new one. And so, we no
longer need a "poisoned" state.
Also, add a few tests for this and the other accessors.
We'll need this accessor to find whether we have any channels to
_any_ of the identities that we're trying to connect to.
We need to handle String, not just str, since some deserializers
have to handle escapes and generate new strings.
Found while writing tests; fixes#605.
In addition to the usual "You named that method wrong!" errors, we
have a new rustdoc error that complains about bogus "HTML tags" that
are actually unquoted usage of types like `Result<Foo>`.
This lets us build channels using different TransportHelpers,
including the (new) default TransportHelper, which just uses the old
connect_to_one() code.
The feature we want is `#[doc = include_str!("README.md")]`, which is
stable since 1.54 and our MSRV is now 1.56.
This commit is precisely the result of the following Perl rune:
perl -i~ -0777 -pe 's{(^//!(?!.*\@\@).*\n)+}{#![doc = include_str!("../README.md")]\n}m' crates/*/src/lib.rs
The traits that launch connections need to be async; the traits that
don't, shouldn't be async.
Additionally, we need a few more "Sync" annotations here for the
futures to work.
This is an internal type (distinct from factory::ChannelFactory)
that we use to make the code in `tor_chanmgr::mgr` agnostic about
what a channel actually is, and how it is actually launched.
Therefore, I'm renaming it and giving better documentation in a
couple of places, to prevent confusion.
Add ,ignore to ignore three examples that don't actually compile.
cargo readme would add these annotations to lib.rs, but the doc
include doesn't do stuff like that. pandoc seems to still render the
result just fine.
This demonstrates that:
* !bridge-client: uncommenting nondefault bridge config generates
urecognized config key warnings (but the config is still accepted)(
* bridge-client, !pt-client: uncommenting nondefault bridges generates
error due to attempting to use a PT. If that's filtered out,
everything is fine.
* pt-client: Everything is good (as before).
* Introduce filter_examples and resolve_examples helpers,
which will become more complex in a moment.
* Move the API test into a { } block to minimise subsequent diff.
It's going to become conditional.
* In subsequent comparisons, use the parsed version, since
the API built one might not exist.
No overall functional change.
We're going to want something that has the standard list builder
methods at the Rust API, but which has different serialisation.
Sadly the implementation is annoying, because macro_rules makes it
hard to parse a nice input syntax.
This is precisely the text from the original version of !744.
There is no implementation yet, so we must add a entry to the
exception list in the tests.
Section headings appear uncommented in the file, so if we have a
whole section which is completely unrecognized (ie, an entry with no
`.`, it will be spotted when we parse the not-uncommented file too.
Right now there aren't any but there will be in a moment.
This test depends on `Duration` having a granularity of 1
nanosecond, which is not the case on OSX, and is probably not the
case on other places too.
We can re-enable this once we have a set of test vectors that use
more realistic RTTs, and a set of testing code that tolerates some
divergence.
Temporary solution for #574.
I think this is quite an inconvenient way to be carrying on.
Maybe we should disable all dead code warnings unless all features are
also enabled, and just let the compiler get rid of unused stuff later.
`${PROGRAM_DIR}` expands to the equivalent of
`std::env::current_exe().parent()`, with appropriate unwrapping and
conversions.
It is expected to be useful for finding the locations of pluggable
transports in some kinds of bundles.
Closes#586.
HasAddr used to mean "Here are addresses that I have, at which I can
be contacted." But "Where (and how) can I be contacted?" is now a
question for HasChannelMethod to answer.
(We still need to have "HasAddr", though, so we can answer things
like "what country is this relay in" and "are these relays in the
same /8?")
So this commit introduces:
* A new trait for adding an implementation of HasChannelMethod in
terms of HasAddr.
* A requirement on ChanTarget that it needs to implement
HasChannelMethod.
There is some temporary breakage here, marked with "TODO pt-client",
that I'll fix later in this branch.
Also add a BridgeRelayWithDesc type (name tbd) to guarantee that
a bridge relay really does have a known descriptor before you
try to build a circuit with it.
This is the one we'll actually use to connect to bridges. It
has a `Bridge` line, and an optional `BridgeDesc`.
Maybe this will turn into a `BridgeRelay<'a>` by analogy to `Relay`
some time; I'm not sure.
BridgeDesc is a separate type to make sure that we do not confuse
bridges' descriptors with the descriptors from other routers down
the road. (Bridges' descriptors need to be used differently, and
treated as more private.)
With this code, BridgerDescList is now just an alias for
`ByRelayIds<BridgeDesc>`, which is pretty keen.
To implement a reasonable RsaIdentity accessor, we have to
store the RsaIdentity in the RouterDesc, or else we'd have to
recalculate it using SHA1 and DER every time.
The Ed25519 identity is hidden inside the identity cert, but it's
safe to get a reference to it.
Nightly rust gives a warning about this "pub use", but the warning
is a false positive. Since it doesn't seem to be going away in a
hurry, let's suppress it for now.
Include the offending word in all the applicable errors.
Always print it with {word:?}.
As a consequence, there are no From impls any more and error
generation/conversion is by hand in all cases.
Clarify InvalidPtOrAddr vs InvalidIAddrorPt, and don't make the
attempted parse be a source error for those.
Where we still have source errors, don't print them in Display.
Using Option<T> as an alias for T was too clever indeed, and it
meant that our HashMaps were declared with the wrong types.
Putting flags here instead gives us an extension point that we can
use in the future.
Every element in the set has up to N keys, each of which may have differnt
types. No value for any key may correspond to more than one element in
the set.
These properties can be provided, via a macro, for values of N between 1
and $BIG_ENOUGH.
We'll use this to implement a type that holds HasRelayIds.
These tests include a few reference cases, as well as a little
framework to make sure that the client and the proxy implementation
will handshake with one another successfully.
(Since the APIs for the `Schedule::sleep*` functions changed, this
is a breaking change in tor-rtcompat. Therefore, the Runtime trait
in tor-rtcompat is now a different trait. Therefore, anything that
uses the Runtime trait in its APIs has also broken.)
Make PtTargetSettings be Default.
No longer wrap it in Arc. We want to be able to update it here during
construction. If we want to save memory with copies of the same
bridge line, we should do this for the whole Bridge I think.
The warning `clippy::bool_to_int_with_if` is meant to shout at you
when you say `if x { 1 } else { 0 }` and instead suggest that you
say `inttype::from(x)`.
I agreed with this for the case in tor-cert, where we are literally
converting a boolean into a flag.
I don't agree with this in tor-netdoc, where we are using a boolean
to decide how many fields to skip in a given document format. So
for this case, I decided to clean up the code a little by renaming
"skip" to "n_skip", and changing the boolean to use an enum instead.
Retain "SocksHandshake" as a deprecated synonym.
Also, make an (on-by-default) feature for SocksProxyHandshake.
(There is about to be a SocksClientHandshake as well.)
Previously we always assumed that the console was ephemeral, and so
we disabled safe logging. But the console can be piped to journald.
And even if we enforce isatty there's no guarantee that the user
isn't using some kind of terminal that logs to disk or something.
Best just to enable SafeLogging unconditionally. I've added a note
about where and how we might re-enable this.
Closes#553.
This covers only the most basic notions of working with bridges:
that we need a separate set of guards, and that they have to
come from the list of known bridges.
Without this, actually building circuits manually is a pain.
This API is behind the `experimental-api` feature, and so it does
not require a semver.md entry.
Try to clarify more that the ChannelUsage is for describing the
usage for one particular channel request, not for the channel as a
whole. This is a potentially confusing point, so we should spell it
out completely.
As a matter of good crypto practice, we shouldn't use
short-circuiting checks to compare keys or key-like objects, since
the amount of time taken by those checks can leak information about
their inputs.
I don't think it's actually _necessary_ to use a constant-time
operation in this case, but let's establish the precedent.
This is a follow-up to !724.
There are some places in the protocol where we have an all-zero RSA
identity that does not truly represent a key, but rather represents
an absent or unknown key. For these, it's better to use
`RsaIdentity::is_zero` instead of manually checking for a set of
zero bytes: it expresses the intent better, and ensures that the
operation is constant-time.
I am deliberately not introducing a more general IsZero trait here,
or implementing is_zero for anything else: This is the only one we
seem to need right now. We can generalize it later if we have to.
This fixes an busy-loop.
When the last `TaskHandle` on a `TaskSchedule` is dropped, the
schedule is permanently canceled: whatever operation it was
scheduling should no longer be performed. But our code was broken:
the `sleep()` and `sleep_until_wallclock()` functions don't verify
whether the handles are dropped or not.
This breakage caused an CPU-eating busy-loop in
`sleep_until_wallclock`.
With this patch, we now return a `Result<(), SleepError>` from these
functions.
Fixes#572.
Because we want to work more on ensuring that our semver stability
story is solid, we are _not_ bumping arti-client to 1.0.0 right now.
Here are the bumps we _are_ doing. Crates with "minor" bumps have
had API breaks; crates with "patch" bumps have had new APIs added.
Note that `tor-congestion` is not bumped here: it's a new crate, and
hasn't been published before.
```
tor-basic-utils minor
fs-mistrust minor
tor-config minor
tor-rtcompat minor
tor-rtmock minor
tor-llcrypto patch
tor-bytes patch
tor-linkspec minor
tor-cell minor
tor-proto minor
tor-netdoc patch
tor-netdir minor
tor-persist patch
tor-chanmgr minor
tor-guardmgr minor
tor-circmgr minor
tor-dirmgr minor
arti-client minor
arti-hyper minor
arti major
arti-bench minor
arti-testing minor
```
Since our last round of releases, these crates have had either
trivial changes, or changes that did not affect their APIs.
Therefore we are bumping their versions, but not changing which
versions of them other crates depend on.
This patch fixes a minor build error where we would call
`compact_home()` on Windows instead of `anonymize_home()` on our PathBuf
instance.
Additionally we change how the `arti_conf` path is constructed such that
we join the individual path components to ensure that no "/" ends up
being present on Windows where path's are separated by "\".
See: tpo/core/arti#555.
See: tpo/core/arti!700.
This patch shortens the duration of the `does_not_predict_old_ports`
test in the preemptive module. AppVeyor spawns its VMs/containers per
build, so the `Instant::now()` call returns a value smaller than `60 *
60 + 1` which causes the subtraction to overflow and thus panic.
Thanks to @trinity-1686a for the help here.
See: tpo/core/arti#563.
This patch comments out a method call to `trust_group()` as this method
is not available on all platforms that Arti builds on right now and thus
fails to compile there.
I have added a comment that the given call is not available on non-Unix
like platforms.
See: tpo/core/arti#557.
This patch disables `readable_ok()`, `multiple_errors()`, and
`check_contents()` as they all rely on permission issues on groups being
detected properly which is not the case on Windows right now.
See: tpo/core/arti#557.
This patch changes our `default_config()` test in `arti/src/cfg.rs` such
that we can define a number of known unrecognized options on different
platforms.
We mark the two keys "storage.permissions.trust_group" and
"storage.permissions.trust_user" as unknown on the Windows platform as
such features is not available using the ordinary Unix UID concept.
This patch also publicly exposes the `tor_config::load::DisfavouredKey`
and `tor_config::load::PathEntry` types and marks them as
non-exhaustive.
See: tpo/core/arti#450.
This patch refactors how we construct the `Mistrust` type in the tests
found in the fs-mistrust crate such that it is possible to construct an
instance of the `Mistrust` type using a set of operations available via
the `MistrustBuilder`'s methods.
We handle some of the portability issues found while testing this code
on Windows in the convenience function `mistrust_build()` instead of
having duplicated code in multiple test cases.
See: tpo/core/arti#557.
This patch adds a comment to the `link_rel()` function in fs-mistrust to
explain why we ignore symlink creation on the Windows platform.
See: tpo/core/arti#557.
This patch disables the simple_cases() test on non-Unix platforms and
hides the LinkType type import on non-Unix where we won't be testing
symbolic link features.
See: tpo/core/arti#557.
We have a test that tries to check that our outputs are the same as
those from `std::fs::canonicalize`. But on Windows, they aren't:
There, `canonicalize` also puts path prefixes into a "Verbatim"
form.
This patch tries to replicate that behavior for the test only. If
we find that it's unreliable, though, our best bet is probably to
revise or disable this check on Windows, rather than chasing
compatibility with `GetFinalPathNameByHandle`.
Should fix part of #557.
On Windows, paths can have a "prefix", like `C:` or
`\\server\share`. Attempts to get metadata for these prefixes
appear to fail with `ERROR_INVALID_FUNCTION`, since they are not
files.
This patch teaches fs-mistrust about prefixes on Windows, and tells
it that attempts to find their metadata are allowed to fail.
Doing this may solve part of #557.
Whereas previously we would say:
```
target/debug/arti: error: invalid escape character in string: `Z` at line 9 column 14 in ../../.config/arti/arti.toml
```
we now say:
```
target/debug/arti: error: invalid escape character in string: `Z` at line 9 column 14 in ../../.config/arti/arti.toml (If you wanted to include a literal \ character, you need to escape it by writing two in a row: \\)
```
The implementation is a bit of a hack, I'm afraid, but I don't think
it's all that bad.
Closes#549.
We need
60b874308e6792a73cc00517a60bbef60a12e3cc
Mixed type arrays (#358)
for a test case in tor-config.
While we're here, drop the dupe entry in tor-config.
(In principle we could make this increase only in tor-config's
dev-dependencies, but that seems unnecessarily fiddly.)
This commit largely follows the example for resolve_alternative_specs.
The difference is that there are two fields, so we use a macro to
avoid recapitulating the field names.
This is not interesting to the user, and violates some of our
safe-logging rules (like "Don't log at info for each user request"
and "don't log ports").
As per
https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/682#note_2830860
And subsequent IRC discussion.
Having done the work as per review comments, I don't much like the
result. It's quite un-ergonomiuc. If we can't have fs autodetection,
I think syntactic autodetection within sources.rs would be nearly as
nice.
However, I seem to be outvoted. At least the externally visible
functionality (of an arti binary, say) is reasonably ergonomic.