Finish API and stuff

[mildsunrise]

I suggest we deprecate update_log and get_session_key because it doesn't make sense on TLSv1.3 when (1) there are multiple sessions, and (2) OpenSSL keylog callback will be used instead of extracting session key manually. I think users should stick to the hook_ API. WDYT?

I've also added a hook_socket function that hooks a single socket (for cases not covered by server or agent) and hook_all which hooks TLSSocket constructor to hook all sockets, ever. It patches an internal function, so if you don't agree I'll exclude that from the PR.

The rest is just pending things (tests, examples, README, changelog, add warning for Node.JS version), I think I'm not forgetting anything.

[kolontsov]

Thanks! I'll check everything later today. So far sounds good.

[mildsunrise]

Thanks :)

[kolontsov]

Thanks for the code (I'm especially glad to see more tests and docs!).

It patches an internal function, so if you don't agree I'll exclude that from the PR.

No problem, because it's "developer's tool". It's good that it's mentioned in README.

I suggest we deprecate update_log and get_session_key because it doesn't make sense on TLSv1.3 when (1) there are multiple sessions, and (2) OpenSSL keylog callback will be used instead of extracting session key manually.

We'll never have OpenSSL keylog callback available in Node 10.x, and 10.x is LTS, so it's important to support compatibility - which means we'll have to use some form of get_session_key internally anyway (but it can be hidden by our API for consistency). Isn't it too early to deprecate it? Let's think again about deprecation during implementing support for TLS 1.3; may be it will become more obvious - because we'll have to keep compatibility with TLS 1.2 as well..

I like idea of hook* API, it makes things a lot simpler. Also, let's switch to camelCase for new API (leaving c_style_function_names as aliases, if required). Should look more familiar for Node programmers.

Also, I think that with new API we need some way to allow user to set own handler for logging secrets (for example, to keep only chosen secrets on a high-load server). I didn't yet think enough how to implement in simple/consistent way. May be optional second argument to hook* (user-specified handler). May be something else. Suggestions are welcome.

[mildsunrise]

Totally agree with switching to camelCase!

We'll never have OpenSSL keylog callback available in Node 10.x, and 10.x is LTS, so it's important to support compatibility - which means we'll have to use some form of get_session_key internally anyway (but it can be hidden by our API for consistency). Isn't it too early to deprecate it? Let's think again about deprecation during implementing support for TLS 1.3; may be it will become more obvious - because we'll have to keep compatibility with TLS 1.2 as well..

Yes, what I meant by deprecating set_update_log was just hiding it from the public API. It'll still be used internally, of course, because we need to provide compatibility with Node < 11.10 (TLSv1.2). But if users switch to the hook* API, we can implement TLSv1.3 support transparently in a future release.

[mildsunrise]

Also, I think it'd be good to call appendFile instead of the sync version, so we don't block the event loop.

Appends are atomic, so it makes no harm to begin another append when one hasn't finished. The only risk is keylog lines ending up in different order, but this shouldn't matter to Wireshark (every index is unique).

[mildsunrise]

About letting the user provide his own handler, I'd vote on leaving that to be implemented in future releases (as an optional second parameter to hook* as you said).

[kolontsov]

Do I understand correctly that you suggest to remove get_session_key() from exports and from documentation, without providing an alternative? Why? This will restrict user from implementing his own logic for key logging - which can be important on high-load server.

About appendFile - I agree that it would be better than appendFileSync, so let's do it. Actually, I don't really care about performance of update_log because it was meant to serve as example; that's why README shows full usage of get_session_key and then just mentions update_log. In real life you would not want to open/close file on every call (like appendFile[Sync] does), and you'll probably want to implement some buffering.. But changing it to async version would not harm, of course :)

About cameCase - glad that we agree.

[kolontsov]

Hi, do you think we should merge this? I think that it would be better to integrate polyfill first (which already includes hook* functions), then merge things from this PR (especially docs), and then I review everything again and will publish npm. Also, if you don't object, I'd like to add you to README as co-author - you contributed a LOT of important code, thanks!

[mildsunrise]

I agree, let's merge pollyfill first :)

[mildsunrise]

Back to this! A lot of things no longer apply, so I've made the commits again.

If I understood correctly from #9, we're now moving in the direction of the polyfill. So, I have:

• Rewritten index.js to implement the hook* API without depending on the native module
• Removed update_log and get_session_key, and removed 'basic' test
• Skipped building native module if not needed

Other things:

• Added extra tests, and modified the code to mark TLSv1.3 test as skipped (as said in Support for TLS 1.3 #4)
• Renamed the API to camelCase (as discussed above)
• Added documentation about how to use this module as a polyfill.

[kolontsov]

Seems good! Thank you a lot. I'm going to publish npm today/tomorrow after additional review; I think now it deserves to be 1.0.0 version because of API breaking changes and a huge rework.

[mildsunrise]

Yeah, I agree :)