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

Merge branch 'netdir_todos' into 'main' 

Resolve or defer most TODO HS items in tor-netdir.

See merge request tpo/core/arti!1310
This commit is contained in:
Nick Mathewson 2023-06-29 12:48:32 +00:00
parent 9c853e5b54 76376b2abd
commit 83552b559f
6 changed files with 81 additions and 51 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
crates/tor-dirmgr/src/state.rs
View File

@ -1163,11 +1163,8 @@ fn pick_download_time(lifetime: &Lifetime) -> SystemTime {
/// clients should fetch the next one.
fn client_download_range(lt: &Lifetime) -> (SystemTime, Duration) {
let valid_after = lt.valid_after();
let fresh_until = lt.fresh_until();
let valid_until = lt.valid_until();
let voting_interval = fresh_until
.duration_since(valid_after)
.expect("valid-after must precede fresh-until");
let voting_interval = lt.voting_period();
let whole_lifetime = valid_until
.duration_since(valid_after)
.expect("valid-after must precede valid-until");

57
crates/tor-netdir/src/hsdir_params.rs
View File

@ -28,7 +28,7 @@ use std::time::{Duration, SystemTime};
use crate::{params::NetParameters, Error, HsDirs, Result};
use time::{OffsetDateTime, UtcOffset};
use tor_hscrypto::time::TimePeriod;
use tor_netdoc::doc::netstatus::{Lifetime, MdConsensus, SharedRandVal};
use tor_netdoc::doc::netstatus::{MdConsensus, SharedRandVal};
/// Parameters for generating and using an HsDir ring.
///
@ -77,19 +77,34 @@ impl HsDirParams {
///
/// (This function's return type is a bit cumbersome; these parameters are
/// bundled together because it is efficient to compute them all at once.)
///
/// Note that this function will only return an error if something is
/// _extremely_ wrong with the provided consensus: for other error cases, it
/// returns a "disaster fallback".
pub(crate) fn compute(
consensus: &MdConsensus,
params: &NetParameters,
) -> Result<HsDirs<HsDirParams>> {
let srvs = extract_srvs(consensus)?;
let srvs = extract_srvs(consensus);
let tp_length: Duration = params.hsdir_timeperiod_length.try_into().map_err(|_| {
// Note that this error should be impossible:
// The type of hsdir_timeperiod_length() is IntegerMinutes<BoundedInt32<30, 14400>>...
// It should be at most 10 days, which _definitely_ fits into a Duration.
Error::InvalidConsensus(
"Minutes in hsdir timeperiod could not be converted to a Duration",
)
})?;
let offset = voting_period(consensus.lifetime())? * VOTING_PERIODS_IN_OFFSET;
let offset = consensus.lifetime().voting_period() * VOTING_PERIODS_IN_OFFSET;
let cur_period = TimePeriod::new(tp_length, consensus.lifetime().valid_after(), offset)
.map_err(|_| {
// This error should be nearly impossible too:
// - It can occur if the time period length is not an integer
// number of minutes--but we took it from an IntegerMinutes,
// so that's unlikely.
// - It can occur if the time period length or the offset is
// greater than can be represented in u32 seconds.
// - It can occur if the valid_after time is so far from the
// epoch that we can't represent the distance as a Duration.
Error::InvalidConsensus("Consensus valid-after did not fall in a time period")
})?;
@ -168,10 +183,10 @@ fn find_srv_for_time(info: &[SrvInfo], when: SystemTime) -> Option<SharedRandVal
/// Return every SRV from a consensus, along with a duration over which it is
/// most recent SRV.
fn extract_srvs(consensus: &MdConsensus) -> Result<Vec<SrvInfo>> {
fn extract_srvs(consensus: &MdConsensus) -> Vec<SrvInfo> {
let mut v = Vec::new();
let consensus_ts = consensus.lifetime().valid_after();
let srv_interval = srv_interval(consensus)?;
let srv_interval = srv_interval(consensus);
if let Some(cur) = consensus.shared_rand_cur() {
let ts_begin = cur
@ -188,11 +203,11 @@ fn extract_srvs(consensus: &MdConsensus) -> Result<Vec<SrvInfo>> {
v.push((*prev.value(), ts_begin..ts_end));
}
Ok(v)
v
}
/// Return the length of time for which a single SRV value is valid.
fn srv_interval(consensus: &MdConsensus) -> Result<Duration> {
fn srv_interval(consensus: &MdConsensus) -> Duration {
// What we _want_ to do, ideally, is is to learn the duration from the
// difference between the declared time for the previous value and the
// declared time for the current one.
@ -201,7 +216,7 @@ fn srv_interval(consensus: &MdConsensus) -> Result<Duration> {
if let (Some(cur), Some(prev)) = (consensus.shared_rand_cur(), consensus.shared_rand_prev()) {
if let (Some(cur_ts), Some(prev_ts)) = (cur.timestamp(), prev.timestamp()) {
if let Ok(d) = cur_ts.duration_since(prev_ts) {
return Ok(d);
return d;
}
}
}
@ -209,20 +224,12 @@ fn srv_interval(consensus: &MdConsensus) -> Result<Duration> {
// But if one of those values is missing, or if it has no timestamp, we have
// to fall back to admitting that we know the schedule for the voting
// algorithm.
voting_period(consensus.lifetime()).map(|d| d * VOTING_PERIODS_IN_SRV_ROUND)
consensus.lifetime().voting_period() * VOTING_PERIODS_IN_SRV_ROUND
}
/// Return the length of the voting period in the consensus.
///
/// (The "voting period" is the length of time between between one consensus and the next.)
fn voting_period(lifetime: &Lifetime) -> Result<Duration> {
// TODO hs: consider moving this function to be a method of Lifetime.
let valid_after = lifetime.valid_after();
let fresh_until = lifetime.fresh_until();
fresh_until
.duration_since(valid_after)
.map_err(|_| Error::InvalidConsensus("Mis-formed lifetime"))
}
/// Return a time at the start of the UTC day containing `t`.
fn start_of_day_containing(t: SystemTime) -> SystemTime {
@ -246,7 +253,7 @@ mod test {
//! <!-- @@ end test lint list maintained by maint/add_warning @@ -->
use super::*;
use hex_literal::hex;
use tor_netdoc::doc::netstatus::{ConsensusBuilder, MdConsensusRouterStatus};
use tor_netdoc::doc::netstatus::{ConsensusBuilder, Lifetime, MdConsensusRouterStatus};
/// Helper: parse an rfc3339 time.
///
@ -309,7 +316,7 @@ mod test {
#[test]
fn vote_period() {
assert_eq!(voting_period(&example_lifetime()).unwrap(), d("1 hour"));
assert_eq!(example_lifetime().voting_period(), d("1 hour"));
let lt2 = Lifetime::new(
t("1985-10-25T07:00:00Z"),
@ -318,14 +325,14 @@ mod test {
)
.unwrap();
assert_eq!(voting_period(&lt2).unwrap(), d("22 min"));
assert_eq!(lt2.voting_period(), d("22 min"));
}
#[test]
fn srv_period() {
// In a basic consensus with no SRV timestamps, we'll assume 24 voting periods.
let consensus = example_consensus_builder().testing_consensus().unwrap();
assert_eq!(srv_interval(&consensus).unwrap(), d("1 day"));
assert_eq!(srv_interval(&consensus), d("1 day"));
// If there are timestamps, we look at the difference between them.
let consensus = example_consensus_builder()
@ -333,7 +340,7 @@ mod test {
.shared_rand_cur(7, SRV2.into(), Some(t("1985-10-25T06:00:05Z")))
.testing_consensus()
.unwrap();
assert_eq!(srv_interval(&consensus).unwrap(), d("6 hours 5 sec"));
assert_eq!(srv_interval(&consensus), d("6 hours 5 sec"));
// Note that if the timestamps are in reversed order, we fall back to 24 hours.
let consensus = example_consensus_builder()
@ -341,13 +348,13 @@ mod test {
.shared_rand_prev(7, SRV2.into(), Some(t("1985-10-25T06:00:05Z")))
.testing_consensus()
.unwrap();
assert_eq!(srv_interval(&consensus).unwrap(), d("1 day"));
assert_eq!(srv_interval(&consensus), d("1 day"));
}
#[test]
fn srvs_extract_and_find() {
let consensus = example_consensus_builder().testing_consensus().unwrap();
let srvs = extract_srvs(&consensus).unwrap();
let srvs = extract_srvs(&consensus);
assert_eq!(
srvs,
vec![
@ -372,7 +379,7 @@ mod test {
.shared_rand_cur(7, SRV2.into(), Some(t("1985-10-25T06:00:05Z")))
.testing_consensus()
.unwrap();
let srvs = extract_srvs(&consensus).unwrap();
let srvs = extract_srvs(&consensus);
assert_eq!(
srvs,
vec![

4
crates/tor-netdir/src/hsdir_ring.rs
View File

@ -13,8 +13,6 @@
//! position or later. ("N" is a "number of replicas" parameter, and "S" is a
//! "Spread" parameter.)
#![allow(unused_variables, dead_code)] //TODO hs: remove
use std::collections::HashMap;
use std::fmt::Debug;
@ -146,7 +144,7 @@ impl HsDirRing {
this_netdir: &NetDir,
prev_netdir: Option<&NetDir>,
) -> Self {
// TODO hs: The ring itself can be a bit expensive to compute, so maybe we should
// TODO: The ring itself can be a bit expensive to compute, so maybe we should
// make sure this happens in a separate task or something, and expose a
// way to do that?
// But: this is being done during netdir ingestion, which is already happening

52
crates/tor-netdir/src/lib.rs
View File

@ -374,14 +374,14 @@ pub(crate) struct HsDirs<D> {
/// secondary rings will be active at a time. We have two here in order
/// to conform with a more flexible regime in proposal 342.
//
// TODO hs: hs clients never need this; so I've made it not-present for thm.
// TODO hss: hs clients never need this; so I've made it not-present for thm.
// But does that risk too much with respect to side channels?
//
// TODO hs: Perhaps we should refactor this so that it is clear that these
// TODO hss: Perhaps we should refactor this so that it is clear that these
// are immutable? On the other hand, the documentation for this type
// declares that it is immutable, so we are likely okay.
//
// TODO hs: this `Vec` is only ever 0,1,2 elements.
// TODO hss: this `Vec` is only ever 0,1,2 elements.
// Maybe it should be an ArrayVec or something.
#[cfg(feature = "hs-service")]
secondary: Vec<D>,
@ -667,9 +667,11 @@ impl PartialNetDir {
#[cfg(feature = "hs-common")]
let hsdir_rings = Arc::new({
let params = HsDirParams::compute(&consensus, &params).expect("Invalid consensus!");
// TODO HS: I dislike using expect above, but this function does not
// return a Result. Perhaps we should change it so that it can? Or as an alternative
// we could let this object exist in a state without any HsDir rings.
// TODO: It's a bit ugly to use expect above, but this function does
// not return a Result. On the other hand, the error conditions under which
// HsDirParams::compute can return Err are _very_ narrow and hard to
// hit; see documentation in that function. As such, we probably
// don't need to have this return a Result.
params.map(HsDirRing::empty_from_params)
});
@ -722,7 +724,7 @@ impl PartialNetDir {
fn compute_rings(&mut self) {
let params = HsDirParams::compute(&self.netdir.consensus, &self.netdir.params)
.expect("Invalid consensus");
// TODO hs: see TODO by similar expect in new()
// TODO: see TODO by similar expect in new()
self.netdir.hsdir_rings =
Arc::new(params.map(|params| {
@ -943,10 +945,6 @@ impl NetDir {
/// This will be very slow if `target` does not have an Ed25519 or RSA
/// identity.
//
// TODO HS: We should use this to check whether a set of linkspecs is
// possible when we're about to use it to make an introduction or rendezvous
// circuit from an externally provided set of linkspecs.
//
// TODO HS: This function could use a better name.
//
// TODO: We could remove the feature restriction here once we think this API is
@ -1342,8 +1340,6 @@ impl NetDir {
/// Specifically, this returns the time period that contains the beginning
/// of the validity period of this `NetDir`'s consensus. That time period
/// is the one we use when acting as an hidden service client.
//
// TODO HS do we need this function?
#[cfg(feature = "hs-common")]
pub fn hs_time_period(&self) -> TimePeriod {
self.hsdir_rings.current.time_period()
@ -1356,7 +1352,7 @@ impl NetDir {
/// plus additional time periods that we publish descriptors for when we are
/// acting as a hidden service.
//
// TODO HS do we need this function?
// TODO HSS do we need this function?
#[cfg(feature = "hs-service")]
pub fn hs_all_time_periods(&self) -> Vec<TimePeriod> {
self.hsdir_rings
@ -1377,7 +1373,6 @@ impl NetDir {
/// Return an error if the time period is not one returned by
/// `onion_service_time_period` or `onion_service_secondary_time_periods`.
#[cfg(feature = "hs-common")]
#[allow(unused, clippy::missing_panics_doc)] // TODO hs: remove.
pub fn hs_dirs<'r, R>(&'r self, hsid: &HsBlindId, op: HsDirOp, rng: &mut R) -> Vec<Relay<'r>>
where
R: rand::Rng,
@ -1398,15 +1393,36 @@ impl NetDir {
// adding them to Dirs until we have added `spread` new elements
// that were not there before.
// 7. return Dirs.
let n_replicas = 2; // TODO HS get this from netdir and/or make it configurable
let spread_fetch = 3; // TODO HS get this from netdir and/or make it configurable
let n_replicas = self
.params
.hsdir_n_replicas
.get()
.try_into()
.expect("BoundedInt did not enforce bounds");
let spread_fetch = self
.params
.hsdir_spread_fetch
.get()
.try_into()
.expect("BoundedInt did not enforce bounds!");
// TODO HS We don't implement this bit of the spec (2.2.3 penultimate para):
// TODO HSS We don't implement this bit of the spec (2.2.3 penultimate para):
//
// ... If any of those
// nodes have already been selected for a lower-numbered replica of the
// service, any nodes already chosen are disregarded (i.e. skipped over)
// when choosing a replica's hsdir_spread_store nodes.
//
// This doesn't yet affect compatibility for clients, but when we
// implement onion services, it will create compatibility issues. We
// need to fix it.
//
// TODO HSS: I may be wrong here but I suspect that this function may
// need refactoring so that it does not look at _all_ of the HsDirRings,
// but only at the ones that corresponds to time periods for which
// HsBlindId is valid. Or I could be mistaken, in which case we should
// have a comment to explain why I am, since the logic is subtle.
// (For clients, there is only one ring.) -nickm
let mut hs_dirs = self
.hsdir_rings

2
crates/tor-netdoc/semver.md
View File

@ -1,2 +1,2 @@
ADDED: accessors for protocol status.
ADDED: Lifetime::voting_interval()

12
crates/tor-netdoc/src/doc/netstatus.rs
View File

@ -141,6 +141,18 @@ impl Lifetime {
pub fn valid_at(&self, when: time::SystemTime) -> bool {
self.valid_after <= when && when <= self.valid_until
}
/// Return the voting period implied by this lifetime.
///
/// (The "voting period" is the amount of time in between when a consensus first
/// becomes valid, and when the next consensus is expected to become valid)
pub fn voting_period(&self) -> time::Duration {
let valid_after = self.valid_after();
let fresh_until = self.fresh_until();
fresh_until
.duration_since(valid_after)
.expect("Mis-formed lifetime")
}
}
/// A set of named network parameters.