From 8f5f19b878fd570c10ee40752b7e7b2afe156d4c Mon Sep 17 00:00:00 2001 From: nightness Date: Wed, 1 Apr 2026 05:36:23 -0500 Subject: [PATCH 1/7] fix(dtls): replace panicking unwrap/expect in crypto hot paths - generate_self_signed_with_alg: replace 3x .unwrap() with ? operator - Config::build(): replace .build().unwrap() on rustls verifier with ? - flight5::generate(): extract certificate ref with ok_or_else() instead of .unwrap(), returning a proper Fatal/InternalError DTLS alert - CryptoPrivateKey::Clone: add Safety comment explaining why re-parsing serialized_der is sound; upgrade bare .unwrap() to .expect() with descriptive message so panics are diagnosable if they ever occur Co-Authored-By: Claude Sonnet 4.6 --- rtc-dtls/src/config.rs | 5 +++-- rtc-dtls/src/crypto/mod.rs | 20 ++++++++++++++------ rtc-dtls/src/flight/flight5.rs | 16 ++++++++++++++-- 3 files changed, 31 insertions(+), 10 deletions(-) diff --git a/rtc-dtls/src/config.rs b/rtc-dtls/src/config.rs index 028950d4..9a0d6fbb 100644 --- a/rtc-dtls/src/config.rs +++ b/rtc-dtls/src/config.rs @@ -344,7 +344,7 @@ impl ConfigBuilder { gen_self_signed_root_cert(), )) .build() - .unwrap(), + .map_err(|e| Error::Other(e.to_string()))?, client_cert_verifier: None, retransmit_interval, initial_epoch: 0, @@ -441,11 +441,12 @@ impl Default for HandshakeConfig { insecure_verification: false, verify_peer_certificate: None, roots_cas: rustls::RootCertStore::empty(), + // Safety: gen_self_signed_root_cert uses hardcoded empty subject; build() cannot fail. server_cert_verifier: rustls::client::WebPkiServerVerifier::builder(Arc::new( gen_self_signed_root_cert(), )) .build() - .unwrap(), + .expect("WebPkiServerVerifier::build with self-signed root cannot fail"), client_cert_verifier: None, retransmit_interval: std::time::Duration::from_secs(0), initial_epoch: 0, diff --git a/rtc-dtls/src/crypto/mod.rs b/rtc-dtls/src/crypto/mod.rs index 9b4b3ef9..07236730 100644 --- a/rtc-dtls/src/crypto/mod.rs +++ b/rtc-dtls/src/crypto/mod.rs @@ -54,9 +54,13 @@ impl Certificate { subject_alt_names: impl Into>, alg: &'static rcgen::SignatureAlgorithm, ) -> Result { - let params = rcgen::CertificateParams::new(subject_alt_names).unwrap(); - let key_pair = rcgen::KeyPair::generate_for(alg).unwrap(); - let cert = params.self_signed(&key_pair).unwrap(); + let params = rcgen::CertificateParams::new(subject_alt_names) + .map_err(|e| Error::Other(e.to_string()))?; + let key_pair = rcgen::KeyPair::generate_for(alg) + .map_err(|e| Error::Other(e.to_string()))?; + let cert = params + .self_signed(&key_pair) + .map_err(|e| Error::Other(e.to_string()))?; Ok(Certificate { certificate: vec![cert.der().to_owned()], @@ -179,10 +183,13 @@ impl PartialEq for CryptoPrivateKey { impl Clone for CryptoPrivateKey { fn clone(&self) -> Self { + // Safety: `serialized_der` is always produced by `from_key_pair` which serialises a + // valid key. Re-parsing the same bytes cannot fail, so these unwraps are sound. match self.kind { CryptoPrivateKeyKind::Ed25519(_) => CryptoPrivateKey { kind: CryptoPrivateKeyKind::Ed25519( - Ed25519KeyPair::from_pkcs8_maybe_unchecked(&self.serialized_der).unwrap(), + Ed25519KeyPair::from_pkcs8_maybe_unchecked(&self.serialized_der) + .expect("CryptoPrivateKey::clone: Ed25519 DER re-parse failed"), ), serialized_der: self.serialized_der.clone(), }, @@ -193,13 +200,14 @@ impl Clone for CryptoPrivateKey { &self.serialized_der, &SystemRandom::new(), ) - .unwrap(), + .expect("CryptoPrivateKey::clone: ECDSA DER re-parse failed"), ), serialized_der: self.serialized_der.clone(), }, CryptoPrivateKeyKind::Rsa256(_) => CryptoPrivateKey { kind: CryptoPrivateKeyKind::Rsa256( - ring::rsa::KeyPair::from_pkcs8(&self.serialized_der).unwrap(), + ring::rsa::KeyPair::from_pkcs8(&self.serialized_der) + .expect("CryptoPrivateKey::clone: RSA DER re-parse failed"), ), serialized_der: self.serialized_der.clone(), }, diff --git a/rtc-dtls/src/flight/flight5.rs b/rtc-dtls/src/flight/flight5.rs index c738cd28..bffc9c20 100644 --- a/rtc-dtls/src/flight/flight5.rs +++ b/rtc-dtls/src/flight/flight5.rs @@ -395,10 +395,22 @@ impl Flight for Flight5 { plain_text.extend_from_slice(&merged); + let cert_ref = certificate.as_ref().ok_or_else(|| { + ( + Some(Alert { + alert_level: AlertLevel::Fatal, + alert_description: AlertDescription::InternalError, + }), + Some(Error::Other( + "no local certificate available for DTLS flight5".to_owned(), + )), + ) + })?; + // Find compatible signature scheme let signature_hash_algo = match select_signature_scheme( &cfg.local_signature_schemes, - &certificate.as_ref().unwrap().private_key, + &cert_ref.private_key, ) { Ok(s) => s, Err(err) => { @@ -414,7 +426,7 @@ impl Flight for Flight5 { let cert_verify = match generate_certificate_verify( &plain_text, - &certificate.as_ref().unwrap().private_key, /*, signature_hash_algo.hash*/ + &cert_ref.private_key, ) { Ok(cert) => cert, Err(err) => { From 85a4167c2dbec53398be96f90626171cae48c610 Mon Sep 17 00:00:00 2001 From: nightness Date: Tue, 7 Apr 2026 11:10:08 -0500 Subject: [PATCH 2/7] style: cargo fmt Co-Authored-By: Claude Opus 4.6 (1M context) --- rtc-dtls/src/crypto/mod.rs | 4 ++-- rtc-dtls/src/flight/flight5.rs | 6 ++---- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/rtc-dtls/src/crypto/mod.rs b/rtc-dtls/src/crypto/mod.rs index 07236730..c77167ba 100644 --- a/rtc-dtls/src/crypto/mod.rs +++ b/rtc-dtls/src/crypto/mod.rs @@ -56,8 +56,8 @@ impl Certificate { ) -> Result { let params = rcgen::CertificateParams::new(subject_alt_names) .map_err(|e| Error::Other(e.to_string()))?; - let key_pair = rcgen::KeyPair::generate_for(alg) - .map_err(|e| Error::Other(e.to_string()))?; + let key_pair = + rcgen::KeyPair::generate_for(alg).map_err(|e| Error::Other(e.to_string()))?; let cert = params .self_signed(&key_pair) .map_err(|e| Error::Other(e.to_string()))?; diff --git a/rtc-dtls/src/flight/flight5.rs b/rtc-dtls/src/flight/flight5.rs index bffc9c20..2935bebc 100644 --- a/rtc-dtls/src/flight/flight5.rs +++ b/rtc-dtls/src/flight/flight5.rs @@ -424,10 +424,8 @@ impl Flight for Flight5 { } }; - let cert_verify = match generate_certificate_verify( - &plain_text, - &cert_ref.private_key, - ) { + let cert_verify = match generate_certificate_verify(&plain_text, &cert_ref.private_key) + { Ok(cert) => cert, Err(err) => { return Err(( From 313c0674efb87b6e6150b103b67f159b67ce6b67 Mon Sep 17 00:00:00 2001 From: nightness Date: Wed, 8 Apr 2026 01:06:57 -0500 Subject: [PATCH 3/7] address review feedback for PR #68 - Replace panicking WebPkiServerVerifier::build() in HandshakeConfig::default() with a PlaceholderServerCertVerifier; ConfigBuilder::build() now sets all fields explicitly instead of relying on ..Default::default() - Make CryptoPrivateKey fields pub(crate) to enforce the kind/serialized_der consistency invariant; add public accessors kind() and serialized_der() - Refactor rtc peer_connection certificate to use CryptoPrivateKey::from_key_pair() instead of constructing the struct directly Co-Authored-By: Claude Opus 4.6 (1M context) --- rtc-dtls/src/config.rs | 65 +++++++++++++++++++--- rtc-dtls/src/crypto/mod.rs | 26 +++++++-- rtc/src/peer_connection/certificate/mod.rs | 39 +------------ 3 files changed, 78 insertions(+), 52 deletions(-) diff --git a/rtc-dtls/src/config.rs b/rtc-dtls/src/config.rs index 9a0d6fbb..1803fd59 100644 --- a/rtc-dtls/src/config.rs +++ b/rtc-dtls/src/config.rs @@ -13,9 +13,10 @@ use std::net::SocketAddr; use std::sync::Arc; use std::time::Duration; -use rustls::client::danger::ServerCertVerifier; -use rustls::pki_types::CertificateDer; +use rustls::client::danger::{HandshakeSignatureValid, ServerCertVerified, ServerCertVerifier}; +use rustls::pki_types::{CertificateDer, ServerName, UnixTime}; use rustls::server::danger::ClientCertVerifier; +use rustls::{DigitallySignedStruct, SignatureScheme as RustlsSignatureScheme}; /// Config is used to configure a DTLS client or server. /// After a Config is passed to a DTLS function it must not be modified. @@ -349,8 +350,9 @@ impl ConfigBuilder { retransmit_interval, initial_epoch: 0, maximum_transmission_unit, + maximum_retransmit_number: 7, replay_protection_window, - ..Default::default() + name_to_certificate: HashMap::new(), }) } } @@ -358,6 +360,55 @@ impl ConfigBuilder { pub type VerifyPeerCertificateFn = Arc], &[CertificateDer<'static>]) -> Result<()>) + Send + Sync>; +/// A placeholder [`ServerCertVerifier`] used only by `HandshakeConfig::default()`. +/// +/// [`ConfigBuilder::build()`] always replaces this with a real +/// [`WebPkiServerVerifier`](rustls::client::WebPkiServerVerifier), +/// so the placeholder methods are never called in practice. +#[derive(Debug)] +struct PlaceholderServerCertVerifier; + +impl ServerCertVerifier for PlaceholderServerCertVerifier { + fn verify_server_cert( + &self, + _end_entity: &CertificateDer<'_>, + _intermediates: &[CertificateDer<'_>], + _server_name: &ServerName<'_>, + _ocsp_response: &[u8], + _now: UnixTime, + ) -> std::result::Result { + Err(rustls::Error::General( + "PlaceholderServerCertVerifier: must not be used for real verification".into(), + )) + } + + fn verify_tls12_signature( + &self, + _message: &[u8], + _cert: &CertificateDer<'_>, + _dss: &DigitallySignedStruct, + ) -> std::result::Result { + Err(rustls::Error::General( + "PlaceholderServerCertVerifier: must not be used for real verification".into(), + )) + } + + fn verify_tls13_signature( + &self, + _message: &[u8], + _cert: &CertificateDer<'_>, + _dss: &DigitallySignedStruct, + ) -> std::result::Result { + Err(rustls::Error::General( + "PlaceholderServerCertVerifier: must not be used for real verification".into(), + )) + } + + fn supported_verify_schemes(&self) -> Vec { + vec![] + } +} + pub fn gen_self_signed_root_cert() -> rustls::RootCertStore { let mut certs = rustls::RootCertStore::empty(); certs @@ -441,12 +492,8 @@ impl Default for HandshakeConfig { insecure_verification: false, verify_peer_certificate: None, roots_cas: rustls::RootCertStore::empty(), - // Safety: gen_self_signed_root_cert uses hardcoded empty subject; build() cannot fail. - server_cert_verifier: rustls::client::WebPkiServerVerifier::builder(Arc::new( - gen_self_signed_root_cert(), - )) - .build() - .expect("WebPkiServerVerifier::build with self-signed root cannot fail"), + // Placeholder: ConfigBuilder::build() always replaces this with a real verifier. + server_cert_verifier: Arc::new(PlaceholderServerCertVerifier), client_cert_verifier: None, retransmit_interval: std::time::Duration::from_secs(0), initial_epoch: 0, diff --git a/rtc-dtls/src/crypto/mod.rs b/rtc-dtls/src/crypto/mod.rs index c77167ba..8d449611 100644 --- a/rtc-dtls/src/crypto/mod.rs +++ b/rtc-dtls/src/crypto/mod.rs @@ -151,12 +151,26 @@ pub enum CryptoPrivateKeyKind { } /// Private key. +/// +/// Fields are intentionally private to enforce the invariant that `kind` and +/// `serialized_der` are always consistent. Construct instances via +/// [`CryptoPrivateKey::from_key_pair`] or the [`TryFrom<&KeyPair>`] impl. #[derive(Debug)] pub struct CryptoPrivateKey { - /// Keypair. - pub kind: CryptoPrivateKeyKind, - /// DER-encoded keypair. - pub serialized_der: Vec, + pub(crate) kind: CryptoPrivateKeyKind, + pub(crate) serialized_der: Vec, +} + +impl CryptoPrivateKey { + /// Returns a reference to the key kind. + pub fn kind(&self) -> &CryptoPrivateKeyKind { + &self.kind + } + + /// Returns the DER-encoded key bytes. + pub fn serialized_der(&self) -> &[u8] { + &self.serialized_der + } } impl PartialEq for CryptoPrivateKey { @@ -183,8 +197,8 @@ impl PartialEq for CryptoPrivateKey { impl Clone for CryptoPrivateKey { fn clone(&self) -> Self { - // Safety: `serialized_der` is always produced by `from_key_pair` which serialises a - // valid key. Re-parsing the same bytes cannot fail, so these unwraps are sound. + // Safety: fields are private, so `serialized_der` is always produced by `from_key_pair` + // which serialises a valid key. Re-parsing the same DER bytes cannot fail. match self.kind { CryptoPrivateKeyKind::Ed25519(_) => CryptoPrivateKey { kind: CryptoPrivateKeyKind::Ed25519( diff --git a/rtc/src/peer_connection/certificate/mod.rs b/rtc/src/peer_connection/certificate/mod.rs index 1c11580c..a5377820 100644 --- a/rtc/src/peer_connection/certificate/mod.rs +++ b/rtc/src/peer_connection/certificate/mod.rs @@ -186,11 +186,8 @@ use std::ops::Add; use std::time::{Duration, SystemTime}; -use dtls::crypto::{CryptoPrivateKey, CryptoPrivateKeyKind}; +use dtls::crypto::CryptoPrivateKey; use rcgen::{CertificateParams, KeyPair}; -use ring::rand::SystemRandom; -use ring::rsa; -use ring::signature::{EcdsaKeyPair, Ed25519KeyPair}; use sha2::{Digest, Sha256}; use crate::peer_connection::transport::dtls::fingerprint::RTCDtlsFingerprint; @@ -342,39 +339,7 @@ impl RTCCertificate { let not_after = params.not_after; let x509_cert = params.self_signed(&key_pair).unwrap(); - let serialized_der = key_pair.serialize_der(); - - let private_key = if key_pair.is_compatible(&rcgen::PKCS_ED25519) { - CryptoPrivateKey { - kind: CryptoPrivateKeyKind::Ed25519( - Ed25519KeyPair::from_pkcs8(&serialized_der) - .map_err(|e| Error::Other(e.to_string()))?, - ), - serialized_der, - } - } else if key_pair.is_compatible(&rcgen::PKCS_ECDSA_P256_SHA256) { - CryptoPrivateKey { - kind: CryptoPrivateKeyKind::Ecdsa256( - EcdsaKeyPair::from_pkcs8( - &ring::signature::ECDSA_P256_SHA256_ASN1_SIGNING, - &serialized_der, - &SystemRandom::new(), - ) - .map_err(|e| Error::Other(e.to_string()))?, - ), - serialized_der, - } - } else if key_pair.is_compatible(&rcgen::PKCS_RSA_SHA256) { - CryptoPrivateKey { - kind: CryptoPrivateKeyKind::Rsa256( - rsa::KeyPair::from_pkcs8(&serialized_der) - .map_err(|e| Error::Other(e.to_string()))?, - ), - serialized_der, - } - } else { - return Err(Error::Other("Unsupported key_pair".to_owned())); - }; + let private_key = CryptoPrivateKey::from_key_pair(&key_pair)?; let expires = if cfg!(target_arch = "arm") { // Workaround for issue overflow when adding duration to instant on armv7 From 2e7018a651234dc09e4eee5a7fb0cee4782e5b55 Mon Sep 17 00:00:00 2001 From: nightness Date: Wed, 8 Apr 2026 03:37:40 -0500 Subject: [PATCH 4/7] fix: address new Copilot review comments (pass 2) - Propagate rcgen error from params.self_signed() instead of unwrap() - Match on &self.kind in Clone impl to avoid moving from &self - Fix doc comment: fields are pub(crate), not private - Add warning doc comment on HandshakeConfig::default() about placeholder verifier - Add test for Flight5 no-certificate error path Co-Authored-By: Claude Opus 4.6 (1M context) --- rtc-dtls/src/config.rs | 6 +++++ rtc-dtls/src/crypto/mod.rs | 12 +++++---- rtc-dtls/src/flight/flight5.rs | 30 ++++++++++++++++++++++ rtc/src/peer_connection/certificate/mod.rs | 4 ++- 4 files changed, 46 insertions(+), 6 deletions(-) diff --git a/rtc-dtls/src/config.rs b/rtc-dtls/src/config.rs index 1803fd59..156bb811 100644 --- a/rtc-dtls/src/config.rs +++ b/rtc-dtls/src/config.rs @@ -476,6 +476,12 @@ impl fmt::Debug for HandshakeConfig { } impl Default for HandshakeConfig { + /// Creates a `HandshakeConfig` with placeholder values. + /// + /// **Warning:** The default `server_cert_verifier` is a placeholder that + /// always rejects certificates. Do not use `HandshakeConfig::default()` + /// directly for real handshakes; instead, go through [`ConfigBuilder`] which + /// installs a proper verifier via `build()`. fn default() -> Self { HandshakeConfig { local_psk_callback: None, diff --git a/rtc-dtls/src/crypto/mod.rs b/rtc-dtls/src/crypto/mod.rs index 8d449611..3c886e0e 100644 --- a/rtc-dtls/src/crypto/mod.rs +++ b/rtc-dtls/src/crypto/mod.rs @@ -152,9 +152,11 @@ pub enum CryptoPrivateKeyKind { /// Private key. /// -/// Fields are intentionally private to enforce the invariant that `kind` and -/// `serialized_der` are always consistent. Construct instances via -/// [`CryptoPrivateKey::from_key_pair`] or the [`TryFrom<&KeyPair>`] impl. +/// Fields are `pub(crate)` to enforce the invariant that `kind` and +/// `serialized_der` are always consistent. External code should construct +/// instances via [`CryptoPrivateKey::from_key_pair`] or the +/// [`TryFrom<&KeyPair>`] impl; crate-internal code that accesses the fields +/// directly must maintain this invariant. #[derive(Debug)] pub struct CryptoPrivateKey { pub(crate) kind: CryptoPrivateKeyKind, @@ -197,9 +199,9 @@ impl PartialEq for CryptoPrivateKey { impl Clone for CryptoPrivateKey { fn clone(&self) -> Self { - // Safety: fields are private, so `serialized_der` is always produced by `from_key_pair` + // Safety: `serialized_der` is always produced by `from_key_pair` // which serialises a valid key. Re-parsing the same DER bytes cannot fail. - match self.kind { + match &self.kind { CryptoPrivateKeyKind::Ed25519(_) => CryptoPrivateKey { kind: CryptoPrivateKeyKind::Ed25519( Ed25519KeyPair::from_pkcs8_maybe_unchecked(&self.serialized_der) diff --git a/rtc-dtls/src/flight/flight5.rs b/rtc-dtls/src/flight/flight5.rs index 2935bebc..ec693be0 100644 --- a/rtc-dtls/src/flight/flight5.rs +++ b/rtc-dtls/src/flight/flight5.rs @@ -771,3 +771,33 @@ fn initalize_cipher_suite( Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::handshake::handshake_cache::HandshakeCache; + use crate::state::State; + + /// Verify that `Flight5::generate` returns a fatal `InternalError` alert + /// when the server requested a client certificate but no local certificate + /// is available. + #[test] + fn generate_no_certificate_returns_internal_error() { + let mut state = State::default(); + // Simulate the server having requested a client certificate. + state.remote_requested_certificate = true; + + let cache = HandshakeCache::new(); + // Config with no local certificates. + let cfg = HandshakeConfig::default(); + + let flight = Flight5; + let result = flight.generate(&mut state, &cache, &cfg); + + let (alert, error) = result.expect_err("expected error when no certificate is available"); + let alert = alert.expect("expected an alert"); + assert_eq!(alert.alert_level, AlertLevel::Fatal); + assert_eq!(alert.alert_description, AlertDescription::InternalError); + assert!(error.is_some(), "expected an error value"); + } +} diff --git a/rtc/src/peer_connection/certificate/mod.rs b/rtc/src/peer_connection/certificate/mod.rs index a5377820..de445533 100644 --- a/rtc/src/peer_connection/certificate/mod.rs +++ b/rtc/src/peer_connection/certificate/mod.rs @@ -338,7 +338,9 @@ impl RTCCertificate { fn from_params(params: CertificateParams, key_pair: KeyPair) -> Result { let not_after = params.not_after; - let x509_cert = params.self_signed(&key_pair).unwrap(); + let x509_cert = params.self_signed(&key_pair).map_err(|err| { + Error::Other(format!("failed to generate self-signed certificate: {err}")) + })?; let private_key = CryptoPrivateKey::from_key_pair(&key_pair)?; let expires = if cfg!(target_arch = "arm") { From 569d5121231a08ef2d64990ee85adc3f2f1dce22 Mon Sep 17 00:00:00 2001 From: nightness Date: Wed, 8 Apr 2026 04:04:57 -0500 Subject: [PATCH 5/7] test: improve patch coverage for dtls crypto changes Add tests for: - CryptoPrivateKey::kind() and serialized_der() accessors - CryptoPrivateKey::clone() for Ed25519 and ECDSA key types - CryptoPrivateKey::from_key_pair() for Ed25519 and ECDSA - TryFrom<&KeyPair> delegation to from_key_pair - generate_certificate_verify with Ed25519 and ECDSA keys - PlaceholderServerCertVerifier (verify_server_cert, supported_verify_schemes) - HandshakeConfig::default() uses placeholder verifier - ConfigBuilder::build() sets explicit fields and real verifier - RTCCertificate::from_key_pair with Ed25519 fingerprint validation - RTCCertificate::from_existing with custom expiry - RTCCertificate::from_key_pair rejects unsupported key types Co-Authored-By: Claude Opus 4.6 (1M context) --- rtc-dtls/src/config.rs | 54 ++++++++++ rtc-dtls/src/crypto/mod.rs | 118 +++++++++++++++++++++ rtc/src/peer_connection/certificate/mod.rs | 34 ++++++ 3 files changed, 206 insertions(+) diff --git a/rtc-dtls/src/config.rs b/rtc-dtls/src/config.rs index 156bb811..22dbefc5 100644 --- a/rtc-dtls/src/config.rs +++ b/rtc-dtls/src/config.rs @@ -510,6 +510,60 @@ impl Default for HandshakeConfig { } } +#[cfg(test)] +mod tests { + use super::*; + use rustls::pki_types::{CertificateDer, ServerName, UnixTime}; + + /// Verify `PlaceholderServerCertVerifier::verify_server_cert` returns an error. + #[test] + fn placeholder_verify_server_cert_rejects() { + let v = PlaceholderServerCertVerifier; + let dummy_cert = CertificateDer::from(vec![0u8; 1]); + let name = ServerName::try_from("localhost").unwrap(); + let result = v.verify_server_cert(&dummy_cert, &[], &name, &[], UnixTime::now()); + assert!(result.is_err()); + } + + /// Verify `PlaceholderServerCertVerifier::supported_verify_schemes` returns empty. + #[test] + fn placeholder_supported_verify_schemes_empty() { + let v = PlaceholderServerCertVerifier; + assert!(v.supported_verify_schemes().is_empty()); + } + + /// Verify `HandshakeConfig::default()` uses the placeholder verifier. + #[test] + fn handshake_config_default_uses_placeholder() { + let cfg = HandshakeConfig::default(); + // The placeholder verifier returns empty supported schemes. + assert!( + cfg.server_cert_verifier + .supported_verify_schemes() + .is_empty() + ); + } + + /// Verify `ConfigBuilder::build()` successfully creates a config with explicit fields. + #[test] + fn config_builder_build_sets_fields() { + let cert = Certificate::generate_self_signed(vec!["localhost".to_owned()]).unwrap(); + let cfg = ConfigBuilder::default() + .with_certificates(vec![cert]) + .with_server_name("localhost".to_string()) + .build(false, None) + .unwrap(); + // The builder installs a real verifier, not the placeholder. + // A real verifier has non-empty supported_verify_schemes. + assert!( + !cfg.server_cert_verifier + .supported_verify_schemes() + .is_empty() + ); + assert_eq!(cfg.maximum_retransmit_number, 7); + } +} + impl HandshakeConfig { pub(crate) fn get_certificate(&self, server_name: &str) -> Result { if self.local_certificates.is_empty() { diff --git a/rtc-dtls/src/crypto/mod.rs b/rtc-dtls/src/crypto/mod.rs index 3c886e0e..0980343c 100644 --- a/rtc-dtls/src/crypto/mod.rs +++ b/rtc-dtls/src/crypto/mod.rs @@ -549,4 +549,122 @@ mod test { Ok(()) } + + use super::*; + + /// Test `CryptoPrivateKey::kind()` accessor for each key variant. + #[test] + fn test_crypto_private_key_kind_accessor() -> Result<()> { + // Ed25519 + let kp_ed = rcgen::KeyPair::generate_for(&rcgen::PKCS_ED25519) + .map_err(|e| Error::Other(e.to_string()))?; + let pk_ed = CryptoPrivateKey::from_key_pair(&kp_ed)?; + assert!( + matches!(pk_ed.kind(), CryptoPrivateKeyKind::Ed25519(_)), + "expected Ed25519 kind" + ); + + // ECDSA + let kp_ec = rcgen::KeyPair::generate_for(&rcgen::PKCS_ECDSA_P256_SHA256) + .map_err(|e| Error::Other(e.to_string()))?; + let pk_ec = CryptoPrivateKey::from_key_pair(&kp_ec)?; + assert!( + matches!(pk_ec.kind(), CryptoPrivateKeyKind::Ecdsa256(_)), + "expected Ecdsa256 kind" + ); + + Ok(()) + } + + /// Test `CryptoPrivateKey::serialized_der()` accessor returns non-empty bytes. + #[test] + fn test_crypto_private_key_serialized_der_accessor() -> Result<()> { + let kp = rcgen::KeyPair::generate_for(&rcgen::PKCS_ED25519) + .map_err(|e| Error::Other(e.to_string()))?; + let pk = CryptoPrivateKey::from_key_pair(&kp)?; + assert!( + !pk.serialized_der().is_empty(), + "serialized_der should not be empty" + ); + // Should match what the key pair serialises + assert_eq!(pk.serialized_der(), kp.serialize_der().as_slice()); + Ok(()) + } + + /// Test `CryptoPrivateKey::clone()` for Ed25519 keys. + #[test] + fn test_clone_ed25519() -> Result<()> { + let kp = rcgen::KeyPair::generate_for(&rcgen::PKCS_ED25519) + .map_err(|e| Error::Other(e.to_string()))?; + let pk = CryptoPrivateKey::from_key_pair(&kp)?; + let cloned = pk.clone(); + assert_eq!(pk, cloned); + assert!(matches!(cloned.kind(), CryptoPrivateKeyKind::Ed25519(_))); + Ok(()) + } + + /// Test `CryptoPrivateKey::clone()` for ECDSA keys. + #[test] + fn test_clone_ecdsa() -> Result<()> { + let kp = rcgen::KeyPair::generate_for(&rcgen::PKCS_ECDSA_P256_SHA256) + .map_err(|e| Error::Other(e.to_string()))?; + let pk = CryptoPrivateKey::from_key_pair(&kp)?; + let cloned = pk.clone(); + assert_eq!(pk, cloned); + assert!(matches!(cloned.kind(), CryptoPrivateKeyKind::Ecdsa256(_))); + Ok(()) + } + + /// Test `CryptoPrivateKey::from_key_pair` for Ed25519. + #[test] + fn test_from_key_pair_ed25519() -> Result<()> { + let kp = rcgen::KeyPair::generate_for(&rcgen::PKCS_ED25519) + .map_err(|e| Error::Other(e.to_string()))?; + let pk = CryptoPrivateKey::from_key_pair(&kp)?; + assert!(matches!(pk.kind, CryptoPrivateKeyKind::Ed25519(_))); + assert_eq!(pk.serialized_der, kp.serialize_der()); + Ok(()) + } + + /// Test `CryptoPrivateKey::from_key_pair` for ECDSA P-256. + #[test] + fn test_from_key_pair_ecdsa() -> Result<()> { + let kp = rcgen::KeyPair::generate_for(&rcgen::PKCS_ECDSA_P256_SHA256) + .map_err(|e| Error::Other(e.to_string()))?; + let pk = CryptoPrivateKey::from_key_pair(&kp)?; + assert!(matches!(pk.kind, CryptoPrivateKeyKind::Ecdsa256(_))); + assert_eq!(pk.serialized_der, kp.serialize_der()); + Ok(()) + } + + /// Test `TryFrom<&KeyPair>` delegates to `from_key_pair`. + #[test] + fn test_try_from_key_pair() -> Result<()> { + let kp = rcgen::KeyPair::generate_for(&rcgen::PKCS_ED25519) + .map_err(|e| Error::Other(e.to_string()))?; + let pk = CryptoPrivateKey::try_from(&kp)?; + assert!(matches!(pk.kind, CryptoPrivateKeyKind::Ed25519(_))); + Ok(()) + } + + /// Test `generate_certificate_verify` succeeds with Ed25519 key. + #[test] + fn test_generate_certificate_verify_ed25519() -> Result<()> { + let cert = Certificate::generate_self_signed_with_alg( + vec!["localhost".to_owned()], + &rcgen::PKCS_ED25519, + )?; + let sig = generate_certificate_verify(b"test handshake data", &cert.private_key)?; + assert!(!sig.is_empty(), "signature should not be empty"); + Ok(()) + } + + /// Test `generate_certificate_verify` succeeds with ECDSA key. + #[test] + fn test_generate_certificate_verify_ecdsa() -> Result<()> { + let cert = Certificate::generate_self_signed(vec!["localhost".to_owned()])?; + let sig = generate_certificate_verify(b"test handshake data", &cert.private_key)?; + assert!(!sig.is_empty(), "signature should not be empty"); + Ok(()) + } } diff --git a/rtc/src/peer_connection/certificate/mod.rs b/rtc/src/peer_connection/certificate/mod.rs index de445533..d6dbb4bb 100644 --- a/rtc/src/peer_connection/certificate/mod.rs +++ b/rtc/src/peer_connection/certificate/mod.rs @@ -702,4 +702,38 @@ mod test { Ok(()) } + + /// Verify `from_key_pair` with Ed25519 successfully calls `from_params` + /// and `CryptoPrivateKey::from_key_pair` internally, producing valid + /// fingerprints. + #[test] + fn test_from_key_pair_ed25519_produces_fingerprints() -> Result<()> { + let kp = KeyPair::generate_for(&rcgen::PKCS_ED25519)?; + let cert = RTCCertificate::from_key_pair(kp)?; + let fps = cert.get_fingerprints(); + assert_eq!(fps.len(), 1); + assert_eq!(fps[0].algorithm, "sha-256"); + Ok(()) + } + + /// Verify `from_existing` creates a valid certificate with custom expiration. + #[test] + fn test_from_existing_with_custom_expiry() -> Result<()> { + let kp = KeyPair::generate_for(&rcgen::PKCS_ECDSA_P256_SHA256)?; + let cert = RTCCertificate::from_key_pair(kp)?; + let custom_expires = SystemTime::now() + Duration::from_secs(3600); + let cert2 = RTCCertificate::from_existing(cert.dtls_certificate.clone(), custom_expires); + assert_eq!(cert2.expires, custom_expires); + assert_eq!(cert, cert2); // PartialEq ignores expires + Ok(()) + } + + /// Verify `from_key_pair` rejects unsupported key types. + #[test] + fn test_from_key_pair_unsupported_rejects() { + // PKCS_ECDSA_P384_SHA384 is not in the supported list + let kp = KeyPair::generate_for(&rcgen::PKCS_ECDSA_P384_SHA384).unwrap(); + let result = RTCCertificate::from_key_pair(kp); + assert!(result.is_err()); + } } From 68783e637512e8a726c93b146e6f44223a9047de Mon Sep 17 00:00:00 2001 From: nightness Date: Wed, 8 Apr 2026 04:34:05 -0500 Subject: [PATCH 6/7] fix: make CryptoPrivateKey fields fully private, fix unreachable code and test - Make `kind` and `serialized_der` fully private (not pub(crate)) and update all crate-internal callers to use the public accessors - Update doc comment and Clone safety comment to reflect truly private fields - Add doc comment on HandshakeConfig warning against direct Default usage - Replace unreachable ok_or_else in flight5.rs with expect + safety comment - Rewrite flight5 test to validate actual behavior with incomplete state Co-Authored-By: Claude Opus 4.6 (1M context) --- rtc-dtls/src/config.rs | 9 ++++- rtc-dtls/src/crypto/mod.rs | 32 +++++++++--------- rtc-dtls/src/flight/flight5.rs | 35 ++++++++------------ rtc-dtls/src/signature_hash_algorithm/mod.rs | 2 +- 4 files changed, 40 insertions(+), 38 deletions(-) diff --git a/rtc-dtls/src/config.rs b/rtc-dtls/src/config.rs index 22dbefc5..25c33e0a 100644 --- a/rtc-dtls/src/config.rs +++ b/rtc-dtls/src/config.rs @@ -270,7 +270,7 @@ impl ConfigBuilder { } for cert in &self.certificates { - match cert.private_key.kind { + match cert.private_key.kind() { CryptoPrivateKeyKind::Ed25519(_) => {} CryptoPrivateKeyKind::Ecdsa256(_) => {} _ => return Err(Error::ErrInvalidPrivateKey), @@ -423,6 +423,13 @@ pub fn gen_self_signed_root_cert() -> rustls::RootCertStore { certs } +/// Internal handshake configuration used by the DTLS state machine. +/// +/// **Do not construct via `HandshakeConfig::default()` directly.** The default +/// `server_cert_verifier` is a placeholder that always rejects certificates, +/// making a default-constructed `HandshakeConfig` unsuitable for real +/// handshakes. Use [`ConfigBuilder`] instead, which installs a proper +/// verifier via [`ConfigBuilder::build()`]. #[derive(Clone)] pub struct HandshakeConfig { pub(crate) local_psk_callback: Option, diff --git a/rtc-dtls/src/crypto/mod.rs b/rtc-dtls/src/crypto/mod.rs index 0980343c..d5dfa08a 100644 --- a/rtc-dtls/src/crypto/mod.rs +++ b/rtc-dtls/src/crypto/mod.rs @@ -110,7 +110,7 @@ impl Certificate { pub fn serialize_pem(&self) -> String { let mut data = vec![pem::Pem::new( "PRIVATE_KEY".to_string(), - self.private_key.serialized_der.clone(), + self.private_key.serialized_der().to_vec(), )]; for rustls_cert in &self.certificate { data.push(pem::Pem::new( @@ -152,15 +152,16 @@ pub enum CryptoPrivateKeyKind { /// Private key. /// -/// Fields are `pub(crate)` to enforce the invariant that `kind` and -/// `serialized_der` are always consistent. External code should construct -/// instances via [`CryptoPrivateKey::from_key_pair`] or the -/// [`TryFrom<&KeyPair>`] impl; crate-internal code that accesses the fields -/// directly must maintain this invariant. +/// Fields are intentionally private to enforce the invariant that `kind` and +/// `serialized_der` are always consistent. Construct instances via +/// [`CryptoPrivateKey::from_key_pair`] or the [`TryFrom<&KeyPair>`] impl; +/// use the [`kind()`](CryptoPrivateKey::kind) and +/// [`serialized_der()`](CryptoPrivateKey::serialized_der) accessors to +/// inspect the key. #[derive(Debug)] pub struct CryptoPrivateKey { - pub(crate) kind: CryptoPrivateKeyKind, - pub(crate) serialized_der: Vec, + kind: CryptoPrivateKeyKind, + serialized_der: Vec, } impl CryptoPrivateKey { @@ -199,8 +200,9 @@ impl PartialEq for CryptoPrivateKey { impl Clone for CryptoPrivateKey { fn clone(&self) -> Self { - // Safety: `serialized_der` is always produced by `from_key_pair` - // which serialises a valid key. Re-parsing the same DER bytes cannot fail. + // Safety: fields are fully private, so `serialized_der` is always + // produced by `from_key_pair` (or `TryFrom<&KeyPair>`) which serialises + // a valid key. Re-parsing the same DER bytes cannot fail. match &self.kind { CryptoPrivateKeyKind::Ed25519(_) => CryptoPrivateKey { kind: CryptoPrivateKeyKind::Ed25519( @@ -621,8 +623,8 @@ mod test { let kp = rcgen::KeyPair::generate_for(&rcgen::PKCS_ED25519) .map_err(|e| Error::Other(e.to_string()))?; let pk = CryptoPrivateKey::from_key_pair(&kp)?; - assert!(matches!(pk.kind, CryptoPrivateKeyKind::Ed25519(_))); - assert_eq!(pk.serialized_der, kp.serialize_der()); + assert!(matches!(pk.kind(), CryptoPrivateKeyKind::Ed25519(_))); + assert_eq!(pk.serialized_der(), kp.serialize_der().as_slice()); Ok(()) } @@ -632,8 +634,8 @@ mod test { let kp = rcgen::KeyPair::generate_for(&rcgen::PKCS_ECDSA_P256_SHA256) .map_err(|e| Error::Other(e.to_string()))?; let pk = CryptoPrivateKey::from_key_pair(&kp)?; - assert!(matches!(pk.kind, CryptoPrivateKeyKind::Ecdsa256(_))); - assert_eq!(pk.serialized_der, kp.serialize_der()); + assert!(matches!(pk.kind(), CryptoPrivateKeyKind::Ecdsa256(_))); + assert_eq!(pk.serialized_der(), kp.serialize_der().as_slice()); Ok(()) } @@ -643,7 +645,7 @@ mod test { let kp = rcgen::KeyPair::generate_for(&rcgen::PKCS_ED25519) .map_err(|e| Error::Other(e.to_string()))?; let pk = CryptoPrivateKey::try_from(&kp)?; - assert!(matches!(pk.kind, CryptoPrivateKeyKind::Ed25519(_))); + assert!(matches!(pk.kind(), CryptoPrivateKeyKind::Ed25519(_))); Ok(()) } diff --git a/rtc-dtls/src/flight/flight5.rs b/rtc-dtls/src/flight/flight5.rs index ec693be0..1743d501 100644 --- a/rtc-dtls/src/flight/flight5.rs +++ b/rtc-dtls/src/flight/flight5.rs @@ -395,17 +395,11 @@ impl Flight for Flight5 { plain_text.extend_from_slice(&merged); - let cert_ref = certificate.as_ref().ok_or_else(|| { - ( - Some(Alert { - alert_level: AlertLevel::Fatal, - alert_description: AlertDescription::InternalError, - }), - Some(Error::Other( - "no local certificate available for DTLS flight5".to_owned(), - )), - ) - })?; + // Safety: this block is guarded by `!cfg.local_certificates.is_empty()`, + // so `certificate` was set to `Some(..)` above and cannot be `None` here. + let cert_ref = certificate + .as_ref() + .expect("certificate is Some when local_certificates is non-empty"); // Find compatible signature scheme let signature_hash_algo = match select_signature_scheme( @@ -778,26 +772,25 @@ mod tests { use crate::handshake::handshake_cache::HandshakeCache; use crate::state::State; - /// Verify that `Flight5::generate` returns a fatal `InternalError` alert - /// when the server requested a client certificate but no local certificate - /// is available. + /// Verify that `Flight5::generate` fails when invoked with an empty + /// handshake cache and otherwise default state. #[test] - fn generate_no_certificate_returns_internal_error() { + fn generate_with_incomplete_handshake_state_returns_error() { let mut state = State::default(); - // Simulate the server having requested a client certificate. state.remote_requested_certificate = true; let cache = HandshakeCache::new(); - // Config with no local certificates. let cfg = HandshakeConfig::default(); let flight = Flight5; let result = flight.generate(&mut state, &cache, &cfg); - let (alert, error) = result.expect_err("expected error when no certificate is available"); - let alert = alert.expect("expected an alert"); - assert_eq!(alert.alert_level, AlertLevel::Fatal); - assert_eq!(alert.alert_description, AlertDescription::InternalError); + let (alert, error) = result.expect_err( + "expected error when generate is called without the required handshake state", + ); + if let Some(alert) = alert { + assert_eq!(alert.alert_level, AlertLevel::Fatal); + } assert!(error.is_some(), "expected an error value"); } } diff --git a/rtc-dtls/src/signature_hash_algorithm/mod.rs b/rtc-dtls/src/signature_hash_algorithm/mod.rs index 9b2062af..3bf0974f 100644 --- a/rtc-dtls/src/signature_hash_algorithm/mod.rs +++ b/rtc-dtls/src/signature_hash_algorithm/mod.rs @@ -96,7 +96,7 @@ pub struct SignatureHashAlgorithm { impl SignatureHashAlgorithm { // is_compatible checks that given private key is compatible with the signature scheme. pub(crate) fn is_compatible(&self, private_key: &CryptoPrivateKey) -> bool { - match &private_key.kind { + match private_key.kind() { CryptoPrivateKeyKind::Ed25519(_) => self.signature == SignatureAlgorithm::Ed25519, CryptoPrivateKeyKind::Ecdsa256(_) => self.signature == SignatureAlgorithm::Ecdsa, CryptoPrivateKeyKind::Rsa256(_) => self.signature == SignatureAlgorithm::Rsa, From 137fe6d8c784a7ba868562a638d0e8f7f4f29d58 Mon Sep 17 00:00:00 2001 From: nightness Date: Wed, 8 Apr 2026 14:12:46 -0500 Subject: [PATCH 7/7] fix: restore graceful error return instead of expect() in flight5 The ok_or_else error path is currently unreachable due to the outer guard, but keeping it avoids introducing a new panic in a PR that removes panics. Defense-in-depth. Co-Authored-By: Claude Opus 4.6 (1M context) --- rtc-dtls/src/flight/flight5.rs | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/rtc-dtls/src/flight/flight5.rs b/rtc-dtls/src/flight/flight5.rs index 1743d501..257eac26 100644 --- a/rtc-dtls/src/flight/flight5.rs +++ b/rtc-dtls/src/flight/flight5.rs @@ -395,11 +395,20 @@ impl Flight for Flight5 { plain_text.extend_from_slice(&merged); - // Safety: this block is guarded by `!cfg.local_certificates.is_empty()`, - // so `certificate` was set to `Some(..)` above and cannot be `None` here. - let cert_ref = certificate - .as_ref() - .expect("certificate is Some when local_certificates is non-empty"); + // Note: this path is currently unreachable because the outer guard + // ensures `!cfg.local_certificates.is_empty()`, but we keep the + // graceful error return rather than panicking for defense-in-depth. + let cert_ref = certificate.as_ref().ok_or_else(|| { + ( + Some(Alert { + alert_level: AlertLevel::Fatal, + alert_description: AlertDescription::InternalError, + }), + Some(Error::Other( + "no local certificate available for DTLS flight5".to_owned(), + )), + ) + })?; // Find compatible signature scheme let signature_hash_algo = match select_signature_scheme(