From 2d9d16aabc34a06596a98e1309a3a7b6f82c01f7 Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Mon, 27 Mar 2023 18:02:32 +0100 Subject: [PATCH 1/5] Stop requiring the caller to supply `AuthClient`s. `AuthClient`s were originally meant to represent parsed `auth-client` lines. In !1070, this struct was repurposed for representing individual authorized clients in the HS descriptor encoder. However, hidden services will likely use a list of public keys to represent the authorized clients rather than a list of `AuthClient`s, as the information from an `AuthClient` (`client_id`, `iv`, `encrypted_cookie`) likely won't be immediately available to the hidden service. This change updates the HS descriptor encoder to represent authorized clients as a list of `curve25519::PublicKey`s. As such, it is now the responsibility of the encoder to create the `client_id`, `iv`, and `encrypted_cookie` using the available keys, the unencrypted descriptor cookie, and HS subcredential. Signed-off-by: Gabriela Moldovan --- crates/tor-hscrypto/src/pk.rs | 15 ++ crates/tor-netdoc/src/doc/hsdesc/build.rs | 30 ++-- .../tor-netdoc/src/doc/hsdesc/build/middle.rs | 139 +++++++++++------- crates/tor-netdoc/src/doc/hsdesc/desc_enc.rs | 37 +++++ crates/tor-netdoc/src/doc/hsdesc/middle.rs | 43 ++---- 5 files changed, 164 insertions(+), 100 deletions(-) diff --git a/crates/tor-hscrypto/src/pk.rs b/crates/tor-hscrypto/src/pk.rs index 39258cec0..be2591863 100644 --- a/crates/tor-hscrypto/src/pk.rs +++ b/crates/tor-hscrypto/src/pk.rs @@ -390,6 +390,8 @@ impl HsBlindIdSecretKey { } /// A blinded Ed25519 keypair. +/// +/// TODO hs: should we extend `define_pk_keypair` to also define a *Keypair struct? #[allow(clippy::exhaustive_structs)] #[derive(Debug)] pub struct HsBlindKeypair { @@ -475,6 +477,19 @@ define_pk_keypair! { pub struct HsSvcDescEncKey(curve25519::PublicKey) / HsSvcDescEncSecretKey(curve25519::StaticSecret); } +/// An ephemeral x25519 keypair, generated by an onion service +/// and used to for onion service encryption. +/// +/// TODO hs: should we extend `define_pk_keypair` to also define a *Keypair struct? +#[allow(clippy::exhaustive_structs)] +#[derive(Debug)] +pub struct HsSvcDescEncKeypair { + /// The public part of the key. + pub public: HsSvcDescEncKey, + /// The secret part of the key. + pub secret: HsSvcDescEncSecretKey, +} + #[cfg(test)] mod test { // @@ begin test lint list maintained by maint/add_warning @@ diff --git a/crates/tor-netdoc/src/doc/hsdesc/build.rs b/crates/tor-netdoc/src/doc/hsdesc/build.rs index 475290671..a59998f36 100644 --- a/crates/tor-netdoc/src/doc/hsdesc/build.rs +++ b/crates/tor-netdoc/src/doc/hsdesc/build.rs @@ -9,8 +9,9 @@ use crate::NetdocBuilder; use rand::{CryptoRng, RngCore}; use tor_bytes::EncodeError; use tor_error::into_bad_api_usage; -use tor_hscrypto::pk::{HsBlindKeypair, HsSvcDescEncKey}; +use tor_hscrypto::pk::{HsBlindKeypair, HsSvcDescEncKeypair}; use tor_hscrypto::{RevisionCounter, Subcredential}; +use tor_llcrypto::pk::curve25519; use tor_llcrypto::pk::ed25519::{self, Ed25519PublicKey}; use tor_units::IntegerMinutes; @@ -25,7 +26,6 @@ use self::middle::HsDescMiddle; use self::outer::HsDescOuter; use super::desc_enc::{HsDescEncNonce, HsDescEncryption, HS_DESC_ENC_NONCE_LEN}; -use super::middle::AuthClient; /// A builder for encoding hidden service descriptors. /// @@ -55,6 +55,9 @@ struct HsDesc<'a> { intro_enc_key_cert_expiry: SystemTime, /// Client authorization parameters, if client authentication is enabled. If set to `None`, /// client authentication is disabled. + /// + /// If client authorization is disabled, the resulting middle document will contain a single + /// `auth-client` line populated with random values. client_auth: Option<&'a ClientAuth>, /// The lifetime of this descriptor, in minutes. /// @@ -71,28 +74,20 @@ struct HsDesc<'a> { /// Client authorization parameters. // TODO HS this ought to go away from the public API (see TODO below), or use a builder? -#[derive(Clone, Debug)] +#[derive(Debug, Builder)] +#[builder(pattern = "owned")] pub struct ClientAuth { /// The ephemeral x25519 ephemeral public key generated by the hidden service /// (`KP_hss_desc_enc`). - pub ephemeral_key: HsSvcDescEncKey, - /// One or more authorized clients, and the key exchange information that - /// they use to compute shared keys for decrypting the encryption layer. - /// - /// If client authorization is disabled (i.e. this array is empty), the resulting middle - /// document will contain a single auth-client client populated with random values. - /// - /// TODO hs: currently it is the responsibility of the hidden service to create an `AuthClient` - /// for each authorized client. Instead of using `Vec` here, it would be better to - /// just have a list of public keys (one for each authorized client), and let - /// `HsDescMiddle` create the underlying `AuthClient`. - pub auth_clients: Vec, - /// The value of `N_hs_desc_enc` descriptor_cookie key generated by the hidden service. + ephemeral_key: HsSvcDescEncKeypair, + /// One or more authorized clients. + auth_clients: Vec, + /// The `N_hs_desc_enc` descriptor_cookie key generated by the hidden service. /// /// TODO hs: Do we even need this field? This is presumed to be randomly generated for each /// descriptor by the hidden service, but since it's random, we might as well let the /// descriptor builder generate it. - pub descriptor_cookie: [u8; HS_DESC_ENC_NONCE_LEN], + descriptor_cookie: [u8; HS_DESC_ENC_NONCE_LEN], } impl<'a> NetdocBuilder for HsDescBuilder<'a> { @@ -137,6 +132,7 @@ impl<'a> NetdocBuilder for HsDescBuilder<'a> { // "superencrypted" field. let middle_plaintext = HsDescMiddle { client_auth: hs_desc.client_auth, + subcredential: hs_desc.subcredential, encrypted: inner_encrypted, } .build_sign(rng)?; diff --git a/crates/tor-netdoc/src/doc/hsdesc/build/middle.rs b/crates/tor-netdoc/src/doc/hsdesc/build/middle.rs index 6ee14b770..6415d58ff 100644 --- a/crates/tor-netdoc/src/doc/hsdesc/build/middle.rs +++ b/crates/tor-netdoc/src/doc/hsdesc/build/middle.rs @@ -4,15 +4,16 @@ //! not meant to be used directly. Hidden services will use `HsDescBuilder` to build and encode //! hidden service descriptors. -use std::borrow::Cow; - use crate::build::NetdocEncoder; use crate::doc::hsdesc::build::ClientAuth; -use crate::doc::hsdesc::desc_enc::{HS_DESC_CLIENT_ID_LEN, HS_DESC_ENC_NONCE_LEN, HS_DESC_IV_LEN}; +use crate::doc::hsdesc::desc_enc::{ + build_descriptor_cookie_key, HS_DESC_CLIENT_ID_LEN, HS_DESC_ENC_NONCE_LEN, HS_DESC_IV_LEN, +}; use crate::doc::hsdesc::middle::{AuthClient, HsMiddleKwd, HS_DESC_AUTH_TYPE}; use crate::NetdocBuilder; use tor_bytes::EncodeError; +use tor_hscrypto::Subcredential; use tor_llcrypto::pk::curve25519::{EphemeralSecret, PublicKey}; use tor_llcrypto::util::ct::CtByteArray; @@ -27,6 +28,8 @@ pub(super) struct HsDescMiddle<'a> { /// Client authorization parameters, if client authentication is enabled. If set to `None`, /// client authentication is disabled. pub(super) client_auth: Option<&'a ClientAuth>, + /// The "subcredential" of the onion service. + pub(super) subcredential: Subcredential, /// The (encrypted) inner document of the onion service descriptor. /// /// The `encrypted` field is created by encrypting an @@ -37,54 +40,77 @@ pub(super) struct HsDescMiddle<'a> { impl<'a> NetdocBuilder for HsDescMiddle<'a> { fn build_sign(self, rng: &mut R) -> Result { + use cipher::{KeyIvInit, StreamCipher}; + use tor_llcrypto::cipher::aes::Aes256Ctr as Cipher; use HsMiddleKwd::*; let HsDescMiddle { client_auth, + subcredential, encrypted, } = self; let mut encoder = NetdocEncoder::new(); - let (ephemeral_key, auth_clients): (_, Cow>) = match client_auth { - Some(client_auth) if client_auth.auth_clients.is_empty() => { - return Err(tor_error::bad_api_usage!( - "client authentication is enabled, but there are no authorized clients" - ) - .into()); - } - Some(client_auth) => { - // Client auth is enabled. - ( - *client_auth.ephemeral_key, - Cow::Borrowed(&client_auth.auth_clients), - ) - } - None => { - // Generate a single client-auth line filled with random values for client-id, - // iv, and encrypted-cookie. - let dummy_auth_client = AuthClient { - client_id: CtByteArray::from(rng.gen::<[u8; HS_DESC_CLIENT_ID_LEN]>()), - iv: rng.gen::<[u8; HS_DESC_IV_LEN]>(), - encrypted_cookie: rng.gen::<[u8; HS_DESC_ENC_NONCE_LEN]>(), - }; + let (ephemeral_key, auth_clients): (_, Box>) = + match client_auth { + Some(client_auth) if client_auth.auth_clients.is_empty() => { + return Err(tor_error::bad_api_usage!( + "client authentication is enabled, but there are no authorized clients" + ) + .into()); + } + Some(client_auth) => { + // Client auth is enabled. + let auth_clients = client_auth.auth_clients.iter().map(|client| { + let (client_id, cookie_key) = build_descriptor_cookie_key( + client_auth.ephemeral_key.secret.as_ref(), + client, + &subcredential, + ); - // As per section 2.5.1.2. of rend-spec-v3, if client auth is disabled, we need to - // generate some fake data for the desc-auth-ephemeral-key and auth-client fields. - let secret = EphemeralSecret::new(rng); - let dummy_ephemeral_key = PublicKey::from(&secret); + // Encrypt the descriptor cookie with the public key of the client. + let mut encrypted_cookie = client_auth.descriptor_cookie; + let iv = rng.gen::<[u8; HS_DESC_IV_LEN]>(); + let mut cipher = Cipher::new(&cookie_key.into(), &iv.into()); + cipher.apply_keystream(&mut encrypted_cookie); - // TODO hs: Remove useless vec![] allocation. - (dummy_ephemeral_key, Cow::Owned(vec![dummy_auth_client])) - } - }; + AuthClient { + client_id, + iv, + encrypted_cookie, + } + }); + + (*client_auth.ephemeral_key.public, Box::new(auth_clients)) + } + None => { + // Generate a single client-auth line filled with random values for client-id, + // iv, and encrypted-cookie. + let dummy_auth_client = AuthClient { + client_id: CtByteArray::from(rng.gen::<[u8; HS_DESC_CLIENT_ID_LEN]>()), + iv: rng.gen::<[u8; HS_DESC_IV_LEN]>(), + encrypted_cookie: rng.gen::<[u8; HS_DESC_ENC_NONCE_LEN]>(), + }; + + // As per section 2.5.1.2. of rend-spec-v3, if client auth is disabled, we need to + // generate some fake data for the desc-auth-ephemeral-key and auth-client fields. + let secret = EphemeralSecret::new(rng); + let dummy_ephemeral_key = PublicKey::from(&secret); + + ( + dummy_ephemeral_key, + Box::new(std::iter::once(dummy_auth_client)), + ) + } + }; encoder.item(DESC_AUTH_TYPE).arg(&HS_DESC_AUTH_TYPE); encoder .item(DESC_AUTH_EPHEMERAL_KEY) .arg(&Base64::encode_string(ephemeral_key.as_bytes())); - for auth_client in &*auth_clients { + for auth_client in auth_clients { encoder .item(AUTH_CLIENT) .arg(&Base64::encode_string(&*auth_client.client_id)) @@ -115,7 +141,11 @@ mod test { create_curve25519_pk, expect_bug, TEST_DESCRIPTOR_COOKIE, }; use crate::doc::hsdesc::build::ClientAuth; + use crate::doc::hsdesc::test::TEST_SUBCREDENTIAL; use tor_basic_utils::test_rng::Config; + use tor_hscrypto::pk::HsSvcDescEncKeypair; + use tor_llcrypto::pk::curve25519; + use tor_llcrypto::util::rand_compat::RngCompatExt; // Some dummy bytes, not actually encrypted. const TEST_ENCRYPTED_VALUE: &[u8] = &[1, 2, 3, 4]; @@ -124,6 +154,7 @@ mod test { fn middle_hsdesc_encoding_no_client_auth() { let hs_desc = HsDescMiddle { client_auth: None, + subcredential: TEST_SUBCREDENTIAL.into(), encrypted: TEST_ENCRYPTED_VALUE.into(), } .build_sign(&mut Config::Deterministic.into_rng()) @@ -144,17 +175,25 @@ AQIDBA== #[test] fn middle_hsdesc_encoding_with_bad_client_auth() { + let mut rng = Config::Deterministic.into_rng().rng_compat(); + let secret = curve25519::StaticSecret::new(&mut rng); + let public = curve25519::PublicKey::from(&secret).into(); + let client_auth = ClientAuth { - ephemeral_key: create_curve25519_pk(&mut Config::Deterministic.into_rng()).into(), + ephemeral_key: HsSvcDescEncKeypair { + public, + secret: secret.into(), + }, auth_clients: vec![], descriptor_cookie: TEST_DESCRIPTOR_COOKIE, }; let err = HsDescMiddle { client_auth: Some(&client_auth), + subcredential: TEST_SUBCREDENTIAL.into(), encrypted: TEST_ENCRYPTED_VALUE.into(), } - .build_sign(&mut Config::Deterministic.into_rng()) + .build_sign(&mut rng) .unwrap_err(); assert!(expect_bug(err) @@ -163,28 +202,28 @@ AQIDBA== #[test] fn middle_hsdesc_encoding_client_auth() { + let mut rng = Config::Deterministic.into_rng().rng_compat(); // 2 authorized clients let auth_clients = vec![ - AuthClient { - client_id: [2; 8].into(), - iv: [2; 16], - encrypted_cookie: [3; 16], - }, - AuthClient { - client_id: [4; 8].into(), - iv: [5; 16], - encrypted_cookie: [6; 16], - }, + create_curve25519_pk(&mut rng), + create_curve25519_pk(&mut rng), ]; + let secret = curve25519::StaticSecret::new(&mut rng); + let public = curve25519::PublicKey::from(&secret).into(); + let client_auth = ClientAuth { - ephemeral_key: create_curve25519_pk(&mut Config::Deterministic.into_rng()).into(), + ephemeral_key: HsSvcDescEncKeypair { + public, + secret: secret.into(), + }, auth_clients, descriptor_cookie: TEST_DESCRIPTOR_COOKIE, }; let hs_desc = HsDescMiddle { client_auth: Some(&client_auth), + subcredential: TEST_SUBCREDENTIAL.into(), encrypted: TEST_ENCRYPTED_VALUE.into(), } .build_sign(&mut Config::Deterministic.into_rng()) @@ -193,9 +232,9 @@ AQIDBA== assert_eq!( hs_desc, r#"desc-auth-type x25519 -desc-auth-ephemeral-key HWIigEAdcOgqgHPDFmzhhkeqvYP/GcMT2fKb5JY6ey8= -auth-client AgICAgICAgI= AgICAgICAgICAgICAgICAg== AwMDAwMDAwMDAwMDAwMDAw== -auth-client BAQEBAQEBAQ= BQUFBQUFBQUFBQUFBQUFBQ== BgYGBgYGBgYGBgYGBgYGBg== +desc-auth-ephemeral-key 9Upi9XNWyqx3ZwHeQ5r3+Dh116k+C4yHeE9BcM68HDc= +auth-client pxfSbhBMPw0= F+Z6EDfG7ofsQhdG2VKjNQ== 2ZH4k2l+bSv+pjhwHQ3pTg== +auth-client DV7nt+CDOno= bRgLOvpjbo2k21IjKIJqFA== fv8A2pPllndWdFwFDPaynw== encrypted -----BEGIN MESSAGE----- AQIDBA== diff --git a/crates/tor-netdoc/src/doc/hsdesc/desc_enc.rs b/crates/tor-netdoc/src/doc/hsdesc/desc_enc.rs index 4f74c34a3..cbfe90dc6 100644 --- a/crates/tor-netdoc/src/doc/hsdesc/desc_enc.rs +++ b/crates/tor-netdoc/src/doc/hsdesc/desc_enc.rs @@ -11,6 +11,8 @@ use arrayref::array_ref; use cipher::{KeyIvInit, StreamCipher}; use digest::{ExtendableOutput, FixedOutput, Update, XofReader}; use rand::{CryptoRng, Rng}; +use tor_llcrypto::pk::curve25519::PublicKey; +use tor_llcrypto::pk::curve25519::StaticSecret; use tor_llcrypto::util::ct::CtByteArray; use zeroize::Zeroizing as Z; @@ -187,6 +189,41 @@ impl<'a> HsDescEncryption<'a> { #[error("Unable to decrypt onion service descriptor.")] pub struct DecryptionError {} +/// Create the CLIENT-ID and COOKIE-KEY required for hidden service client auth. +/// +/// This is used by HS clients to decrypt the descriptor cookie from the onion service descriptor, +/// and by HS services to build the client-auth sections of descriptors. +/// +/// Section 2.5.1.2. of rend-spec-v3 says: +/// ```text +/// SECRET_SEED = x25519(hs_y, client_X) +/// = x25519(client_y, hs_X) +/// KEYS = KDF(N_hs_subcred | SECRET_SEED, 40) +/// CLIENT-ID = first 8 bytes of KEYS +/// COOKIE-KEY = last 32 bytes of KEYS +/// +/// Where: +/// hs_{X,y} = K{P,S}_hss_desc_enc +/// client_{X,Y} = K{P,S}_hsc_desc_enc +/// ``` +pub(crate) fn build_descriptor_cookie_key( + our_secret_key: &StaticSecret, + their_public_key: &PublicKey, + subcredential: &Subcredential, +) -> (CtByteArray<8>, [u8; 32]) { + let secret_seed = our_secret_key.diffie_hellman(their_public_key); + let mut kdf = KDF::default(); + kdf.update(subcredential.as_ref()); + kdf.update(secret_seed.as_bytes()); + let mut keys = kdf.finalize_xof(); + let mut client_id = CtByteArray::from([0_u8; 8]); + let mut cookie_key = [0_u8; 32]; + keys.read(client_id.as_mut()); + keys.read(&mut cookie_key); + + (client_id, cookie_key) +} + #[cfg(test)] mod test { // @@ begin test lint list maintained by maint/add_warning @@ diff --git a/crates/tor-netdoc/src/doc/hsdesc/middle.rs b/crates/tor-netdoc/src/doc/hsdesc/middle.rs index 6dd08e872..c6b967be2 100644 --- a/crates/tor-netdoc/src/doc/hsdesc/middle.rs +++ b/crates/tor-netdoc/src/doc/hsdesc/middle.rs @@ -1,12 +1,12 @@ //! Handle the middle document of an onion service descriptor. -use digest::XofReader; use once_cell::sync::Lazy; use tor_hscrypto::pk::{HsBlindId, HsClientDescEncSecretKey, HsSvcDescEncKey}; use tor_hscrypto::{RevisionCounter, Subcredential}; use tor_llcrypto::pk::curve25519; use tor_llcrypto::util::ct::CtByteArray; +use crate::doc::hsdesc::desc_enc::build_descriptor_cookie_key; use crate::parse::tokenize::{Item, NetDocReader}; use crate::parse::{keyword::Keyword, parser::SectionRules}; use crate::types::misc::B64; @@ -88,36 +88,13 @@ impl HsDescMiddle { ks_hsc_desc_enc: &HsClientDescEncSecretKey, ) -> Option { use cipher::{KeyIvInit, StreamCipher}; - use digest::{ExtendableOutput, Update}; use tor_llcrypto::cipher::aes::Aes256Ctr as Cipher; - use tor_llcrypto::d::Shake256 as KDF; - - // Perform a diffie hellman handshake using `KS_hsc_desc_enc` and `KP_hss_desc_enc`, - // and use it to find our client_id and cookie_key. - // - // The spec says: - // - // SECRET_SEED = x25519(hs_y, client_X) - // = x25519(client_y, hs_X) - // KEYS = KDF(N_hs_subcred | SECRET_SEED, 40) - // CLIENT-ID = fist 8 bytes of KEYS - // COOKIE-KEY = last 32 bytes of KEYS - // - // Where: - // hs_{X,y} = K{P,S}_hss_desc_enc - // client_{X,Y} = K{P,S}_hsc_desc_enc - let secret_seed = ks_hsc_desc_enc - .as_ref() - .diffie_hellman(&self.svc_desc_enc_key); - let mut kdf = KDF::default(); - kdf.update(subcredential.as_ref()); - kdf.update(secret_seed.as_bytes()); - let mut keys = kdf.finalize_xof(); - let mut client_id = CtByteArray::from([0_u8; 8]); - let mut cookie_key = [0_u8; 32]; - keys.read(client_id.as_mut()); - keys.read(&mut cookie_key); + let (client_id, cookie_key) = build_descriptor_cookie_key( + ks_hsc_desc_enc.as_ref(), + &self.svc_desc_enc_key, + subcredential, + ); // See whether there is any matching client_id in self.auth_ids. // TODO HS: Perhaps we should use `tor_proto::util::ct::lookup`. We would // have to put it in a lower level module. @@ -137,15 +114,15 @@ impl HsDescMiddle { /// Information that a single authorized client can use to decrypt the onion /// service descriptor. #[derive(Debug, Clone)] -pub struct AuthClient { +pub(super) struct AuthClient { /// A check field that clients can use to see if this [`AuthClient`] entry corresponds to a key they hold. /// /// This is the first part of the `auth-client` line. - pub(crate) client_id: CtByteArray, + pub(super) client_id: CtByteArray, /// An IV used to decrypt `encrypted_cookie`. /// /// This is the second item on the `auth-client` line. - pub(crate) iv: [u8; HS_DESC_IV_LEN], + pub(super) iv: [u8; HS_DESC_IV_LEN], /// An encrypted value used to find the descriptor cookie `N_hs_desc_enc`, /// which in turn is /// needed to decrypt the [HsDescMiddle]'s `encrypted_body`. @@ -153,7 +130,7 @@ pub struct AuthClient { /// This is the third item on the `auth-client` line. When decrypted, it /// reveals a `DescEncEncryptionCookie` (`N_hs_desc_enc`, not yet so named /// in the spec). - pub(crate) encrypted_cookie: [u8; HS_DESC_ENC_NONCE_LEN], + pub(super) encrypted_cookie: [u8; HS_DESC_ENC_NONCE_LEN], } impl AuthClient { From 8aa930a645b2d5275628e0c874da66799bb52612 Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Tue, 28 Mar 2023 14:35:42 +0100 Subject: [PATCH 2/5] Use constants instead of magic numbers. Signed-off-by: Gabriela Moldovan --- crates/tor-netdoc/src/doc/hsdesc/build.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/crates/tor-netdoc/src/doc/hsdesc/build.rs b/crates/tor-netdoc/src/doc/hsdesc/build.rs index a59998f36..0fec73733 100644 --- a/crates/tor-netdoc/src/doc/hsdesc/build.rs +++ b/crates/tor-netdoc/src/doc/hsdesc/build.rs @@ -276,6 +276,11 @@ mod test { #[test] fn encode_decode() { + const CREATE2_FORMATS: &[u32] = &[1, 2]; + const LIFETIME_MINS: u16 = 100; + const REVISION_COUNT: u64 = 2; + const CERT_EXPIRY_SECS: u64 = 60 * 60; + let mut rng = Config::Deterministic.into_rng().rng_compat(); // The identity keypair of the hidden service. let hs_id = ed25519::Keypair::generate(&mut rng); @@ -292,8 +297,7 @@ mod test { .unwrap(); let blinded_id = HsBlindKeypair { public, secret }; - let create2_formats = &[1, 2]; - let expiry = SystemTime::now() + Duration::from_secs(60 * 60); + let expiry = SystemTime::now() + Duration::from_secs(CERT_EXPIRY_SECS); let mut rng = Config::Deterministic.into_rng().rng_compat(); let intro_points = vec![IntroPointDesc { link_specifiers: vec![LinkSpec::OrPort(Ipv4Addr::LOCALHOST.into(), 9999)], @@ -307,15 +311,15 @@ mod test { .blinded_id(&blinded_id) .hs_desc_sign(&hs_desc_sign) .hs_desc_sign_cert_expiry(expiry) - .create2_formats(create2_formats) + .create2_formats(CREATE2_FORMATS) .auth_required(None) .is_single_onion_service(true) .intro_points(&intro_points) .intro_auth_key_cert_expiry(expiry) .intro_enc_key_cert_expiry(expiry) .client_auth(None) - .lifetime(100.into()) - .revision_counter(2.into()) + .lifetime(LIFETIME_MINS.into()) + .revision_counter(REVISION_COUNT.into()) .subcredential(subcredential) .build_sign(&mut Config::Deterministic.into_rng()) .unwrap(); @@ -344,7 +348,7 @@ mod test { .hs_desc_sign_cert_expiry(expiry) // create2_formats is hard-coded rather than extracted from desc, because // create2_formats is ignored while parsing - .create2_formats(create2_formats) + .create2_formats(CREATE2_FORMATS) .auth_required(None) .is_single_onion_service(desc.is_single_onion_service) .intro_points(&intro_points) From cd66781577f8373155c77ad7755b888bc38f86d4 Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Tue, 28 Mar 2023 14:16:56 +0100 Subject: [PATCH 3/5] Add an encode-decode test for descriptors with client auth. This adds a test for an `encode -> decode -> encode` flow for a hidden service descriptor with client authorization enabled. Signed-off-by: Gabriela Moldovan --- crates/tor-netdoc/src/doc/hsdesc/build.rs | 123 +++++++++++++++++----- 1 file changed, 98 insertions(+), 25 deletions(-) diff --git a/crates/tor-netdoc/src/doc/hsdesc/build.rs b/crates/tor-netdoc/src/doc/hsdesc/build.rs index 0fec73733..f78e0fccb 100644 --- a/crates/tor-netdoc/src/doc/hsdesc/build.rs +++ b/crates/tor-netdoc/src/doc/hsdesc/build.rs @@ -31,7 +31,7 @@ use super::desc_enc::{HsDescEncNonce, HsDescEncryption, HS_DESC_ENC_NONCE_LEN}; /// /// TODO hs: a comprehensive usage example. #[derive(Builder)] -#[builder(public, derive(Debug), pattern = "owned", build_fn(vis = ""))] +#[builder(public, derive(Debug, Clone), pattern = "owned", build_fn(vis = ""))] struct HsDesc<'a> { /// The blinded hidden service signing keys used to sign descriptor signing keys /// (KP_hs_blind_id, KS_hs_blind_id). @@ -222,10 +222,10 @@ mod test { use std::time::Duration; use super::*; - use crate::doc::hsdesc::{EncryptedHsDesc, HsDesc as HsDescDecoder}; + use crate::doc::hsdesc::{EncryptedHsDesc, HsDesc as ParsedHsDesc}; use tor_basic_utils::test_rng::Config; use tor_checkable::{SelfSigned, Timebound}; - use tor_hscrypto::pk::HsIdSecretKey; + use tor_hscrypto::pk::{HsClientDescEncKey, HsClientDescEncSecretKey, HsIdSecretKey}; use tor_hscrypto::time::TimePeriod; use tor_linkspec::LinkSpec; use tor_llcrypto::pk::curve25519; @@ -274,6 +274,32 @@ mod test { (&ephemeral_key).into() } + /// Parse the specified hidden service descriptor. + fn parse_hsdesc( + unparsed_desc: &str, + blinded_pk: ed25519::PublicKey, + subcredential: &Subcredential, + hsc_desc_enc: Option<(&HsClientDescEncKey, &HsClientDescEncSecretKey)>, + ) -> ParsedHsDesc { + const TIMESTAMP: &str = "2023-01-23T15:00:00Z"; + + let id = ed25519::Ed25519Identity::from(blinded_pk); + let enc_desc: EncryptedHsDesc = ParsedHsDesc::parse(unparsed_desc, &id.into()) + .unwrap() + .check_signature() + .unwrap() + .check_valid_at(&humantime::parse_rfc3339(TIMESTAMP).unwrap()) + .unwrap(); + + enc_desc + .decrypt(subcredential, hsc_desc_enc) + .unwrap() + .check_valid_at(&humantime::parse_rfc3339(TIMESTAMP).unwrap()) + .unwrap() + .check_signature() + .unwrap() + } + #[test] fn encode_decode() { const CREATE2_FORMATS: &[u32] = &[1, 2]; @@ -297,6 +323,7 @@ mod test { .unwrap(); let blinded_id = HsBlindKeypair { public, secret }; + let id = ed25519::Ed25519Identity::from(blinded_id.public_key()); let expiry = SystemTime::now() + Duration::from_secs(CERT_EXPIRY_SECS); let mut rng = Config::Deterministic.into_rng().rng_compat(); let intro_points = vec![IntroPointDesc { @@ -306,8 +333,7 @@ mod test { svc_ntor_key: create_curve25519_pk(&mut rng).into(), }]; - // Build and encode a new descriptor: - let encoded_desc = HsDescBuilder::default() + let builder = HsDescBuilder::default() .blinded_id(&blinded_id) .hs_desc_sign(&hs_desc_sign) .hs_desc_sign_cert_expiry(expiry) @@ -320,28 +346,25 @@ mod test { .client_auth(None) .lifetime(LIFETIME_MINS.into()) .revision_counter(REVISION_COUNT.into()) - .subcredential(subcredential) + .subcredential(subcredential); + + // Build and encode a new descriptor (cloning `builder` because it's needed later, when we + // test if client auth works): + let encoded_desc = builder + .clone() .build_sign(&mut Config::Deterministic.into_rng()) .unwrap(); - let id = ed25519::Ed25519Identity::from(blinded_id.public_key()); - // Now decode it - let enc_desc: EncryptedHsDesc = HsDescDecoder::parse(&encoded_desc, &id.into()) - .unwrap() - .check_signature() - .unwrap() - .check_valid_at(&humantime::parse_rfc3339("2023-01-23T15:00:00Z").unwrap()) - .unwrap(); + // Now decode it... + let desc = parse_hsdesc( + encoded_desc.as_str(), + *blinded_id.public, + &subcredential, + None, /* No client auth */ + ); - let desc = enc_desc - .decrypt(&subcredential, None) - .unwrap() - .check_valid_at(&humantime::parse_rfc3339("2023-01-23T15:00:00Z").unwrap()) - .unwrap() - .check_signature() - .unwrap(); - - // Now encode it again and check the result is identical to the original + // ...and build a new descriptor using the information from the parsed descriptor, + // asserting that the resulting descriptor is identical to the original. let reencoded_desc = HsDescBuilder::default() .blinded_id(&blinded_id) .hs_desc_sign(&hs_desc_sign) @@ -362,7 +385,57 @@ mod test { .unwrap(); assert_eq!(&*encoded_desc, &*reencoded_desc); - } - // TODO hs: encode a descriptor with client auth enabled + // The same test, this time with client auth enabled (with a single authorized client): + let client_skey: HsClientDescEncSecretKey = curve25519::StaticSecret::new(&mut rng).into(); + let client_pkey: HsClientDescEncKey = + curve25519::PublicKey::from(client_skey.as_ref()).into(); + let auth_clients = vec![*client_pkey]; + + let secret = curve25519::StaticSecret::new(&mut rng); + let client_auth = ClientAuth { + ephemeral_key: HsSvcDescEncKeypair { + public: curve25519::PublicKey::from(&secret).into(), + secret: secret.into(), + }, + auth_clients, + descriptor_cookie: TEST_DESCRIPTOR_COOKIE, + }; + + let encoded_desc = builder + .client_auth(Some(&client_auth)) + .build_sign(&mut Config::Deterministic.into_rng()) + .unwrap(); + + // Now decode it... + let desc = parse_hsdesc( + encoded_desc.as_str(), + *blinded_id.public, + &subcredential, + Some((&client_pkey, &client_skey)), /* With client auth */ + ); + + // ...and build a new descriptor using the information from the parsed descriptor, + // asserting that the resulting descriptor is identical to the original. + let reencoded_desc = HsDescBuilder::default() + .blinded_id(&blinded_id) + .hs_desc_sign(&hs_desc_sign) + .hs_desc_sign_cert_expiry(expiry) + // create2_formats is hard-coded rather than extracted from desc, because + // create2_formats is ignored while parsing + .create2_formats(CREATE2_FORMATS) + .auth_required(None) + .is_single_onion_service(desc.is_single_onion_service) + .intro_points(&intro_points) + .intro_auth_key_cert_expiry(expiry) + .intro_enc_key_cert_expiry(expiry) + .client_auth(Some(&client_auth)) + .lifetime(desc.idx_info.lifetime) + .revision_counter(desc.idx_info.revision) + .subcredential(subcredential) + .build_sign(&mut Config::Deterministic.into_rng()) + .unwrap(); + + assert_eq!(&*encoded_desc, &*reencoded_desc); + } } From 94a4d2ea43b5a74c21c82cbc3224cda96f23f3e7 Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Fri, 31 Mar 2023 12:20:40 +0100 Subject: [PATCH 4/5] Remove unnecessary test constant. It's not really needed, it can just be generated at (test) runtime. Signed-off-by: Gabriela Moldovan --- crates/tor-netdoc/src/doc/hsdesc/build.rs | 10 +++------- crates/tor-netdoc/src/doc/hsdesc/build/middle.rs | 12 +++++------- 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/crates/tor-netdoc/src/doc/hsdesc/build.rs b/crates/tor-netdoc/src/doc/hsdesc/build.rs index f78e0fccb..ae3fa1203 100644 --- a/crates/tor-netdoc/src/doc/hsdesc/build.rs +++ b/crates/tor-netdoc/src/doc/hsdesc/build.rs @@ -232,12 +232,8 @@ mod test { use tor_llcrypto::pk::keymanip::ExpandedSecretKey; use tor_llcrypto::util::rand_compat::RngCompatExt; - // TODO: move the test helpers and constants to a separate module and make them more broadly - // available if necessary. - - // Not a real cookie, just a bunch of ones. - pub(super) const TEST_DESCRIPTOR_COOKIE: [u8; HS_DESC_ENC_NONCE_LEN] = - [1; HS_DESC_ENC_NONCE_LEN]; + // TODO: move the test helpers to a separate module and make them more broadly available if + // necessary. /// Expect `err` to be a `Bug`, and return its string representation. /// @@ -399,7 +395,7 @@ mod test { secret: secret.into(), }, auth_clients, - descriptor_cookie: TEST_DESCRIPTOR_COOKIE, + descriptor_cookie: rand::Rng::gen::<[u8; HS_DESC_ENC_NONCE_LEN]>(&mut rng), }; let encoded_desc = builder diff --git a/crates/tor-netdoc/src/doc/hsdesc/build/middle.rs b/crates/tor-netdoc/src/doc/hsdesc/build/middle.rs index 6415d58ff..975c13fc9 100644 --- a/crates/tor-netdoc/src/doc/hsdesc/build/middle.rs +++ b/crates/tor-netdoc/src/doc/hsdesc/build/middle.rs @@ -137,9 +137,7 @@ mod test { //! use super::*; - use crate::doc::hsdesc::build::test::{ - create_curve25519_pk, expect_bug, TEST_DESCRIPTOR_COOKIE, - }; + use crate::doc::hsdesc::build::test::{create_curve25519_pk, expect_bug}; use crate::doc::hsdesc::build::ClientAuth; use crate::doc::hsdesc::test::TEST_SUBCREDENTIAL; use tor_basic_utils::test_rng::Config; @@ -185,7 +183,7 @@ AQIDBA== secret: secret.into(), }, auth_clients: vec![], - descriptor_cookie: TEST_DESCRIPTOR_COOKIE, + descriptor_cookie: rand::Rng::gen::<[u8; HS_DESC_ENC_NONCE_LEN]>(&mut rng), }; let err = HsDescMiddle { @@ -218,7 +216,7 @@ AQIDBA== secret: secret.into(), }, auth_clients, - descriptor_cookie: TEST_DESCRIPTOR_COOKIE, + descriptor_cookie: rand::Rng::gen::<[u8; HS_DESC_ENC_NONCE_LEN]>(&mut rng), }; let hs_desc = HsDescMiddle { @@ -233,8 +231,8 @@ AQIDBA== hs_desc, r#"desc-auth-type x25519 desc-auth-ephemeral-key 9Upi9XNWyqx3ZwHeQ5r3+Dh116k+C4yHeE9BcM68HDc= -auth-client pxfSbhBMPw0= F+Z6EDfG7ofsQhdG2VKjNQ== 2ZH4k2l+bSv+pjhwHQ3pTg== -auth-client DV7nt+CDOno= bRgLOvpjbo2k21IjKIJqFA== fv8A2pPllndWdFwFDPaynw== +auth-client pxfSbhBMPw0= F+Z6EDfG7ofsQhdG2VKjNQ== fEursUD9Bj5Q9mFP8sIddA== +auth-client DV7nt+CDOno= bRgLOvpjbo2k21IjKIJqFA== 2yVT+Lpm/WL4JAU64zlGpQ== encrypted -----BEGIN MESSAGE----- AQIDBA== From 3d0bf0d6ab3ee6f6248055eeaf060102c5895a76 Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Fri, 31 Mar 2023 12:25:26 +0100 Subject: [PATCH 5/5] Generate a new KP_hss_desc_enc keypair for each new descriptor. Previously, to build descriptors for hidden services with client auth enabled, in addition to the list of authorized clients, users of `HsDescBuilder` were required to also provide a descriptor encryption keypair and a descriptor cookie. This was potentially dangerous and/or error-prone, because the ephemeral encryption key and the descriptor cookie are expected to be randomly generated and unique for each descriptor. This change makes `ClientAuth` private to the `hsdesc::build` module and updates `HsDescBuilder` to build `ClientAuth`s internally. Users now only need to provide the list of authorized client public keys. Signed-off-by: Gabriela Moldovan --- crates/tor-netdoc/src/doc/hsdesc/build.rs | 78 ++++++++++++------- .../tor-netdoc/src/doc/hsdesc/build/middle.rs | 6 +- 2 files changed, 51 insertions(+), 33 deletions(-) diff --git a/crates/tor-netdoc/src/doc/hsdesc/build.rs b/crates/tor-netdoc/src/doc/hsdesc/build.rs index ae3fa1203..09facd2e9 100644 --- a/crates/tor-netdoc/src/doc/hsdesc/build.rs +++ b/crates/tor-netdoc/src/doc/hsdesc/build.rs @@ -53,12 +53,13 @@ struct HsDesc<'a> { intro_auth_key_cert_expiry: SystemTime, /// The expiration time of an introduction point encryption key certificate. intro_enc_key_cert_expiry: SystemTime, - /// Client authorization parameters, if client authentication is enabled. If set to `None`, - /// client authentication is disabled. + /// The list of clients authorized to access the hidden service. If empty, client + /// authentication is disabled. /// /// If client authorization is disabled, the resulting middle document will contain a single /// `auth-client` line populated with random values. - client_auth: Option<&'a ClientAuth>, + #[builder(default)] + auth_clients: &'a [curve25519::PublicKey], /// The lifetime of this descriptor, in minutes. /// /// This doesn't actually list the starting time or the end time for the @@ -73,23 +74,51 @@ struct HsDesc<'a> { } /// Client authorization parameters. -// TODO HS this ought to go away from the public API (see TODO below), or use a builder? -#[derive(Debug, Builder)] -#[builder(pattern = "owned")] -pub struct ClientAuth { - /// The ephemeral x25519 ephemeral public key generated by the hidden service - /// (`KP_hss_desc_enc`). +#[derive(Debug)] +pub(super) struct ClientAuth<'a> { + /// An ephemeral x25519 keypair generated by the hidden service (`KP_hss_desc_enc`). + /// + /// A new keypair MUST be generated every time a descriptor is encoded, or the descriptor + /// encryption will not be secure. ephemeral_key: HsSvcDescEncKeypair, /// One or more authorized clients. - auth_clients: Vec, + auth_clients: &'a [curve25519::PublicKey], /// The `N_hs_desc_enc` descriptor_cookie key generated by the hidden service. /// - /// TODO hs: Do we even need this field? This is presumed to be randomly generated for each - /// descriptor by the hidden service, but since it's random, we might as well let the - /// descriptor builder generate it. + /// A new descriptor cookie is randomly generated for each descriptor. descriptor_cookie: [u8; HS_DESC_ENC_NONCE_LEN], } +impl<'a> ClientAuth<'a> { + /// Create a new `ClientAuth` using the specified authorized clients. + /// + /// This returns `None` if the list of authorized clients is empty. + fn new( + auth_clients: &'a [curve25519::PublicKey], + rng: &mut R, + ) -> Option> { + // Client auth is disabled + if auth_clients.is_empty() { + return None; + } + + // Generate a new `N_hs_desc_enc` descriptor_cookie key for this descriptor. + let descriptor_cookie = rand::Rng::gen::<[u8; HS_DESC_ENC_NONCE_LEN]>(rng); + + let secret = curve25519::StaticSecret::new(rng); + let ephemeral_key = HsSvcDescEncKeypair { + public: curve25519::PublicKey::from(&secret).into(), + secret: secret.into(), + }; + + Some(ClientAuth { + ephemeral_key, + auth_clients, + descriptor_cookie, + }) + } +} + impl<'a> NetdocBuilder for HsDescBuilder<'a> { fn build_sign(self, rng: &mut R) -> Result { /// The superencrypted field must be padded to the nearest multiple of 10k bytes @@ -101,6 +130,8 @@ impl<'a> NetdocBuilder for HsDescBuilder<'a> { .build() .map_err(into_bad_api_usage!("the HsDesc could not be built"))?; + let client_auth = ClientAuth::new(hs_desc.auth_clients, rng); + // Construct the inner (second layer) plaintext. This is the unencrypted value of the // "encrypted" field. let inner_plaintext = HsDescInner { @@ -114,8 +145,7 @@ impl<'a> NetdocBuilder for HsDescBuilder<'a> { } .build_sign(rng)?; - let desc_enc_nonce = hs_desc - .client_auth + let desc_enc_nonce = client_auth .as_ref() .map(|client_auth| client_auth.descriptor_cookie.into()); @@ -131,7 +161,7 @@ impl<'a> NetdocBuilder for HsDescBuilder<'a> { // Construct the middle (first player) plaintext. This is the unencrypted value of the // "superencrypted" field. let middle_plaintext = HsDescMiddle { - client_auth: hs_desc.client_auth, + client_auth: client_auth.as_ref(), subcredential: hs_desc.subcredential, encrypted: inner_encrypted, } @@ -339,7 +369,6 @@ mod test { .intro_points(&intro_points) .intro_auth_key_cert_expiry(expiry) .intro_enc_key_cert_expiry(expiry) - .client_auth(None) .lifetime(LIFETIME_MINS.into()) .revision_counter(REVISION_COUNT.into()) .subcredential(subcredential); @@ -373,7 +402,6 @@ mod test { .intro_points(&intro_points) .intro_auth_key_cert_expiry(expiry) .intro_enc_key_cert_expiry(expiry) - .client_auth(None) .lifetime(desc.idx_info.lifetime) .revision_counter(desc.idx_info.revision) .subcredential(subcredential) @@ -388,18 +416,8 @@ mod test { curve25519::PublicKey::from(client_skey.as_ref()).into(); let auth_clients = vec![*client_pkey]; - let secret = curve25519::StaticSecret::new(&mut rng); - let client_auth = ClientAuth { - ephemeral_key: HsSvcDescEncKeypair { - public: curve25519::PublicKey::from(&secret).into(), - secret: secret.into(), - }, - auth_clients, - descriptor_cookie: rand::Rng::gen::<[u8; HS_DESC_ENC_NONCE_LEN]>(&mut rng), - }; - let encoded_desc = builder - .client_auth(Some(&client_auth)) + .auth_clients(&auth_clients) .build_sign(&mut Config::Deterministic.into_rng()) .unwrap(); @@ -425,7 +443,7 @@ mod test { .intro_points(&intro_points) .intro_auth_key_cert_expiry(expiry) .intro_enc_key_cert_expiry(expiry) - .client_auth(Some(&client_auth)) + .auth_clients(&auth_clients) .lifetime(desc.idx_info.lifetime) .revision_counter(desc.idx_info.revision) .subcredential(subcredential) diff --git a/crates/tor-netdoc/src/doc/hsdesc/build/middle.rs b/crates/tor-netdoc/src/doc/hsdesc/build/middle.rs index 975c13fc9..55a78d046 100644 --- a/crates/tor-netdoc/src/doc/hsdesc/build/middle.rs +++ b/crates/tor-netdoc/src/doc/hsdesc/build/middle.rs @@ -27,7 +27,7 @@ use rand::{CryptoRng, Rng, RngCore}; pub(super) struct HsDescMiddle<'a> { /// Client authorization parameters, if client authentication is enabled. If set to `None`, /// client authentication is disabled. - pub(super) client_auth: Option<&'a ClientAuth>, + pub(super) client_auth: Option<&'a ClientAuth<'a>>, /// The "subcredential" of the onion service. pub(super) subcredential: Subcredential, /// The (encrypted) inner document of the onion service descriptor. @@ -182,7 +182,7 @@ AQIDBA== public, secret: secret.into(), }, - auth_clients: vec![], + auth_clients: &[], descriptor_cookie: rand::Rng::gen::<[u8; HS_DESC_ENC_NONCE_LEN]>(&mut rng), }; @@ -215,7 +215,7 @@ AQIDBA== public, secret: secret.into(), }, - auth_clients, + auth_clients: &auth_clients, descriptor_cookie: rand::Rng::gen::<[u8; HS_DESC_ENC_NONCE_LEN]>(&mut rng), };