| Commit message (Collapse) | Author | Age | Files | Lines |
| |
|
|
|
|
|
|
|
|
|
|
|
| |
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.
|
| | |
|
| |
|
|
| |
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]>
|
| |
|
|
|
|
|
|
|
|
|
|
| |
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]>
|
| |
|
|
| |
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]>
|
| |
|
|
|
|
|
|
|
|
|
|
| |
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]>
|
| |
|
|
|
|
|
|
|
|
|
| |
Instead of giving the `CreateRequestHandler` to the channel after it's
constructed, we integrate it into the handshake so that we can give it
to the channel constructor.
The `ChannelType` is no longer part of the `Channel`.
Some of the tests could be cleaned up slightly now that the channel
doesn't need the `ChannelType`, but I don't want to conflict with !3853.
|
| |
|
|
|
| |
When used for relay channels, the channel reactor will soon need to
spawn relay circuit reactors.
|
| |
|
|
| |
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
|
|
|
|
|
|
| |
Mostly, identity a `ChanTarget` as a "target" since we juggle with
PeerInfo and OwnedChanTarget nowadays.
All certificate and keys have very specific names which attempts to
match the spec as much as possible.
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
|
|
|
|
| |
https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/3791#note_3374476
and
https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/3791#note_3374478
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
|
|
|
|
|
|
| |
Make read_msg() into a helper and use it accross the handshake code.
No behavior change.
https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/3791#note_3374466
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
| |
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
|
|
|
|
|
| |
We can remove the "/* take_slog */ true" pattern and instead have an
explicit type at the callsite for semantic.
https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/3791#note_3374457
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
|
|
| |
https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/3791#note_3374457
Signed-off-by: David Goulet <[email protected]>
|
| | |
|
| |
|
|
| |
Typos found with codespell
|
| |
|
|
|
|
| |
In other words kp_relaysign_ed.
Signed-off-by: David Goulet <[email protected]>
|
| | |
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
It used to work for an initiator to set the link protocol once a
VERSIONS is received because initiator send their VERSIONS before. This
failed with responders because a responder channel sends their VERSIONS
after receiving one from the initiator.
This reverse logic means that the channel cell handler was transitionned
to the Handshake state before a responder was able to send a VERSIONS
cell leading to a failure because VERSIONS cell aren't allowed at the
Handshake state.
To fix this, the send/recv or recv/send is now explicit per channel type
and once this is done and successful, the link protocol is set. A
`set_link_protocol()` is added to the ChannelBaseHandshake trait so it
can be used to set the cell handler.
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
| |
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
| |
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
|
|
|
|
|
|
|
| |
The initiator and responder are quite different. Building an
AUTHENTICATE cell is delicate and so this change differenticates clearly
between the two.
This allows us to remove the peer_cert_digest from an UnverifiedChannel
which is only something that makes sense for an initiator.
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
|
|
|
|
| |
Both client and relay specialized channel now use it as their inner base
channel so they can use the same common verify() function since it is
the same validation for both.
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Remove the last part from check_internal() that is specific to an
initiator channel.
At this commit, all three specialized channel do the verify process
within their own verify() function.
The client and relay initiator both look at the TLS cert (code
duplication unfortunately). And the relay responder looks at the
LINK_AUTH cert extracting the peer KP_link_ed key for validation.
The CERTS cell is removed from UnverifiedChannel as it is now only
useful within the verification process which is now specialized.
A series of TODO(relay) is added to point out the current problem and
how to fix them.
The next step is to create an UnverifiedInitiatorChannel that will hold
the verity_tls_cert() function and peer cert information which is only
relevant to an initiator. This will remove code duplication.
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
|
|
| |
The responder channel will soon use it.
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
In order to pull this off, make
UnverifiedChannel::check_relay_identities() to return a RelayIds that it
builds after checking if they match the peer we were expecting.
This part is moved in this commit so once check_relay_identities()
returns, we are certain of the relay identity validity on both "it
identified properly" and "it is the right expected relay".
This makes it that the check_relay_identities() returns the RelayIds,
the signing key and the RSA id digest (which is needed for
authentication later).
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
|
|
|
|
|
|
| |
This introduces verify_tls_cert() standalone function. It is such
because both client and relay initiator will use it.
For now, the check_internal() has been modified to use it. We are slowly
building towards having specialized check function per channel type.
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
This is a loaded commit, apologize in advance but not many way around
this.
One thing that is generic to all verifiable channel (authenticated) is
that they all need to check the relay identities and signing key from
the CERTS cell.
This commits extracts that part into
UnverifiedChannel::check_relay_identities() which returns those said
identities and the signing key (KP_relaysign_ed).
The signing key is actually needed for only one context, the initiator
part because the TLS cert is signed with it. The LINK AUTH cert is
signed by the ed25519 identity key itself which is what the responder
will look for.
This commit has two side effects which I believe are OK:
1. The timeliness check of the identity certs is now done prior to the
other cert (TLS/LINK).
2. We no longer check signatures in batch mode as we can't batch ed25519
sig check with the RSA crosscert sig. It appears the batch validation
was there for performance and not for security purposes.
The end goal of this piece of work is that the specialized channel will
start by calling a generic check function that will call
check_relay_identities(). And then, the secondary certificates will get
checked depending on the side of the channel.
Expect also a variable rename commit at the end as the naming in this
function is really bad.
Signed-off-by: David Goulet <[email protected]>
|
| |
|
|
|
|
|
|
|
|
|
| |
Move two inline functions located in UnverifiedChannel::check_internal()
into the UnverifiedChannel object itself.
Laying down the ground work for the more specialized objects to use
those as the check_internal() is about to get massively refactored into
more specific channel types.
Signed-off-by: David Goulet <[email protected]>
|
| | |
|
| | |
|
| | |
|
| | |
|
| | |
|