machinezone / IXWebSocket

websocket and http client and server library, with TLS support and very few dependencies
BSD 3-Clause "New" or "Revised" License
512 stars 167 forks source link

[Feature Request] Add support for OpenSSL SSLKEYLOGFILE #499

Open WSL-Ben opened 4 months ago

WSL-Ben commented 4 months ago

Firstly, thank you very much for writing such a simple to use and well documented C++ Websocket library. As someone who's relatively new to the language I've found that to something of a rarity, and I really appreciate it.

With that out of the way, would you be willing to consider implementing support for OpenSSL's SSLKEYLOGFILE? :)

I've hacked it into a quick fork I setup for a POC I'm writing, but think this might be something that other people would be interested in too.

My specific use-case is a little niche - we passively tap and capture packets that go to/from our websocket client application and would like to be able to log the ssl decryption keys in order to compare the applications output against the raw traffic - but I'm sure it would be helpful in all kinds of data collection contexts.

Thanks!

bsergean commented 4 months ago

Thanks for the good words.

If you can think of a generic way to handle that it would be good. Is it more focused on the client or on the server at this point ? Is your POC really complicated.

Apparently curl supports this, can you see how they do it ? https://wiki.wireshark.org/TLS

WSL-Ben commented 4 months ago

The POC is pretty simple and can be compared against your master branch here: https://github.com/machinezone/IXWebSocket/compare/master...WSL-Ben:IXWebSocket:ssl-keylogfile

Not sure it's the best solution, but I can confirm that it works. Ensure you're using OpenSSL and set the tlsOptions.keyLogFile and you should see the keys written out to the specified location.

bsergean commented 4 months ago

Code is pretty simple, can you explain why you need the mutex and the global variable ?

WSL-Ben commented 4 months ago

Not really. Being honest, I have no idea what I'm doing and just kind of stopped once it worked.

Regarding the mutex, I was under the impression that due to multiple SSL contexts existing and being used in different threads that I'd need to protect it from a race condition. Sounds like I might have been wrong.

bsergean commented 4 months ago

lol it's fine not to be sure ... programming is hard :)

There should be one context by IXSocket object already, so I'm confident we don't need the mutex.

WSL-Ben commented 4 months ago

Ok, I've dropped the mutex and replaced the global variable with a static class member. Thanks for the direction, I always appreciate an opportunity to learn.

bsergean commented 4 months ago

I think static member is pretty equivalent, unless you initialize it with a getenv(“OPENSSL_VALUE”), but we need some logic to run for that.

What I am after is something that runs in the constructor of the class, and to make that _isNewOpenSSLThing a normal member variable.

On Feb 12, 2024, at 2:58 AM, WSL-Ben @.***> wrote:

Ok, I've dropped the mutex and replaced the global variable with a static class member. Thanks for the direction, I always appreciate an opportunity to learn.

— Reply to this email directly, view it on GitHub https://github.com/machinezone/IXWebSocket/issues/499#issuecomment-1938453290, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC2O6UMITZYGIJV3YKC5R73YTHYV3AVCNFSM6AAAAABDCREEDCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZYGQ2TGMRZGA. You are receiving this because you commented.

WSL-Ben commented 4 months ago

The callback for OpenSSL needs to be static in order to match the expected callback signature. It also doesn't accept a void* callback, which in my limited experience means that it won't really be possible to store it as a member variable, at least not in a way where it's accessible to the callback - though I'll be happy to find I'm incorrect as I've used this pattern in the past and find it less than ideal.

To clarify on the feature as well, I don't think there is any necessity to support the SSLKEYLOGFILE environment variable, simply to expose it as an option via the TLSOptions class.

I've also noticed with my patch that the first SSL key is written out, but subsequent ones fail to either call the callback or find a mapping from the SSL context to the filename. Are SSL context's designed to last the lifetime of the connection?

bsergean commented 4 months ago

Yeah you’re right.

On Feb 12, 2024, at 2:04 PM, WSL-Ben @.***> wrote:

The callback for OpenSSL needs to be static in order to match the expected callback signature. It also doesn't accept a void* callback, which in my limited experience means that it won't really be possible to store it as a member variable - though I'll be happy to find I'm incorrect as I've used this pattern in the past and find it less than ideal.

— Reply to this email directly, view it on GitHub https://github.com/machinezone/IXWebSocket/issues/499#issuecomment-1939670698, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC2O6UOPCWP7G5IWQHEKZ6LYTKGXVAVCNFSM6AAAAABDCREEDCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZZGY3TANRZHA. You are receiving this because you commented.