diff --git a/Cargo.lock b/Cargo.lock index e07904c6b..60da709f1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4630,6 +4630,7 @@ version = "0.2.3" dependencies = [ "async-trait", "derive-adhoc", + "derive_more", "educe", "futures", "humantime 2.1.0", @@ -4640,6 +4641,7 @@ dependencies = [ "rand_core 0.6.4", "safelog", "serde", + "serde_json", "thiserror", "tor-async-utils", "tor-basic-utils", @@ -4673,6 +4675,7 @@ dependencies = [ "rand 0.8.5", "sec1", "serde", + "serde_json", "ssh-key", "tempfile", "thiserror", diff --git a/crates/tor-hsservice/Cargo.toml b/crates/tor-hsservice/Cargo.toml index e4cb5f170..3006e60b0 100644 --- a/crates/tor-hsservice/Cargo.toml +++ b/crates/tor-hsservice/Cargo.toml @@ -27,6 +27,7 @@ full = [ [dependencies] async-trait = "0.1.54" derive-adhoc = "0.7.3" +derive_more = "0.99.17" educe = "0.4.6" futures = "0.3.14" itertools = "0.11.0" @@ -60,4 +61,5 @@ void = "1" [dev-dependencies] humantime = "2" +serde_json = "1.0.104" tor-rtmock = { path = "../tor-rtmock", version = "0.9.0" } diff --git a/crates/tor-hsservice/src/config.rs b/crates/tor-hsservice/src/config.rs index 6f39a187e..f0bfbba5e 100644 --- a/crates/tor-hsservice/src/config.rs +++ b/crates/tor-hsservice/src/config.rs @@ -1,18 +1,18 @@ //! Configuration information for onion services. +use crate::HsNickname; + /// Configuration for a single onion service. #[derive(Debug, Clone)] pub struct OnionServiceConfig { - /// An arbitrary identifier or "nickname" used to look up this service's - /// keys, state, configuration, etc, - /// and distinguish them from other services. This is local-only. + /// The nickname used to look up this service's keys, state, configuration, etc, // // TODO HSS: It's possible that instead of having this be _part_ of the // service's configuration, we want this to be the key for a map in // which the service's configuration is stored. We'll see how the code // evolves. // (^ ipt_mgr::IptManager contains a copy of this nickname, that should be fixed too) - nickname: String, + nickname: HsNickname, /// Whether we want this to be a non-anonymous "single onion service". /// We could skip this in v1. We should make sure that our state diff --git a/crates/tor-hsservice/src/ipt_mgr.rs b/crates/tor-hsservice/src/ipt_mgr.rs index 211fc07ac..7955f3e03 100644 --- a/crates/tor-hsservice/src/ipt_mgr.rs +++ b/crates/tor-hsservice/src/ipt_mgr.rs @@ -31,7 +31,7 @@ use tor_rtcompat::Runtime; use crate::svc::ipt_establish; use crate::timeout_track::{TrackingInstantOffsetNow, TrackingNow}; -use crate::{FatalError, OnionServiceConfig, RendRequest, StartupError}; +use crate::{FatalError, HsNickname, OnionServiceConfig, RendRequest, StartupError}; use ipt_establish::{IptEstablisher, IptParameters, IptStatus, IptStatusStatus, IptWantsToRetire}; use IptStatusStatus as ISS; @@ -45,9 +45,6 @@ const IPT_RELAY_ROTATION_TIME: RangeInclusive = { Duration::from_secs(DAY * 4)..=Duration::from_secs(DAY * 7) }; -/// TODO HSS make HsNickname a newtype, somewhere more central, and document it properly -type HsNickname = String; - /// Persistent local identifier for an introduction point /// /// Changes when the IPT relay changes, or the IPT key material changes. diff --git a/crates/tor-hsservice/src/lib.rs b/crates/tor-hsservice/src/lib.rs index eb19d818c..1bb567d9e 100644 --- a/crates/tor-hsservice/src/lib.rs +++ b/crates/tor-hsservice/src/lib.rs @@ -46,6 +46,7 @@ mod config; mod err; mod ipt_mgr; mod keys; +mod nickname; mod req; mod status; mod svc; @@ -53,6 +54,7 @@ mod timeout_track; pub use config::OnionServiceConfig; pub use err::{ClientError, FatalError, StartupError}; +pub use nickname::{HsNickname, InvalidNickname}; pub use req::{OnionServiceDataStream, RendRequest, StreamRequest}; pub use status::OnionServiceStatus; pub use svc::OnionService; diff --git a/crates/tor-hsservice/src/nickname.rs b/crates/tor-hsservice/src/nickname.rs new file mode 100644 index 000000000..ed9c452a8 --- /dev/null +++ b/crates/tor-hsservice/src/nickname.rs @@ -0,0 +1,109 @@ +//! `HsNickname` module itself is private, but `HsNickname` etc. are re-exported + +use derive_more::{Display, From, Into}; +use serde::{Deserialize, Serialize}; +use thiserror::Error; + +use tor_keymgr::ArtiPathComponent; + +/// Nickname (local identifier) for a Tor hidden service +/// +/// Used to look up this services's +/// keys, state, configuration, etc, +/// and distinguish them from other services. +/// +/// Must be a nonempty valid unicode string containing only alphanumerics +/// (possibly non-ASCII ones) +/// plus the punctuation characters `_` `-` and `.` +/// (but punctuation is not allowed at the start or end). +/// +/// (These are the same rules as [`tor_keymgr::ArtiPathComponent`] +#[derive( + Clone, Debug, Hash, Eq, PartialEq, Ord, PartialOrd, Display, From, Into, Serialize, Deserialize, +)] +#[serde(try_from = "String", into = "String")] +pub struct HsNickname(ArtiPathComponent); + +/// Local nickname for Tor Hidden Service (`.onion` service) was syntactically invalid +#[derive(Clone, Debug, Hash, Eq, PartialEq, Error)] +#[non_exhaustive] +#[error("Invalid syntax for hidden service nickname")] +pub struct InvalidNickname {} + +impl HsNickname { + /// Create a new `HsNickname` from a `String` + /// + /// Returns an error if the syntax is not valid + fn new(s: String) -> Result { + Ok(Self(s.try_into().map_err(|_| InvalidNickname {})?)) + } +} + +impl From for String { + fn from(nick: HsNickname) -> String { + nick.0.into() + } +} + +impl TryFrom for HsNickname { + type Error = InvalidNickname; + fn try_from(s: String) -> Result { + Self::new(s) + } +} + +impl AsRef for HsNickname { + fn as_ref(&self) -> &str { + self.0.as_ref() + } +} + +impl AsRef for HsNickname { + fn as_ref(&self) -> &ArtiPathComponent { + &self.0 + } +} + +#[cfg(test)] +mod test { + // @@ begin test lint list maintained by maint/add_warning @@ + #![allow(clippy::bool_assert_comparison)] + #![allow(clippy::clone_on_copy)] + #![allow(clippy::dbg_macro)] + #![allow(clippy::print_stderr)] + #![allow(clippy::print_stdout)] + #![allow(clippy::single_char_pattern)] + #![allow(clippy::unwrap_used)] + #![allow(clippy::unchecked_duration_subtraction)] + #![allow(clippy::useless_vec)] + #![allow(clippy::needless_pass_by_value)] + //! + use super::*; + + #[test] + fn mk() { + assert_eq!(HsNickname::new("".into()), Err(InvalidNickname {})); + assert_eq!(HsNickname::new("-a".into()), Err(InvalidNickname {})); + assert_eq!(HsNickname::new("b.".into()), Err(InvalidNickname {})); + assert_eq!(HsNickname::new("_c".into()), Err(InvalidNickname {})); + assert_eq!(&HsNickname::new("x".into()).unwrap().to_string(), "x"); + } + + #[test] + fn serde() { + // TODO HSS clone-and-hack with tor_keymgr::::key_specifier::test::serde + #[derive(Serialize, Deserialize, Debug)] + struct T { + n: HsNickname, + } + let j = serde_json::from_str(r#"{ "n": "x" }"#).unwrap(); + let t: T = serde_json::from_value(j).unwrap(); + assert_eq!(&t.n.to_string(), "x"); + + assert_eq!(&serde_json::to_string(&t).unwrap(), r#"{"n":"x"}"#); + + let j = serde_json::from_str(r#"{ "n": "!" }"#).unwrap(); + let e = serde_json::from_value::(j).unwrap_err(); + assert!(e.to_string().contains("Invalid syntax"), "wrong msg {e:?}"); + } +} diff --git a/crates/tor-keymgr/Cargo.toml b/crates/tor-keymgr/Cargo.toml index ba70168c2..51f6921b3 100644 --- a/crates/tor-keymgr/Cargo.toml +++ b/crates/tor-keymgr/Cargo.toml @@ -51,6 +51,7 @@ tor-llcrypto = { path = "../tor-llcrypto", version = "0.5.2", features = ["keymg zeroize = "1" [dev-dependencies] +serde_json = "1.0.104" tempfile = "3" tor-basic-utils = { path = "../tor-basic-utils", version = "0.7.3" } diff --git a/crates/tor-keymgr/semver.md b/crates/tor-keymgr/semver.md index 85bd805af..4bfd62705 100644 --- a/crates/tor-keymgr/semver.md +++ b/crates/tor-keymgr/semver.md @@ -3,3 +3,7 @@ ADDED: `SshKeypairData` (re-exported from `ssh-key`) REMOVED: `KeyType::to_ssh_format` BREAKING: re-export `ssh_key` rather than just `ssh_key::private::KeypairData` BREAKING: ssh-key is bumped to 0.6.0 (we re-export `ssh_key`) +ADDED: ArtiPathComponent is TryFrom +ADDED: ArtiPathComponent is AsRef +ADDED: ArtiPathComponent is Hash, Eq, PartialEq, Ord, PartialOrd +ADDED: ArtiPathComponent is Serialize, Deserialize diff --git a/crates/tor-keymgr/src/key_specifier.rs b/crates/tor-keymgr/src/key_specifier.rs index 18bc2bed7..a1e2e800b 100644 --- a/crates/tor-keymgr/src/key_specifier.rs +++ b/crates/tor-keymgr/src/key_specifier.rs @@ -2,6 +2,7 @@ use crate::Result; use derive_more::{Deref, DerefMut, Display, Into}; +use serde::{Deserialize, Serialize}; use tor_error::internal; /// The path of a key in the Arti key store. @@ -27,6 +28,7 @@ impl ArtiPath { /// Create a new [`ArtiPath`]. /// /// This function returns an error if `inner` is not a valid `ArtiPath`. + // TODO HSS this function (and validate_str) should have a bespoke error type pub fn new(inner: String) -> Result { if let Some(e) = inner .split(PATH_SEP) @@ -46,8 +48,21 @@ impl ArtiPath { // // TODO HSS: disallow consecutive `.` to prevent path traversal. #[derive( - Clone, Debug, derive_more::Deref, derive_more::DerefMut, derive_more::Into, derive_more::Display, + Clone, + Debug, + derive_more::Deref, + derive_more::DerefMut, + derive_more::Into, + derive_more::Display, + Hash, + Eq, + PartialEq, + Ord, + PartialOrd, + Serialize, + Deserialize, )] +#[serde(try_from = "String", into = "String")] pub struct ArtiPathComponent(String); impl ArtiPathComponent { @@ -84,6 +99,19 @@ impl ArtiPathComponent { } } +impl TryFrom for ArtiPathComponent { + type Error = crate::Error; // TODO HSS should be bespoke error type + fn try_from(s: String) -> Result { + Self::new(s) + } +} + +impl AsRef for ArtiPathComponent { + fn as_ref(&self) -> &str { + &self.0 + } +} + /// The path of a key in the C Tor key store. #[derive(Clone, Debug, derive_more::Deref, derive_more::DerefMut)] pub struct CTorPath(String); @@ -191,4 +219,26 @@ mod test { check_valid!(ArtiPath, &path, true); check_valid!(ArtiPathComponent, &path, false); } + + #[test] + fn serde() { + // TODO HSS clone-and-hack with tor_hsservice::::nickname::test::serde + // perhaps there should be some utility in tor-basic-utils for testing + // validated string newtypes, or something + #[derive(Serialize, Deserialize, Debug)] + struct T { + n: ArtiPathComponent, + } + let j = serde_json::from_str(r#"{ "n": "x" }"#).unwrap(); + let t: T = serde_json::from_value(j).unwrap(); + assert_eq!(&t.n.to_string(), "x"); + + assert_eq!(&serde_json::to_string(&t).unwrap(), r#"{"n":"x"}"#); + + let j = serde_json::from_str(r#"{ "n": "!" }"#).unwrap(); + let e = serde_json::from_value::(j).unwrap_err(); + // TODO HSS this is the wrong error message, this might not be a bug; + // it could be bad config or something + assert!(e.to_string().contains("internal error"), "wrong msg {e:?}"); + } }