| Commit message (Collapse) | Author | Age | Files | Lines |
| ... | |
| | |
|
| |
|
|
|
|
|
|
|
|
|
|
|
| |
This avoids us replacing our cached hsdesc with one that has a lower
revision counter.
This was not a problem before, because we'd only ever fetch a new
descriptor when our cahced one expired, but now that we refetch the
descriptor on introduction NACK, we need to make sure the new descriptor
is actually more recent than the one we have.
The implementation is a bit convoluted because I had to avoid retaining
a reference to the known-timely cached `desc` so as not to anger borrowck.
|
| |
|
|
| |
This will soon need to be called from two places, unfortunately.
|
| | |
|
| |
|
|
|
| |
As suggested in
https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/3925?commit_id=22ae30205a5f18fee43f424f8c0f9768b95a2ae6#note_3402779
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Prior to this change, arti clients would retain their cached HS
descriptors until expiry. This is causing us some problems in
[arti#2458], where arti frequently fails to connect to C Tor services
in a chutney test net. One of the problems in #2458 is that all
introduction points are NACK-ing the client's introduction requests,
ultimately causing the connection attempt to fail:
```
2026-04-23T16:25:23Z DEBUG tor_hsclient::state: HS connection failure for ijalr3vpf67zradtlaxze6pex5ypadu5qfsltn42cmhioqory6363tad.onion error=error: Unable to connect to hidden service using any Rendezvous Point / Introduction Point: Tried to make circuit to hidden service 6 times, but all attempts failed
Attempt 1: Introduction point #3 reported error in its INTRODUCE_ACK: NOT_RECOGNIZED
Attempt 2: Introduction point #2 reported error in its INTRODUCE_ACK: NOT_RECOGNIZED
Attempt 3: Introduction point #1 reported error in its INTRODUCE_ACK: NOT_RECOGNIZED
Attempt 4: Introduction point #3 reported error in its INTRODUCE_ACK: NOT_RECOGNIZED
Attempt 5: Introduction point #2 reported error in its INTRODUCE_ACK: NOT_RECOGNIZED
Attempt 6: Introduction point #1 reported error in its INTRODUCE_ACK: NOT_RECOGNIZED
```
I think this is happening because the C Tor service has rotated intro
points and republished its descriptor, before the descriptor's planned
expiry. I believe is something that can (and does) happen, and so arti
should be able to handle it gracefully.
I added some more logs to arti and reran the test, and noticed the
descriptor's lifetime works out to be just over 2 days (54h), which
seems excessive (especially in our test net, where the voting interval
is 20s and the hsdir interval is 8min). In any case, holding on to a
service's descriptor for too long, and not refetching a new one, will
cause the client's introduction requests to be rejected (because the
service might switch intro points).
I think there are at least 2 things we need to do to improve arti's
handling of HS connections:
* In the case of an introduce NACK (with status = `NOT_RECOGNIZED`),
the client should refetch the descriptor and then retry the
introduction. This behavior is not codified in the spec yet (see
torspec#245), but I am told this is what C Tor does
* Rethink the cached descriptor expiry calculation
This commit addresses the first point. The second one seems trickier, so
I have not looked into it yet.
Part of #966
[arti#913]: https://gitlab.torproject.org/tpo/core/arti/-/work_items/913#note_2914448
[arti#2458]: https://gitlab.torproject.org/tpo/core/arti/-/work_items/2458#note_3400768
|
| | |
|
| | |
|
| |
|
|
|
|
|
|
| |
This will soon be used to force a refetch in the case of an
`INTRODUCE_NACK`.
(This commit is intentionally left misindented to make reviewing a bit
easier. A future commit will rustfmt the file).
|
| |
|
|
|
|
|
| |
Gabi correctly points out that since the client uses guarded
circuits for introduction points, whereas the service uses naive
circuits, we shouldn't expect the peer's circuits to be any longer
than ours.
|
| |
|
|
|
|
|
|
|
| |
Both C tor and Arti will retry building a circuit if the first
attempt fails. This means that it can be worthwhile waiting longer
than we might otherwise for the HS to build its rendezvous circuit.
We don't need to make this change for _our_ circuits, since
the CircMgr code takes care of those timeouts for us.
|
| | |
|
| |
|
|
|
|
|
|
| |
We don't actually need to use this method on any circuits that have
a virtual hop, so instead of "fixing" this method to ignore virtual
hops, the simpler approach is to change its name and its documented
behavior, and to explain how the documented behavior is appropriate
for our needs.
|
| | |
|
| | |
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
| |
This time we _do_ need to use the BuildCircuit estimator, since we
have to consider the peer's circuit building.
The peer may be using full vanguards, so we need to use 5 as their
maximum hop estimate.
Additionally, their circuit may be longer than ours, so we ought to
possibly wait a bit longer for them to get our INTRODUCE2.
There are XXXXs here about OneWay timeout estimators, for immediate
followup.
|
| |
|
|
|
|
| |
The circmgr handles timeouts on its own, so we can let it do that.
Use the actual circuit length for calculating round-trip timeouts.
|
| |
|
|
|
|
|
|
| |
The circmgr code handles circuit timeouts, so we don't need to
include that redundantly.
Also, we look at the circuit to find out its number of hops,
so that we estimate the timeout more accurately.
|
| |
|
|
|
|
|
|
|
| |
The hspool operations already include their own timeouts, so we
don't need to recalculate them.
For the directory related operations, we now calculate the timeouts
based on actual circuit lengths, and use those timeouts on the
operations themselves.
|
| |
|
|
| |
We'll use these for timeout estimations.
|
| | |
|
| | |
|
| | |
|
| | |
|
| | |
|
| | |
|
| | |
|
| |
|
|
|
| |
These are all aimed at figuring out in more detail what's going on
in #2079 and related issues.
|
| |
|
|
| |
Run maint/add_warning
|
| |
|
|
| |
Part of #1885.
|
| |
|
|
|
|
|
| |
This is just code motion (I suggest reviewing with `--color-moved`).
This also moves the implementation-agnostic parts from
`tor_proto::client::circuit` to a new `tor_proto::circuit` module.
|
| |
|
|
|
|
|
|
|
|
|
|
| |
The `stream` module is client-specific, for the most part, so I am
moving it under `client`. Later on, we will factor out the parts that
can be shared with the relay implementation.
Note: this is a breaking change as the deleted `stream` module was
`pub`. We could've kept the module and reexported from it the public
types from `tor_proto::client::stream`, but I think it's better to have
this `client` namespacing, because it makes the separation between the
client and relay parts clearer.
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
| |
First, run
```
git grep -l "^edition =" |
xargs perl -i -pe 's/^edition *=.*/edition = "2024"/;'
```
Second, manually verify that all Cargo.toml files have changed,
and nothing else has changed.
Third, run cargo fmt again.
|
| |
|
|
|
|
|
|
|
|
| |
The BaseTunnel now has a start_conversation() which takes a TargetHop
meaning it can be used with a multi path tunnel.
The Conversation object has been moved into the tunnel namespace out of
the circuit one.
Signed-off-by: David Goulet <[email protected]>
|
| | |
|
| |
|
|
| |
Closes #2012.
|
| |
|
|
|
| |
These errors are suspicious as hsdir inflation attacks, in the
context of prop360.
|
| | |
|
| |
|
|
|
|
|
|
|
|
| |
This is in the spirit of making everything going inbound the tor-proto
crate to use a TargetHop.
This becomes much easier for the HS subsystem as it only uses the last
hop for its conversation and setup.
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
|
|
| |
We will construct this object based on the circuit parameters _and_
on the target's supported protocol versions, so we need to do so
when we have both pieces of info.
|
| |
|
|
|
|
|
|
|
|
|
|
|
| |
Apparently clippy nightly is better (or worse?) about detecting
complex functions than before, so I'm suppressing these warnings
where they occur.
I have mixed feelings about these warnings: On the plus side,
they really do help to detect functions that are twistier than they
need to be. On the minus side, they get confused by tracing macros,
and the "allows" do pile up. But on the plus side, those "allows"
do provide a way to find functions that need to be refactored,
and they are never uglier than the functions they decorate.
|
| |
|
|
| |
- The Rng::gen() functions have been renamed to Rng::random().
|
| |
|
|
| |
- `rand::thread_rng()` has been deprecated and renamed to `rand::rng()`
|
| |
|
|
|
|
|
| |
MockSleepProvider and MockSleepRuntime have been declared deprecated
by the docs for some time. We're about to mark them `#[deprecated]`.
This commit has been split out for clarity of review.
|
| |
|
|
|
|
|
| |
This commit adds the RSA ID of a relay into a debug statement, as found
in other places in the code. It mostly serves the purpose that the
Ed25519 ID in itself is rather inconvenient, as metrics.torproject.org
only allows querying from the RSA ID.
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
It is unused but most importantly it allows any RELAY cell to be sent
from anywhere in the code which is really not desirable because it is
skipping congestion control.
It also allows us to remove the `control_tx` from the reactor which is
one less channel to track/understand/think about.
This opens up the door to all sorts of problems especially side channel
that can be exploited if we are not careful.
We can always bring this back if we need it but for now, it is unused
and allows us to remove the `CtrlMsg::SendRelayCell` control message.
No code behavior change.
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
This rewrites the circuit reactor main loop to use `select_biased!` to poll
multiple futures simultaneously.
The new `run_once()`, like the old, first waits for an initial
`CtrlMsg::Create`. Then, it uses a `select_biased!` to poll the
`chan_sender` sink and shutdown channel for readiness.
When the channel sink is ready, we poll the `control` and `input`
channels like before, as well as the new `ready_streams` `Stream`
(`ready_streams` is a `futures::Stream` that replaces the previous
`send_outbound()` function).
Most of the implementation remains unchanged, except the `handle_input`,
`handle_cell` and `handle_control` functions no longer send anything on
the `chan_sender` channel. Instead, they may do some (synchronous)
processing, and send instructions for the remaining work that needs to
be done (for example, for writing the cell to the `chan_sender`
channel). These instructions are handled at the end of `run_once()`,
and are encoded in the `RunOnceCmdInner` enum.
What this change does **not** do:
* the control channel *still* bypasses congestion control. We could
fix this by making the various reactor functions send the
`RunOnceCmdInner` commands to `run_once()` via a channel
(instead of returning them). This would enable the reactor to stop
reading the commands (except for handle `Sendme`, which would be
handled separately) if it's blocked on congestion control.
|
| |
|
|
|
|
| |
Instead, return an error and make all call site handle it.
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Congestion control parameters have specific values depending on the
circuit type. Instead of using a CircuitType, which is removed in this
commit, specialize the function in this case onion and exit.
This allows us to get rid of CircuitType and solely use TargetCircUsage
instead.
At this commit, we use .expect() on the Builder. Future commit will
remove this to return a Result in case of failure. Worth noting that we
don't expect one.
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
| |
The congestion control parameters are created from the consensus
parameters (netparams) and then put into the CircParameters object that
is then passed down the tor-proto crate.
Because different parameters are selected depending on the circuit type
(onion vs exit vs sbws), a CircuitType enum is introduced for the sole
purpose of being used to select the right parameters.
Related #534
Signed-off-by: David Goulet <[email protected]>
|