From 9160b55c57e9b2d68b82059139a2be0dd7954b8a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 7 Apr 2022 10:01:19 -0400 Subject: [PATCH] chanmgr: bubble ClockSkew up through the Error object. Fortunately, we don't need a separate type here: authenticated clock skew can only come attached to a `tor_proto::Error`. We also remove skew from `tor_proto::Error::HandshakeCertsExpired`, since it would now be redundant. --- crates/tor-chanmgr/src/builder.rs | 18 +++++++-- crates/tor-chanmgr/src/err.rs | 46 ++++++++++++++++++++--- crates/tor-chanmgr/src/lib.rs | 3 +- crates/tor-proto/src/channel/handshake.rs | 2 +- crates/tor-proto/src/util/err.rs | 2 - doc/semver_status.md | 5 +++ 6 files changed, 64 insertions(+), 12 deletions(-) diff --git a/crates/tor-chanmgr/src/builder.rs b/crates/tor-chanmgr/src/builder.rs index be82fe626..344d791b3 100644 --- a/crates/tor-chanmgr/src/builder.rs +++ b/crates/tor-chanmgr/src/builder.rs @@ -184,10 +184,22 @@ impl ChanBuilder { let chan = builder .launch(tls) .connect(|| self.runtime.wallclock()) - .await?; + .await + .map_err(Error::from_proto_no_skew)?; + let clock_skew = Some(chan.clock_skew()); // Not yet authenticated; can't use it till `check` is done. let now = self.runtime.wallclock(); - let chan = chan.check(target, &peer_cert, Some(now))?; - let (chan, reactor) = chan.finish().await?; + let chan = chan + .check(target, &peer_cert, Some(now)) + .map_err(|source| match &source { + tor_proto::Error::HandshakeCertsExpired { .. } => { + Error::Proto { source, clock_skew } + } + _ => Error::from_proto_no_skew(source), + })?; + let (chan, reactor) = chan + .finish() + .await + .map_err(|source| Error::Proto { source, clock_skew })?; { self.event_sender diff --git a/crates/tor-chanmgr/src/err.rs b/crates/tor-chanmgr/src/err.rs index 3776700ac..6b5317f45 100644 --- a/crates/tor-chanmgr/src/err.rs +++ b/crates/tor-chanmgr/src/err.rs @@ -7,6 +7,7 @@ use futures::task::SpawnError; use thiserror::Error; use tor_error::{internal, ErrorKind}; +use tor_proto::ClockSkew; /// An error returned by a channel manager. #[derive(Debug, Error, Clone)] @@ -25,8 +26,15 @@ pub enum Error { ChanTimeout, /// A protocol error while making a channel - #[error("Protocol error while opening a channel: {0}")] - Proto(#[from] tor_proto::Error), + #[error("Protocol error while opening a channel.")] + Proto { + /// The underlying error + #[source] + source: tor_proto::Error, + /// An authenticated ClockSkew (if available) that we received from the + /// peer. + clock_skew: Option, + }, /// Network IO error or TLS error #[error("Network IO error, or TLS error, in {action}, talking to {peer}")] @@ -82,9 +90,14 @@ impl tor_error::HasKind for Error { use Error as E; use ErrorKind as EK; match self { - E::ChanTimeout | E::Io { .. } | E::Proto(ProtoErr::ChanIoErr(_)) => EK::TorAccessFailed, + E::ChanTimeout + | E::Io { .. } + | E::Proto { + source: ProtoErr::ChanIoErr(_), + .. + } => EK::TorAccessFailed, E::Spawn { cause, .. } => cause.kind(), - E::Proto(e) => e.kind(), + E::Proto { source, .. } => source.kind(), E::PendingFailed => EK::TorAccessFailed, E::UnusableTarget(_) | E::Internal(_) => EK::Internal, Error::ChannelBuild { .. } => EK::TorAccessFailed, @@ -104,7 +117,7 @@ impl tor_error::HasRetryTime for Error { // // TODO: Someday we might want to distinguish among different kinds of IO // errors. - E::PendingFailed | E::Proto(_) | E::Io { .. } => RT::AfterWaiting, + E::PendingFailed | E::Proto { .. } | E::Io { .. } => RT::AfterWaiting, // This error reflects multiple attempts, but every failure is an IO // error, so we can also retry this after a delay. @@ -131,4 +144,27 @@ impl Error { cause: Arc::new(err), } } + + /// Construct a new `Error` from a `tor_proto::Error`, with no additional + /// clock skew information. + /// + /// This is not an `Into` implementation because we don't want to call it + /// accidentally when we actually do have clock skew information. + pub(crate) fn from_proto_no_skew(source: tor_proto::Error) -> Self { + Error::Proto { + source, + clock_skew: None, + } + } + + /// Return the clock skew information from this error (or from an internal + /// error). + /// + /// Only returns the clock skew information if it is authenticated. + pub fn clock_skew(&self) -> Option { + match self { + Error::Proto { clock_skew, .. } => *clock_skew, + _ => None, + } + } } diff --git a/crates/tor-chanmgr/src/lib.rs b/crates/tor-chanmgr/src/lib.rs index a61ceab6a..92a3ef500 100644 --- a/crates/tor-chanmgr/src/lib.rs +++ b/crates/tor-chanmgr/src/lib.rs @@ -125,7 +125,8 @@ impl ChanMgr { let chan = self.mgr.get_or_launch(*ed_identity, targetinfo).await?; // Double-check the match to make sure that the RSA identity is // what we wanted too. - chan.check_match(target)?; + chan.check_match(target) + .map_err(Error::from_proto_no_skew)?; Ok(chan) } diff --git a/crates/tor-proto/src/channel/handshake.rs b/crates/tor-proto/src/channel/handshake.rs index caf75c1f8..c7a216d2b 100644 --- a/crates/tor-proto/src/channel/handshake.rs +++ b/crates/tor-proto/src/channel/handshake.rs @@ -328,7 +328,7 @@ impl UnverifiedChannel { (TimeValidityError::Expired(expired_by), ClockSkew::Fast(skew)) if expired_by < skew => { - Error::HandshakeCertsExpired { expired_by, skew } + Error::HandshakeCertsExpired { expired_by } } // As it so happens, we don't need to check for this case, since the certs in use // here only have an expiration time in them. diff --git a/crates/tor-proto/src/util/err.rs b/crates/tor-proto/src/util/err.rs index 991ef5d22..b6936c078 100644 --- a/crates/tor-proto/src/util/err.rs +++ b/crates/tor-proto/src/util/err.rs @@ -50,8 +50,6 @@ pub enum Error { HandshakeCertsExpired { /// For how long has the circuit been expired? expired_by: Duration, - /// How fast does the relay claim that our clock is? - skew: Duration, }, /// Protocol violation at the channel level, other than at the handshake /// stage. diff --git a/doc/semver_status.md b/doc/semver_status.md index 564712484..d5bdfa75b 100644 --- a/doc/semver_status.md +++ b/doc/semver_status.md @@ -22,6 +22,10 @@ We can delete older sections here after we bump the releases. MODIFIED: Added `reset()` method to RetrySchedule. +### tor-chanmgr + +BREAKING: Added members to `Error::Proto` + ### tor-circmgr MODIFIED: Added a new variant in tor_circmgr::Error. @@ -29,6 +33,7 @@ MODIFIED: Added a new variant in tor_circmgr::Error. ### tor-proto MODIFIED: New accessors in tor_proto::Channel. +BREAKING: Removed clock skew from Error::HandshakeCertsExpired. ### tor-rtmock