From c09a17b66d94e6687afd7c8ef0ac5842a4d2e8fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C4=99drzej=20Stuczy=C5=84ski?= Date: Thu, 24 Oct 2024 15:00:34 +0100 Subject: [PATCH] bugfix: verifying signed information of legacy nodes (#5029) * Added new legacy variant of HostInformation * fixed 'option_bs58_x25519_pubkey' for empty string * 'Debug' impl for x25519 and ed25519 to use human-readable representation * HttpClient to use explicit 'serde_json' conversion for better errors * additional 'Debug' derives --- .../crypto/src/asymmetric/encryption/mod.rs | 12 ++- .../asymmetric/encryption/serde_helpers.rs | 14 ++- common/crypto/src/asymmetric/identity/mod.rs | 12 ++- common/http-api-client/src/lib.rs | 9 +- nym-api/src/node_describe_cache/mod.rs | 2 + .../src/node_describe_cache/query_helpers.rs | 1 + nym-node/nym-node-requests/src/api/mod.rs | 92 ++++++++++++++++++- .../src/api/v1/node/models.rs | 53 +++++++++-- 8 files changed, 174 insertions(+), 21 deletions(-) diff --git a/common/crypto/src/asymmetric/encryption/mod.rs b/common/crypto/src/asymmetric/encryption/mod.rs index fb841541fc..7d7b988fc8 100644 --- a/common/crypto/src/asymmetric/encryption/mod.rs +++ b/common/crypto/src/asymmetric/encryption/mod.rs @@ -2,7 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 use nym_pemstore::traits::{PemStorableKey, PemStorableKeyPair}; -use std::fmt::{self, Display, Formatter}; +use std::fmt::{self, Debug, Display, Formatter}; use std::str::FromStr; use thiserror::Error; use zeroize::{Zeroize, ZeroizeOnDrop}; @@ -112,12 +112,18 @@ impl PemStorableKeyPair for KeyPair { } } -#[derive(PartialEq, Eq, Hash, Copy, Clone, Debug)] +#[derive(PartialEq, Eq, Hash, Copy, Clone)] pub struct PublicKey(x25519_dalek::PublicKey); impl Display for PublicKey { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - write!(f, "{}", self.to_base58_string()) + Display::fmt(&self.to_base58_string(), f) + } +} + +impl Debug for PublicKey { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + Debug::fmt(&self.to_base58_string(), f) } } diff --git a/common/crypto/src/asymmetric/encryption/serde_helpers.rs b/common/crypto/src/asymmetric/encryption/serde_helpers.rs index 374afbd40d..02a5282cdd 100644 --- a/common/crypto/src/asymmetric/encryption/serde_helpers.rs +++ b/common/crypto/src/asymmetric/encryption/serde_helpers.rs @@ -31,8 +31,16 @@ pub mod option_bs58_x25519_pubkey { pub fn deserialize<'de, D: Deserializer<'de>>( deserializer: D, ) -> Result, D::Error> { - let s = Option::::deserialize(deserializer)?; - s.map(|s| PublicKey::from_base58_string(&s).map_err(serde::de::Error::custom)) - .transpose() + match Option::::deserialize(deserializer)? { + None => Ok(None), + Some(s) => { + if s.is_empty() { + Ok(None) + } else { + Some(PublicKey::from_base58_string(&s).map_err(serde::de::Error::custom)) + .transpose() + } + } + } } } diff --git a/common/crypto/src/asymmetric/identity/mod.rs b/common/crypto/src/asymmetric/identity/mod.rs index e2d3df624d..4b51aa2f64 100644 --- a/common/crypto/src/asymmetric/identity/mod.rs +++ b/common/crypto/src/asymmetric/identity/mod.rs @@ -5,7 +5,7 @@ pub use ed25519_dalek::SignatureError; use ed25519_dalek::{Signer, SigningKey}; pub use ed25519_dalek::{Verifier, PUBLIC_KEY_LENGTH, SECRET_KEY_LENGTH, SIGNATURE_LENGTH}; use nym_pemstore::traits::{PemStorableKey, PemStorableKeyPair}; -use std::fmt::{self, Display, Formatter}; +use std::fmt::{self, Debug, Display, Formatter}; use std::str::FromStr; use thiserror::Error; use zeroize::{Zeroize, ZeroizeOnDrop}; @@ -119,12 +119,18 @@ impl PemStorableKeyPair for KeyPair { } /// ed25519 EdDSA Public Key -#[derive(Debug, Copy, Clone, Eq, PartialEq)] +#[derive(Copy, Clone, Eq, PartialEq)] pub struct PublicKey(ed25519_dalek::VerifyingKey); impl Display for PublicKey { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - write!(f, "{}", self.to_base58_string()) + Display::fmt(&self.to_base58_string(), f) + } +} + +impl Debug for PublicKey { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + Debug::fmt(&self.to_base58_string(), f) } } diff --git a/common/http-api-client/src/lib.rs b/common/http-api-client/src/lib.rs index a8ad8e64d1..84ab2d2cb9 100644 --- a/common/http-api-client/src/lib.rs +++ b/common/http-api-client/src/lib.rs @@ -35,6 +35,9 @@ pub enum HttpClientError { source: reqwest::Error, }, + #[error("failed to deserialise received response: {source}")] + ResponseDeserialisationFailure { source: serde_json::Error }, + #[error("provided url is malformed: {source}")] MalformedUrl { #[from] @@ -526,7 +529,11 @@ where } if res.status().is_success() { - Ok(res.json().await?) + let text = res.text().await?; + match serde_json::from_str(&text) { + Ok(res) => Ok(res), + Err(source) => Err(HttpClientError::ResponseDeserialisationFailure { source }), + } } else if res.status() == StatusCode::NOT_FOUND { Err(HttpClientError::NotFound) } else { diff --git a/nym-api/src/node_describe_cache/mod.rs b/nym-api/src/node_describe_cache/mod.rs index d8e0b3c32b..8368e58a97 100644 --- a/nym-api/src/node_describe_cache/mod.rs +++ b/nym-api/src/node_describe_cache/mod.rs @@ -145,6 +145,7 @@ impl NodeDescriptionTopologyExt for NymNodeDescription { } } +#[derive(Debug, Clone)] pub struct DescribedNodes { nodes: HashMap, } @@ -290,6 +291,7 @@ async fn try_get_description( }) } +#[derive(Debug)] struct RefreshData { host: String, node_id: NodeId, diff --git a/nym-api/src/node_describe_cache/query_helpers.rs b/nym-api/src/node_describe_cache/query_helpers.rs index 1752345f2c..78611ca624 100644 --- a/nym-api/src/node_describe_cache/query_helpers.rs +++ b/nym-api/src/node_describe_cache/query_helpers.rs @@ -210,6 +210,7 @@ impl ResolvedNodeDescribedInfo { } } +#[derive(Debug)] pub(crate) struct UnwrappedResolvedNodeDescribedInfo { pub(crate) build_info: BinaryBuildInformationOwned, pub(crate) roles: DeclaredRoles, diff --git a/nym-node/nym-node-requests/src/api/mod.rs b/nym-node/nym-node-requests/src/api/mod.rs index 4db9d49297..6bf586f71b 100644 --- a/nym-node/nym-node-requests/src/api/mod.rs +++ b/nym-node/nym-node-requests/src/api/mod.rs @@ -1,7 +1,9 @@ // Copyright 2023 - Nym Technologies SA // SPDX-License-Identifier: Apache-2.0 -use crate::api::v1::node::models::{HostInformation, LegacyHostInformation}; +use crate::api::v1::node::models::{ + HostInformation, LegacyHostInformation, LegacyHostInformationV2, +}; use crate::error::Error; use nym_crypto::asymmetric::identity; use schemars::JsonSchema; @@ -61,9 +63,18 @@ impl SignedHostInformation { return true; } - // attempt to verify legacy signature + // attempt to verify legacy signatures + let legacy_v2 = SignedData { + data: LegacyHostInformationV2::from(self.data.clone()), + signature: self.signature.clone(), + }; + + if legacy_v2.verify(&self.keys.ed25519_identity) { + return true; + } + SignedData { - data: LegacyHostInformation::from(self.data.clone()), + data: LegacyHostInformation::from(legacy_v2.data), signature: self.signature.clone(), } .verify(&self.keys.ed25519_identity) @@ -133,6 +144,81 @@ mod tests { assert!(signed_info.verify_host_information()); } + #[test] + fn dummy_legacy_v2_signed_host_verification() { + let mut rng = rand_chacha::ChaCha20Rng::from_seed([0u8; 32]); + let ed22519 = ed25519::KeyPair::new(&mut rng); + let x25519_sphinx = x25519::KeyPair::new(&mut rng); + let x25519_noise = x25519::KeyPair::new(&mut rng); + + let legacy_info_no_noise = crate::api::v1::node::models::LegacyHostInformationV2 { + ip_address: vec!["1.1.1.1".parse().unwrap()], + hostname: Some("foomp.com".to_string()), + keys: crate::api::v1::node::models::LegacyHostKeysV2 { + ed25519_identity: ed22519.public_key().to_base58_string(), + x25519_sphinx: x25519_sphinx.public_key().to_base58_string(), + x25519_noise: "".to_string(), + }, + }; + + let legacy_info_noise = crate::api::v1::node::models::LegacyHostInformationV2 { + ip_address: vec!["1.1.1.1".parse().unwrap()], + hostname: Some("foomp.com".to_string()), + keys: crate::api::v1::node::models::LegacyHostKeysV2 { + ed25519_identity: ed22519.public_key().to_base58_string(), + x25519_sphinx: x25519_sphinx.public_key().to_base58_string(), + x25519_noise: x25519_noise.public_key().to_base58_string(), + }, + }; + + let host_info_no_noise = crate::api::v1::node::models::HostInformation { + ip_address: legacy_info_no_noise.ip_address.clone(), + hostname: legacy_info_no_noise.hostname.clone(), + keys: crate::api::v1::node::models::HostKeys { + ed25519_identity: legacy_info_no_noise.keys.ed25519_identity.parse().unwrap(), + x25519_sphinx: legacy_info_no_noise.keys.x25519_sphinx.parse().unwrap(), + x25519_noise: None, + }, + }; + + let host_info_noise = crate::api::v1::node::models::HostInformation { + ip_address: legacy_info_noise.ip_address.clone(), + hostname: legacy_info_noise.hostname.clone(), + keys: crate::api::v1::node::models::HostKeys { + ed25519_identity: legacy_info_noise.keys.ed25519_identity.parse().unwrap(), + x25519_sphinx: legacy_info_noise.keys.x25519_sphinx.parse().unwrap(), + x25519_noise: Some(legacy_info_noise.keys.x25519_noise.parse().unwrap()), + }, + }; + + // signature on legacy data + let signature_no_noise = SignedData::new(legacy_info_no_noise, ed22519.private_key()) + .unwrap() + .signature; + + let signature_noise = SignedData::new(legacy_info_noise, ed22519.private_key()) + .unwrap() + .signature; + + // signed blob with the 'current' structure + let current_struct_no_noise = SignedData { + data: host_info_no_noise, + signature: signature_no_noise, + }; + + let current_struct_noise = SignedData { + data: host_info_noise, + signature: signature_noise, + }; + + assert!(!current_struct_no_noise.verify(ed22519.public_key())); + assert!(current_struct_no_noise.verify_host_information()); + + // if noise key is present, the signature is actually valid + assert!(current_struct_noise.verify(ed22519.public_key())); + assert!(current_struct_noise.verify_host_information()) + } + #[test] fn dummy_legacy_signed_host_verification() { let mut rng = rand_chacha::ChaCha20Rng::from_seed([0u8; 32]); diff --git a/nym-node/nym-node-requests/src/api/v1/node/models.rs b/nym-node/nym-node-requests/src/api/v1/node/models.rs index dcaa12435f..6979beb949 100644 --- a/nym-node/nym-node-requests/src/api/v1/node/models.rs +++ b/nym-node/nym-node-requests/src/api/v1/node/models.rs @@ -59,6 +59,13 @@ pub struct HostInformation { pub keys: HostKeys, } +#[derive(Serialize)] +pub struct LegacyHostInformationV2 { + pub ip_address: Vec, + pub hostname: Option, + pub keys: LegacyHostKeysV2, +} + #[derive(Serialize)] pub struct LegacyHostInformation { pub ip_address: Vec, @@ -66,8 +73,18 @@ pub struct LegacyHostInformation { pub keys: LegacyHostKeys, } -impl From for LegacyHostInformation { +impl From for LegacyHostInformationV2 { fn from(value: HostInformation) -> Self { + LegacyHostInformationV2 { + ip_address: value.ip_address, + hostname: value.hostname, + keys: value.keys.into(), + } + } +} + +impl From for LegacyHostInformation { + fn from(value: LegacyHostInformationV2) -> Self { LegacyHostInformation { ip_address: value.ip_address, hostname: value.hostname, @@ -99,13 +116,11 @@ pub struct HostKeys { pub x25519_noise: Option, } -impl From for LegacyHostKeys { - fn from(value: HostKeys) -> Self { - LegacyHostKeys { - ed25519: value.ed25519_identity.to_base58_string(), - x25519: value.x25519_sphinx.to_base58_string(), - } - } +#[derive(Serialize)] +pub struct LegacyHostKeysV2 { + pub ed25519_identity: String, + pub x25519_sphinx: String, + pub x25519_noise: String, } #[derive(Serialize)] @@ -114,6 +129,28 @@ pub struct LegacyHostKeys { pub x25519: String, } +impl From for LegacyHostKeysV2 { + fn from(value: HostKeys) -> Self { + LegacyHostKeysV2 { + ed25519_identity: value.ed25519_identity.to_base58_string(), + x25519_sphinx: value.x25519_sphinx.to_base58_string(), + x25519_noise: value + .x25519_noise + .map(|k| k.to_base58_string()) + .unwrap_or_default(), + } + } +} + +impl From for LegacyHostKeys { + fn from(value: LegacyHostKeysV2) -> Self { + LegacyHostKeys { + ed25519: value.ed25519_identity, + x25519: value.x25519_sphinx, + } + } +} + #[derive(Clone, Default, Debug, Serialize, Deserialize, JsonSchema)] #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] pub struct HostSystem {