Skip to content
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

Remove quinn-proto rustls config newtypes #511

Merged
merged 1 commit into from
Nov 23, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion interop/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ fn run(options: Opt) -> Result<()> {
tls_config.key_log = Arc::new(rustls::KeyLogFile::new());
}
let client_config = quinn::ClientConfig {
crypto: quinn::crypto::rustls::ClientConfig::new(tls_config),
crypto: Arc::new(tls_config),
transport: Arc::new(quinn::TransportConfig {
idle_timeout: 1_000,
..Default::default()
Expand Down
3 changes: 1 addition & 2 deletions quinn-h3/examples/h3.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,14 @@ use structopt::{self, StructOpt};
use tokio::io::{AsyncReadExt, AsyncWriteExt};
use url::Url;

use quinn::ConnectionDriver as QuicDriver;
use quinn::{Certificate, CertificateChain, ConnectionDriver as QuicDriver, PrivateKey};
use quinn_h3::{
self,
body::RecvBody,
client::{Builder as ClientBuilder, Client},
connection::ConnectionDriver,
server::{Builder as ServerBuilder, IncomingRequest, Sender},
};
use quinn_proto::crypto::rustls::{Certificate, CertificateChain, PrivateKey};

mod shared;
use shared::build_certs;
Expand Down
2 changes: 1 addition & 1 deletion quinn-h3/examples/shared/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::{fs, io, path::PathBuf};

use anyhow::{bail, Context, Result};
use quinn_proto::crypto::rustls::{Certificate, CertificateChain, PrivateKey};
use quinn::{Certificate, CertificateChain, PrivateKey};
use tracing::info;

pub fn build_certs(
Expand Down
14 changes: 12 additions & 2 deletions quinn-proto/src/crypto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@ use crate::{

/// Cryptography interface based on *ring*
#[cfg(feature = "ring")]
pub mod ring;
pub(crate) mod ring;
/// TLS interface based on rustls
#[cfg(feature = "rustls")]
pub mod rustls;
pub(crate) mod rustls;

/// A cryptographic session (commonly TLS)
pub trait Session: Sized {
Expand Down Expand Up @@ -87,6 +87,11 @@ pub trait ClientConfig<S>
where
S: Session,
{
/// Construct the default configuration
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use Default for this, instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't implement Default on Arc<rustls::Client/ServerConfig>.

fn new() -> Self
where
Self: Sized;

/// Start a client session with this configuration
fn start_session(
&self,
Expand All @@ -100,6 +105,11 @@ pub trait ServerConfig<S>
where
S: Session,
{
/// Construct the default configuration
fn new() -> Self
where
Self: Sized;

/// Start a server session with this configuration
fn start_session(&self, params: &TransportParameters) -> S;
}
Expand Down
254 changes: 16 additions & 238 deletions quinn-proto/src/crypto/rustls.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use std::{
fmt, io,
io,
ops::{Deref, DerefMut},
str,
sync::Arc,
Expand All @@ -9,9 +9,9 @@ use ring::{hkdf, hmac};
pub use rustls::TLSError;
use rustls::{
self,
internal::{msgs::enums::HashAlgorithm, pemfile},
internal::msgs::enums::HashAlgorithm,
quic::{ClientQuicExt, Secrets, ServerQuicExt},
KeyLogFile, NoClientAuth, ProtocolVersion, Session,
Session,
};
use webpki::DNSNameRef;

Expand Down Expand Up @@ -39,10 +39,10 @@ impl TlsSession {
}

impl crypto::Session for TlsSession {
type ClientConfig = ClientConfig;
type ClientConfig = Arc<rustls::ClientConfig>;
type HmacKey = hmac::Key;
type Keys = Crypto;
type ServerConfig = ServerConfig;
type ServerConfig = Arc<rustls::ServerConfig>;

fn alpn_protocol(&self) -> Option<&[u8]> {
self.get_alpn_protocol()
Expand Down Expand Up @@ -159,77 +159,14 @@ impl DerefMut for TlsSession {
}
}

/// rustls configuration for client sessions
#[derive(Clone)]
pub struct ClientConfig(Arc<rustls::ClientConfig>);

impl ClientConfig {
/// Initialize new configuration with an existing rustls `ClientConfig`
pub fn new(config: rustls::ClientConfig) -> Self {
Self(Arc::new(config))
}

/// Add a trusted certificate authority.
///
/// For more advanced/less secure certificate verification, construct a [`ClientConfig`]
/// manually and use rustls's `dangerous_configuration` feature to override the certificate
/// verifier.
pub fn add_certificate_authority(&mut self, cert: Certificate) -> Result<(), webpki::Error> {
let anchor = webpki::trust_anchor_util::cert_der_as_trust_anchor(&cert.inner.0)?;
Arc::make_mut(&mut self.0)
.root_store
.add_server_trust_anchors(&webpki::TLSServerTrustAnchors(&[anchor]));
Ok(())
}

/// Enable NSS-compatible cryptographic key logging to the `SSLKEYLOGFILE` environment variable
///
/// Useful for debugging encrypted communications with protocol analyzers such as Wireshark.
pub fn enable_keylog(&mut self) {
Arc::make_mut(&mut self.0).key_log = Arc::new(KeyLogFile::new());
}

/// Set the application-layer protocols to accept, in order of descending preference
///
/// When set, clients which don't declare support for at least one of the supplied protocols will be rejected.
///
/// The IANA maintains a [registry] of standard protocol IDs, but custom IDs may be used as well.
///
/// [registry]: https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#alpn-protocol-ids
pub fn set_protocols(&mut self, protocols: &[&[u8]]) {
Arc::make_mut(&mut self.0).alpn_protocols = protocols.iter().map(|x| x.to_vec()).collect();
}
}

impl Default for ClientConfig {
fn default() -> ClientConfig {
impl crypto::ClientConfig<TlsSession> for Arc<rustls::ClientConfig> {
fn new() -> Self {
let mut cfg = rustls::ClientConfig::new();
cfg.versions = vec![ProtocolVersion::TLSv1_3];
cfg.versions = vec![rustls::ProtocolVersion::TLSv1_3];
cfg.enable_early_data = true;
Self(Arc::new(cfg))
}
}

impl fmt::Debug for ClientConfig {
fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(fmt, "ClientConfig(rustls::ClientConfig)")
}
}

impl Deref for ClientConfig {
type Target = Arc<rustls::ClientConfig>;
fn deref(&self) -> &Self::Target {
&self.0
}
}

impl DerefMut for ClientConfig {
fn deref_mut(&mut self) -> &mut Arc<rustls::ClientConfig> {
&mut self.0
Arc::new(cfg)
}
}

impl crypto::ClientConfig<TlsSession> for ClientConfig {
fn start_session(
&self,
server_name: &str,
Expand All @@ -238,170 +175,23 @@ impl crypto::ClientConfig<TlsSession> for ClientConfig {
let pki_server_name = DNSNameRef::try_from_ascii_str(server_name)
.map_err(|_| ConnectError::InvalidDnsName(server_name.into()))?;
Ok(TlsSession::Client(rustls::ClientSession::new_quic(
&self.0,
self,
pki_server_name,
to_vec(params),
)))
}
}

/// rustls configuration for server sessions
#[derive(Clone)]
pub struct ServerConfig(Arc<rustls::ServerConfig>);

impl ServerConfig {
/// Initialize new configuration with an existing rustls `ServerConfig`
pub fn new(config: rustls::ServerConfig) -> Self {
Self(Arc::new(config))
}

/// Set the certificate chain that will be presented to clients
pub fn set_certificate(
&mut self,
cert_chain: CertificateChain,
key: PrivateKey,
) -> Result<(), TLSError> {
Arc::make_mut(&mut self.0).set_single_cert(cert_chain.certs, key.inner)?;
Ok(())
}

/// Enable NSS-compatible cryptographic key logging to the `SSLKEYLOGFILE` environment variable
///
/// Useful for debugging encrypted communications with protocol analyzers such as Wireshark.
pub fn enable_keylog(&mut self) {
Arc::make_mut(&mut self.0).key_log = Arc::new(KeyLogFile::new());
}

/// Set the application-layer protocols to accept, in order of descending preference
///
/// When set, clients which don't declare support for at least one of the supplied protocols will be rejected.
///
/// The IANA maintains a [registry] of standard protocol IDs, but custom IDs may be used as well.
///
/// [registry]: https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#alpn-protocol-ids
pub fn set_protocols(&mut self, protocols: &[&[u8]]) {
Arc::make_mut(&mut self.0).alpn_protocols = protocols.iter().map(|x| x.to_vec()).collect();
}
}

impl fmt::Debug for ServerConfig {
fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(fmt, "ServerConfig(rustls::ServerConfig)")
}
}

impl Default for ServerConfig {
fn default() -> Self {
let mut cfg = rustls::ServerConfig::new(NoClientAuth::new());
cfg.versions = vec![ProtocolVersion::TLSv1_3];
impl crypto::ServerConfig<TlsSession> for Arc<rustls::ServerConfig> {
fn new() -> Self {
let mut cfg = rustls::ServerConfig::new(rustls::NoClientAuth::new());
cfg.versions = vec![rustls::ProtocolVersion::TLSv1_3];
cfg.max_early_data_size = u32::max_value();
Self(Arc::new(cfg))
Arc::new(cfg)
}
}

impl crypto::ServerConfig<TlsSession> for ServerConfig {
fn start_session(&self, params: &TransportParameters) -> TlsSession {
TlsSession::Server(rustls::ServerSession::new_quic(&self.0, to_vec(params)))
}
}

impl Deref for ServerConfig {
type Target = Arc<rustls::ServerConfig>;
fn deref(&self) -> &Self::Target {
&self.0
}
}

impl DerefMut for ServerConfig {
fn deref_mut(&mut self) -> &mut Arc<rustls::ServerConfig> {
&mut self.0
}
}

/// A single TLS certificate
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you move these into quinn?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the newtypes removed from quinn-proto, they're only used in the quinn builders, so they seemed out of place here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like they should live closer to the rustls implementation bits, so that they could also be swapped out along with them. Do you think that would make sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They also seem like things that a quinn-proto user might still find useful if they wanted to bypass the tokio stuff.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like they should live closer to the rustls implementation bits, so that they could also be swapped out along with them.

Other cryptographic protocols (e.g. noise) won't necessarily have analogous abstractions. For the specific case of other TLS implementations, we might prefer to expose a single abstract type that can be consumed by all backends, though that's pretty speculative.

They also seem like things that a quinn-proto user might still find useful if they wanted to bypass the tokio stuff.

The sole function of these types was to allow certificates/keys to be constructed without leaving the crate and passed into the helper methods that were removed, and the analogous builder methods in quinn which have been retained. While there's nonzero value in providing those helpers to hypothetical quinn-proto users, I think the resulting quinn-proto API was gratuitously weird for both quinn and quinn-proto users who need to access the rustls configuration directly, and maintaining two separate incomplete abstractions over rustls configurations wasn't a great situation.

Meanwhile, the genuine rustls API is perfectly usable. The justification for wrapping it as all is to maximize approachability for a new high-level API user, but at the quinn-proto level I think the calculus is different.

#[derive(Debug, Clone)]
pub struct Certificate {
inner: rustls::Certificate,
}

impl Certificate {
/// Parse a DER-formatted certificate
pub fn from_der(der: &[u8]) -> Result<Self, ParseError> {
Ok(Self {
inner: rustls::Certificate(der.to_vec()),
})
}
}

/// A chain of signed TLS certificates ending the one to be used by a server
#[derive(Debug, Clone)]
pub struct CertificateChain {
certs: Vec<rustls::Certificate>,
}

impl CertificateChain {
/// Parse a PEM-formatted certificate chain
///
/// ```no_run
/// let pem = std::fs::read("fullchain.pem").expect("error reading certificates");
/// let cert_chain = quinn_proto::crypto::rustls::PrivateKey::from_pem(&pem).expect("error parsing certificates");
/// ```
pub fn from_pem(pem: &[u8]) -> Result<Self, ParseError> {
Ok(Self {
certs: pemfile::certs(&mut &pem[..])
.map_err(|()| ParseError("malformed certificate chain"))?,
})
}

/// Construct a certificate chain from a list of certificates
pub fn from_certs(certs: impl IntoIterator<Item = Certificate>) -> Self {
certs.into_iter().collect()
}
}

impl std::iter::FromIterator<Certificate> for CertificateChain {
fn from_iter<T>(iter: T) -> Self
where
T: IntoIterator<Item = Certificate>,
{
CertificateChain {
certs: iter.into_iter().map(|x| x.inner).collect(),
}
}
}

/// The private key of a TLS certificate to be used by a server
#[derive(Debug, Clone)]
pub struct PrivateKey {
inner: rustls::PrivateKey,
}

impl PrivateKey {
/// Parse a PEM-formatted private key
///
/// ```no_run
/// let pem = std::fs::read("key.pem").expect("error reading key");
/// let key = quinn_proto::crypto::rustls::PrivateKey::from_pem(&pem).expect("error parsing key");
/// ```
pub fn from_pem(pem: &[u8]) -> Result<Self, ParseError> {
let pkcs8 = pemfile::pkcs8_private_keys(&mut &pem[..])
.map_err(|()| ParseError("malformed PKCS #8 private key"))?;
if let Some(x) = pkcs8.into_iter().next() {
return Ok(Self { inner: x });
}
let rsa = pemfile::rsa_private_keys(&mut &pem[..])
.map_err(|()| ParseError("malformed PKCS #1 private key"))?;
if let Some(x) = rsa.into_iter().next() {
return Ok(Self { inner: x });
}
Err(ParseError("no private key found"))
}

/// Parse a DER-encoded (binary) private key
pub fn from_der(der: &[u8]) -> Result<Self, ParseError> {
Ok(Self {
inner: rustls::PrivateKey(der.to_vec()),
})
TlsSession::Server(rustls::ServerSession::new_quic(self, to_vec(params)))
}
}

Expand All @@ -419,18 +209,6 @@ fn update_secrets(hash_alg: HashAlgorithm, client: &hkdf::Prk, server: &hkdf::Pr
}
}

/// Errors encountered while parsing a TLS certificate or private key
#[derive(Debug, Clone)]
pub struct ParseError(&'static str);

impl std::error::Error for ParseError {}

impl fmt::Display for ParseError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
f.pad(self.0)
}
}

fn to_vec(params: &TransportParameters) -> Vec<u8> {
let mut bytes = Vec::new();
params.write(&mut bytes);
Expand Down
Loading