refactor(ext/tls): Implement required functionality for later SNI support #23686
Conversation
mmastrac
changed the title
| let alpn = args | ||
| .alpn_protocols | ||
| .unwrap_or_default() | ||
| .into_iter() | ||
| .map(|s| s.into_bytes()) | ||
| .collect(); | ||
| let listener = match keys.take() { | ||
| TlsKeys::Null => Err(anyhow!("Deno.listenTls requires a key")), | ||
| TlsKeys::Static(TlsKey(cert, key)) => { | ||
| let mut tls_config = ServerConfig::builder() | ||
| .with_safe_defaults() | ||
| .with_no_client_auth() | ||
| .with_single_cert(cert, key) | ||
| .map_err(|e| anyhow!(e))?; | ||
| tls_config.alpn_protocols = alpn; | ||
| Ok(TlsListener { | ||
| tcp_listener, | ||
| tls_config: Some(tls_config.into()), | ||
| server_config_provider: None, | ||
| }) | ||
| } | ||
| TlsKeys::Resolver(resolver) => Ok(TlsListener { | ||
| tcp_listener, | ||
| tls_config: None, | ||
| server_config_provider: Some(resolver.into_server_config_provider(alpn)), | ||
| }), | ||
| } |
There was a problem hiding this comment.
Do you think it would make sense to move this to a helper fn and add unit tests for it long term?
There was a problem hiding this comment.
The rustls configs aren't really designed for easy testing, so I think we're stuck with integration tests. We could put this into a separate function, but then we'd need to write a bunch of Rust code to create raw TLS connections. 😢
I think there's probably a bunch of refactoring that could happen with these streams to make them more testable in general, but our coverage is pretty good right now thru tests IMO.
| .with_client_auth_cert(cert_chain, private_key) | ||
| .expect("invalid client key or certificate"), | ||
| TlsKeys::Null => client_config.with_no_client_auth(), | ||
| TlsKeys::Resolver(_) => unimplemented!(), |
There was a problem hiding this comment.
Double-checking: it's not possible to hit this code path right now, correct?
There was a problem hiding this comment.
Correct. We don't allow resolvers for client TLS connections. I don't know if this will make sense to implement unless we rip reqwest out and start making our own outgoing connections. Otherwise you should always know which server you're connecting to and can provide a static cert.
| .with_client_auth_cert(cert_chain, private_key) | ||
| .expect("invalid client key or certificate"), | ||
| TlsKeys::Null => client_config.with_no_client_auth(), | ||
| TlsKeys::Resolver(_) => unimplemented!(), |
| .pending | ||
| .borrow_mut() | ||
| .remove(&sni) | ||
| .unwrap() |
There was a problem hiding this comment.
Should we gracefully error here?
There was a problem hiding this comment.
This would definitely be a serious bug if this was somehow called twice. I think it's probably fine if we panic.
Signed-off-by: Matt Mastracci <[email protected]>
…port (#23686) Precursor to #23236 This implements the SNI features, but uses private symbols to avoid exposing the functionality at this time. Note that to properly test this feature, we need to add a way for `connectTls` to specify a hostname. This is something that should be pushed into that API at a later time as well. ```ts Deno.test( { permissions: { net: true, read: true } }, async function listenResolver() { let sniRequests = []; const listener = Deno.listenTls({ hostname: "localhost", port: 0, [resolverSymbol]: (sni: string) => { sniRequests.push(sni); return { cert, key, }; }, }); { const conn = await Deno.connectTls({ hostname: "localhost", [serverNameSymbol]: "server-1", port: listener.addr.port, }); const [_handshake, serverConn] = await Promise.all([ conn.handshake(), listener.accept(), ]); conn.close(); serverConn.close(); } { const conn = await Deno.connectTls({ hostname: "localhost", [serverNameSymbol]: "server-2", port: listener.addr.port, }); const [_handshake, serverConn] = await Promise.all([ conn.handshake(), listener.accept(), ]); conn.close(); serverConn.close(); } assertEquals(sniRequests, ["server-1", "server-2"]); listener.close(); }, ); ``` --------- Signed-off-by: Matt Mastracci <[email protected]>
Precursor to #23236
This implements the SNI features, but uses private symbols to avoid exposing the functionality at this time. Note that to properly test this feature, we need to add a way for
connectTlsto specify a hostname. This is something that should be pushed into that API at a later time as well.