Update JDK SSL Tests to use SSL Context Builder. #4520
Conversation
|
|
||
| return new JdkSslServerContext(crtFile, keyFile, "12345"); | ||
| String keyPassword = "12345"; | ||
| return SslContextBuilder.forServer(crtFile, keyFile, keyPassword).build(); |
There was a problem hiding this comment.
just inline the password as before ?
ifesdjeen
force-pushed
the
ssl-tests
branch
3 times, most recently
from
6e30c36
to
51e0336
Compare
Last I checked OpenSSL did not support failing the handshake due to no compatible protocols. See openssl/openssl#188. |
| @@ -59,13 +62,15 @@ public void testNpn() throws Exception { | |||
| // Check in this test just in case we have multiple tests that just the class and we already ignored the | |||
| // initialization error. | |||
| if (!JdkNpnSslEngine.isAvailable()) { | |||
| throw new RuntimeException("NPN not on classpath"); | |||
| throw new SkipTestException("NPN not on classpath"); | |||
There was a problem hiding this comment.
consider moving this to a method to ensure consistency:
// Maybe use Protocol instead of String....
private SkipTestException tlsExtensionNotFound(String extensionName) {
throw new SkipTestException(extensionName + " not on classpath");
}
private SkipTestException npnExtensionNotFound() {
return tlsExtensionNotFound("NPN");
}
// also one for ALPN
...
Ah, I'm was talking about JdkSsl... I've only ported the tests for now. This test ( |
|
@ifesdjeen - I see what you are saying now. This is a pretty specific test that we wouldn't necessarily expect a user to replicate this use case. I'm OK with ignoring the deprecation here. I think the path forward when we re-analyze the deprecated constructor is to make it package private (as opposed to completely removing it) for testing purposes. |
|
@ifesdjeen - Can you just update the commit message to follow the format defined in http://netty.io/wiki/writing-a-commit-message.html and then I can pull this in? |
Motivation: Use new / non-deprecated APIs for creating SSL Context in tests, in order to be able to implement OpenSsl tests with maximum code reuse. Modifications: Use `SslContextBuilder.(forServer|forClient)` instead of deprecated `JdkSslServerContext` constructor. Use `ApplicationProtocolConfig` instead of Protocol Negotiator. Use custom exception type for skipping tests to avoid swallowing exceptions arising from tests. Result: Exceptions from tests aren't swallowed. Using new APIs allows reusing same test code for OpenSsl tests.
|
Oh sure absolutely. Somehow I missed the commit message part completely, sorry about that. |
|
Cherry-picked 4.0 (38838b8) 4.1 (43ebbc3) master (613c8b2) Thanks @ifesdjeen ! |
|
<3 thank you @Scottmitch, I hope more PRs to come! |
|
+1 @ifesdjeen - keem em' coming! |
This is the first patch to get closer to #4293. I'm cleaning up all the plances and am trying to bring test cases as close together as possible to reuse the code for both JDK and OpenSSL.
So far, the following things have been updated:
However, I still have a question about
testAlpnNoCompatibleProtocolsClientHandshakeFailure. I couldn't find a way to reproduce same behaviour withSslContextBuilder, since in this case we're limited toApplicationProtocolConfigand how it maps to Negotiator. Closest way would be to check for theSslHandshakeExceptionon the server side.