From cb12a9350332860971387e3a1fb40dc77fa992d3 Mon Sep 17 00:00:00 2001 From: Matt Mastracci Date: Mon, 8 Apr 2024 15:01:02 -0600 Subject: [PATCH] refactor(ext/tls): use cppgc to deduplicate the tls key loading code (#23289) Pass the certificates and key files as CPPGC objects. Towards #23233 --- ext/fetch/22_http_client.js | 8 ++ ext/fetch/lib.rs | 25 ++----- ext/http/00_serve.js | 4 +- ext/kv/remote.rs | 3 +- ext/net/02_tls.js | 51 ++++++++++++- ext/net/lib.rs | 3 + ext/net/ops_tls.rs | 142 ++++++++++++++++++++---------------- ext/tls/lib.rs | 37 ++++++---- 8 files changed, 172 insertions(+), 101 deletions(-) diff --git a/ext/fetch/22_http_client.js b/ext/fetch/22_http_client.js index c1ddbd7c4f..e1389bbe1f 100644 --- a/ext/fetch/22_http_client.js +++ b/ext/fetch/22_http_client.js @@ -14,6 +14,7 @@ import { core, primordials } from "ext:core/mod.js"; import { SymbolDispose } from "ext:deno_web/00_infra.js"; import { op_fetch_custom_client } from "ext:core/ops"; +import { loadTlsKeyPair } from "ext:deno_net/02_tls.js"; const { internalRidSymbol } = core; const { ObjectDefineProperty } = primordials; @@ -24,9 +25,16 @@ const { ObjectDefineProperty } = primordials; */ function createHttpClient(options) { options.caCerts ??= []; + const keyPair = loadTlsKeyPair( + options.cert, + undefined, + options.key, + undefined, + ); return new HttpClient( op_fetch_custom_client( options, + keyPair, ), ); } diff --git a/ext/fetch/lib.rs b/ext/fetch/lib.rs index aeac33973b..e384a918e4 100644 --- a/ext/fetch/lib.rs +++ b/ext/fetch/lib.rs @@ -44,6 +44,8 @@ use deno_tls::Proxy; use deno_tls::RootCertStoreProvider; use data_url::DataUrl; +use deno_tls::TlsKey; +use deno_tls::TlsKeys; use http_v02::header::CONTENT_LENGTH; use http_v02::Uri; use reqwest::header::HeaderMap; @@ -78,7 +80,7 @@ pub struct Options { pub request_builder_hook: Option Result>, pub unsafely_ignore_certificate_errors: Option>, - pub client_cert_chain_and_key: Option<(String, String)>, + pub client_cert_chain_and_key: Option, pub file_fetch_handler: Rc, } @@ -794,8 +796,6 @@ impl HttpClientResource { pub struct CreateHttpClientArgs { ca_certs: Vec, proxy: Option, - cert: Option, - key: Option, pool_max_idle_per_host: Option, pool_idle_timeout: Option, #[serde(default = "default_true")] @@ -815,6 +815,7 @@ fn default_true() -> bool { pub fn op_fetch_custom_client( state: &mut OpState, #[serde] args: CreateHttpClientArgs, + #[cppgc] tls_keys: &deno_tls::TlsKeys, ) -> Result where FP: FetchPermissions + 'static, @@ -825,19 +826,9 @@ where permissions.check_net_url(&url, "Deno.createHttpClient()")?; } - let client_cert_chain_and_key = { - if args.cert.is_some() || args.key.is_some() { - let cert_chain = args - .cert - .ok_or_else(|| type_error("No certificate chain provided"))?; - let private_key = args - .key - .ok_or_else(|| type_error("No private key provided"))?; - - Some((cert_chain, private_key)) - } else { - None - } + let client_cert_chain_and_key = match tls_keys { + TlsKeys::Null => None, + TlsKeys::Static(key) => Some(key.clone()), }; let options = state.borrow::(); @@ -885,7 +876,7 @@ pub struct CreateHttpClientOptions { pub ca_certs: Vec>, pub proxy: Option, pub unsafely_ignore_certificate_errors: Option>, - pub client_cert_chain_and_key: Option<(String, String)>, + pub client_cert_chain_and_key: Option, pub pool_max_idle_per_host: Option, pub pool_idle_timeout: Option>, pub http1: bool, diff --git a/ext/http/00_serve.js b/ext/http/00_serve.js index c87480a112..660287edb5 100644 --- a/ext/http/00_serve.js +++ b/ext/http/00_serve.js @@ -70,7 +70,7 @@ import { resourceForReadableStream, } from "ext:deno_web/06_streams.js"; import { listen, listenOptionApiName, TcpConn } from "ext:deno_net/01_net.js"; -import { listenTls } from "ext:deno_net/02_tls.js"; +import { hasTlsKeyPairOptions, listenTls } from "ext:deno_net/02_tls.js"; import { SymbolAsyncDispose } from "ext:deno_web/00_infra.js"; const _upgraded = Symbol("_upgraded"); @@ -535,7 +535,7 @@ function serve(arg1, arg2) { options = {}; } - const wantsHttps = options.cert || options.key; + const wantsHttps = hasTlsKeyPairOptions(options); const wantsUnix = ObjectHasOwn(options, "path"); const signal = options.signal; const onError = options.onError ?? function (error) { diff --git a/ext/kv/remote.rs b/ext/kv/remote.rs index 007f6aff60..88127fc8fa 100644 --- a/ext/kv/remote.rs +++ b/ext/kv/remote.rs @@ -16,6 +16,7 @@ use deno_fetch::CreateHttpClientOptions; use deno_tls::rustls::RootCertStore; use deno_tls::Proxy; use deno_tls::RootCertStoreProvider; +use deno_tls::TlsKey; use denokv_remote::MetadataEndpoint; use denokv_remote::Remote; use url::Url; @@ -26,7 +27,7 @@ pub struct HttpOptions { pub root_cert_store_provider: Option>, pub proxy: Option, pub unsafely_ignore_certificate_errors: Option>, - pub client_cert_chain_and_key: Option<(String, String)>, + pub client_cert_chain_and_key: Option, } impl HttpOptions { diff --git a/ext/net/02_tls.js b/ext/net/02_tls.js index 04b0a58508..8d43e8604f 100644 --- a/ext/net/02_tls.js +++ b/ext/net/02_tls.js @@ -7,11 +7,15 @@ import { op_net_connect_tls, op_net_listen_tls, op_tls_handshake, + op_tls_key_null, + op_tls_key_static, + op_tls_key_static_from_file, op_tls_start, } from "ext:core/ops"; const { Number, ObjectDefineProperty, + ReflectHas, TypeError, } = primordials; @@ -91,9 +95,11 @@ async function connectTls({ } cert ??= certChain; key ??= privateKey; + const keyPair = loadTlsKeyPair(cert, undefined, key, undefined); const { 0: rid, 1: localAddr, 2: remoteAddr } = await op_net_connect_tls( { hostname, port }, { certFile, caCerts, cert, key, alpnProtocols }, + keyPair, ); localAddr.transport = "tcp"; remoteAddr.transport = "tcp"; @@ -131,6 +137,36 @@ class TlsListener extends Listener { } } +function hasTlsKeyPairOptions(options) { + return (ReflectHas(options, "cert") || ReflectHas(options, "key") || + ReflectHas(options, "certFile") || + ReflectHas(options, "keyFile")); +} + +function loadTlsKeyPair( + cert, + certFile, + key, + keyFile, +) { + if ((certFile !== undefined) ^ (keyFile !== undefined)) { + throw new TypeError( + "If certFile is specified, keyFile must also be specified", + ); + } + if ((cert !== undefined) ^ (key !== undefined)) { + throw new TypeError("If cert is specified, key must also be specified"); + } + + if (certFile !== undefined) { + return op_tls_key_static_from_file("Deno.listenTls", certFile, keyFile); + } else if (cert !== undefined) { + return op_tls_key_static(cert, key); + } else { + return op_tls_key_null(); + } +} + function listenTls({ port, cert, @@ -159,9 +195,12 @@ function listenTls({ "Pass the cert file contents to the `Deno.ListenTlsOptions.cert` option instead.", ); } + + const keyPair = loadTlsKeyPair(cert, certFile, key, keyFile); const { 0: rid, 1: localAddr } = op_net_listen_tls( { hostname, port: Number(port) }, - { cert, certFile, key, keyFile, alpnProtocols, reusePort }, + { alpnProtocols, reusePort }, + keyPair, ); return new TlsListener(rid, localAddr); } @@ -184,4 +223,12 @@ async function startTls( return new TlsConn(rid, remoteAddr, localAddr); } -export { connectTls, listenTls, startTls, TlsConn, TlsListener }; +export { + connectTls, + hasTlsKeyPairOptions, + listenTls, + loadTlsKeyPair, + startTls, + TlsConn, + TlsListener, +}; diff --git a/ext/net/lib.rs b/ext/net/lib.rs index 675cffbe3b..d6e1d9dc23 100644 --- a/ext/net/lib.rs +++ b/ext/net/lib.rs @@ -84,6 +84,9 @@ deno_core::extension!(deno_net, ops::op_set_nodelay, ops::op_set_keepalive, + ops_tls::op_tls_key_null, + ops_tls::op_tls_key_static, + ops_tls::op_tls_key_static_from_file

, ops_tls::op_tls_start

, ops_tls::op_net_connect_tls

, ops_tls::op_net_listen_tls

, diff --git a/ext/net/ops_tls.rs b/ext/net/ops_tls.rs index e83173e909..874f795f27 100644 --- a/ext/net/ops_tls.rs +++ b/ext/net/ops_tls.rs @@ -12,9 +12,9 @@ use deno_core::error::bad_resource; use deno_core::error::custom_error; use deno_core::error::generic_error; use deno_core::error::invalid_hostname; -use deno_core::error::type_error; use deno_core::error::AnyError; use deno_core::op2; +use deno_core::v8; use deno_core::AsyncRefCell; use deno_core::AsyncResult; use deno_core::CancelHandle; @@ -31,7 +31,8 @@ use deno_tls::rustls::PrivateKey; use deno_tls::rustls::ServerConfig; use deno_tls::rustls::ServerName; use deno_tls::SocketUse; -use io::Read; +use deno_tls::TlsKey; +use deno_tls::TlsKeys; use rustls_tokio_stream::TlsStreamRead; use rustls_tokio_stream::TlsStreamWrite; use serde::Deserialize; @@ -43,9 +44,9 @@ use std::cell::RefCell; use std::convert::From; use std::convert::TryFrom; use std::fs::File; -use std::io; use std::io::BufReader; use std::io::ErrorKind; +use std::io::Read; use std::num::NonZeroUsize; use std::path::Path; use std::rc::Rc; @@ -145,8 +146,6 @@ impl Resource for TlsStreamResource { pub struct ConnectTlsArgs { cert_file: Option, ca_certs: Vec, - cert: Option, - key: Option, alpn_protocols: Option>, } @@ -159,6 +158,59 @@ pub struct StartTlsArgs { alpn_protocols: Option>, } +#[op2] +pub fn op_tls_key_null<'s>( + scope: &mut v8::HandleScope<'s>, +) -> Result, AnyError> { + Ok(deno_core::cppgc::make_cppgc_object(scope, TlsKeys::Null)) +} + +#[op2] +pub fn op_tls_key_static<'s>( + scope: &mut v8::HandleScope<'s>, + #[string] cert: String, + #[string] key: String, +) -> Result, AnyError> { + let cert = load_certs(&mut BufReader::new(cert.as_bytes()))?; + let key = load_private_keys(key.as_bytes())? + .into_iter() + .next() + .unwrap(); + Ok(deno_core::cppgc::make_cppgc_object( + scope, + TlsKeys::Static(TlsKey(cert, key)), + )) +} + +/// Legacy op -- will be removed in Deno 2.0. +#[op2] +pub fn op_tls_key_static_from_file<'s, NP>( + state: &mut OpState, + scope: &mut v8::HandleScope<'s>, + #[string] api: String, + #[string] cert_file: String, + #[string] key_file: String, +) -> Result, AnyError> +where + NP: NetPermissions + 'static, +{ + { + let permissions = state.borrow_mut::(); + permissions.check_read(Path::new(&cert_file), &api)?; + permissions.check_read(Path::new(&key_file), &api)?; + } + + let cert = load_certs_from_file(&cert_file)?; + let key = load_private_keys_from_file(&key_file)? + .into_iter() + .next() + .unwrap(); + Ok(deno_core::cppgc::make_cppgc_object( + scope, + TlsKeys::Static(TlsKey(cert, key)), + )) +} + #[op2] #[serde] pub fn op_tls_start( @@ -251,6 +303,7 @@ pub async fn op_net_connect_tls( state: Rc>, #[serde] addr: IpAddr, #[serde] args: ConnectTlsArgs, + #[cppgc] key_pair: &TlsKeys, ) -> Result<(ResourceId, IpAddr, IpAddr), AnyError> where NP: NetPermissions + 'static, @@ -297,18 +350,10 @@ where let local_addr = tcp_stream.local_addr()?; let remote_addr = tcp_stream.peer_addr()?; - let cert_and_key = if args.cert.is_some() || args.key.is_some() { - let cert = args - .cert - .ok_or_else(|| type_error("No certificate chain provided"))?; - let key = args - .key - .ok_or_else(|| type_error("No private key provided"))?; - Some((cert, key)) - } else { - None + let cert_and_key = match key_pair { + TlsKeys::Null => None, + TlsKeys::Static(key) => Some(key.clone()), }; - let mut tls_config = create_client_config( root_cert_store, ca_certs, @@ -373,12 +418,6 @@ impl Resource for TlsListenerResource { #[derive(Deserialize)] #[serde(rename_all = "camelCase")] pub struct ListenTlsArgs { - cert: Option, - // TODO(kt3k): Remove this option at v2.0. - cert_file: Option, - key: Option, - // TODO(kt3k): Remove this option at v2.0. - key_file: Option, alpn_protocols: Option>, reuse_port: bool, } @@ -389,6 +428,7 @@ pub fn op_net_listen_tls( state: &mut OpState, #[serde] addr: IpAddr, #[serde] args: ListenTlsArgs, + #[cppgc] keys: &TlsKeys, ) -> Result<(ResourceId, IpAddr), AnyError> where NP: NetPermissions + 'static, @@ -397,54 +437,30 @@ where super::check_unstable(state, "Deno.listenTls({ reusePort: true })"); } - let cert_file = args.cert_file.as_deref(); - let key_file = args.key_file.as_deref(); - let cert = args.cert.as_deref(); - let key = args.key.as_deref(); - { let permissions = state.borrow_mut::(); permissions .check_net(&(&addr.hostname, Some(addr.port)), "Deno.listenTls()")?; - if let Some(path) = cert_file { - permissions.check_read(Path::new(path), "Deno.listenTls()")?; - } - if let Some(path) = key_file { - permissions.check_read(Path::new(path), "Deno.listenTls()")?; - } } - let cert_chain = if cert_file.is_some() && cert.is_some() { - return Err(generic_error("Both cert and certFile is specified. You can specify either one of them.")); - } else if let Some(path) = cert_file { - load_certs_from_file(path)? - } else if let Some(cert) = cert { - load_certs(&mut BufReader::new(cert.as_bytes()))? - } else { - return Err(generic_error("`cert` is not specified.")); - }; - let key_der = if key_file.is_some() && key.is_some() { - return Err(generic_error( - "Both key and keyFile is specified. You can specify either one of them.", - )); - } else if let Some(path) = key_file { - load_private_keys_from_file(path)?.remove(0) - } else if let Some(key) = key { - load_private_keys(key.as_bytes())?.remove(0) - } else { - return Err(generic_error("`key` is not specified.")); - }; - - let mut tls_config = ServerConfig::builder() + let tls_config = ServerConfig::builder() .with_safe_defaults() - .with_no_client_auth() - .with_single_cert(cert_chain, key_der) - .map_err(|e| { - custom_error( - "InvalidData", - format!("Error creating TLS certificate: {:?}", e), - ) - })?; + .with_no_client_auth(); + + let mut tls_config = match keys { + TlsKeys::Null => { + unreachable!() + } + TlsKeys::Static(TlsKey(cert, key)) => { + tls_config.with_single_cert(cert.clone(), key.clone()) + } + } + .map_err(|e| { + custom_error( + "InvalidData", + format!("Error creating TLS certificate: {:?}", e), + ) + })?; if let Some(alpn_protocols) = args.alpn_protocols { tls_config.alpn_protocols = diff --git a/ext/tls/lib.rs b/ext/tls/lib.rs index be8cabadc1..63fc046286 100644 --- a/ext/tls/lib.rs +++ b/ext/tls/lib.rs @@ -174,19 +174,9 @@ pub fn create_client_config( root_cert_store: Option, ca_certs: Vec>, unsafely_ignore_certificate_errors: Option>, - client_cert_chain_and_key: Option<(String, String)>, + maybe_cert_chain_and_key: Option, socket_use: SocketUse, ) -> Result { - let maybe_cert_chain_and_key = - if let Some((cert_chain, private_key)) = client_cert_chain_and_key { - // The `remove` is safe because load_private_keys checks that there is at least one key. - let private_key = load_private_keys(private_key.as_bytes())?.remove(0); - let cert_chain = load_certs(&mut cert_chain.as_bytes())?; - Some((cert_chain, private_key)) - } else { - None - }; - if let Some(ic_allowlist) = unsafely_ignore_certificate_errors { let client_config = ClientConfig::builder() .with_safe_defaults() @@ -199,7 +189,7 @@ pub fn create_client_config( // are not type-compatible - one wants "client cert", the other wants "transparency policy // or client cert". let mut client = - if let Some((cert_chain, private_key)) = maybe_cert_chain_and_key { + if let Some(TlsKey(cert_chain, private_key)) = maybe_cert_chain_and_key { client_config .with_client_auth_cert(cert_chain, private_key) .expect("invalid client key or certificate") @@ -236,7 +226,7 @@ pub fn create_client_config( }); let mut client = - if let Some((cert_chain, private_key)) = maybe_cert_chain_and_key { + if let Some(TlsKey(cert_chain, private_key)) = maybe_cert_chain_and_key { client_config .with_client_auth_cert(cert_chain, private_key) .expect("invalid client key or certificate") @@ -270,8 +260,7 @@ pub fn load_certs( .map_err(|_| custom_error("InvalidData", "Unable to decode certificate"))?; if certs.is_empty() { - let e = custom_error("InvalidData", "No certificates found in cert file"); - return Err(e); + return Err(cert_not_found_err()); } Ok(certs.into_iter().map(Certificate).collect()) @@ -282,7 +271,11 @@ fn key_decode_err() -> AnyError { } fn key_not_found_err() -> AnyError { - custom_error("InvalidData", "No keys found in key file") + custom_error("InvalidData", "No keys found in key data") +} + +fn cert_not_found_err() -> AnyError { + custom_error("InvalidData", "No certificates found in certificate data") } /// Starts with -----BEGIN RSA PRIVATE KEY----- @@ -331,3 +324,15 @@ pub fn load_private_keys(bytes: &[u8]) -> Result, AnyError> { Ok(keys) } + +/// A loaded key. +// FUTURE(mmastrac): add resolver enum value to support dynamic SNI +pub enum TlsKeys { + // TODO(mmastrac): We need Option<&T> for cppgc -- this is a workaround + Null, + Static(TlsKey), +} + +/// A TLS certificate/private key pair. +#[derive(Clone, Debug)] +pub struct TlsKey(pub Vec, pub PrivateKey);