Skip to content
This repository has been archived by the owner on Jun 4, 2021. It is now read-only.
This repository has been archived by the owner on Jun 4, 2021. It is now read-only.

Support for TLS 1.3 #4

Closed
mildsunrise opened this issue May 8, 2019 · 16 comments
Closed

Support for TLS 1.3 #4

mildsunrise opened this issue May 8, 2019 · 16 comments

Comments

@mildsunrise
Copy link
Contributor

Starting with Node 11.10 (I think), Node.JS supports making TLS1.3 connections:
nodejs/node@7393e37af1 (commit on v11.x)
nodejs/node@42dbaed460 (commit on v12.x)

Info on TLS 1.3 secrets and how Wireshark decrypts them:
https://security.stackexchange.com/a/42350/5067 (would be nice adding to README)

All this means:

  • The API changes, we should use session event if Node >= 11.10 is detected, instead of secureConnection or secureConnect.
  • We should log secrets in a different format.

I'll try to work on this.

@kolontsov
Copy link
Owner

Thanks! It would be nice to support TLS 1.3 properly. I do have plans to work on it, but I didn't touched it yet, so any PRs/suggestions are welcome.

@kolontsov
Copy link
Owner

Reference: NodeJS/OpenSSL versions (from merged PR)

@mildsunrise
Copy link
Contributor Author

mildsunrise commented May 11, 2019

Tracking nodejs/node#27419 for TLSv1.3 / keylog support in v10.

@mildsunrise
Copy link
Contributor Author

I have started working on a PR to Node.JS, which exports the SSL_CTX_set_keylog_callback API as a keylog event which is emitted with the line of text.

So for instance:

const req = https.request("https://example.org")
req.on("socket", socket => {
  socket.on("keylog", console.log)
})

Screenshot from 2019-05-11 12 45 41

If it's accepted in Node.JS, we could then polyfill this API for older versions(?)

@kolontsov
Copy link
Owner

I have started working on a PR to Node.JS, which exports the SSL_CTX_set_keylog_callback API as a keylog event which is emitted with the line of text.

Very cool! This is a very right thing to do, good luck with that.

If it's accepted in Node.JS, we could then polyfill this API for older versions(?)

Agree. This implementation with events and handlers looks natural and flexible.

@mildsunrise
Copy link
Contributor Author

Pull request created: nodejs/node#27654

@kolontsov
Copy link
Owner

Wow, it was fast. Nice work!

@mildsunrise
Copy link
Contributor Author

It just landed on Node.JS 🎊 It was faster than I expected... I'm working on a polyfill for the API, do you think we could include it in this version?

@kolontsov
Copy link
Owner

Congratulations!

I'm working on a polyfill for the API, do you think we could include it in this version?

Yes. It would be great.

@mildsunrise
Copy link
Contributor Author

Update: Node v12.3.0 was just released with the keylog API ✨

@mildsunrise
Copy link
Contributor Author

In reference to #4 (comment), Node v10.16.0 was released 3 days ago, and it includes OpenSSL 1.1.1b, so keylog is available.

However it seems I misunderstood, TLSv1.3 has not been backported.
In fact, an exception is thrown if you attempt to use it (that's why 3060046 failed CI).

@mildsunrise
Copy link
Contributor Author

I'll modify the tests to detect that exception and skip the test in that case.

@kolontsov
Copy link
Owner

Currently, I just skip TLS 1.3 test on Nodejs <11 (b47ba91), so it's not issue :)

@mildsunrise
Copy link
Contributor Author

True! But I'd prefer to mark the test as skipped, rather than not registering it at all 🤔

@kolontsov
Copy link
Owner

It makes sense. I agree.

@mildsunrise
Copy link
Contributor Author

We can close this now (unless I'm missing something)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants