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

refactor(ext/tls): use cppgc to deduplicate the tls key loading code #23289

Merged
merged 2 commits into from
Apr 8, 2024
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
8 changes: 8 additions & 0 deletions ext/fetch/22_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
),
);
}
Expand Down
25 changes: 8 additions & 17 deletions ext/fetch/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -78,7 +80,7 @@ pub struct Options {
pub request_builder_hook:
Option<fn(RequestBuilder) -> Result<RequestBuilder, AnyError>>,
pub unsafely_ignore_certificate_errors: Option<Vec<String>>,
pub client_cert_chain_and_key: Option<(String, String)>,
pub client_cert_chain_and_key: Option<TlsKey>,
pub file_fetch_handler: Rc<dyn FetchHandler>,
}

Expand Down Expand Up @@ -794,8 +796,6 @@ impl HttpClientResource {
pub struct CreateHttpClientArgs {
ca_certs: Vec<String>,
proxy: Option<Proxy>,
cert: Option<String>,
key: Option<String>,
pool_max_idle_per_host: Option<usize>,
pool_idle_timeout: Option<serde_json::Value>,
#[serde(default = "default_true")]
Expand All @@ -815,6 +815,7 @@ fn default_true() -> bool {
pub fn op_fetch_custom_client<FP>(
state: &mut OpState,
#[serde] args: CreateHttpClientArgs,
#[cppgc] tls_keys: &deno_tls::TlsKeys,
) -> Result<ResourceId, AnyError>
where
FP: FetchPermissions + 'static,
Expand All @@ -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::<Options>();
Expand Down Expand Up @@ -885,7 +876,7 @@ pub struct CreateHttpClientOptions {
pub ca_certs: Vec<Vec<u8>>,
pub proxy: Option<Proxy>,
pub unsafely_ignore_certificate_errors: Option<Vec<String>>,
pub client_cert_chain_and_key: Option<(String, String)>,
pub client_cert_chain_and_key: Option<TlsKey>,
pub pool_max_idle_per_host: Option<usize>,
pub pool_idle_timeout: Option<Option<u64>>,
pub http1: bool,
Expand Down
4 changes: 2 additions & 2 deletions ext/http/00_serve.js
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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) {
Expand Down
3 changes: 2 additions & 1 deletion ext/kv/remote.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -26,7 +27,7 @@ pub struct HttpOptions {
pub root_cert_store_provider: Option<Arc<dyn RootCertStoreProvider>>,
pub proxy: Option<Proxy>,
pub unsafely_ignore_certificate_errors: Option<Vec<String>>,
pub client_cert_chain_and_key: Option<(String, String)>,
pub client_cert_chain_and_key: Option<TlsKey>,
}

impl HttpOptions {
Expand Down
51 changes: 49 additions & 2 deletions ext/net/02_tls.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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";
Expand Down Expand Up @@ -131,6 +137,36 @@ class TlsListener extends Listener {
}
}

function hasTlsKeyPairOptions(options) {
return (ReflectHas(options, "cert") || ReflectHas(options, "key") ||
Copy link

@pi0 pi0 Apr 17, 2024

Choose a reason for hiding this comment

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

This change made a regression. Previously, if cert and key in options had undefined value, the server was not failing but now it does fail.

Context: nitrojs/nitro#2373 -- tools might get these options from sources like environment variables. now need to explicitly call delete on prop.

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,
Expand Down Expand Up @@ -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);
}
Expand All @@ -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,
};
3 changes: 3 additions & 0 deletions ext/net/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<P>,
ops_tls::op_tls_start<P>,
ops_tls::op_net_connect_tls<P>,
ops_tls::op_net_listen_tls<P>,
Expand Down
Loading