From ca65605e2f71e308a864e88edf730087a03e8504 Mon Sep 17 00:00:00 2001 From: Rostislav Rumenov Date: Fri, 19 Jan 2024 14:49:10 +0000 Subject: [PATCH] refactor: remove unused code and the custom error for failed TLS public key creation --- Cargo.lock | 14 ++-- .../public_and_secret_key_store/tests.rs | 2 +- rs/crypto/src/tls/mod.rs | 2 +- rs/crypto/tls_interfaces/BUILD.bazel | 2 +- rs/crypto/tls_interfaces/Cargo.toml | 2 +- rs/crypto/tls_interfaces/src/lib.rs | 73 +++---------------- rs/crypto/tls_interfaces/src/tests.rs | 13 +--- 7 files changed, 27 insertions(+), 81 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8ba143e5884..3f9b29cfa47 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -406,9 +406,9 @@ checksum = "34fde25430d87a9388dadbe6e34d7f72a462c8b43ac8d309b42b0a8505d7e2a5" [[package]] name = "anyhow" -version = "1.0.75" +version = "1.0.79" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a4668cab20f66d8d020e1fbc0ebe47217433c1b6c8f2040faf858554e394ace6" +checksum = "080e9890a082662b09c1ad45f567faeeb47f22b5fb23895fbe1e651e718e25ca" [[package]] name = "arbitrary" @@ -7243,7 +7243,6 @@ version = "0.9.0" dependencies = [ "assert_matches", "async-trait", - "ic-crypto-sha2", "ic-crypto-test-utils-reproducible-rng", "ic-crypto-test-utils-tls", "ic-protobuf", @@ -7251,6 +7250,7 @@ dependencies = [ "json5", "maplit", "serde", + "thiserror", "tokio", "tokio-rustls", "x509-parser 0.15.1", @@ -18345,18 +18345,18 @@ checksum = "222a222a5bfe1bba4a77b45ec488a741b3cb8872e5e499451fd7d0129c9c7c3d" [[package]] name = "thiserror" -version = "1.0.50" +version = "1.0.56" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f9a7210f5c9a7156bb50aa36aed4c95afb51df0df00713949448cf9e97d382d2" +checksum = "d54378c645627613241d077a3a79db965db602882668f9136ac42af9ecb730ad" dependencies = [ "thiserror-impl", ] [[package]] name = "thiserror-impl" -version = "1.0.50" +version = "1.0.56" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "266b2e40bc00e5a6c09c3584011e08b06f123c00362c92b975ba9843aaaa14b8" +checksum = "fa0faa943b50f3db30a20aa7e265dbc66076993efed8463e8de414e5d06d3471" dependencies = [ "proc-macro2 1.0.75", "quote 1.0.35", diff --git a/rs/crypto/internal/crypto_service_provider/src/vault/local_csp_vault/public_and_secret_key_store/tests.rs b/rs/crypto/internal/crypto_service_provider/src/vault/local_csp_vault/public_and_secret_key_store/tests.rs index aa388acccc8..45be3aa7835 100644 --- a/rs/crypto/internal/crypto_service_provider/src/vault/local_csp_vault/public_and_secret_key_store/tests.rs +++ b/rs/crypto/internal/crypto_service_provider/src/vault/local_csp_vault/public_and_secret_key_store/tests.rs @@ -999,7 +999,7 @@ mod validate_pks_and_sks { ..required_node_public_keys_and_time().0 }, expected: ValidatePksAndSksError::TlsCertificateError(PublicKeyInvalid( - "Malformed certificate: TlsPublicKeyCertCreationError { internal_error: \"Error parsing DER: Parsing Error: InvalidDate\" }".to_string(), + "Malformed certificate: TlsPublicKeyCertCreationError(\"Error parsing DER: Parsing Error: InvalidDate\"".to_string(), )), }, ParameterizedTest { diff --git a/rs/crypto/src/tls/mod.rs b/rs/crypto/src/tls/mod.rs index ce82f73ab37..c571ea32b33 100644 --- a/rs/crypto/src/tls/mod.rs +++ b/rs/crypto/src/tls/mod.rs @@ -288,7 +288,7 @@ fn tls_cert_from_registry( let raw_cert = tls_cert_from_registry_raw(registry, node_id, registry_version)?; TlsPublicKeyCert::try_from(raw_cert).map_err(|e| { TlsCertFromRegistryError::CertificateMalformed { - internal_error: e.internal_error, + internal_error: format!("{e}"), } }) } diff --git a/rs/crypto/tls_interfaces/BUILD.bazel b/rs/crypto/tls_interfaces/BUILD.bazel index 97e2072d45e..3b065e834c7 100644 --- a/rs/crypto/tls_interfaces/BUILD.bazel +++ b/rs/crypto/tls_interfaces/BUILD.bazel @@ -28,10 +28,10 @@ rust_library( ], version = "0.9.0", deps = [ - "//rs/crypto/sha2", "//rs/protobuf", "//rs/types/types", "@crate_index//:serde", + "@crate_index//:thiserror", "@crate_index//:tokio", "@crate_index//:tokio-rustls", "@crate_index//:x509-parser", diff --git a/rs/crypto/tls_interfaces/Cargo.toml b/rs/crypto/tls_interfaces/Cargo.toml index f8f2825d2b9..8660f934784 100644 --- a/rs/crypto/tls_interfaces/Cargo.toml +++ b/rs/crypto/tls_interfaces/Cargo.toml @@ -10,8 +10,8 @@ documentation.workspace = true async-trait = "0.1.41" ic-types = { path = "../../types/types" } ic-protobuf = { path = "../../protobuf" } -ic-crypto-sha2 = { path = "../sha2" } serde = { workspace = true } +thiserror = "1.0.56" tokio = { workspace = true } tokio-rustls = "0.24.0" x509-parser = "0.15.1" diff --git a/rs/crypto/tls_interfaces/src/lib.rs b/rs/crypto/tls_interfaces/src/lib.rs index 70b397bf2ae..035829a186a 100644 --- a/rs/crypto/tls_interfaces/src/lib.rs +++ b/rs/crypto/tls_interfaces/src/lib.rs @@ -3,16 +3,13 @@ #![deny(clippy::unwrap_used)] use async_trait::async_trait; -use core::fmt; -use ic_crypto_sha2::Sha256; use ic_protobuf::registry::crypto::v1::X509PublicKeyCert; use ic_types::registry::RegistryClientError; use ic_types::{NodeId, RegistryVersion}; use serde::{Deserialize, Deserializer, Serialize}; -use std::cmp::Ordering; use std::collections::BTreeSet; -use std::fmt::{Display, Formatter}; -use std::hash::{Hash, Hasher}; +use std::fmt::{self, Display, Formatter}; +use thiserror::Error; use tokio::io::{AsyncRead, AsyncWrite}; use tokio::net::TcpStream; use tokio_rustls::rustls::{ClientConfig, ServerConfig}; @@ -21,33 +18,26 @@ use x509_parser::certificate::X509Certificate; #[cfg(test)] mod tests; -#[derive(Clone, Debug, Serialize)] +#[derive(Clone, Debug, Serialize, PartialEq, Eq)] /// An X.509 certificate pub struct TlsPublicKeyCert { // rename, to match previous serializations (which used X509PublicKeyCert) #[serde(rename = "certificate_der")] der_cached: Vec, - #[serde(skip_serializing)] - hash_cached: Vec, } impl TlsPublicKeyCert { /// Creates a certificate from ASN.1 DER encoding pub fn new_from_der(cert_der: Vec) -> Result { use x509_parser::prelude::FromDer; - let (remainder, _cert) = - X509Certificate::from_der(&cert_der).map_err(|e| TlsPublicKeyCertCreationError { - internal_error: format!("Error parsing DER: {}", e), - })?; + let (remainder, _cert) = X509Certificate::from_der(&cert_der) + .map_err(|e| TlsPublicKeyCertCreationError(format!("Error parsing DER: {}", e)))?; if !remainder.is_empty() { - return Err(TlsPublicKeyCertCreationError { - internal_error: format!( - "DER not fully consumed when parsing. Remainder: {remainder:?}", - ), - }); + return Err(TlsPublicKeyCertCreationError(format!( + "DER not fully consumed when parsing. Remainder: {remainder:?}" + ))); } Ok(Self { - hash_cached: Sha256::hash(&cert_der).to_vec(), der_cached: cert_der, }) } @@ -55,9 +45,7 @@ impl TlsPublicKeyCert { /// Creates a certificate from PEM encoding pub fn new_from_pem(pem_cert: &str) -> Result { let cert_der = x509_parser::pem::Pem::read(std::io::Cursor::new(pem_cert)) - .map_err(|e| TlsPublicKeyCertCreationError { - internal_error: format!("Error parsing PEM: {e}"), - })? + .map_err(|e| TlsPublicKeyCertCreationError(format!("Error parsing PEM: {e}")))? .0 .contents; Self::new_from_der(cert_der) @@ -76,32 +64,9 @@ impl TlsPublicKeyCert { } } -impl PartialEq for TlsPublicKeyCert { - /// Equality is determined by comparison of the SHA256 hash byte arrays. - fn eq(&self, rhs: &Self) -> bool { - self.hash_cached == rhs.hash_cached - } -} - -impl Eq for TlsPublicKeyCert {} - -impl Hash for TlsPublicKeyCert { - fn hash(&self, state: &mut H) { - self.hash_cached.hash(state) - } -} - -impl PartialOrd for TlsPublicKeyCert { - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) - } -} - -impl Ord for TlsPublicKeyCert { - fn cmp(&self, other: &Self) -> Ordering { - self.hash_cached.cmp(&other.hash_cached) - } -} +#[derive(Error, Debug)] +#[error("{0}")] +pub struct TlsPublicKeyCertCreationError(String); impl<'de> Deserialize<'de> for TlsPublicKeyCert { fn deserialize(deserializer: D) -> Result @@ -129,20 +94,6 @@ impl TryFrom for TlsPublicKeyCert { } } -#[derive(Clone, Debug, PartialEq, Eq)] -/// Errors encountered during creation of a `TlsPublicKeyCert`. -pub struct TlsPublicKeyCertCreationError { - pub internal_error: String, -} - -impl Display for TlsPublicKeyCertCreationError { - fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - write!(f, "{:?}", self) - } -} - -impl std::error::Error for TlsPublicKeyCertCreationError {} - #[derive(Clone, Debug, PartialEq, Eq)] /// Errors from a TLS handshake performed as the server. Please refer to the /// `TlsHandshake` method for detailed error variant descriptions. diff --git a/rs/crypto/tls_interfaces/src/tests.rs b/rs/crypto/tls_interfaces/src/tests.rs index 8998e5f82b7..878dffcf6b1 100644 --- a/rs/crypto/tls_interfaces/src/tests.rs +++ b/rs/crypto/tls_interfaces/src/tests.rs @@ -1,7 +1,7 @@ #![allow(clippy::unwrap_used)] mod tls_public_key_cert { - use crate::{TlsPublicKeyCert, TlsPublicKeyCertCreationError}; + use crate::TlsPublicKeyCert; use assert_matches::assert_matches; use ic_crypto_test_utils_reproducible_rng::reproducible_rng; use ic_crypto_test_utils_tls::x509_certificates::generate_ed25519_cert; @@ -57,10 +57,7 @@ mod tls_public_key_cert { let empty_der = Vec::new(); let error = TlsPublicKeyCert::new_from_der(empty_der).unwrap_err(); - - assert_matches!(error, TlsPublicKeyCertCreationError { internal_error } - if internal_error.contains("Error parsing DER") - ); + assert!(format!("{}", error).contains("Error parsing DER")); } #[test] @@ -69,9 +66,7 @@ mod tls_public_key_cert { let error = TlsPublicKeyCert::new_from_der(malformed_der).unwrap_err(); - assert_matches!(error, TlsPublicKeyCertCreationError { internal_error } - if internal_error.contains("Error parsing DER") - ); + assert!(format!("{}", error).contains("Error parsing DER")); } #[test] @@ -118,7 +113,7 @@ mod tls_public_key_cert { let error: Result = json5::from_str(bad_serialized); assert_matches!(error, Err(json5::Error::Message { msg, .. } ) - if msg.contains("TlsPublicKeyCertCreationError") + if msg.contains("Error parsing DER") ); }