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

Polyfill for keylog API #9

Merged
merged 5 commits into from
Jun 1, 2019
Merged

Conversation

mildsunrise
Copy link
Contributor

@mildsunrise mildsunrise commented May 15, 2019

This is not (at least initially) intended to be merged, just as a proposal.
Basically implements a polyfill that:

  • for Node.JS >= 11.9.0, calls SSL_CTX_set_keylog_callback natively and emits keylog events based on it
  • for Node.JS < 11.9.0, emulates the OpenSSL API by manually assembling the CLIENT_RANDOM line as we do now, and emitting keylog events.
  • for Node.JS >= v12.3.0 (probably), if it detects that keylog is available natively, does nothing

Polyfilling the keylog API would allow us to implement index.js without needing the native module:

require('./lib/pollyfill.js');

E.filename = // ... (no changes)
E.set_log = // ... (no changes)
E.log_line = line => fs.appendFile(E.filename, line, err => {
    if (err) console.error(`Warning: Failed to log to SSLKEYLOGFILE: ${err}`);
});

E.hook_socket = socket => socket.on('keylog', E.log_line);
E.hook_server = server => server.on('keylog', E.log_line);
E.hook_agent = // ... (no changes)
E.hook_all = // ... (no changes)

Which would in turn allow us to skip building the native module entirely, if the native API is present. So, for recent Node.JS versions, this would simply be a small JS module for easy SSLKEYLOGFILE logging.

The downside is that we'd have to drop update_log and get_session_key. If someone needs custom logging he can subscribe to keylog directly, and only use this module as a polyfill. The second one is not entirely replaceable, but you can parse keylog lines to get the secrets you need (with TLSv1.3 this should be even more reliable than calling get_session_key manually, IMHO). What are your thoughts on this?

@kolontsov
Copy link
Owner

Hi Alba, sorry for pause, busy days.. Everything sounds logical, and you're right - advantages are clear. Let's move along the way you've suggested, and we'll see how it will work out. I'll add more comments after reading proposed code.

@mildsunrise
Copy link
Contributor Author

No worries, and thanks for your time ^^

@kolontsov kolontsov marked this pull request as ready for review June 1, 2019 13:07
@kolontsov
Copy link
Owner

Hi, long time no see! I reviewed all the code (and thanks for tests!), and I'd like to merge it to move forward. What do you think?

@mildsunrise
Copy link
Contributor Author

Hi! Seems good to me, I've been using it for a while, it works well

@kolontsov kolontsov merged commit 3060046 into kolontsov:master Jun 1, 2019
@mildsunrise mildsunrise deleted the feature/polyfill branch June 1, 2019 18:53
@mildsunrise mildsunrise mentioned this pull request Jun 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants