Skip to content

Introduce test that manually decrypts TLS via OpenSSL#8699

Merged
normanmaurer merged 1 commit intonetty:4.1from
fzakaria:test-tc-native
Jun 28, 2019
Merged

Introduce test that manually decrypts TLS via OpenSSL#8699
normanmaurer merged 1 commit intonetty:4.1from
fzakaria:test-tc-native

Conversation

@fzakaria
Copy link
Copy Markdown
Contributor

@fzakaria fzakaria commented Dec 31, 2018

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!

It would be great to include the test in netty-tcnative itself however I leveraged the JDK SSLEngine implementation to perform the TLS handshake.

Result:
Tests pass!

@fzakaria fzakaria mentioned this pull request Dec 31, 2018
Merged
@netty-bot
Copy link
Copy Markdown

netty-bot commented Dec 31, 2018

Can one of the admins verify this patch?

fzakaria
fzakaria commented Dec 31, 2018
View reviewed changes
normanmaurer
normanmaurer requested changes Feb 25, 2019
View reviewed changes
@normanmaurer
Copy link
Copy Markdown
Member

normanmaurer commented Mar 1, 2019

@fzakaria netty-tcnative 2.0.21.Final was released

@fzakaria fzakaria mentioned this pull request Mar 1, 2019
Closed
@fzakaria
Copy link
Copy Markdown
Contributor Author

fzakaria commented Mar 1, 2019

Thanks for the feedback @normanmaurer
I've updated the PR with 2.0.22.Final

normanmaurer
normanmaurer requested changes Mar 8, 2019
View reviewed changes
@fzakaria fzakaria mentioned this pull request Mar 8, 2019
Merged
@normanmaurer normanmaurer added this to the 4.1.35.Final milestone Apr 9, 2019
normanmaurer
normanmaurer requested changes Apr 9, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@normanmaurer normanmaurer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review. Ping me when ready

handler/src/test/java/io/netty/handler/ssl/OpenSslEngineTest.java Outdated
Copy link
Copy Markdown
Member

@normanmaurer normanmaurer Apr 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it seems your implementation is based on the code linked above I think we would also do something related to licensing after looking at the header of the file:

https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/samples/sslengine/SSLEngineSimpleDemo.java

If this code is completely different you may want to just remove the link tho.

Copy link
Copy Markdown
Contributor Author

@fzakaria fzakaria Apr 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's imitated in the sense that I had no clue how to use the engine API.
There is only a single way really to use it though to go through the state machine.

@normanmaurer
Copy link
Copy Markdown
Member

normanmaurer commented Apr 10, 2019

@netty-bot let me know once addressed

@fzakaria
Copy link
Copy Markdown
Contributor Author

fzakaria commented Apr 16, 2019

Sorry for the delay @normanmaurer -- hopefully I addressed all your feedback.

@fzakaria
Copy link
Copy Markdown
Contributor Author

fzakaria commented Jun 26, 2019

ping.

@normanmaurer
Copy link
Copy Markdown
Member

normanmaurer commented Jun 27, 2019

@netty-bot test this please

@fzakaria
Copy link
Copy Markdown
Contributor Author

fzakaria commented Jun 27, 2019 •
edited
Loading

@normanmaurer

io.netty.channel.pool.FixedChannelPoolTest.testAcquireNewConnection:
io.netty.channel.ChannelException: address already in use by: [id: 0x10105d6f, L:local:test.id]

Looks like an intermittent failure. Not related to the code change.
I took a quick look at the failing test -- it could randomize the local address to avoid conflicts with failing tests.

@normanmaurer
Copy link
Copy Markdown
Member

normanmaurer commented Jun 27, 2019

@fzakaria please fix compile error:

12:47:25 [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.0:testCompile (default-testCompile) on project netty-handler: Compilation failure
12:47:25 [ERROR] /code/handler/src/test/java/io/netty/handler/ssl/OpenSslEngineTest.java:[28,31] error: package org.apache.commons.lang3 does not exist

@fzakaria
Copy link
Copy Markdown
Contributor Author

fzakaria commented Jun 27, 2019

@normanmaurer must be a change from 4.1 master
I just ran

mvn -pl handler clean install
...
[INFO] --- maven-install-plugin:2.4:install (default-install) @ netty-handler ---
[INFO] Installing /home/fmzakari/Development/netty/handler/target/netty-handler-4.1.35.Final-SNAPSHOT.jar to /home/fmzakari/.m2/repository/io/netty/netty-handler/4.1.35.Final-SNAPSHOT/netty-handler-4.1.35.Final-SNAPSHOT.jar
[INFO] Installing /home/fmzakari/Development/netty/handler/pom.xml to /home/fmzakari/.m2/repository/io/netty/netty-handler/4.1.35.Final-SNAPSHOT/netty-handler-4.1.35.Final-SNAPSHOT.pom
[INFO] Installing /home/fmzakari/Development/netty/handler/target/netty-handler-4.1.35.Final-SNAPSHOT-sources.jar to /home/fmzakari/.m2/repository/io/netty/netty-handler/4.1.35.Final-SNAPSHOT/netty-handler-4.1.35.Final-SNAPSHOT-sources.jar
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 42.284 s
[INFO] Finished at: 2019-06-27T13:21:14-07:00
[INFO] Final Memory: 59M/1417M
[INFO] ------------------------------------------------------------------------

Let me remove the offending line.

@fzakaria
Add a test for OpenSslEngine which decrypts traffic
11c4140 
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>
@fzakaria
Copy link
Copy Markdown
Contributor Author

fzakaria commented Jun 27, 2019

@netty-bot test this please

@normanmaurer
Copy link
Copy Markdown
Member

normanmaurer commented Jun 28, 2019

@netty-bot Test this please

@normanmaurer normanmaurer merged commit efe40ac into netty:4.1 Jun 28, 2019
@normanmaurer
Copy link
Copy Markdown
Member

normanmaurer commented Jun 28, 2019

@fzakaria thanks a lot

normanmaurer pushed a commit that referenced this pull request Jun 28, 2019
@fzakaria @normanmaurer
Add a test for OpenSslEngine which decrypts traffic (#8699)
cc1528b 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@normanmaurer normanmaurer normanmaurer approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

4.1.38.Final

Development

Successfully merging this pull request may close these issues.

3 participants

@fzakaria @netty-bot @normanmaurer