diff --git a/rtc-dtls/src/config.rs b/rtc-dtls/src/config.rs index 028950d4..25c33e0a 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. @@ -269,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), @@ -344,13 +345,14 @@ 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, 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 @@ -372,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, @@ -425,6 +483,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, @@ -441,11 +505,8 @@ impl Default for HandshakeConfig { insecure_verification: false, verify_peer_certificate: None, roots_cas: rustls::RootCertStore::empty(), - server_cert_verifier: rustls::client::WebPkiServerVerifier::builder(Arc::new( - gen_self_signed_root_cert(), - )) - .build() - .unwrap(), + // 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, @@ -456,6 +517,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 9b4b3ef9..d5dfa08a 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()], @@ -106,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( @@ -147,12 +151,29 @@ 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; +/// use the [`kind()`](CryptoPrivateKey::kind) and +/// [`serialized_der()`](CryptoPrivateKey::serialized_der) accessors to +/// inspect the key. #[derive(Debug)] pub struct CryptoPrivateKey { - /// Keypair. - pub kind: CryptoPrivateKeyKind, - /// DER-encoded keypair. - pub serialized_der: Vec, + kind: CryptoPrivateKeyKind, + 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 { @@ -179,10 +200,14 @@ impl PartialEq for CryptoPrivateKey { impl Clone for CryptoPrivateKey { fn clone(&self) -> Self { - match self.kind { + // 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( - 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 +218,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(), }, @@ -525,4 +551,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().as_slice()); + 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().as_slice()); + 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-dtls/src/flight/flight5.rs b/rtc-dtls/src/flight/flight5.rs index c738cd28..257eac26 100644 --- a/rtc-dtls/src/flight/flight5.rs +++ b/rtc-dtls/src/flight/flight5.rs @@ -395,10 +395,25 @@ impl Flight for Flight5 { plain_text.extend_from_slice(&merged); + // 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( &cfg.local_signature_schemes, - &certificate.as_ref().unwrap().private_key, + &cert_ref.private_key, ) { Ok(s) => s, Err(err) => { @@ -412,10 +427,8 @@ impl Flight for Flight5 { } }; - let cert_verify = match generate_certificate_verify( - &plain_text, - &certificate.as_ref().unwrap().private_key, /*, signature_hash_algo.hash*/ - ) { + let cert_verify = match generate_certificate_verify(&plain_text, &cert_ref.private_key) + { Ok(cert) => cert, Err(err) => { return Err(( @@ -761,3 +774,32 @@ 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` fails when invoked with an empty + /// handshake cache and otherwise default state. + #[test] + fn generate_with_incomplete_handshake_state_returns_error() { + let mut state = State::default(); + state.remote_requested_certificate = true; + + let cache = HandshakeCache::new(); + let cfg = HandshakeConfig::default(); + + let flight = Flight5; + let result = flight.generate(&mut state, &cache, &cfg); + + 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, diff --git a/rtc/src/peer_connection/certificate/mod.rs b/rtc/src/peer_connection/certificate/mod.rs index 1c11580c..d6dbb4bb 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; @@ -341,40 +338,10 @@ 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 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 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") { // Workaround for issue overflow when adding duration to instant on armv7 @@ -735,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()); + } }