Repository navigation
chore: enforce hybrid PQ key exchange in mTLS handshakes #922
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,15 +13,22 @@ use tfhe_versionable::{Versionize, VersionsDispatch}; | |
| use tokio_rustls::rustls::{ | ||
| DigitallySignedStruct, DistinguishedName, Error, RootCertStore, SignatureScheme, | ||
| client::{ | ||
| WebPkiServerVerifier, | ||
| danger::{HandshakeSignatureValid, ServerCertVerified, ServerCertVerifier}, | ||
| ClientConfig, ResolvesClientCert, WebPkiServerVerifier, | ||
| danger::{ | ||
| DangerousClientConfigBuilder, HandshakeSignatureValid, ServerCertVerified, | ||
| ServerCertVerifier, | ||
| }, | ||
| }, | ||
| crypto::{ | ||
| CryptoProvider, WebPkiSupportedAlgorithms, | ||
| aws_lc_rs::{default_provider, kx_group}, | ||
| }, | ||
| crypto::{CryptoProvider, WebPkiSupportedAlgorithms}, | ||
| pki_types::{CertificateDer, ServerName, UnixTime}, | ||
| server::{ | ||
| WebPkiClientVerifier, | ||
| ResolvesServerCert, ServerConfig, WebPkiClientVerifier, | ||
| danger::{ClientCertVerified, ClientCertVerifier}, | ||
| }, | ||
| version::TLS13, | ||
| }; | ||
| use x509_parser::{certificate::X509Certificate, parse_x509_certificate, pem::Pem}; | ||
|
|
||
|
|
@@ -154,6 +161,32 @@ impl std::fmt::Debug for AttestedVerifier { | |
| } | ||
| } | ||
|
|
||
| /// Constructs mutually authenticated P2P TLS configurations with hybrid-only key exchange. | ||
| /// | ||
| /// Both endpoints require TLS 1.3 and prefer [`kx_group::X25519MLKEM768`] over [`kx_group::SECP256R1MLKEM768`]. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason why we support both ? and in that order ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason was that
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. x25519 is generally consider a safer choice than NIST, in particular when it comes to NIST's *R1 curves as there has been a bit of fear of weaknesses in the randomness.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, under further investigation X25519 is not FIPS approved. So we should probably go with secp256r1 as the default |
||
| /// Returns an error if the provider cannot support TLS 1.3. | ||
| pub fn build_p2p_tls_config<R>( | ||
| verifier: Arc<AttestedVerifier>, | ||
| cert_resolver: Arc<R>, | ||
| ) -> Result<(ServerConfig, ClientConfig), Error> | ||
| where | ||
| R: ResolvesServerCert + ResolvesClientCert + 'static, | ||
| { | ||
| let mut provider = default_provider(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR talks about handshake only, but I'm wondering do we want to force AES 256 as well ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, will restrict session ciphers too.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that this would be greater than the classical signature (128 bits) and PQ (192 bits). Still good but might be a bit overkill with current paramters |
||
| provider.kx_groups = vec![kx_group::X25519MLKEM768, kx_group::SECP256R1MLKEM768]; | ||
| let provider = Arc::new(provider); | ||
| let server_config = ServerConfig::builder_with_provider(provider.clone()) | ||
| .with_protocol_versions(&[&TLS13])? | ||
| .with_client_cert_verifier(verifier.clone()) | ||
| .with_cert_resolver(cert_resolver.clone()); | ||
| let client_config = DangerousClientConfigBuilder { | ||
| cfg: ClientConfig::builder_with_provider(provider).with_protocol_versions(&[&TLS13])?, | ||
| } | ||
| .with_custom_certificate_verifier(verifier) | ||
| .with_client_cert_resolver(cert_resolver); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Claude pointed out that session resuming skips the attested verifier, and hence leaves a potential attack vector. It should be easy to solve with |
||
| Ok((server_config, client_config)) | ||
| } | ||
|
|
||
| impl AttestedVerifier { | ||
| pub fn new( | ||
| user_data_verifier: Option<Arc<UserDataVerifier>>, | ||
|
|
@@ -886,6 +919,180 @@ pub async fn generate_mock_tls_cert_with_attestation( | |
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod p2p_tests { | ||
| use super::*; | ||
| use rcgen::{ | ||
| CertificateParams, DnType, ExtendedKeyUsagePurpose, KeyPair, PKCS_ECDSA_P256_SHA256, | ||
| }; | ||
| use std::{collections::HashMap, time::Duration}; | ||
| use threshold_types::{party::MpcIdentity, session_id::SessionId}; | ||
| use tokio_rustls::{ | ||
| TlsAcceptor, TlsConnector, | ||
| rustls::{ | ||
| NamedGroup, PeerIncompatible, | ||
| pki_types::PrivateKeyDer, | ||
| sign::{CertifiedKey, SingleCertAndKey}, | ||
| }, | ||
| }; | ||
|
|
||
| async fn handshake( | ||
| server: ServerConfig, | ||
| client: ClientConfig, | ||
| ) -> ( | ||
| std::io::Result<tokio_rustls::server::TlsStream<tokio::io::DuplexStream>>, | ||
| std::io::Result<tokio_rustls::client::TlsStream<tokio::io::DuplexStream>>, | ||
| ) { | ||
| let (server_io, client_io) = tokio::io::duplex(16384); | ||
| let acceptor = TlsAcceptor::from(Arc::new(server)); | ||
| let connector = TlsConnector::from(Arc::new(client)); | ||
| tokio::time::timeout(Duration::from_secs(5), async { | ||
| tokio::join!( | ||
| acceptor.accept(server_io), | ||
| connector.connect("p2p-test".try_into().unwrap(), client_io), | ||
| ) | ||
| }) | ||
| .await | ||
| .expect("TLS handshake timed out") | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn p2p_tls_requires_hybrid_key_exchange() { | ||
| let _ = default_provider().install_default(); | ||
| let key = KeyPair::generate_for(&PKCS_ECDSA_P256_SHA256).unwrap(); | ||
| let mut params = CertificateParams::new(vec!["p2p-test".to_string()]).unwrap(); | ||
| params | ||
| .distinguished_name | ||
| .push(DnType::CommonName, "p2p-test"); | ||
| params.extended_key_usages = vec![ | ||
| ExtendedKeyUsagePurpose::ServerAuth, | ||
| ExtendedKeyUsagePurpose::ClientAuth, | ||
| ]; | ||
| let cert = params.self_signed(&key).unwrap(); | ||
| let cert_resolver = Arc::new(SingleCertAndKey::from( | ||
| CertifiedKey::from_der( | ||
| vec![cert.der().clone()], | ||
| PrivateKeyDer::try_from(key.serialize_der()).unwrap(), | ||
| &default_provider(), | ||
| ) | ||
| .unwrap(), | ||
| )); | ||
| let verifier = Arc::new( | ||
| AttestedVerifier::new( | ||
| None, | ||
| false, | ||
| #[cfg(feature = "insecure")] | ||
| false, | ||
| ) | ||
| .unwrap(), | ||
| ); | ||
| let (server, client) = build_p2p_tls_config(verifier.clone(), cert_resolver).unwrap(); | ||
| verifier | ||
| .add_context( | ||
| SessionId::new(&"hybrid-only").unwrap(), | ||
| HashMap::from([( | ||
| MpcIdentity("p2p-test".to_string()), | ||
| x509_parser::pem::parse_x509_pem(cert.pem().as_bytes()) | ||
| .unwrap() | ||
| .1, | ||
| )]), | ||
| None, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| for config_groups in [ | ||
| &server.crypto_provider().kx_groups, | ||
| &client.crypto_provider().kx_groups, | ||
| ] { | ||
| assert_eq!( | ||
| config_groups | ||
| .iter() | ||
| .map(|group| group.name()) | ||
| .collect::<Vec<_>>(), | ||
| vec![NamedGroup::X25519MLKEM768, NamedGroup::secp256r1MLKEM768], | ||
| ); | ||
| } | ||
|
|
||
| for group in [ | ||
| kx_group::X25519MLKEM768, | ||
| kx_group::SECP256R1MLKEM768, | ||
| kx_group::X25519, | ||
| kx_group::SECP256R1, | ||
| kx_group::SECP384R1, | ||
| kx_group::MLKEM768, | ||
| kx_group::MLKEM1024, | ||
| ] { | ||
| let mut provider = default_provider(); | ||
| provider.kx_groups = vec![group]; | ||
| let provider = Arc::new(provider); | ||
| let peer_server = ServerConfig::builder_with_provider(provider.clone()) | ||
| .with_protocol_versions(&[&TLS13]) | ||
| .unwrap() | ||
| .with_client_cert_verifier(verifier.clone()) | ||
| .with_cert_resolver(server.cert_resolver.clone()); | ||
| let peer_client = DangerousClientConfigBuilder { | ||
| cfg: ClientConfig::builder_with_provider(provider) | ||
| .with_protocol_versions(&[&TLS13]) | ||
| .unwrap(), | ||
| } | ||
| .with_custom_certificate_verifier(verifier.clone()) | ||
| .with_client_cert_resolver(client.client_auth_cert_resolver.clone()); | ||
|
|
||
| for (server_result, client_result) in [ | ||
| handshake(server.clone(), peer_client).await, | ||
| handshake(peer_server, client.clone()).await, | ||
| ] { | ||
| if matches!( | ||
| group.name(), | ||
| NamedGroup::X25519MLKEM768 | NamedGroup::secp256r1MLKEM768 | ||
| ) { | ||
| let server_stream = server_result.unwrap(); | ||
| let client_stream = client_result.unwrap(); | ||
| assert_eq!( | ||
| server_stream.get_ref().1.protocol_version(), | ||
| Some(tokio_rustls::rustls::ProtocolVersion::TLSv1_3) | ||
| ); | ||
| assert_eq!( | ||
| server_stream | ||
| .get_ref() | ||
| .1 | ||
| .negotiated_key_exchange_group() | ||
| .unwrap() | ||
| .name(), | ||
| group.name() | ||
| ); | ||
| assert_eq!( | ||
| client_stream | ||
| .get_ref() | ||
| .1 | ||
| .negotiated_key_exchange_group() | ||
| .unwrap() | ||
| .name(), | ||
| group.name() | ||
| ); | ||
| assert!(server_stream.get_ref().1.peer_certificates().is_some()); | ||
| assert!(client_stream.get_ref().1.peer_certificates().is_some()); | ||
| } else { | ||
| let error = server_result.unwrap_err(); | ||
| assert!( | ||
| matches!( | ||
| error | ||
| .get_ref() | ||
| .and_then(|error| error.downcast_ref::<Error>()), | ||
| Some(Error::PeerIncompatible( | ||
| PeerIncompatible::NoKxGroupsInCommon | ||
| )) | ||
| ), | ||
| "unexpected rejection for {:?}: {error}", | ||
| group.name(), | ||
| ); | ||
| assert!(client_result.is_err(), "client accepted {:?}", group.name()); | ||
|
Comment on lines
+1076
to
+1089
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit, why an assert on the client's error but an unwrap on the server's one ? |
||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Claude pointed out that it would be a good idea to also add a test to validate the HelloRetryRequest path, to ensure that the next release can work gracefully with the 0.15 during a rolling upgrade. In theory it should work without issue, but would be good to have a test |
||
|
|
||
| #[cfg(all(test, feature = "insecure"))] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I could read from the NIST documents MLKEM768 provides level 3, equivalent to 192 bit AES, whereas secp256r1 (and by extension) x25519 provides around 128 bits security. So I guess we either want to use P384 or MLKEM512 to have the levels consistent.
However, we already used MLKEM1024 with P384, so we have had a tendency of selecting PQ parameters higher than their classical counterparts.