vincenzopalazzo
/
arti
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge branch 'sendme_versions' into 'main' 

Fix a bug in requiring sendme authentication, and document some future work

Closes #53 and #294

See merge request tpo/core/arti!238
This commit is contained in:
eta 2022-01-13 11:09:07 +00:00
parent e0ee3b5049 bbf7c59b2d
commit 3f42ad19ba
2 changed files with 80 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
crates/tor-proto/src/circuit.rs
View File

@ -74,6 +74,8 @@ use tor_cell::relaycell::StreamId;
use crate::crypto::handshake::ntor::NtorPublicKey;
use self::reactor::RequireSendmeAuth;
/// The size of the buffer for communication between `ClientCirc` and its reactor.
pub const CIRCUIT_BUFFER_SIZE: usize = 128;
@ -206,9 +208,7 @@ impl ClientCirc {
linkspecs.retain(|ls| !matches!(ls, LinkSpec::Ed25519Id(_)));
}
// FlowCtrl=1 means that this hop supports authenticated SENDMEs
let supports_flowctrl_1 = target
.protovers()
.supports_known_subver(tor_protover::ProtoKind::FlowCtrl, 1);
let require_sendme_auth = RequireSendmeAuth::from_protocols(target.protovers());
let (tx, rx) = oneshot::channel();
@ -216,7 +216,7 @@ impl ClientCirc {
.unbounded_send(CtrlMsg::ExtendNtor {
public_key: key,
linkspecs,
supports_authenticated_sendme: supports_flowctrl_1,
require_sendme_auth,
params: params.clone(),
done: tx,
})
@ -476,7 +476,7 @@ impl PendingClientCirc {
.unbounded_send(CtrlMsg::Create {
recv_created: self.recvcreated,
handshake: CircuitHandshake::CreateFast,
supports_authenticated_sendme: false,
require_sendme_auth: RequireSendmeAuth::No,
params: params.clone(),
done: tx,
})
@ -500,9 +500,8 @@ impl PendingClientCirc {
Tg: tor_linkspec::CircTarget,
{
let (tx, rx) = oneshot::channel();
let supports_flowctrl_1 = target
.protovers()
.supports_known_subver(tor_protover::ProtoKind::FlowCtrl, 1);
let require_sendme_auth = RequireSendmeAuth::from_protocols(target.protovers());
self.circ
.control
.unbounded_send(CtrlMsg::Create {
@ -514,7 +513,7 @@ impl PendingClientCirc {
},
ed_identity: *target.ed_identity(),
},
supports_authenticated_sendme: supports_flowctrl_1,
require_sendme_auth,
params: params.clone(),
done: tx,
})

94
crates/tor-proto/src/circuit/reactor.rs
View File

@ -78,7 +78,8 @@ pub(super) enum CtrlMsg {
/// The handshake type to use for the first hop.
handshake: CircuitHandshake,
/// Whether the hop supports authenticated SENDME cells.
supports_authenticated_sendme: bool,
/// (And therefore, whether we should require them.)
require_sendme_auth: RequireSendmeAuth,
/// Other parameters relevant for circuit creation.
params: CircParameters,
/// Oneshot channel to notify on completion.
@ -91,7 +92,8 @@ pub(super) enum CtrlMsg {
/// Information about how to connect to the relay we're extending to.
linkspecs: Vec<LinkSpec>,
/// Whether the hop supports authenticated SENDME cells.
supports_authenticated_sendme: bool,
/// (And therefore, whether we should require them.)
require_sendme_auth: RequireSendmeAuth,
/// Other parameters relevant for circuit extension.
params: CircParameters,
/// Oneshot channel to notify on completion.
@ -161,7 +163,7 @@ pub(super) struct CircHop {
recvwindow: sendme::CircRecvWindow,
/// If true, this hop is using an older link protocol and we
/// shouldn't expect good authenticated SENDMEs from it.
auth_sendme_optional: bool,
auth_sendme_required: RequireSendmeAuth,
/// Window used to say how many cells we can send.
sendwindow: sendme::CircSendWindow,
/// Buffer for messages we can't send to this hop yet due to congestion control.
@ -174,6 +176,42 @@ pub(super) struct CircHop {
outbound: VecDeque<([u8; 20], ChanCell)>,
}
/// Enumeration to determine whether we require circuit-level SENDME cells to be
/// authenticated.
///
/// (This is an enumeration rather than a boolean to prevent accidental sense
/// inversion.)
#[derive(Copy, Clone, Debug, Eq, PartialEq)]
pub(super) enum RequireSendmeAuth {
/// Sendme authentication is expected from this hop, and therefore is
/// required.
Yes,
/// Sendme authentication is not expected from this hop, and therefore not
/// required.
No,
}
impl RequireSendmeAuth {
/// Create an appropriate [`RequireSendmeAuth`] for a given set of relay
/// subprotocol versions.
//
// TODO(nickm): At some point in the future, once there are no 0.3.5 relays
// on the Tor network, we can safely require authenticated SENDMEs from all
// relays.
//
// At that point, if we have a relay implementation in Rust, it should look
// at the network parameter `SendmeAcceptMinVersion` when deciding whether
// to require authenticated SENDMEs.
pub(super) fn from_protocols(protocols: &tor_protover::Protocols) -> Self {
if protocols.supports_known_subver(tor_protover::ProtoKind::FlowCtrl, 1) {
// The relay supports FlowCtrl=1, and therefore will authenticate.
RequireSendmeAuth::Yes
} else {
RequireSendmeAuth::No
}
}
}
/// An indicator on what we should do when we receive a cell for a circuit.
#[derive(Copy, Clone, Debug, Eq, PartialEq)]
enum CellStatus {
@ -185,11 +223,11 @@ enum CellStatus {
impl CircHop {
/// Create a new hop.
pub(super) fn new(auth_sendme_optional: bool, initial_window: u16) -> Self {
pub(super) fn new(auth_sendme_required: RequireSendmeAuth, initial_window: u16) -> Self {
CircHop {
map: streammap::StreamMap::new(),
recvwindow: sendme::CircRecvWindow::new(1000),
auth_sendme_optional,
auth_sendme_required,
sendwindow: sendme::CircSendWindow::new(initial_window),
outbound: VecDeque::new(),
}
@ -227,7 +265,8 @@ where
/// Handshake state.
state: Option<H::StateType>,
/// Whether the hop supports authenticated SENDME cells.
supports_flowctrl_1: bool,
/// (And therefore, whether we require them.)
require_sendme_auth: RequireSendmeAuth,
/// Parameters used for this extension.
params: CircParameters,
/// An identifier for logging about this reactor's circuit.
@ -263,7 +302,7 @@ where
handshake_id: u16,
key: &H::KeyType,
linkspecs: Vec<LinkSpec>,
supports_flowctrl_1: bool,
require_sendme_auth: RequireSendmeAuth,
params: CircParameters,
reactor: &mut Reactor,
) -> Result<Self> {
@ -297,7 +336,7 @@ where
Ok(Self {
state: Some(state),
supports_flowctrl_1,
require_sendme_auth,
params,
unique_id,
expected_hop: hop,
@ -355,7 +394,7 @@ where
// If we get here, it succeeded. Add a new hop to the circuit.
let (layer_fwd, layer_back) = layer.split();
reactor.add_hop(
self.supports_flowctrl_1,
self.require_sendme_auth,
Box::new(layer_fwd),
Box::new(layer_back),
&self.params,
@ -580,7 +619,7 @@ impl Reactor {
if let Some(CtrlMsg::Create {
recv_created,
handshake,
supports_authenticated_sendme,
require_sendme_auth,
params,
done,
}) = create_message
@ -597,7 +636,7 @@ impl Reactor {
recv_created,
ed_identity,
public_key,
supports_authenticated_sendme,
require_sendme_auth,
&params,
)
.await
@ -626,7 +665,7 @@ impl Reactor {
recvcreated: oneshot::Receiver<CreateResponse>,
wrap: &W,
key: &H::KeyType,
supports_flowctrl_1: bool,
require_sendme_auth: RequireSendmeAuth,
params: &CircParameters,
) -> Result<()>
where
@ -668,7 +707,7 @@ impl Reactor {
let (layer_fwd, layer_back) = layer.split();
self.add_hop(
supports_flowctrl_1,
require_sendme_auth,
Box::new(layer_fwd),
Box::new(layer_back),
params,
@ -693,7 +732,7 @@ impl Reactor {
recvcreated,
&wrap,
&(),
false,
RequireSendmeAuth::No,
params,
)
.await
@ -708,7 +747,7 @@ impl Reactor {
recvcreated: oneshot::Receiver<CreateResponse>,
ed_identity: pk::ed25519::Ed25519Identity,
pubkey: NtorPublicKey,
supports_flowctrl_1: bool,
require_sendme_auth: RequireSendmeAuth,
params: &CircParameters,
) -> Result<()> {
// Exit now if we have an Ed25519 or RSA identity mismatch.
@ -735,7 +774,7 @@ impl Reactor {
recvcreated,
&wrap,
&pubkey,
supports_flowctrl_1,
require_sendme_auth,
params,
)
.await
@ -744,13 +783,13 @@ impl Reactor {
/// Add a hop to the end of this circuit.
fn add_hop(
&mut self,
supports_flowctrl_1: bool,
require_sendme_auth: RequireSendmeAuth,
fwd: Box<dyn OutboundClientLayer + 'static + Send>,
rev: Box<dyn InboundClientLayer + 'static + Send>,
params: &CircParameters,
) {
let hop = crate::circuit::reactor::CircHop::new(
supports_flowctrl_1,
require_sendme_auth,
params.initial_send_window(),
);
self.hops.push(hop);
@ -834,7 +873,7 @@ impl Reactor {
}
}
None => {
if !hop.auth_sendme_optional {
if hop.auth_sendme_required == RequireSendmeAuth::Yes {
return Err(Error::CircProto("missing tag on circuit sendme".into()));
} else {
None
@ -984,7 +1023,7 @@ impl Reactor {
CtrlMsg::ExtendNtor {
public_key,
linkspecs,
supports_authenticated_sendme,
require_sendme_auth,
params,
done,
} => {
@ -993,7 +1032,7 @@ impl Reactor {
0x02,
&public_key,
linkspecs,
supports_authenticated_sendme,
require_sendme_auth,
params,
self,
) {
@ -1030,9 +1069,16 @@ impl Reactor {
} => {
use crate::circuit::test::DummyCrypto;
// This kinds of conversion is okay for testing, but just for testing.
let require_sendme_auth = if supports_flowctrl_1 {
RequireSendmeAuth::Yes
} else {
RequireSendmeAuth::No
};
let fwd = Box::new(DummyCrypto::new(fwd_lasthop));
let rev = Box::new(DummyCrypto::new(rev_lasthop));
self.add_hop(supports_flowctrl_1, fwd, rev, &params);
self.add_hop(require_sendme_auth, fwd, rev, &params);
let _ = done.send(Ok(()));
}
#[cfg(test)]
@ -1156,6 +1202,10 @@ impl Reactor {
};
// If we do need to send a circuit-level SENDME cell, do so.
if send_circ_sendme {
// This always sends a V1 (tagged) sendme cell, and thereby assumes
// that SendmeEmitMinVersion is no more than 1. If the authorities
// every increase that parameter to a higher number, this will
// become incorrect. (Higher numbers are not currently defined.)
let sendme = Sendme::new_tag(tag);
let cell = RelayCell::new(0.into(), sendme.into());
self.send_relay_cell(cx, hopnum, false, cell)?;