Commit d05022de authored by Nick Mathewson's avatar Nick Mathewson 🦞
Browse files

Treat expired/not-yet-valid directory objects as Errors. 

Doing this will make us treat caches that send us these objects as
not-working, and close circuits to them instead of trying over and
over.

The case where we add a document from the cache requires special
handling: it isn't actually a error to find an expired document in
our cache (unless the passage of time itself is erroneous, which is
a debatable proposition at best).

Fixes #431.
parent 314f5707

crates/tor-dirmgr/src/bootstrap.rs

+10 −1
Original line number Original line Diff line number Diff line
@@ -16,6 +16,7 @@ use crate::{ 
use futures::channel::oneshot;

use futures::channel::oneshot;

use futures::FutureExt;

use futures::FutureExt;

use futures::StreamExt;

use futures::StreamExt;

use tor_checkable::TimeValidityError;

use tor_dirclient::DirResponse;

use tor_dirclient::DirResponse;

use tor_rtcompat::{Runtime, SleepProviderExt};

use tor_rtcompat::{Runtime, SleepProviderExt};

use tracing::{debug, info, trace, warn};

use tracing::{debug, info, trace, warn};
@@ -132,7 +133,15 @@ async fn load_once<R: Runtime>( 
            missing.len()

            missing.len()

        );

        );

        let documents = load_all(dirmgr, missing)?;

        let documents = load_all(dirmgr, missing)?;

        state.add_from_cache(documents, dirmgr.store_if_rw())



        match state.add_from_cache(documents, dirmgr.store_if_rw()) {

            Err(Error::UntimelyObject(TimeValidityError::Expired(_))) => {

                // This is just an expired object from the cache; we don't need

                // to call that an error.  Treat it as if it were absent.

                Ok(false)

            }

            other => other,

        }

    };

    };





    if matches!(outcome, Ok(true)) {

    if matches!(outcome, Ok(true)) {

crates/tor-dirmgr/src/err.rs

+6 −1
Original line number Original line Diff line number Diff line
@@ -68,6 +68,9 @@ pub enum Error { 
        #[source]

        #[source]

        cause: tor_netdoc::Error,

        cause: tor_netdoc::Error,

    },

    },

    /// An error caused by an expired or not-yet-valid object.

    #[error("object expired or not yet valid.")]

    UntimelyObject(#[from] tor_checkable::TimeValidityError),

    /// An error given by dirclient

    /// An error given by dirclient

    #[error("dirclient error: {0}")]

    #[error("dirclient error: {0}")]

    DirClientError(#[from] tor_dirclient::Error),

    DirClientError(#[from] tor_dirclient::Error),
@@ -145,7 +148,8 @@ impl Error { 
            | Error::BadUtf8FromDirectory(_)

            | Error::BadUtf8FromDirectory(_)

            | Error::ConsensusDiffError(_)

            | Error::ConsensusDiffError(_)

            | Error::SignatureError(_)

            | Error::SignatureError(_)

            | Error::IOError(_) => true,

            | Error::IOError(_)

            | Error::UntimelyObject(_) => true,





            // These errors cannot come from a directory cache.

            // These errors cannot come from a directory cache.

            Error::NoDownloadSupport

            Error::NoDownloadSupport
@@ -221,6 +225,7 @@ impl HasKind for Error { 
                DocSource::LocalCache => EK::CacheCorrupted,

                DocSource::LocalCache => EK::CacheCorrupted,

                DocSource::DirServer { .. } => EK::TorProtocolViolation,

                DocSource::DirServer { .. } => EK::TorProtocolViolation,

            },

            },

            E::UntimelyObject(_) => EK::TorProtocolViolation,

            E::DirClientError(e) => e.kind(),

            E::DirClientError(e) => e.kind(),

            E::SignatureError(_) => EK::TorProtocolViolation,

            E::SignatureError(_) => EK::TorProtocolViolation,

            E::IOError(_) => EK::CacheAccessFailed,

            E::IOError(_) => EK::CacheAccessFailed,

crates/tor-dirmgr/src/state.rs

+9 −16
Original line number Original line Diff line number Diff line
@@ -294,12 +294,9 @@ impl<DM: WriteNetDir> GetConsensusState<DM> { 
                parsed

                parsed

            };

            };

            let now = current_time(&self.writedir)?;

            let now = current_time(&self.writedir)?;

            if let Ok(timely) = parsed.check_valid_at(&now) {

            let timely = parsed.check_valid_at(&now)?;

            let meta = ConsensusMeta::from_unvalidated(signedval, remainder, &timely);

            let meta = ConsensusMeta::from_unvalidated(signedval, remainder, &timely);

            (meta, timely)

            (meta, timely)

            } else {

                return Ok(None);

            }

        };

        };





        // Check out what authorities we believe in, and see if enough

        // Check out what authorities we believe in, and see if enough
@@ -421,13 +418,10 @@ impl<DM: WriteNetDir> DirState for GetCertsState<DM> { 
                    .map_err(|e| Error::from_netdoc(DocSource::LocalCache, e))?

                    .map_err(|e| Error::from_netdoc(DocSource::LocalCache, e))?

                    .check_signature()?;

                    .check_signature()?;

                let now = current_time(&self.writedir)?;

                let now = current_time(&self.writedir)?;

                if let Ok(cert) = parsed.check_valid_at(&now) {

                let cert = parsed.check_valid_at(&now)?;

                self.missing_certs.remove(cert.key_ids());

                self.missing_certs.remove(cert.key_ids());

                self.certs.push(cert);

                self.certs.push(cert);

                changed = true;

                changed = true;

                } else {

                    warn!("Got a cert from our cache that we couldn't parse");

                }

            }

            }

        }

        }

        Ok(changed)

        Ok(changed)
@@ -451,9 +445,8 @@ impl<DM: WriteNetDir> DirState for GetCertsState<DM> { 
                    .expect("Certificate was not in input as expected");

                    .expect("Certificate was not in input as expected");

                if let Ok(wellsigned) = parsed.check_signature() {

                if let Ok(wellsigned) = parsed.check_signature() {

                    let now = current_time(&self.writedir)?;

                    let now = current_time(&self.writedir)?;

                    if let Ok(timely) = wellsigned.check_valid_at(&now) {

                    let timely = wellsigned.check_valid_at(&now)?;

                    newcerts.push((timely, s));

                    newcerts.push((timely, s));

                    }

                } else {

                } else {

                    // TODO: note the source.

                    // TODO: note the source.

                    warn!("Badly signed certificate received and discarded.");

                    warn!("Badly signed certificate received and discarded.");