Merge branch 'nickname' into 'main'

Sort out HsNickname, and improve ArtiPathComponent

See merge request tpo/core/arti!1544

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",

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" }

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

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> = {
     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.

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;

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<HsNickname, InvalidNickname> {
+        Ok(Self(s.try_into().map_err(|_| InvalidNickname {})?))
+    }
+}
+
+impl From<HsNickname> for String {
+    fn from(nick: HsNickname) -> String {
+        nick.0.into()
+    }
+}
+
+impl TryFrom<String> for HsNickname {
+    type Error = InvalidNickname;
+    fn try_from(s: String) -> Result<HsNickname, InvalidNickname> {
+        Self::new(s)
+    }
+}
+
+impl AsRef<str> for HsNickname {
+    fn as_ref(&self) -> &str {
+        self.0.as_ref()
+    }
+}
+
+impl AsRef<ArtiPathComponent> 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)]
+    //! <!-- @@ end test lint list maintained by maint/add_warning @@ -->
+    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::<T>(j).unwrap_err();
+        assert!(e.to_string().contains("Invalid syntax"), "wrong msg {e:?}");
+    }
+}

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" }
 

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<String>
+ADDED: ArtiPathComponent is AsRef<str>
+ADDED: ArtiPathComponent is Hash, Eq, PartialEq, Ord, PartialOrd
+ADDED: ArtiPathComponent is Serialize, Deserialize

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<Self> {
         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<String> for ArtiPathComponent {
+    type Error = crate::Error; // TODO HSS should be bespoke error type
+    fn try_from(s: String) -> Result<ArtiPathComponent> {
+        Self::new(s)
+    }
+}
+
+impl AsRef<str> 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::<T>(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:?}");
+    }
 }