| Commit message (Collapse) | Author | Age | Files | Lines |
| |\
| |
| |
| |
| |
| |
| | |
proto: Silently drop DESTROY/RELAY/CREATED cells on unknown circuits
Closes #2655
See merge request tpo/core/arti!4301
|
| | |
| |
| |
| | |
And say why it's okay to do so.
|
| | |
| |
| |
| |
| | |
These are no longer causing the channel reactor to shut down, so we need
to update this test accordingly.
|
| | | |
|
| | | |
|
| | |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
If we're a relay, we need to tolerate CREATED* with unrecognized
CircIds: for example, if we time out[^1] while trying to extend the circuit
by another hop, we will send a DESTROY to the extending hop, which can
race with the CREATED* response. In other words, a CREATED* cell
arriving on a closed circuit shouldn't be treated as a protocol
violation.
There are, however, a few cases where a CREATED* with an unknown CircId
*is* a protocol violation (and probably *should* cause us to close down
the channel):
* if the CREATED* is moving in the forward direction (towards the
exit), or
* if we have not previously sent a CREATE* with that particular CircId
As before, distinguishing these from the "closed circuit" case above
would involve some tricky logic, and the benefits are unclear, while the
downsides of closing a channel when we shouldn't have are significant.
It seems better to just drop these cells for now.
Closes #2655
[^1]: at the time of writing, we don't have timeouts for the circuit
extension logic, so what I've described here cannot actually happen
today. However, we *do* have a TODO for it, so the time outs I've
described here will be implemented at some point
|
| | | |
|
| | |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
An unrecognized circuit ID is not always a protocol violation, so we
shouldn't close down the channel if it happens.
This change makes the channel reactor drop DESTROY and RELAY cells with
unknown CircIds without closing down the channel. It affects both
clients and relays.
Instead of dropping these unconditionally, we could have implemented
some more sophisticated checks to distinguish the bogus CircIds from the
CircIds of closed circuits, but it's unclear if it's worth the added
complexity (see discussion in #2655).
This partly addresses #2646 (an unrecognized circuit ID shouldn't cause
us to close down the channel if we're a relay).
This commit partially undoes the changes from
4f567e4a9432b340c2799e600c8ceb3724ad3082,
which was originally intended to mitigate flooding attacks.
Part of #2655
|
| |/ |
|
| |
|
|
| |
We now have `add_relay_ent()` for this, so we can remove the TODO.
|
| |
|
|
|
| |
I think this might have been c&p from the half-stream code ("hs" =
"half-stream"). "hc" ("half-circuit") seems like a better name.
|
| | |
|
| | |
|
| |
|
|
| |
Otherwise the new circuit gets closed immediately by the relay.
|
| |
|
|
| |
These help to establish connected channel objects to be used for tests.
|
| |
|
|
|
|
|
|
|
|
|
|
|
| |
Inbound channel (as responder) don't have a ChannelMethod as they are
not initiating the type of transport to use (PT vs Direct). It would
result in a log line when receiving a channel request:
DEBUG tor_proto::channel::handshake: Completed handshake without authentication to [? ] stream_id=Chan 2
This commit uses the `PeerInfo` which is wrapped in a `MaybeSensitive`
and thus safe to log.
Signed-off-by: David Goulet <[email protected]>
|
| | |
|
| |
|
|
|
| |
I find this names confusing. To my mind "is" implies a function
returning `bool`.
|
| |
|
|
|
| |
It wouldn't make much sense for one concrete type to be unwrappable
variously as different inner types.
|
| |
|
|
|
| |
This was always TimeValidityError. And we want to rely on that so we
can do the validity checking more centrally.
|
| |
|
|
|
|
|
|
|
|
|
| |
We need to return the "futures::Stream of Tor streams" from the CREATE
handler, because these need to be handled from `arti-relay`,
as per `doc/dev/notes/relay-streams.md`
This commit is intentionally (slightly) misformatted to make reviewing a
bit easier (the next commit will rustfmt everything).
Part of #2612
|
| | |
|
| | |
|
| |
|
|
|
|
|
|
|
|
|
| |
This enables us to make these configurable: any relays that are not
configured to be an exit will exclude BEGIN and RESOLVE from their list
of allowed commands, causing exit and DNS streams to be rejected as soon
as the BEGIN/RESOLVE cell is received in the circuit reactor.
Context: https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/4145#note_3430345
Part of #2606
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
This implements what we discussed in
`doc/dev/notes/relay-streams.md` (lines 218-234):
> Currently, to allow incoming stream requests on a circuit,
> you first need to call `RelayCirc::allow_stream_requests()`
> to install a `CmdChecker` and `IncomingStreamRequestFilter`.
> This is not ideal, because `allow_stream_requests()` will need to be
> called unconditionally, on each `RelayCirc`,
> right after it's created in the `CreateHandler` impl
> (which in turn, would mean making `handle_create()` async too,
> because `allow_stream_requests()` is async, which wouldn't be great).
>
> So, the first step here is to rework the `RelayCirc` API to make relay circuits
> be constructable with a list of allowed `RelayCmd`s and `IncomingStreamRequestFilter`
> from the get-go ([#2582]), and to get rid of `allow_stream_requests()`,
> which will enable the `CREATE*` handler to remain non-`async`.
>
> In any case, the `CREATE*` handler will still require some changes,
> because it needs to be initialized with an `IncomingStreamRequestFilter`,
I am not sure using an `IncomingStreamRequestFilter` "factory" is
necessarily the right approach here, but the circuit `Reactor`'s
constructor needs to take an `IncomingStreamRequestFilter`, and
`IncomingStreamRequestFilter` is not `Clone` (and FWIW, I think it's
better if we don't make it `Clone`).
One obvious limitation is that the `IncomingStreamRequestFilter` of the
circuit reactor is fixed for the entire lifetime of the circuit.
In practice, I don't think this is going to be a problem,
because the arti-relay `IncomingStreamRequestFilter` is only going
be used for
* preventing single-hop exit streams
* per-circuit rate-limiting.
Both of these checks will require the filter to have access to a recent
`NetDir`, which is straightforward if the filter has an Arc<dyn
NetDirProvider> (as mentioned in doc/dev/notes/relay-streams.md,
`NetDirProvider` has a handy non-async `timely_netdir()` function we can
use). And since these checks are based on consensus params, we don't
really need to ever update an already-built circuit with a new
`IncomingStreamRequestFilter` (because all `IncomingStreamRequestFilter`
will have the ability to obtain a fresh `NetDir` as needed).
Nevertheless, I left a TODO about this, because I expect this type to
change once we figure out all the other pieces needed for #1448.
|
| | |
|
| |
|
|
|
|
|
|
|
|
|
|
| |
This commit executes maint/add_warning with the just added change to
deny string slices except in tests.
I recommend auditing this by checking out the previous commit followed
by running the script yourself and then verifying that the diff is
identical to this commit.
This commit makes cargo clippy fail. We will add exceptions in the next
commit.
|
| | |
|
| |
|
|
| |
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
|
|
|
|
|
|
|
|
| |
Missed at review and no unit tests at the time catched those. The next
commit has a unit tests to make sure these checks are now valid.
Fixes #2501
Fixes #2502
Special thanks to Moumen Alaoui for reporting this early!
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
|
|
| |
`/channel/handler.rs`
Closes #1690.
|
| |
|
|
|
|
|
|
|
|
|
| |
When the circ-padding feature is enabled, we use maybenot, which does
not yet support rand 0.10. In the meantime, enabling this feature pulls
in rand 0.9. This is not ideal, but should be okay as a temporary
situation.
This also replaces the use of ReseedingRng (which was removed in 0.10)
with the reseeding_rng crate. This is somewhat less performant, but it
should be okay.
|
| | |
|
| |
|
|
|
| |
The new relay circuit reactor test expect the `PeerInfo` to be populated
with the identity keys of the peer, and won't work without this change.
|
| |
|
|
|
|
|
|
|
|
|
|
| |
Because of the sorting requirement and the fact that a `KeyMgr` can have
multiple Ntor keys, the caller now explicitly extract the two Ntor keys
it wants from the `KeyMgr` and then creates a `RelayNtorKeys` object
with them.
Future changes (#2495) will move this into a view and warn if there are
more than 2 keys.
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
| |
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
| |
This will be needed for ntor handshakes.
|
| |
|
|
| |
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
|
|
|
| |
The CreateRequestHandler was introduced prior to the relay handshake
unit tests so fix them now that we have a way to build one.
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
|
|
|
|
|
|
|
|
|
| |
Instead of copying the client unit tests into the channel module, just
make both current unit tests run on a client and relay handshake.
This required a bit of trickery with type HandshakeConnectFn but works
out in the end.
It also adds the RelayMsgBuf that wraps a MsgBuf in order to implement
CertifiedConn which is very relay only.
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
|
|
|
|
|
| |
Just for mental sanity to have all const at the same place and easily
readable.
No behavior change.
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
|
|
|
|
|
| |
No behavior change.
Make the const cells public as they will be useful for more upcoming
unit tests especially on the relay side.
Signed-off-by: David Goulet <[email protected]>
|
| | |
|
| | |
|
| |
|
|
| |
This will be needed for ntor handshakes.
|
| |\
| |
| |
| |
| | |
tor-proto: Move CREATE_FAST handling to a helper
See merge request tpo/core/arti!3869
|
| | |
| |
| |
| |
| | |
Clippy has started warning about this since we moved the CREATE_FAST
handling to a helper, so this resolves that.
|
| | |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
From opara's comment:
https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/3844#note_3388789
Keep the low level AuthLogDigest type alias and return it. The callsite
is the one deciding if the returned digest is a Clog or a Slog.
Related to #2441
Signed-off-by: David Goulet <[email protected]>
|
| |/
|
|
|
|
|
|
|
| |
Introduce those types in order to avoid mixing them up as the previous
AuthLogDigest was just a type alias over [u8; 32]
Fixes #2441
Signed-off-by: David Goulet <[email protected]>
|
| | |
|