From 938e4fd3bf12a4c21bf0d6d37aeddf27c38d6a6f Mon Sep 17 00:00:00 2001 From: Andrew Schran Date: Thu, 10 Oct 2024 18:48:58 +0100 Subject: [PATCH] Consolidate `HashSetAllow` and `AllowedPublicKeys` types (#19755) --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API: --- Cargo.lock | 1 + consensus/core/src/network/tonic_tls.rs | 39 ++++++------------------ crates/sui-tls/Cargo.toml | 1 + crates/sui-tls/src/lib.rs | 36 +++++++--------------- crates/sui-tls/src/verifier.rs | 40 +++++++++++-------------- 5 files changed, 38 insertions(+), 79 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b5509a6a76c71..e4ccab9513b9b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -14991,6 +14991,7 @@ name = "sui-tls" version = "0.0.0" dependencies = [ "anyhow", + "arc-swap", "axum 0.7.5", "axum-server", "ed25519 1.5.3", diff --git a/consensus/core/src/network/tonic_tls.rs b/consensus/core/src/network/tonic_tls.rs index 88a40f88c4103..13377934e3b18 100644 --- a/consensus/core/src/network/tonic_tls.rs +++ b/consensus/core/src/network/tonic_tls.rs @@ -1,19 +1,22 @@ // Copyright (c) Mysten Labs, Inc. // SPDX-License-Identifier: Apache-2.0 -use std::collections::BTreeSet; - +use crate::context::Context; use consensus_config::{AuthorityIndex, NetworkKeyPair}; -use fastcrypto::ed25519::Ed25519PublicKey; +use sui_tls::AllowPublicKeys; use tokio_rustls::rustls::{ClientConfig, ServerConfig}; -use crate::context::Context; - pub(crate) fn create_rustls_server_config( context: &Context, network_keypair: NetworkKeyPair, ) -> ServerConfig { - let allower = AllowedPublicKeys::new(context); + let allower = AllowPublicKeys::new( + context + .committee + .authorities() + .map(|(_i, a)| a.network_key.clone().into_inner()) + .collect(), + ); let verifier = sui_tls::ClientCertVerifier::new(allower, certificate_server_name(context)); // TODO: refactor to use key bytes let self_signed_cert = sui_tls::SelfSignedCertificate::new( @@ -56,30 +59,6 @@ pub(crate) fn create_rustls_client_config( tls_config } -// Checks if the public key from a TLS certificate belongs to one of the validators. -#[derive(Debug)] -struct AllowedPublicKeys { - // TODO: refactor to use key bytes - keys: BTreeSet, -} - -impl AllowedPublicKeys { - fn new(context: &Context) -> Self { - let keys = context - .committee - .authorities() - .map(|(_i, a)| a.network_key.clone().into_inner()) - .collect(); - Self { keys } - } -} - -impl sui_tls::Allower for AllowedPublicKeys { - fn allowed(&self, key: &Ed25519PublicKey) -> bool { - self.keys.contains(key) - } -} - fn certificate_server_name(context: &Context) -> String { format!("consensus_epoch_{}", context.committee.epoch()) } diff --git a/crates/sui-tls/Cargo.toml b/crates/sui-tls/Cargo.toml index 4f1eaea244784..4a03d9c81dc30 100644 --- a/crates/sui-tls/Cargo.toml +++ b/crates/sui-tls/Cargo.toml @@ -9,6 +9,7 @@ publish = false [dependencies] anyhow.workspace = true +arc-swap.workspace = true ed25519.workspace = true pkcs8.workspace = true rcgen.workspace = true diff --git a/crates/sui-tls/src/lib.rs b/crates/sui-tls/src/lib.rs index 91a6cb666b267..7b3b9c23f5796 100644 --- a/crates/sui-tls/src/lib.rs +++ b/crates/sui-tls/src/lib.rs @@ -10,14 +10,16 @@ pub const SUI_VALIDATOR_SERVER_NAME: &str = "sui"; pub use acceptor::{TlsAcceptor, TlsConnectionInfo}; pub use certgen::SelfSignedCertificate; pub use verifier::{ - public_key_from_certificate, AllowAll, Allower, ClientCertVerifier, HashSetAllow, - ServerCertVerifier, ValidatorAllowlist, + public_key_from_certificate, AllowAll, AllowPublicKeys, Allower, ClientCertVerifier, + ServerCertVerifier, }; pub use rustls; #[cfg(test)] mod tests { + use std::collections::BTreeSet; + use super::*; use fastcrypto::ed25519::Ed25519KeyPair; use fastcrypto::traits::KeyPair; @@ -100,23 +102,16 @@ mod tests { let allowed = Ed25519KeyPair::generate(&mut rng); let disallowed = Ed25519KeyPair::generate(&mut rng); - let allowed_public_key = allowed.public().to_owned(); + let allowed_public_keys = BTreeSet::from([allowed.public().to_owned()]); let allowed_cert = SelfSignedCertificate::new(allowed.private(), SUI_VALIDATOR_SERVER_NAME); let disallowed_cert = SelfSignedCertificate::new(disallowed.private(), SUI_VALIDATOR_SERVER_NAME); - let mut allowlist = HashSetAllow::new(); + let allowlist = AllowPublicKeys::new(allowed_public_keys); let verifier = ClientCertVerifier::new(allowlist.clone(), SUI_VALIDATOR_SERVER_NAME.to_string()); - // Add our public key to the allower - allowlist - .inner_mut() - .write() - .unwrap() - .insert(allowed_public_key); - // The allowed cert passes validation verifier .verify_client_cert(&allowed_cert.rustls_certificate(), &[], UnixTime::now()) @@ -132,7 +127,7 @@ mod tests { ); // After removing the allowed public key from the set it now fails validation - allowlist.inner_mut().write().unwrap().clear(); + allowlist.update(BTreeSet::new()); let err = verifier .verify_client_cert(&allowed_cert.rustls_certificate(), &[], UnixTime::now()) .unwrap_err(); @@ -149,17 +144,10 @@ mod tests { let public_key = keypair.public().to_owned(); let cert = SelfSignedCertificate::new(keypair.private(), "not-sui"); - let mut allowlist = HashSetAllow::new(); + let allowlist = AllowPublicKeys::new(BTreeSet::from([public_key.clone()])); let client_verifier = ClientCertVerifier::new(allowlist.clone(), SUI_VALIDATOR_SERVER_NAME.to_string()); - // Add our public key to the allower - allowlist - .inner_mut() - .write() - .unwrap() - .insert(public_key.clone()); - // Allowed public key but the server-name in the cert is not the required "sui" let err = client_verifier .verify_client_cert(&cert.rustls_certificate(), &[], UnixTime::now()) @@ -210,7 +198,7 @@ mod tests { .build() .unwrap(); - let mut allowlist = HashSetAllow::new(); + let allowlist = AllowPublicKeys::new(BTreeSet::new()); let tls_config = ClientCertVerifier::new(allowlist.clone(), SUI_VALIDATOR_SERVER_NAME.to_string()) .rustls_server_config( @@ -240,11 +228,7 @@ mod tests { client.get(&server_url).send().await.unwrap_err(); // Insert the client's public key into the allowlist and verify the request is successful - allowlist - .inner_mut() - .write() - .unwrap() - .insert(client_public_key.clone()); + allowlist.update(BTreeSet::from([client_public_key.clone()])); let res = client.get(&server_url).send().await.unwrap(); let body = res.text().await.unwrap(); diff --git a/crates/sui-tls/src/verifier.rs b/crates/sui-tls/src/verifier.rs index b2dff221ffd7c..562cc34d48973 100644 --- a/crates/sui-tls/src/verifier.rs +++ b/crates/sui-tls/src/verifier.rs @@ -1,6 +1,7 @@ // Copyright (c) Mysten Labs, Inc. // SPDX-License-Identifier: Apache-2.0 +use arc_swap::ArcSwap; use fastcrypto::ed25519::Ed25519PublicKey; use fastcrypto::traits::ToFromBytes; use rustls::crypto::WebPkiSupportedAlgorithms; @@ -10,10 +11,8 @@ use rustls::pki_types::ServerName; use rustls::pki_types::SignatureVerificationAlgorithm; use rustls::pki_types::TrustAnchor; use rustls::pki_types::UnixTime; -use std::{ - collections::HashSet, - sync::{Arc, RwLock}, -}; +use std::collections::BTreeSet; +use std::sync::Arc; static SUPPORTED_SIG_ALGS: &[&dyn SignatureVerificationAlgorithm] = &[webpki::ring::ED25519]; @@ -22,13 +21,12 @@ static SUPPORTED_ALGORITHMS: WebPkiSupportedAlgorithms = WebPkiSupportedAlgorith mapping: &[(rustls::SignatureScheme::ED25519, SUPPORTED_SIG_ALGS)], }; -pub type ValidatorAllowlist = Arc>>; - /// The Allower trait provides an interface for callers to inject decsions whether /// to allow a cert to be verified or not. This does not prform actual cert validation /// it only acts as a gatekeeper to decide if we should even try. For example, we may want /// to filter our actions to well known public keys. pub trait Allower: std::fmt::Debug + Send + Sync { + // TODO: change allower interface to use raw key bytes. fn allowed(&self, key: &Ed25519PublicKey) -> bool; } @@ -42,32 +40,28 @@ impl Allower for AllowAll { } } -/// HashSetAllow restricts keys to those that are found in the member set. non-members will not be -/// allowed. +/// AllowPublicKeys restricts keys to those that are found in the member set. non-members will +/// not be allowed. #[derive(Debug, Clone, Default)] -pub struct HashSetAllow { - inner: ValidatorAllowlist, +pub struct AllowPublicKeys { + inner: Arc>>, } -impl HashSetAllow { - pub fn new() -> Self { - let inner = Arc::new(RwLock::new(HashSet::new())); - Self { inner } - } - /// Get a reference to the inner service - pub fn inner(&self) -> &ValidatorAllowlist { - &self.inner +impl AllowPublicKeys { + pub fn new(allowed: BTreeSet) -> Self { + Self { + inner: Arc::new(ArcSwap::from_pointee(allowed)), + } } - /// Get a mutable reference to the inner service - pub fn inner_mut(&mut self) -> &mut ValidatorAllowlist { - &mut self.inner + pub fn update(&self, new_allowed: BTreeSet) { + self.inner.store(Arc::new(new_allowed)); } } -impl Allower for HashSetAllow { +impl Allower for AllowPublicKeys { fn allowed(&self, key: &Ed25519PublicKey) -> bool { - self.inner.read().unwrap().contains(key) + self.inner.load().contains(key) } }