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.
The parameter to FileWatcher::new is not a polling time fallback; it
is a "debounce time". Events are always delayed by at least this
much.
10s is much too long for this. 1s is more appropriate.
We're going to need to do config file reading in two phases.
Right now this isn't actually necessary, because the set of files
is fixed since we don't support dynamically scanning directories.
But the new API will be needed in a moment.
Code motion and API changes, but no overall functional change.
Review with `git show -b` may be helpful.
The new API also provides for dealing with directories, but right now
that doesn't happen.
The previous approach (inherited from the API of notify) was kind of
odd.
Soon we are going to want to be able to drop the watcher and replace
it. That really wants the same object to contain all the things that
ought to be dropped together. (notify's watchers stop generating
events and give EOF on the channel, when dropped.)
These blocks were in the wrong order.
Previously, if you tried to turn on process hardening in the config
and then reloaded rather than restarting, it wouldn't take effect.
At one point in this MR I thought I was going to want this for
arti::cfg::ListenConfig (which we don't want to be Default).
In fact ListenConfig is being handled specially, but having written
this function it seemed sensible to keep it. Since resolve_option
becomes a wrapper for it, the existing tests exercise it.
Making ChannelPaddingInstructions::default() accurately reflect the
initial state of the reactor's padding timer simplifies the code
somewhat.
(When padding is wanted, parameters are computed and inserted
explicitly, so the only change is that if we start out dormant, we
defer setting the timer parameters until necessary.)
As per
https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/657#note_2827249
Get rid of unneeded constructor.
We never need to use hardcoded reduced padding parameters during
negotiation cell construction. If we are using reduced padding
parameters, the layers which decide this have netparams to use.
Prompted by
https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/657#note_2826092
The network consensus parameters use (0,0) to mean "no padding"
(which is not the same as (0,0) means in a PADDING_NEGOTIATE cell).
Representing "no padding" this way is actually quite convoluted and
un-Rustic. Ensure that we convert (0,0) to None, and do the primary
logic in Option.
This gets rid of many Result(). Many parameters are renamed.
Test cases of the now-impossible branch are removed.
Deleting the match from padding_parameters will come in a moment.
I've split off that commit since it has much whitespace noise.
for now, change the error type to Void.
A "transient" error is one that does not indicate a true failure,
but rather an _expected_ need to retry. When we hit one of these,
we do not count it against the total number of permitted failures.
(We do impose a higher limit on "real failures plus transient
failures", though, to prevent infinite loops in the event of a
programming error.
Closes#517.
This is actually a general facility for inserting locally-generated
cells into the outgoing stream.
It doesn't seem to be possible to do this without adding an additional
condition check to the reactor, since we need to insert it into the
right place in the stream, giving it priority over data, and only
using it up if there was room in the output.
We don't engage this machinery yet, because nothing sets
special_outgoing.
Ian Jackson
Nick Mathewson
eta
Ian Jackson
EliTheCoder
trinity-1686a
Reylaba
Yuan Lyu
Alexander Færøy
Arturo Marquez