feat: ALPN support in `Deno.connectTls()` #12786

1st1 · 2021-11-16T17:54:52Z

Make connectTls() accept alpnProtocols option
(string[]), just like listenTls().
Add alpnProtocol to TlsConn#handshake return
value to query the protocol agreed with the peer via ALPN.

The alpnProtocols is added as an unstable API (mirroring
how it's supported in listenTls()).

We need this change for making a Deno client for EdgeDB. EdgeDB
requires TLS by default and uses ALPN to upgrade connections to
its binary protocol. Without these APIs we can't support Deno,
something we really want to have.

Fixes: #11479

CLAassistant · 2021-11-16T17:54:58Z

All committers have signed the CLA.

ext/net/02_tls.js

ext/net/ops_tls.rs

ext/net/02_tls.js

lucacasonato

Thanks for the work on this @1st1! I have added some comments about the API surface.

We'll also need some tests for this feature (in cli/tests/unit/tls_test.ts). For test cases you could create a server with Deno.listenTls with some ALPN protocols and then connect to it during the test and check that the negotiation works correctly. There are some examples on how to do that in that file.

Also, this option should be added to Deno.startTls in addition to Deno.connectTls.

1st1 · 2021-11-17T04:03:04Z

@lucacasonato Thanks for the review! I've updated the PR implementing all of your suggestions. Please take another look.

lucacasonato

Test looks great, thanks.

ext/net/ops_tls.rs

1st1 · 2021-11-17T19:00:58Z

I think the CI timeouts aren't related to this PR. @lucacasonato let me know if there's anything else left to do.

1st1 · 2021-11-17T19:27:31Z

I think the CI timeouts aren't related to this PR.

Actually they are. Investigating.

piscisaureus · 2021-11-17T20:02:29Z

[Fix tests hang; move `get_alpn_protocol()` helper to WriteHalf](/denoland/deno/pull/12786/commits/b00251d26876a9eee6a684ad8f6b52d532f0a1ef)

I'm not sure that is the right fix. The same hang could happen, it just isn't triggered by our tests?

lucacasonato

Maybe turn handshake_done into a Cell<Option<TlsHandshakeInfo>> and store the handshake info in there right after handshaking?

piscisaureus · 2021-11-17T20:09:08Z

Maybe turn handshake_done into a Cell<Option<TlsHandshakeInfo>> and store the handshake info in there right after handshaking?

Just looked into it, that seems to be the right way to go about it indeed.

1st1 · 2021-11-17T20:24:20Z

I'm not sure that is the right fix. The same hang could happen, it just isn't triggered by our tests?

My thinking was that handshake() already acquires the WriteHalf so it should be safer. But yeah, it's still not 100% safe.

Maybe turn handshake_done into a Cell<Option> and store the handshake info in there right after handshaking?

This is what I also settled on, although it seems that I can't wrap Option<TlsHandshakeInfo> into a Cell, because TlsHandshakeInfo must implement Copy, and so transitively, ByteString must also be copyable which it can't be:

720 | handshake_done: Cell<Option>,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait std::marker::Copy is not implemented for TlsHandshakeInfo

So I'm using RefCell which is a bit verbose but seems to do the job. Please take a look at the latest push.

piscisaureus · 2021-11-17T20:30:24Z

This is what I also settled on, although it seems that I can't wrap Option<TlsHandshakeInfo> into a Cell, because TlsHandshakeInfo must implement Copy, and so transitively, ByteString must also be copyable which it can't be:

Copy is only required for Cell::get() though. Presumably TlsHandshakeInfo could implement Clone so you could replace handshake_done.get() with handshake_done.clone().take().

1st1 · 2021-11-17T20:36:58Z

Presumably TlsHandshakeInfo could implement Clone so you could replace handshake_done.get() with handshake_done.clone().take().

Yes, I've made TlsHandshakeInfo implement Clone in the latest commit. I'm trying your suggestion to use Cell but the compiler still isn't happy and wants TlsHandshakeInfo to implement Copy:

Compile error:

error[E0599]: the method `clone` exists for struct `Cell<std::option::Option<TlsHandshakeInfo>>`, but its trait bounds were not satisfied
   --> ext/net/ops_tls.rs:769:49
    |
769 |     if let Some(tls_info) = self.handshake_done.clone().take() {
    |                                                 ^^^^^ method cannot be called on `Cell<std::option::Option<TlsHandshakeInfo>>` due to unsatisfied trait bounds
    |
   ::: /usr/lib/rust/1.56.1/lib/rustlib/src/rust/library/core/src/cell.rs:236:1
    |
236 | pub struct Cell<T: ?Sized> {
    | -------------------------- doesn't satisfy `Cell<std::option::Option<TlsHandshakeInfo>>: Clone`
    |
   ::: /usr/lib/rust/1.56.1/lib/rustlib/src/rust/library/core/src/option.rs:510:1
    |
510 | pub enum Option<T> {
    | ------------------ doesn't satisfy `_: std::marker::Copy`
    |
    = note: the following trait bounds were not satisfied:
            `std::option::Option<TlsHandshakeInfo>: std::marker::Copy`
            which is required by `Cell<std::option::Option<TlsHandshakeInfo>>: Clone`

error[E0277]: the trait bound `TlsHandshakeInfo: std::marker::Copy` is not satisfied
   --> ext/net/ops_tls.rs:720:3
    |
716 | #[derive(Debug)]
    |          ----- in this derive macro expansion
...
720 |   handshake_done: Cell<Option<TlsHandshakeInfo>>,
    |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::marker::Copy` is not implemented for `TlsHandshakeInfo`
    |
    = note: required because of the requirements on the impl of `std::marker::Copy` for `std::option::Option<TlsHandshakeInfo>`
    = note: required because of the requirements on the impl of `std::fmt::Debug` for `Cell<std::option::Option<TlsHandshakeInfo>>`
    = note: 1 redundant requirements hidden
    = note: required because of the requirements on the impl of `std::fmt::Debug` for `&Cell<std::option::Option<TlsHandshakeInfo>>`
    = note: required for the cast to the object type `dyn std::fmt::Debug`
    = note: this error originates in the derive macro `Debug` (in Nightly builds, run with -Z macro-backtrace for more info)

piscisaureus · 2021-11-17T20:41:00Z

Yes, I've made TlsHandshakeInfo implement Clone in the latest commit. I'm trying your suggestion to use Cell but the compiler still isn't happy and wants TlsHandshakeInfo to implement Copy:

Ok let me look into it.

1st1 · 2021-11-17T20:42:39Z

I can get around not using Cell::get entirely like this:

See the diff

diff --git a/ext/net/ops_tls.rs b/ext/net/ops_tls.rs
index 2edbfc08c..d98247c9d 100644
--- a/ext/net/ops_tls.rs
+++ b/ext/net/ops_tls.rs
@@ -56,6 +56,7 @@ use io::Read;
 use io::Write;
 use serde::Deserialize;
 use std::borrow::Cow;
+use std::cell::Cell;
 use std::cell::RefCell;
 use std::convert::From;
 use std::fs::File;
@@ -716,7 +717,7 @@ pub fn init<P: NetPermissions + 'static>() -> Vec<OpPair> {
 pub struct TlsStreamResource {
   rd: AsyncRefCell<ReadHalf>,
   wr: AsyncRefCell<WriteHalf>,
-  handshake_done: RefCell<Option<TlsHandshakeInfo>>,
+  handshake_done: Cell<Option<TlsHandshakeInfo>>,
   cancel_handle: CancelHandle, // Only read and handshake ops get canceled.
 }
 
@@ -725,7 +726,7 @@ impl TlsStreamResource {
     Self {
       rd: rd.into(),
       wr: wr.into(),
-      handshake_done: RefCell::new(None),
+      handshake_done: Cell::new(None),
       cancel_handle: Default::default(),
     }
   }
@@ -765,8 +766,10 @@ impl TlsStreamResource {
   pub async fn handshake(
     self: &Rc<Self>,
   ) -> Result<TlsHandshakeInfo, AnyError> {
-    if let Some(tls_info) = &*self.handshake_done.borrow() {
-      return Ok(tls_info.clone());
+    let hs_result = self.handshake_done.replace(None);
+    if let Some(tls_info) = hs_result {
+      self.handshake_done.set(Some(tls_info.clone()));
+      return Ok(tls_info);
     }
 
     let mut wr = RcRef::map(self, |r| &r.wr).borrow_mut().await;
@@ -775,7 +778,7 @@ impl TlsStreamResource {
 
     let alpn_protocol = wr.get_alpn_protocol();
     let tls_info = TlsHandshakeInfo { alpn_protocol };
-    self.handshake_done.replace(Some(tls_info.clone()));
+    self.handshake_done.set(Some(tls_info.clone()));
     Ok(tls_info)
   }
 }

but the compiler is still not happy with declaring handshake_done: Cell<Option<TlsHandshakeInfo>>:

Error:

~/d/deno (alpn *) » cargo test
    Blocking waiting for file lock on package cache
   Compiling deno_net v0.17.0 (/home/yury/dev/deno/ext/net)
error[E0277]: the trait bound `TlsHandshakeInfo: std::marker::Copy` is not satisfied
   --> ext/net/ops_tls.rs:720:3
    |
716 | #[derive(Debug)]
    |          ----- in this derive macro expansion
...
720 |   handshake_done: Cell<Option<TlsHandshakeInfo>>,
    |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::marker::Copy` is not implemented for `TlsHandshakeInfo`
    |
    = note: required because of the requirements on the impl of `std::marker::Copy` for `std::option::Option<TlsHandshakeInfo>`
    = note: required because of the requirements on the impl of `std::fmt::Debug` for `Cell<std::option::Option<TlsHandshakeInfo>>`
    = note: 1 redundant requirements hidden
    = note: required because of the requirements on the impl of `std::fmt::Debug` for `&Cell<std::option::Option<TlsHandshakeInfo>>`
    = note: required for the cast to the object type `dyn std::fmt::Debug`
    = note: this error originates in the derive macro `Debug` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0277`.
error: could not compile `deno_net` due to previous error
warning: build failed, waiting for other jobs to finish...
error: build failed

1st1 · 2021-11-17T20:43:46Z

Ok let me look into it.

Sure, really appreciate this! Is there an issue with swapping Cell with RefCell though? That seems to work in the latest commit.

piscisaureus · 2021-11-17T20:47:13Z

Sure, really appreciate this! Is there an issue with swapping Cell with RefCell though? That seems to work in the latest commit.

Not really, I was just being pedantic.

piscisaureus · 2021-11-17T20:50:00Z

Final nit, maybe we can rename handshake_done to handshake_info and add a comment that the value will be None if the handshake isn't done yet.

Other than that, this looks good to go.

1st1 · 2021-11-17T20:57:13Z

Final nit, maybe we can rename handshake_done to handshake_info and add a comment that the value will be None if the handshake isn't done yet.

Sure, done.

lucacasonato

LGTM @1st1. Excellent work on your first contribution! 🎉

I can't land the PR right away, because the 1.17 feature merge window has not opened yet. It will open after the 1.16.3 release next Tuesday (23 November 2021).

1st1 · 2021-11-17T22:28:42Z

Thanks @lucacasonato. Feel free to ping me if you need help with rebasing when the merge window is open.

1st1 · 2021-11-26T00:07:21Z

A friendly ping: let's merge this? Just want to make sure we get this into the next release.

lucacasonato · 2021-11-26T09:17:16Z

@1st1 Could you rebase the PR and apply the following patch:

From b933c778addf96d816f003a58816ec2cc1fcae5e Mon Sep 17 00:00:00 2001
From: Luca Casonato <hello@lcas.dev>
Date: Fri, 26 Nov 2021 10:14:13 +0100
Subject: [PATCH] update tests

---
 cli/tests/unit/tls_test.ts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/cli/tests/unit/tls_test.ts b/cli/tests/unit/tls_test.ts
index f47c6ac90..7e6d68900 100644
--- a/cli/tests/unit/tls_test.ts
+++ b/cli/tests/unit/tls_test.ts
@@ -364,7 +364,7 @@ Deno.test(
   },
 );
 
-unitTest(
+Deno.test(
   { permissions: { read: true, net: true } },
   async function tlsServerAlpnListenStartTls() {
     const [serverConn, clientConn] = await tlsAlpn(true);
@@ -380,7 +380,7 @@ unitTest(
   },
 );
 
-unitTest(
+Deno.test(
   { permissions: { read: true, net: true } },
   async function tlsServerStreamHalfCloseSendOneByte() {
     const [serverConn, clientConn] = await tlsPair();
-- 
2.34.0

I'd do it myself, but I don't have write access to your fork :-)

* Make `connectTls()` accept `alpnProtocols` option (string[]), just like `listenTls()`. * Add `getAgreedAlpnProtocol(): Promise<string | null>` method to query the protocol agreed with the peer via ALPN. The `alpnProtocols` is added as an unstable API.

While there: * Fix the selected ALPN protocol from being stored as a UTF8-encoded `String` to `deno_core::ByteString`. * Add `alpnProtocols` option to `Deno.startTls()`. * Add tests.

1st1 · 2021-11-26T18:13:30Z

@1st1 Could you rebase the PR and apply the following patch:

Done!

I'd do it myself, but I don't have write access to your fork :-)

Hm, I don't see a checkbox to grant you push privs... I'll look for it, but in the meantime if there's anything else you want to be fixed in the PR let me know.

1st1 mentioned this pull request Nov 16, 2021

Implement ALPN support for the Deno adapter. edgedb/edgedb-js#199

Merged

1st1 force-pushed the alpn branch 3 times, most recently from 7345c69 to 9875b60 Compare November 16, 2021 18:28

lucacasonato reviewed Nov 16, 2021

View reviewed changes

ext/net/02_tls.js Outdated Show resolved Hide resolved

lucacasonato reviewed Nov 16, 2021

View reviewed changes

ext/net/ops_tls.rs Outdated Show resolved Hide resolved

ext/net/02_tls.js Outdated Show resolved Hide resolved

lucacasonato reviewed Nov 16, 2021

View reviewed changes

1st1 requested a review from lucacasonato November 17, 2021 04:02

lucacasonato reviewed Nov 17, 2021

View reviewed changes

ext/net/ops_tls.rs Outdated Show resolved Hide resolved

lucacasonato requested a review from piscisaureus November 17, 2021 11:18

lucacasonato changed the title ~~Add ALPN support to Deno.connectTls().~~ feat: ALPN support in Deno.connectTls() Nov 17, 2021

lucacasonato added this to the 1.17.0 milestone Nov 17, 2021

1st1 requested a review from lucacasonato November 17, 2021 16:46

1st1 force-pushed the alpn branch from 3e4b784 to efc5cd0 Compare November 17, 2021 16:48

lucacasonato reviewed Nov 17, 2021

View reviewed changes

1st1 force-pushed the alpn branch from dd81a2b to bcdc46f Compare November 17, 2021 20:57

lucacasonato reviewed Nov 17, 2021

View reviewed changes

lucacasonato approved these changes Nov 17, 2021

View reviewed changes

piscisaureus approved these changes Nov 17, 2021

View reviewed changes

1st1 added 7 commits November 26, 2021 10:08

Implement the suggested API: handshake() returning TlsInfo.

3d4e8ce

While there: * Fix the selected ALPN protocol from being stored as a UTF8-encoded `String` to `deno_core::ByteString`. * Add `alpnProtocols` option to `Deno.startTls()`. * Add tests.

Make TlsStreamResource.handshake() return TlsHandshakeInfo directly

ef8a695

Fix tests hang; move get_alpn_protocol() helper to WriteHalf

c121c78

Use TlsStreamResource.handshake_done to store the outcome of handshake

8336e7e

Rename handshake_done to handshake_info

d826916

Apply Luca's patch

04f96d9

1st1 force-pushed the alpn branch from bcdc46f to 04f96d9 Compare November 26, 2021 18:10

lucacasonato merged commit 1d3f734 into denoland:main Nov 26, 2021

1st1 deleted the alpn branch November 26, 2021 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ALPN support in `Deno.connectTls()` #12786

feat: ALPN support in `Deno.connectTls()` #12786

1st1 commented Nov 16, 2021 •

edited

Loading

CLAassistant commented Nov 16, 2021 •

edited

Loading

lucacasonato left a comment

1st1 commented Nov 17, 2021 •

edited

Loading

lucacasonato left a comment

1st1 commented Nov 17, 2021

1st1 commented Nov 17, 2021

piscisaureus commented Nov 17, 2021

lucacasonato left a comment

piscisaureus commented Nov 17, 2021

1st1 commented Nov 17, 2021

piscisaureus commented Nov 17, 2021

1st1 commented Nov 17, 2021 •

edited

Loading

piscisaureus commented Nov 17, 2021

1st1 commented Nov 17, 2021 •

edited

Loading

1st1 commented Nov 17, 2021

piscisaureus commented Nov 17, 2021

piscisaureus commented Nov 17, 2021

1st1 commented Nov 17, 2021

lucacasonato left a comment

1st1 commented Nov 17, 2021

1st1 commented Nov 26, 2021

lucacasonato commented Nov 26, 2021

1st1 commented Nov 26, 2021

feat: ALPN support in Deno.connectTls() #12786

feat: ALPN support in Deno.connectTls() #12786

Conversation

1st1 commented Nov 16, 2021 • edited Loading

CLAassistant commented Nov 16, 2021 • edited Loading

lucacasonato left a comment

Choose a reason for hiding this comment

1st1 commented Nov 17, 2021 • edited Loading

lucacasonato left a comment

Choose a reason for hiding this comment

1st1 commented Nov 17, 2021

1st1 commented Nov 17, 2021

piscisaureus commented Nov 17, 2021

lucacasonato left a comment

Choose a reason for hiding this comment

piscisaureus commented Nov 17, 2021

1st1 commented Nov 17, 2021

piscisaureus commented Nov 17, 2021

1st1 commented Nov 17, 2021 • edited Loading

piscisaureus commented Nov 17, 2021

1st1 commented Nov 17, 2021 • edited Loading

1st1 commented Nov 17, 2021

piscisaureus commented Nov 17, 2021

piscisaureus commented Nov 17, 2021

1st1 commented Nov 17, 2021

lucacasonato left a comment

Choose a reason for hiding this comment

1st1 commented Nov 17, 2021

1st1 commented Nov 26, 2021

lucacasonato commented Nov 26, 2021

1st1 commented Nov 26, 2021

feat: ALPN support in `Deno.connectTls()` #12786

feat: ALPN support in `Deno.connectTls()` #12786

1st1 commented Nov 16, 2021 •

edited

Loading

CLAassistant commented Nov 16, 2021 •

edited

Loading

1st1 commented Nov 17, 2021 •

edited

Loading

1st1 commented Nov 17, 2021 •

edited

Loading

1st1 commented Nov 17, 2021 •

edited

Loading