Introduce a new native method to access the master key#421
Introduce a new native method to access the master key#421normanmaurer merged 1 commit intonetty:masterfrom
Conversation
|
Can one of the admins verify this patch? |
|
@netty-bot test this please |
openssl-dynamic/src/main/c/ssl.c
Outdated
| size_t SSL_SESSION_get_master_key(const SSL_SESSION *session, | ||
| unsigned char *out, size_t outlen) | ||
| { | ||
| if (outlen == 0) |
|
@normanmaurer Thanks for the comments. I'll address the feedback. I'd like to include a unit test, do you have a good example of how to start a TLS session with the code in this JAR? I'd only know how to write a test by starting client+server but that would need a cyclic dependency on SslHandler in Netty. |
|
@fzakaria unfortually no :( I guess the best bet is to include a test in Netty itself. |
|
Thanks. I'll work on that for a post PR commit I guess. |
|
@normanmaurer Addressed your feedback -- I've packaged the work on the test case so far: |
|
@normanmaurer I've included a test in netty that proves the methods work by manually decrypting the TLS application data -- netty/netty#8699 |
|
@normanmaurer ping |
|
@fzakaria sorry for the delay... Will try to review tomorrow latest. |
| TCN_ASSERT(bytesMoved == keyLength); | ||
|
|
||
| jbyteArray jKey = (*e)->NewByteArray(e, (jsize) bytesMoved); | ||
| (*e)->SetByteArrayRegion(e, jKey, 0, bytesMoved, (jbyte*) key); |
There was a problem hiding this comment.
I think we need to guard against when NewByteArray(...) returns NULL (like if there is no memory left).
There was a problem hiding this comment.
I can add the NULL checks however I stuck to previous convention where the other JNI calls did no such guard.
Maybe a separate post-PR to add the null checks to all the methods ?
| size_t bytesMoved = SSL_get_server_random(ssl_, key, keyLength); | ||
| TCN_ASSERT(bytesMoved == keyLength); | ||
|
|
||
| jbyteArray jKey = (*e)->NewByteArray(e, (jsize) bytesMoved); |
| size_t bytesMoved = SSL_SESSION_get_master_key(session, key, keyLength); | ||
| TCN_ASSERT(bytesMoved == keyLength); | ||
|
|
||
| jbyteArray jKey = (*e)->NewByteArray(e, (jsize) bytesMoved); |
|
@Lukasa may I ask you to check this as well... |
|
@normanmaurer @Lukasa Please see https://github.com/netty/netty/pull/8699/files#diff-7a4cbf2f526e2066184fb9ef098cd815R1127 also on a test-case of it being used to then decrypt the TLS data by hand. |
|
@fzakaria please sign our ICLA and let me know once done: https://netty.io/s/icla I will pull it in after this is done . Thanks for the work! |
|
Done. |
|
@fzakaria can you do me a favour and squash + adjust the commit message to match our template: |
5b1bdd4 to
f8ae901
Compare
Motivation: This change accompanies the broader pull-request introduced by netty/netty#8653 to allow access to the SSL session master key for debugging purposes. For instance, you would be able to log the master key to decrypt the SSL session in Wireshark. Modifications: Exported two new functions through the JNI interface that allow access to the necessary information to decrypt the SSL session. Result: After this change, you can easily decrypt SSL sessions that use Diffie-Hellman key exchange (perfect forward secrecy) with tools like Wireshark or decrypt the payloads in Java following the example outlined https://github.com/netty/netty/pull/8699/files#diff-7a4cbf2f526e2066184fb9ef098cd815R1127 Signed-off-by: Farid Zakaria <farid.m.zakaria@gmail.com>
c379c5d to
c7508ca
Compare
|
@fzakaria thanks |
|
Thanks for merging @normanmaurer |
Motivation: I've introduced netty/netty-tcnative#421 that introduced exposing OpenSSL master key & client/server random values with the purpose of allowing someone to log them to debug the traffic via auxiliary tools like Wireshark (see also netty#8653) Modification: Augmented OpenSslEngineTest to include a test which manually decrypts the TLS ciphertext after exposing the masterkey + client/server random. This acts as proof that the tc-native new methods work correctly! Signed-off-by: Farid Zakaria <farid.m.zakaria@gmail.com>
Motivation: I've introduced netty/netty-tcnative#421 that introduced exposing OpenSSL master key & client/server random values with the purpose of allowing someone to log them to debug the traffic via auxiliary tools like Wireshark (see also #8653) Modification: Augmented OpenSslEngineTest to include a test which manually decrypts the TLS ciphertext after exposing the masterkey + client/server random. This acts as proof that the tc-native new methods work correctly! Result: More tests Signed-off-by: Farid Zakaria <farid.m.zakaria@gmail.com>
Motivation: I've introduced netty/netty-tcnative#421 that introduced exposing OpenSSL master key & client/server random values with the purpose of allowing someone to log them to debug the traffic via auxiliary tools like Wireshark (see also #8653) Modification: Augmented OpenSslEngineTest to include a test which manually decrypts the TLS ciphertext after exposing the masterkey + client/server random. This acts as proof that the tc-native new methods work correctly! Result: More tests Signed-off-by: Farid Zakaria <farid.m.zakaria@gmail.com>
This change accompanies the broader pull-request introduced by
netty/netty#8653 to allow access to the SSL session master key for debugging purposes.
Accessing the master key is useful if you ever need to decrypt the TLS session for debugging purposes with tools such as Wireshark.
Access of the field from the SSL session struct is direct as the wrapper method was only introduced in 1.1.0+ https://www.openssl.org/docs/man1.1.0/ssl/SSL_SESSION_get_master_key.html