[#4736] Clear disabled SSL protocols before enabling provided SSL pro… #4737

blucas · 2016-01-20T21:52:26Z

…tocols

Motivation:

Attempts to enable SSL protocols which are currently disabled fail when using the OpenSslEngine. Related to #4736

Modifications:

Clear out all options that have disabled SSL protocols before attempting to enable any SSL protocol.

Result:

setEnabledProtocols works as expected.

blucas · 2016-01-20T21:53:48Z

@normanmaurer @Scottmitch PTAL

Note: this pull request assumes that a new netty-tcnative tag (1.1.33.Fork11) has been created. This pull request will not build successfully until that tag exists.

Scottmitch · 2016-01-20T23:19:27Z

handler/src/main/java/io/netty/handler/ssl/OpenSslEngine.java

@@ -1076,21 +1076,31 @@ public void setEnabledProtocols(String[] protocols) {
                // Enable all and then disable what we not want
                SSL.setOptions(ssl, SSL.SSL_OP_ALL);

+                int opts = SSL.SSL_OP_NO_SSLv2 | SSL.SSL_OP_NO_SSLv3 | SSL.SSL_OP_NO_TLSv1 |


nit: i guess there is no need to assign this to a temp variable ... consider just passing in the mask directly to clearOptions

Initially, I did what you suggested, but then noticed I needed the temp variable for the call to setOptions so thought I might as well use it for clearOptions as well. I'm not bothered either way, let me know your thoughts and I'll make the change if you still think I should.

@blucas please adjust as @Scottmitch suggested.

Scottmitch · 2016-01-20T23:21:46Z

@blucas - Any thoughts on a test for this, maybe create peers that don't support the same versions and verify the handshake fails?

I'll assign to @normanmaurer to pull in after tcnative is released.

johnou · 2016-01-21T09:08:30Z

pom.xml

@@ -298,7 +298,7 @@
      <dependency>
        <groupId>${project.groupId}</groupId>
        <artifactId>netty-tcnative</artifactId>
-        <version>1.1.33.Fork10</version>
+        <version>1.1.33.Fork11</version>


revert? this artifact does not exist in central Maven and is the reason why the CI build failed.

No it's fine. We need a new netty-tcnative release for this PR

@normanmaurer - Do you think we could cut a netty-tcnative release for this soon?

Yes

Am 21.01.2016 um 11:02 schrieb Brendt notifications@github.com:

In pom.xml:

@@ -298,7 +298,7 @@

${project.groupId}
netty-tcnative

<version>1.1.33.Fork10</version>

<version>1.1.33.Fork11</version>
@normanmaurer - Do you think we could cut a netty-tcnative release for this soon?

—
Reply to this email directly or view it on GitHub.

Release was just done

blucas · 2016-01-21T10:06:14Z

Any thoughts on a test for this, maybe create peers that don't support the same versions and verify the handshake fails?

I think a more fundamental test would be nice. Something that:

Creates an OpenSslEngine
Disables an already enabled protocol (via setEnabledProtocols)
Re-enables the protocol (via setEnabledProtocols) that was disabled in step 2

WDYT?

normanmaurer · 2016-01-21T11:04:42Z

+1 ... Maybe you could make it part of SSLEngineTest ?

Am 21.01.2016 um 11:06 schrieb Brendt notifications@github.com:

Any thoughts on a test for this, maybe create peers that don't support the same versions and verify the handshake fails?

I think a more fundamental test would be nice. Something that:

Creates an OpenSslEngine

Disables an already enabled protocol

Re-enables the protocol that was disabled in step 2

WDYT?

—
Reply to this email directly or view it on GitHub.

blucas · 2016-01-21T21:25:55Z

@normanmaurer @Scottmitch - I've added a test and removed the temp variable. PTAL

Scottmitch · 2016-01-22T01:54:05Z

handler/src/test/java/io/netty/handler/ssl/JdkSslEngineTest.java

@@ -263,6 +267,40 @@ public String select(List<String> protocols) {
        }
    }

+    @Test
+    public void testEnablingAnAlreadyDisabledSslProtocol() throws Exception {


can we just have this test live in SSLEngineTest? That way we won't need to duplicate the code.

@blucas to make the same test work with OpenSslEngine and the jdk one you can do and instanceof .

Scottmitch · 2016-01-22T01:54:51Z

@blucas - Code looks good! after #4737 (comment) is addressed and squashed we can pull in.

normanmaurer · 2016-01-22T06:55:42Z

handler/src/test/java/io/netty/handler/ssl/OpenSslEngineTest.java

@@ -60,6 +66,42 @@ public void testAlpnCompatibleProtocolsDifferentClientOrder() throws Exception {
        runTest(PREFERRED_APPLICATION_LEVEL_PROTOCOL);
    }

+    @Test
+    public void testEnablingAnAlreadyDisabledSslProtocol() throws Exception {
+        SSLEngine sslEngine = null;


Check if openssl is present first. See the tests above

normanmaurer · 2016-01-22T13:01:42Z

@blucas can you address this today as I want to cut 4.1.0.CR1 today ? If not we will need to include it in the next release.

johnou · 2016-01-22T13:52:57Z

@normanmaurer #4744

blucas · 2016-01-22T14:44:01Z

@normanmaurer - I've made the changes, if you are happy I can squash them.

johnou · 2016-01-22T14:47:42Z

@blucas I think @Scottmitch may have meant moving the entire test to the super class like this johnou@5d8557b it will be run for all concrete classes which will set the correct provider via io.netty.handler.ssl.SSLEngineTest#sslProvider

blucas · 2016-01-22T14:51:43Z

@johnou - Looking at your test, I don't think it will pass in the case where sslEngine is an instance of OpenSslEngine (OpenSslEngine will always include SSLv2Hello when calling getEnabledProtocols). Diff my version to yours. AFAIK Scott wanted me to get rid of the code duplication, which, in essence, I've done.

normanmaurer · 2016-01-22T15:00:51Z

@blucas @johnou I will cherry-pick this one once the ci pass.

blucas · 2016-01-22T15:01:45Z

@normanmaurer - Do you need me to squash?

normanmaurer · 2016-01-22T15:01:55Z

@blucas yes please

blucas · 2016-01-22T15:02:34Z

@normanmaurer - Can I do that even while the CI build is running?

…tocols Motivation: Attempts to enable SSL protocols which are currently disabled fail when using the OpenSslEngine. Related to #4736 Modifications: Clear out all options that have disabled SSL protocols before attempting to enable any SSL protocol. Result: setEnabledProtocols works as expected.

normanmaurer · 2016-01-22T15:09:00Z

@blucas yes

johnou · 2016-01-22T15:11:59Z

@blucas wouldn't it make more sense to have the actual test in the super class so if a new provider is added it is automatically covered?

blucas · 2016-01-22T15:13:44Z

@normanmaurer - Done

@johnou - You are right, but maybe we can worry about fixing that in a separate PR so that Norman can get this one cherry-picked and cut a release.

johnou · 2016-01-22T15:17:22Z

alright cool, good work with the contribution 👍

normanmaurer · 2016-01-22T16:33:26Z

@blucas thanks again!

Cherry-picked into 4.1 as 7090d13 and into 4.0 as 357720e

Scottmitch · 2016-01-22T18:38:18Z

good work @blucas !

blucas · 2016-01-22T20:47:12Z

@normanmaurer @Scottmitch @johnou - Thanks guys, I'm glad I could help

Scottmitch reviewed Jan 20, 2016
View reviewed changes

Scottmitch added the defect label Jan 20, 2016

Scottmitch added this to the 4.0.34.Final milestone Jan 20, 2016

Scottmitch assigned normanmaurer Jan 20, 2016

johnou reviewed Jan 21, 2016
View reviewed changes

Scottmitch reviewed Jan 22, 2016
View reviewed changes

normanmaurer reviewed Jan 22, 2016
View reviewed changes

normanmaurer mentioned this pull request Jan 22, 2016

[#4736] Clear disabled SSL protocols before enabling provided SSL pro… #4744

Closed

normanmaurer closed this Jan 22, 2016

normanmaurer mentioned this pull request Jan 22, 2016

OpenSslEngine.setEnabledProtocols fails to enable protocols that are currently disabled #4736

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#4736] Clear disabled SSL protocols before enabling provided SSL pro… #4737

[#4736] Clear disabled SSL protocols before enabling provided SSL pro… #4737

blucas commented Jan 20, 2016

blucas commented Jan 20, 2016

Scottmitch Jan 20, 2016

blucas Jan 21, 2016

normanmaurer Jan 21, 2016

Scottmitch commented Jan 20, 2016

johnou Jan 21, 2016

normanmaurer Jan 21, 2016

blucas Jan 21, 2016

normanmaurer Jan 21, 2016

normanmaurer Jan 21, 2016

blucas commented Jan 21, 2016

normanmaurer commented Jan 21, 2016

blucas commented Jan 21, 2016

Scottmitch Jan 22, 2016

normanmaurer Jan 22, 2016

normanmaurer Jan 22, 2016

Scottmitch commented Jan 22, 2016

normanmaurer Jan 22, 2016

normanmaurer commented Jan 22, 2016

johnou commented Jan 22, 2016

blucas commented Jan 22, 2016

johnou commented Jan 22, 2016

blucas commented Jan 22, 2016

normanmaurer commented Jan 22, 2016

blucas commented Jan 22, 2016

normanmaurer commented Jan 22, 2016

blucas commented Jan 22, 2016

normanmaurer commented Jan 22, 2016

johnou commented Jan 22, 2016

blucas commented Jan 22, 2016

johnou commented Jan 22, 2016

normanmaurer commented Jan 22, 2016

Scottmitch commented Jan 22, 2016

blucas commented Jan 22, 2016

[#4736] Clear disabled SSL protocols before enabling provided SSL pro… #4737

[#4736] Clear disabled SSL protocols before enabling provided SSL pro… #4737

Conversation

blucas commented Jan 20, 2016

blucas commented Jan 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scottmitch commented Jan 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blucas commented Jan 21, 2016

normanmaurer commented Jan 21, 2016

blucas commented Jan 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scottmitch commented Jan 22, 2016

Choose a reason for hiding this comment

normanmaurer commented Jan 22, 2016

johnou commented Jan 22, 2016

blucas commented Jan 22, 2016

johnou commented Jan 22, 2016

blucas commented Jan 22, 2016

normanmaurer commented Jan 22, 2016

blucas commented Jan 22, 2016

normanmaurer commented Jan 22, 2016

blucas commented Jan 22, 2016

normanmaurer commented Jan 22, 2016

johnou commented Jan 22, 2016

blucas commented Jan 22, 2016

johnou commented Jan 22, 2016

normanmaurer commented Jan 22, 2016

Scottmitch commented Jan 22, 2016

blucas commented Jan 22, 2016