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

Add SocketInfo and alpn/sni negotiation #18

Merged
merged 2 commits into from
Oct 28, 2023
Merged

Conversation

mmastrac
Copy link
Contributor

@mmastrac mmastrac commented Sep 28, 2023

Adds alpn and sni members to the SocketOptions dictionary to address part of #14 and #15.

Adds an info attribute to Socket to retrieve the negotiated alpn protocol.

I left the negotiated SNI out of the info attribute as it may be better to offer the full server certificate in that case and I don't see a concrete use case for it (the TLS connection will abort if SNI was ignored or incorrect).

Note that sni and alpn are valid for starttls connections as well so the specification allows them for either on or starttls connections. An example of Server Name Indication with STARTTLS can be seen here: http://www.postfix.org/TLS_README.html

Once #12 is landed, it may make sense to update the text to specify that ready resolves when a TLS socket has been fully negotiated and

@mmastrac mmastrac mentioned this pull request Sep 28, 2023
@jasnell
Copy link
Collaborator

jasnell commented Sep 28, 2023

A key question for the startTls case... should alpn and sni be provided when the Socket is opened or when startTls() is called? I can see arguments for either.

index.bs Outdated Show resolved Hide resolved
@mmastrac
Copy link
Contributor Author

A key question for the startTls case... should alpn and sni be provided when the Socket is opened or when startTls() is called? I can see arguments for either.

That's an interesting question and I might lean towards the latter. I am not aware of any protocols that may send SNI-influencing information in plaintext, but it feels more correct.

@dom96
Copy link
Collaborator

dom96 commented Sep 29, 2023

A key question for the startTls case... should alpn and sni be provided when the Socket is opened or when startTls() is called? I can see arguments for either.

I actually think it may make sense to have the new opened promise (introduced in #12) resolve to a SocketInfo. For the startTls case this will work nicely since startTls returns a new Socket.

@jasnell
Copy link
Collaborator

jasnell commented Sep 29, 2023

So instead of having socket.info it would be const info = await socket.opened ? That would work for me, I think.

@dom96
Copy link
Collaborator

dom96 commented Sep 29, 2023

Yeah, precisely.

@jasnell
Copy link
Collaborator

jasnell commented Sep 30, 2023

PR adding SocketInfo and clarifying a bit more about the opened promise. #19 ... once that lands, this PR can be updated to add the new alpn property to the SocketInfo dictionary.

@dom96
Copy link
Collaborator

dom96 commented Oct 27, 2023

Gentle ping to update this to use the new SocketInfo dictionary :)

@mmastrac
Copy link
Contributor Author

@dom96 @jasnell Ah thanks.. let me try to get to this one today.

@mmastrac
Copy link
Contributor Author

Updated w/alpn and sni fields. I added the SocketInfo attribute for now on the Socket but we can definitely move it around.

@jasnell jasnell merged commit 9302e53 into wintercg:main Oct 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants