Skip to content
Snippets Groups Projects
Commit 72ebaed1 authored by Nick Mathewson's avatar Nick Mathewson :game_die:
Browse files

Do not blame a guard for failures on non-random circuits.

We must not apply our new path-bias behavior (where we blame a guard
if it gives us too many indeterminate circuit failures) if the path
was not chosen at random.  If too many random paths fail, we know
that's suspicious, since the other relays are a random sample.  But
if a bunch of user-provided paths fail, that could simply be because
the user's chosen exit is down.
parent 8c69b5d3
No related branches found
No related tags found
No related merge requests found
......@@ -133,6 +133,7 @@ impl<'a> ExitPathBuilder<'a> {
} else {
None
};
let path_is_fully_random = chosen_exit.is_none();
// TODO-SPEC: Because of limitations in guard selection, we have to
// pick the guard before the exit, which is not what our spec says.
......@@ -149,10 +150,19 @@ impl<'a> ExitPathBuilder<'a> {
b.restriction(tor_guardmgr::GuardRestriction::AvoidId(*id));
}
let guard_usage = b.build().expect("Failed while building guard usage!");
let (guard, mon, usable) = guardmgr.select_guard(guard_usage, Some(netdir))?;
let (guard, mut mon, usable) = guardmgr.select_guard(guard_usage, Some(netdir))?;
let guard = guard.get_relay(netdir).ok_or_else(|| {
Error::Internal("Somehow the guardmgr gave us an unlisted guard!".to_owned())
})?;
if !path_is_fully_random {
// We were given a specific exit relay to use, and
// the choice of exit relay might be forced by
// something outside of our control.
//
// Therefore, we must not blame the guard for any failure
// to complete the circuit.
mon.ignore_indeterminate_status();
}
(guard, Some(mon), Some(usable))
}
None => {
......
......@@ -102,6 +102,13 @@ pub struct GuardMonitor {
id: RequestId,
/// The status that we will report if this monitor is dropped now.
pending_status: GuardStatus,
/// If set, we change `Indeterminate` to `AttemptAbandoned` before
/// reporting it to the guard manager.
///
/// We do this when a failure to finish the circuit doesn't reflect
/// badly against the guard at all: typically, because the circuit's
/// path is not random.
ignore_indeterminate: bool,
/// A sender that needs to get told when the attempt to use the guard is
/// finished or abandoned.
snd: Option<oneshot::Sender<daemon::Msg>>,
......@@ -115,6 +122,7 @@ impl GuardMonitor {
GuardMonitor {
id,
pending_status: GuardStatus::AttemptAbandoned,
ignore_indeterminate: false,
snd: Some(snd),
},
rcv,
......@@ -145,7 +153,8 @@ impl GuardMonitor {
/// or that we can't tell whether the guard is working.
///
/// Dropping a `GuardMonitor` is without calling `succeeded` or
/// `failed` is equivalent to calling this function.
/// `failed` or `pending_status` is equivalent to calling this
/// function.
pub fn attempt_abandoned(self) {
self.report(GuardStatus::AttemptAbandoned);
}
......@@ -156,8 +165,26 @@ impl GuardMonitor {
self.pending_status = status;
}
/// Configure this monitor to ignore any indeterminate status
/// values, and treat them as abandoned attempts.
///
/// We should use this whenever the path being built with this guard
/// is not randomly generated.
pub fn ignore_indeterminate_status(&mut self) {
self.ignore_indeterminate = true;
}
/// Report a message for this guard.
pub fn report(mut self, msg: GuardStatus) {
self.report_impl(msg);
}
/// As [`GuardMonitor::report`], but take a &mut reference.
fn report_impl(&mut self, msg: GuardStatus) {
let msg = match (msg, self.ignore_indeterminate) {
(GuardStatus::Indeterminate, true) => GuardStatus::AttemptAbandoned,
(m, _) => m,
};
let _ignore = self
.snd
.take()
......@@ -174,8 +201,8 @@ impl GuardMonitor {
impl Drop for GuardMonitor {
fn drop(&mut self) {
if let Some(snd) = self.snd.take() {
let _ignore = snd.send(daemon::Msg::Status(self.id, self.pending_status));
if self.snd.is_some() {
self.report_impl(self.pending_status);
}
}
}
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment