Skip to content

Commit 1b4f0db

Browse files
chessbytechessbyte
committed
refactor(tls): address PR review feedback and consolidate code
- Consolidate TLSSocket with llrt_net by sharing common types (ReadyState, LOCALHOST, get_hostname, rw_join) from llrt_net - Merge SecureContextOptions into BuildClientConfigOptions to reduce duplication between tls and http modules - Replace custom hex_encode with llrt_encoding::bytes_to_hex_string - Use [].concat() pattern for string formatting in keylog.rs - Capture actual cipher/protocol info from TLS connection after handshake instead of returning hardcoded values - Add clarifying comment for servername SNI requirement
1 parent 2319678 commit 1b4f0db

File tree

9 files changed

+196
-159
lines changed

9 files changed

+196
-159
lines changed

‎Cargo.lock‎

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎modules/llrt_http/src/agent.rs‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,7 @@ impl Agent {
4545
let config = llrt_tls::build_client_config(llrt_tls::BuildClientConfigOptions {
4646
reject_unauthorized,
4747
ca,
48-
cert: None,
49-
key: None,
50-
key_log: None,
48+
..Default::default()
5149
})
5250
.or_throw_msg(&ctx, "Failed to build TLS config")?;
5351
let client =

‎modules/llrt_net/src/lib.rs‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,12 @@ mod socket;
2828

2929
use self::{server::Server, socket::Socket};
3030

31-
const LOCALHOST: &str = "localhost";
31+
/// Localhost constant shared across socket implementations
32+
pub const LOCALHOST: &str = "localhost";
3233

33-
#[allow(dead_code)]
34-
enum ReadyState {
34+
/// Socket ready state, shared between net::Socket and tls::TLSSocket
35+
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
36+
pub enum ReadyState {
3537
Opening,
3638
Open,
3739
Closed,
@@ -103,11 +105,13 @@ impl Listener {
103105
}
104106
}
105107

106-
fn get_hostname(host: &str, port: u16) -> String {
108+
/// Build a hostname:port string from host and port
109+
pub fn get_hostname(host: &str, port: u16) -> String {
107110
[host, itoa::Buffer::new().format(port)].join(":")
108111
}
109112

110-
fn get_address_parts(
113+
/// Extract address parts (ip, port, family) from a socket address result
114+
pub fn get_address_parts(
111115
ctx: &Ctx,
112116
addr: StdResult<SocketAddr, std::io::Error>,
113117
) -> Result<(String, u16, String)> {
@@ -119,7 +123,8 @@ fn get_address_parts(
119123
))
120124
}
121125

122-
async fn rw_join(
126+
/// Wait for both readable and writable streams to complete
127+
pub async fn rw_join(
123128
ctx: &Ctx<'_>,
124129
readable_done: Receiver<bool>,
125130
writable_done: Receiver<bool>,

‎modules/llrt_tls/Cargo.toml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,9 @@ base64-simd = { version = "0.8", features = ["alloc"], default-features = false
2222
itoa = { version = "1", default-features = false }
2323
llrt_buffer = { version = "0.7.0-beta", path = "../llrt_buffer" }
2424
llrt_context = { version = "0.7.0-beta", path = "../../libs/llrt_context" }
25+
llrt_encoding = { version = "0.7.0-beta", path = "../../libs/llrt_encoding" }
2526
llrt_events = { version = "0.7.0-beta", path = "../llrt_events" }
27+
llrt_net = { version = "0.7.0-beta", path = "../llrt_net" }
2628
llrt_stream = { version = "0.7.0-beta", path = "../llrt_stream" }
2729
llrt_utils = { version = "0.7.0-beta", path = "../../libs/llrt_utils", default-features = false }
2830
once_cell = { version = "1", features = ["std"], default-features = false }

‎modules/llrt_tls/src/config.rs‎

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -44,24 +44,42 @@ pub fn get_tls_versions() -> Option<Vec<&'static SupportedProtocolVersion>> {
4444
}
4545

4646
pub static TLS_CONFIG: Lazy<Result<ClientConfig, Box<dyn std::error::Error + Send + Sync>>> =
47-
Lazy::new(|| {
48-
build_client_config(BuildClientConfigOptions {
49-
reject_unauthorized: true,
50-
ca: None,
51-
cert: None,
52-
key: None,
53-
key_log: None,
54-
})
55-
});
47+
Lazy::new(|| build_client_config(BuildClientConfigOptions::default()));
5648

49+
/// Unified TLS client configuration options.
50+
/// Used by SecureContext, tls.connect(), and HTTP agent.
5751
pub struct BuildClientConfigOptions {
52+
/// Whether to reject unauthorized certificates (default: true)
5853
pub reject_unauthorized: bool,
54+
/// Custom CA certificates in PEM format
5955
pub ca: Option<Vec<Vec<u8>>>,
6056
/// Client certificate in PEM format for mTLS
6157
pub cert: Option<Vec<u8>>,
6258
/// Client private key in PEM format for mTLS
6359
pub key: Option<Vec<u8>>,
60+
/// Key log callback for debugging TLS connections
6461
pub key_log: Option<Arc<dyn rustls::KeyLog>>,
62+
/// Cipher suites (OpenSSL format) - currently unused, reserved for future
63+
pub ciphers: Option<String>,
64+
/// Minimum TLS version (e.g., "TLSv1.2") - currently unused, reserved for future
65+
pub min_version: Option<String>,
66+
/// Maximum TLS version (e.g., "TLSv1.3") - currently unused, reserved for future
67+
pub max_version: Option<String>,
68+
}
69+
70+
impl Default for BuildClientConfigOptions {
71+
fn default() -> Self {
72+
Self {
73+
reject_unauthorized: true, // Secure by default
74+
ca: None,
75+
cert: None,
76+
key: None,
77+
key_log: None,
78+
ciphers: None,
79+
min_version: None,
80+
max_version: None,
81+
}
82+
}
6583
}
6684

6785
pub fn build_client_config(

‎modules/llrt_tls/src/keylog.rs‎

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
use std::fmt::Debug;
1313
use std::sync::Arc;
1414

15+
use llrt_encoding::bytes_to_hex_string;
1516
use rustls::KeyLog;
1617
use tokio::sync::mpsc;
1718

@@ -24,10 +25,10 @@ pub struct KeyLogLine {
2425
impl KeyLogLine {
2526
/// Format a key log line in NSS format
2627
pub fn new(label: &str, client_random: &[u8], secret: &[u8]) -> Self {
27-
let client_random_hex = hex_encode(client_random);
28-
let secret_hex = hex_encode(secret);
28+
let client_random_hex = bytes_to_hex_string(client_random);
29+
let secret_hex = bytes_to_hex_string(secret);
2930
Self {
30-
line: format!("{} {} {}\n", label, client_random_hex, secret_hex),
31+
line: [label, " ", &client_random_hex, " ", &secret_hex, "\n"].concat(),
3132
}
3233
}
3334

@@ -37,11 +38,6 @@ impl KeyLogLine {
3738
}
3839
}
3940

40-
/// Encode bytes as hexadecimal string
41-
fn hex_encode(bytes: &[u8]) -> String {
42-
bytes.iter().map(|b| format!("{:02x}", b)).collect()
43-
}
44-
4541
/// A KeyLog implementation that sends key log lines through a channel
4642
pub struct ChannelKeyLog {
4743
sender: mpsc::UnboundedSender<KeyLogLine>,
@@ -84,12 +80,6 @@ mod tests {
8480
assert_eq!(line.line, "CLIENT_RANDOM 010203 aabbcc\n");
8581
}
8682

87-
#[test]
88-
fn test_hex_encode() {
89-
assert_eq!(hex_encode(&[0x00, 0xff, 0x10]), "00ff10");
90-
assert_eq!(hex_encode(&[]), "");
91-
}
92-
9383
#[tokio::test]
9484
async fn test_channel_keylog() {
9585
let (keylog, mut receiver) = ChannelKeyLog::new();

‎modules/llrt_tls/src/lib.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ fn get_ciphers(ctx: Ctx<'_>) -> Result<Array<'_>> {
130130
}
131131

132132
/// Convert rustls CipherSuite to OpenSSL-style name
133-
fn cipher_suite_to_openssl_name(suite: rustls::CipherSuite) -> &'static str {
133+
pub fn cipher_suite_to_openssl_name(suite: rustls::CipherSuite) -> &'static str {
134134
use rustls::CipherSuite::*;
135135
match suite {
136136
// TLS 1.3 cipher suites

‎modules/llrt_tls/src/secure_context.rs‎

Lines changed: 82 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -14,75 +14,73 @@ use rustls::pki_types::{pem::PemObject, CertificateDer, PrivateKeyDer};
1414
use rustls::{ClientConfig, RootCertStore, ServerConfig};
1515
use webpki_roots::TLS_SERVER_ROOTS;
1616

17-
/// Options for creating a secure context
18-
#[derive(Default)]
19-
pub struct SecureContextOptions {
20-
pub cert: Option<Vec<u8>>,
21-
pub key: Option<Vec<u8>>,
22-
pub ca: Option<Vec<Vec<u8>>>,
23-
pub ciphers: Option<String>,
24-
pub min_version: Option<String>,
25-
pub max_version: Option<String>,
26-
}
27-
28-
impl SecureContextOptions {
29-
pub fn from_js_options<'js>(ctx: &Ctx<'js>, opts: &Object<'js>) -> Result<Self> {
30-
let mut options = Self::default();
31-
32-
// Handle certificate
33-
if let Some(cert_value) = opts.get_optional::<_, Value>("cert")? {
34-
if let Some(s) = cert_value.as_string() {
35-
options.cert = Some(s.to_string()?.into_bytes());
36-
} else if let Some(bytes) = get_bytes_from_value(ctx, &cert_value)? {
37-
options.cert = Some(bytes);
38-
}
17+
use crate::BuildClientConfigOptions;
18+
19+
/// Parse BuildClientConfigOptions from a JavaScript options object
20+
pub fn options_from_js<'js>(
21+
ctx: &Ctx<'js>,
22+
opts: &Object<'js>,
23+
) -> Result<BuildClientConfigOptions> {
24+
let mut options = BuildClientConfigOptions::default();
25+
26+
// Handle certificate
27+
if let Some(cert_value) = opts.get_optional::<_, Value>("cert")? {
28+
if let Some(s) = cert_value.as_string() {
29+
options.cert = Some(s.to_string()?.into_bytes());
30+
} else if let Some(bytes) = get_bytes_from_value(ctx, &cert_value)? {
31+
options.cert = Some(bytes);
3932
}
33+
}
4034

41-
// Handle private key
42-
if let Some(key_value) = opts.get_optional::<_, Value>("key")? {
43-
if let Some(s) = key_value.as_string() {
44-
options.key = Some(s.to_string()?.into_bytes());
45-
} else if let Some(bytes) = get_bytes_from_value(ctx, &key_value)? {
46-
options.key = Some(bytes);
47-
}
35+
// Handle private key
36+
if let Some(key_value) = opts.get_optional::<_, Value>("key")? {
37+
if let Some(s) = key_value.as_string() {
38+
options.key = Some(s.to_string()?.into_bytes());
39+
} else if let Some(bytes) = get_bytes_from_value(ctx, &key_value)? {
40+
options.key = Some(bytes);
4841
}
42+
}
4943

50-
// Handle CA certificates
51-
if let Some(ca_value) = opts.get_optional::<_, Value>("ca")? {
52-
let mut ca_certs = Vec::new();
53-
if let Some(ca_array) = ca_value.as_array() {
54-
for item in ca_array.iter::<Value>() {
55-
let item = item?;
56-
if let Some(s) = item.as_string() {
57-
ca_certs.push(s.to_string()?.into_bytes());
58-
} else if let Some(bytes) = get_bytes_from_value(ctx, &item)? {
59-
ca_certs.push(bytes);
60-
}
44+
// Handle CA certificates
45+
if let Some(ca_value) = opts.get_optional::<_, Value>("ca")? {
46+
let mut ca_certs = Vec::new();
47+
if let Some(ca_array) = ca_value.as_array() {
48+
for item in ca_array.iter::<Value>() {
49+
let item = item?;
50+
if let Some(s) = item.as_string() {
51+
ca_certs.push(s.to_string()?.into_bytes());
52+
} else if let Some(bytes) = get_bytes_from_value(ctx, &item)? {
53+
ca_certs.push(bytes);
6154
}
62-
} else if let Some(s) = ca_value.as_string() {
63-
ca_certs.push(s.to_string()?.into_bytes());
64-
} else if let Some(bytes) = get_bytes_from_value(ctx, &ca_value)? {
65-
ca_certs.push(bytes);
66-
}
67-
if !ca_certs.is_empty() {
68-
options.ca = Some(ca_certs);
6955
}
56+
} else if let Some(s) = ca_value.as_string() {
57+
ca_certs.push(s.to_string()?.into_bytes());
58+
} else if let Some(bytes) = get_bytes_from_value(ctx, &ca_value)? {
59+
ca_certs.push(bytes);
7060
}
71-
72-
if let Some(ciphers) = opts.get_optional::<_, String>("ciphers")? {
73-
options.ciphers = Some(ciphers);
61+
if !ca_certs.is_empty() {
62+
options.ca = Some(ca_certs);
7463
}
64+
}
7565

76-
if let Some(min_version) = opts.get_optional::<_, String>("minVersion")? {
77-
options.min_version = Some(min_version);
78-
}
66+
// Handle rejectUnauthorized
67+
if let Some(reject_unauthorized) = opts.get_optional::<_, bool>("rejectUnauthorized")? {
68+
options.reject_unauthorized = reject_unauthorized;
69+
}
7970

80-
if let Some(max_version) = opts.get_optional::<_, String>("maxVersion")? {
81-
options.max_version = Some(max_version);
82-
}
71+
if let Some(ciphers) = opts.get_optional::<_, String>("ciphers")? {
72+
options.ciphers = Some(ciphers);
73+
}
8374

84-
Ok(options)
75+
if let Some(min_version) = opts.get_optional::<_, String>("minVersion")? {
76+
options.min_version = Some(min_version);
8577
}
78+
79+
if let Some(max_version) = opts.get_optional::<_, String>("maxVersion")? {
80+
options.max_version = Some(max_version);
81+
}
82+
83+
Ok(options)
8684
}
8785

8886
fn get_bytes_from_value<'js>(ctx: &Ctx<'js>, value: &Value<'js>) -> Result<Option<Vec<u8>>> {
@@ -132,7 +130,7 @@ impl SecureContext {
132130

133131
impl SecureContext {
134132
/// Create a new SecureContext from options
135-
pub fn from_options(ctx: &Ctx<'_>, options: SecureContextOptions) -> Result<Self> {
133+
pub fn from_options(ctx: &Ctx<'_>, options: BuildClientConfigOptions) -> Result<Self> {
136134
let mut secure_context = Self::new();
137135

138136
// Build client config if we have CA certs or need default trust
@@ -165,26 +163,32 @@ impl SecureContext {
165163
})?
166164
.with_root_certificates(root_store);
167165

168-
let client_config = if let (Some(cert_pem), Some(key_pem)) = (&options.cert, &options.key) {
169-
// Client certificate authentication
170-
let certs: Vec<CertificateDer<'static>> = CertificateDer::pem_slice_iter(cert_pem)
171-
.collect::<std::result::Result<Vec<_>, _>>()
172-
.map_err(|e| {
173-
Exception::throw_message(ctx, &format!("Invalid certificate: {}", e))
166+
let mut client_config =
167+
if let (Some(cert_pem), Some(key_pem)) = (&options.cert, &options.key) {
168+
// Client certificate authentication
169+
let certs: Vec<CertificateDer<'static>> = CertificateDer::pem_slice_iter(cert_pem)
170+
.collect::<std::result::Result<Vec<_>, _>>()
171+
.map_err(|e| {
172+
Exception::throw_message(ctx, &format!("Invalid certificate: {}", e))
173+
})?;
174+
175+
let key = PrivateKeyDer::from_pem_slice(key_pem).map_err(|e| {
176+
Exception::throw_message(ctx, &format!("Invalid private key: {}", e))
174177
})?;
175178

176-
let key = PrivateKeyDer::from_pem_slice(key_pem).map_err(|e| {
177-
Exception::throw_message(ctx, &format!("Invalid private key: {}", e))
178-
})?;
179-
180-
client_builder
181-
.with_client_auth_cert(certs, key)
182-
.map_err(|e| {
183-
Exception::throw_message(ctx, &format!("Failed to set client auth: {}", e))
184-
})?
185-
} else {
186-
client_builder.with_no_client_auth()
187-
};
179+
client_builder
180+
.with_client_auth_cert(certs, key)
181+
.map_err(|e| {
182+
Exception::throw_message(ctx, &format!("Failed to set client auth: {}", e))
183+
})?
184+
} else {
185+
client_builder.with_no_client_auth()
186+
};
187+
188+
// Set key log if provided
189+
if let Some(key_log) = options.key_log {
190+
client_config.key_log = key_log;
191+
}
188192

189193
secure_context.client_config = Some(Arc::new(client_config));
190194

@@ -220,9 +224,9 @@ pub fn create_secure_context<'js>(
220224
options: Option<Object<'js>>,
221225
) -> Result<Class<'js, SecureContext>> {
222226
let opts = if let Some(opts) = options {
223-
SecureContextOptions::from_js_options(&ctx, &opts)?
227+
options_from_js(&ctx, &opts)?
224228
} else {
225-
SecureContextOptions::default()
229+
BuildClientConfigOptions::default()
226230
};
227231

228232
let secure_context = SecureContext::from_options(&ctx, opts)?;

0 commit comments

Comments
 (0)