Commit Graph

798 Commits

Author SHA1 Message Date
Scott Mitchell
a2304287a1 SslContext to support TLS/SSL protocols
Motivation:
SslContext and SslContextBuilder do not support a way to specify the desired TLS protocols. This currently requires that the user extracts the SSLEngine once a context is built and manually call SSLEngine#setEnabledProtocols(String[]). Something this critical should be supported at the SslContext level.

Modifications:
- SslContextBuilder should accept a list of protocols to configure for each SslEngine

Result:
SslContext consistently sets the supported TLS/SSL protocols.
2017-03-08 09:24:59 -08:00
Scott Mitchell
6bb661302f OpenSsl tests incomplete check for supporting key manager
Motivaiton:
It is possible that if the OpenSSL library supports the interfaces required to use the KeyManagerFactory, but we fail to get the io.netty.handler.ssl.openssl.useKeyManagerFactory system property (or this property is set to false) that SSLEngineTest based unit tests which use a KeyManagerFactory will fail.

Modifications:
- We should check if the OpenSSL library supports the KeyManagerFactory interfaces and if the system property allows them to be used in OpenSslEngineTests

Result:
Unit tests which use OpenSSL and KeyManagerFactory will be skipped instead of failing.
2017-03-07 08:24:06 +01:00
Scott Mitchell
53fc693901 SslHandler and OpenSslEngine miscalculation of wrap destination buffer size
Motivation:
When we do a wrap operation we calculate the maximum size of the destination buffer ahead of time, and return a BUFFER_OVERFLOW exception if the destination buffer is not big enough. However if there is a CompositeByteBuf the wrap operation may consist of multiple ByteBuffers and each incurs its own overhead during the encryption. We currently don't account for the overhead required for encryption if there are multiple ByteBuffers and we assume the overhead will only apply once to the entire input size. If there is not enough room to write an entire encrypted packed into the BIO SSL_write will return -1 despite having actually written content to the BIO. We then attempt to retry the write with a bigger buffer, but because SSL_write is stateful the remaining bytes from the previous operation are put into the BIO. This results in sending the second half of the encrypted data being sent to the peer which is not of proper format and the peer will be confused and ultimately not get the expected data (which may result in a fatal error). In this case because SSL_write returns -1 we have no way to know how many bytes were actually consumed and so the best we can do is ensure that we always allocate a destination buffer with enough space so we are guaranteed to complete the write operation synchronously.

Modifications:
- SslHandler#allocateNetBuf should take into account how many ByteBuffers will be wrapped and apply the encryption overhead for each
- Include the TLS header length in the overhead computation

Result:
Fixes https://github.com/netty/netty/issues/6481
2017-03-06 08:15:13 -08:00
Scott Mitchell
2cff918044 Correct usages of internalNioBuffer
Motivation:
There are numerous usages of internalNioBuffer which hard code 0 for the index when the intention was to use the readerIndex().

Modifications:
- Remove hard coded 0 for the index and use readerIndex()

Result:
We are less susceptible to using the wrong index, and don't make assumptions about the ByteBufAllocator.
2017-03-02 12:51:22 -08:00
Scott Mitchell
1f6782894a OpenSslEngine wrap with heap buffer bug
Motivation:
ReferenceCountedOpenSslEngine#wrap must have a direct buffer for a destination to interact with JNI. If the user doesn't supply a direct buffer we internally allocate one to write the results of wrap into. After this operation completes we copy the contents of the direct buffer into the heap buffer and use internalNioBuffer to get the content. However we pass in the end index but the internalNioBuffer expects a length.

Modifications:
- pass the length instead of end index to internalNioBuffer

Result:
ReferenceCountedOpenSslEngine#wrap will copy the correct amount of data into the destination buffer when heap buffers are wrapped.
2017-03-02 12:49:35 -08:00
Norman Maurer
7e7e10fb1e Correct SslContextBuilder javadocs
Motivation:

SslContextBuilder sill state the KeyManagerFactory and TrustManagerFactory are only supported when SslProvider.JDK is used. This is not correct anymore.

Modifications:

Fix javadocs.

Result:

Correct javadocs.
2017-03-02 09:36:45 +01:00
Stephane Landelle
79e24d1a17 Add javadoc warning on SslContext#newHandler client-side
Motivation:

SslContext#newHandler currently creates underlying SSLEngine without
enabling HTTPS endpointIdentificationAlgorithm. This behavior in
unsecured when used on the client side.

We can’t harden the behavior for now, as it would break existing
behavior, for example tests using self signed certificates.

Proper hardening will happen in a future major version when we can
break behavior.

Modifications:

Add javadoc warnings with code snippets.

Result:

Existing unsafe behavior and workaround documented.
2017-03-01 10:44:44 +01:00
Norman Maurer
ca2c349a4a Correctly have exceptions thrown from decode(...) method be wrapped with DecodingException
Motivation:

Normally if a decoder produces an exception its wrapped with DecodingException. This is not the cause for NotSslRecordException in SslHandler and SniHandler.

Modifications:

Just throw the NotSslRecordException exception for decode(...) and so ensure its correctly wrapped in a DecodingException before its passed through the pipeline.

Result:

Consist behavior.
2017-03-01 06:41:30 +01:00
Scott Mitchell
70bcb40855 OpenSslEngine may use networkBIO after calling shutdown
Motivation:
To ensure that all bytes queued in OpenSSL/tcnative internal buffers we invoke SSL_shutdown again to stimulate OpenSSL to write any pending bytes. If this call fails we may call SSL_free and the associated shutdown method to free resources. At this time we may attempt to use the networkBIO which has already been freed and get a NPE.

Modifications:
- Don't call bioLengthByteBuffer(networkBIO) if we have called shutdown() in ReferenceCountedOpenSslEngine

Result:
Fixes https://github.com/netty/netty/issues/6466
2017-02-28 12:01:12 -08:00
Nikolay Fedorovskikh
c0cd3db5b1 Fix class references of its subclass issue
Motivation:

Realization of `AbstractTrafficShapingHandler.userDefinedWritabilityIndex()` has references to subclasses.
In addition, one of the subclasses overriding it, but the other does not.

Modifications:

Add overriding to the second subclass. Remove references to subclasses from parent class.

Result:

More consistent and clean code (OOP-stylish).
2017-02-27 07:48:51 +01:00
Norman Maurer
22ccf6c7b1 Fix test-failures introduced 325cc84a2e on Java7
Motivation:

325cc84a2e introduced new tests which uses classes only provided by Java8+. We need to ensure we only try to load classes needed for these when we run the tests on Java8+ so we still can run the testsuite with Java7.

Modifications:

Add extra class which only gets loaded when Java8+ is used and move code there.

Result:

No more class-loader issue when running tests with Java7.
2017-02-24 10:36:30 +01:00
Norman Maurer
325cc84a2e Throw if SSLParameters contains settings that are not supported by ReferenceCountedOpenSslEngine
Motivation:

We not support all SSLParameters settings so we should better throw if a user try to use them.

Modifications:

- Check for unsupported parameters
- Add unit test

Result:

Less surprising behavior.
2017-02-23 20:00:40 +01:00
Nikolay Fedorovskikh
0623c6c533 Fix javadoc issues
Motivation:

Invalid javadoc in project

Modifications:

Fix it

Result:

More correct javadoc
2017-02-22 07:31:07 +01:00
Scott Mitchell
e08a3b1f35 Fix SSLException check for JDK work around missed by 2dffc2f9fb 2017-02-20 21:38:04 -08:00
Scott Mitchell
2dffc2f9fb SSLEngineTest issue introduced by d8e6fbb9c3
Motivation:
d8e6fbb9c3 attempted to account for the JDK not throwing the expected SSLHandshakeException by allowing a SSLException to also pass the test. However in some situations the SSLException will not be the top level exception and the Throwable must be unwrapped to see if the root cause is an SSLException.

Modifications:
- Unwrap exceptions thrown by the JDK's SSLEngine to check for SSLException.

Result:
SSLEngineTest (and derived classes) are more reliable.
2017-02-20 12:19:41 -08:00
Norman Maurer
c57a1bdb2d Log used native library by netty-tcnative
Motivation:

As netty-tcnative can be build against different native libraries and versions we should log the used one.

Modifications:

Log the used native library after netty-tcnative was loaded.

Result:

Easier to understand what native SSL library was used.
2017-02-20 20:52:22 +01:00
Scott Mitchell
d8e6fbb9c3 OpenSslEngine should respect hostname verification
Motivation:
OpenSSL doesn't automatically verify hostnames and requires extract method calls to enable this feature [1]. We should allow this to be configured.

Modifications:
- SSLParamaters#getEndpointIdentificationAlgorithm() should be respected and configured via tcnative interfaces.

Result:
OpenSslEngine respects hostname verification.

[1] https://wiki.openssl.org/index.php/Hostname_validation
2017-02-17 13:21:29 -08:00
Scott Mitchell
5de4029b43 Checkstyle fix from 56694eb 2017-02-16 22:28:56 -08:00
Scott Mitchell
56694ebc0f Cleanup from fbf0e5f4dd
Motivation:
ThreadLocalInsecureRandom still referenced ThreadLocalRandom directly, but shouldn't.

Modifications:
ThreadLocalInsecureRandom should reference PlatformDependent#threadLocalRandom() in comments

Result:
Less usage of internal.ThreadLocalRandom.
2017-02-16 15:56:23 -08:00
Norman Maurer
fbf0e5f4dd Prefer JDK ThreadLocalRandom implementation over ours.
Motivation:

We have our own ThreadLocalRandom implementation to support older JDKs . That said we should prefer the JDK provided when running on JDK >= 7

Modification:

Using ThreadLocalRandom implementation of the JDK when possible.

Result:

Make use of JDK implementations when possible.
2017-02-16 15:44:00 -08:00
Norman Maurer
b2f7e8648e Fix ReferenceCountedOpenSslEngine.getEnabledProtocols() when using boringssl
Motivation:

Commit cd3bf3df58 made netty observe the latest version of netty-tcnative which changed the way how static fields are computed for various SSL.* values. This lead to have SSL_OP_NO_SSLv2 become 0 when using boringssl as boringssl not supports SSLv2 at all. In the logic of ReferenceCountedOpenSslEngine.getEnabledProtocols() we not expect to have a zero value and so our logic fails.

Modifications:

Check we actual support the protocol before return it as enabled.

Result:

SSLEngineTest.testEnablingAnAlreadyDisabledSslProtocol passes again with boringssl
2017-02-16 07:47:06 +01:00
Scott Mitchell
4431ad894d OpenSslEngine may lose data if the non-application buffer is small/full
Motivation:
If an event occurs which generates non-application data (shutdown, handshake failure, alert generation, etc...) and the non-application buffer in the ByteBuffer BIO is full (or sufficiently small) we may not propagate all data to our peer before tearing down the socket.

Modifications:
- when wrap() detects the outbound is closed, but there is more data pending in the non-application buffers, we must also check if OpenSSL will generate more data from calling SSL_shutdown again
- when wrap() detects a handshakeExcpetion during failure we should check if OpenSSL has any pending data (in addition to the non-application buff) before throwing the handshake exception

Result:
OpenSslEngine more reliably transmits data to the peer before closing the socket.
2017-02-15 16:14:14 -08:00
Norman Maurer
847359fd36 Fix incorrect delegate in overriden method in JdkOpenSslEngineInteroptTest
Motivation:

JdkOpenSslEngineInteroptTest.mySetupMutualAuthServerIsValidClientException(...) delegated to the wrong super method.

Modifications:

Fix delegate

Result:

Correct test-code.
2017-02-15 19:20:06 +01:00
Norman Maurer
43a2315372 Skip SSLEngineTests that depend on KeyManagerFactory when this is not supported by the openssl version.
Motivation:

Some version of openssl dont support the needed APIs to use a KeyManagerFactory. In this case we should skip the tests.

Modifications:

- Use assumeTrue(...) to skip tests that need a KeyManagerFactory and its not supported.

Result:

Tests pass on all openssl versions we support.
2017-02-15 19:17:43 +01:00
Norman Maurer
b7acae03f2 Update tcnative package names
Motivation:

tcnative was moved into an internal package.

Modifications:

Update package for tcnative imports.

Result:

Use correct package names for tcnative.
2017-02-15 13:51:41 +01:00
Norman Maurer
5728e0eb2c Use the correct arguments when run with jdk9
Motivation:

We need to pass special arguments to run with jdk9 as otherwise some tests will not be able to run.

Modifications:

Allow to define extra arguments when running with jdk9

Result:

Tests pass with jdk9
2017-02-15 10:15:00 +01:00
Scott Mitchell
d60e37cb3d OpenSslEngine wrap may not consume all data
Motivation:
If the OpenSslEngine has bytes pending in the non-application buffer and also generates wrapped data during the handshake then the handshake data will be missed. This will lead to a handshake stall and eventually timeout. This can occur if the non-application buffer becomes full due to a large certificate/hello message.

Modification:
- ReferenceCountedOpenSslEngine should not assume if no data is flushed from the non-application buffer that no data will be produced by the handshake.

Result:
New unit tests with larger certificate chains don't fail.
2017-02-15 09:29:32 +01:00
Scott Mitchell
c521c72178 SSLEngineTest cleanup
Motivation:
OpenSslEngineTest has unused imports and SSLEngineTest uses a fixed port which was used for debugging.

Modifications:
- Remove unused imports
- Use ephemeral port

Result:
Cleaner test code.
2017-02-14 17:28:13 -08:00
Scott Mitchell
84ebb4c315 Fix checkstyle issues introduced by fdcad3150e 2017-02-14 14:49:22 -08:00
Scott Mitchell
fdcad3150e Use tcnative's new setVerify modes
Modifications:
tcnative made some fixes and API changes related to setVerify. We should absorb these changes in Netty.

Modifications:
- Use tcnatives updated APIs
- Add unit tests to demonstrate correct behavior

Result:
Updated to latest tcnative code and more unit tests to verify expected behavior.
2017-02-14 12:14:58 -08:00
Scott Mitchell
cd3bf3df58 Consume tcnative options update
Motivation:
tcnative has updated how constants are defined and removed some constants which are either obsolete or now set directly in tcnative.

Modifications:
- update to compile against tcnative changes.

Result:
Netty compiles with tcnative options changes.
2017-02-14 12:09:10 -08:00
Norman Maurer
591293bfb4 Change minimum JDK version for compilation to 1.8
Motivation:

We previously changed netty to always compile with java7 as otherwise source compatibility was broken.

This was reported in [#3548] but was fixed in the meantime:
- https://bugs.openjdk.java.net/browse/JDK-8029240
- https://bugs.openjdk.java.net/browse/JDK-8030855

Modifications:

Change minimum JDK version for compilation to 1.8

Result:

Easier to maintain code.
2017-02-14 19:06:59 +01:00
Norman Maurer
adcde84253 Allow to unwrap ByteBuffer > MAX_ENCRYPTED_PACKET_LENGTH
Motivation:

We should remove the restriction to only allow to call unwrap with a ByteBuffer[] whose cumulative length exceeds MAX_ENCRYPTED_PACKET_LENGTH.

Modifications:

Remove guard.

Result:

Fixes [#6335].
2017-02-14 08:30:35 +01:00
Norman Maurer
f7c8cf9cb9 Cleanup code in ssl package.
Motivation:

There were some warnings for the code in the ssl package.

Modifications:

- Remove not needed else blocks
- Use correctly base class for static usage
- Replace String.length() == 0 with String.isEmpty()
- Remove unused code

Result:

Less warnings and cleaner code.
2017-02-14 08:23:04 +01:00
Scott Mitchell
6765e9f99d CipherSuiteConverter NPE
Motivation:
CipherSuiteConverter may throw a NPE if a cipher suite from OpenSSL does not match the precomputed regular expression for OpenSSL ciphers. This method shouldn't throw and instead just return null.

Modifications:
- if cacheFromOpenSsl(..) fails the conversion toJava should return null

Result:
Fixes https://github.com/netty/netty/issues/6336.
2017-02-13 15:02:09 -08:00
Scott Mitchell
d06990f434 OpenSSL ByteBuffer BIO
Motivation:
Currently Netty utilizes BIO_new_bio_pair so we can control all FD lifetime and event notification but delegates to OpenSSL for encryption/decryption. The current implementation sets up a pair of BIO buffers to read/write encrypted/plaintext data. This approach requires copying of data from Java ByteBuffers to native memory BIO buffers, and also requires both BIO buffers to be sufficiently large to hold application data. If direct ByteBuffers are used we can avoid coyping to/from the intermediate BIO buffer and just read/write directly from the direct ByteBuffer memory. We still need an internal buffer because OpenSSL may generate write data as a result of read calls (e.g. handshake, alerts, renegotiation, etc..), but this buffer doesn't have to be be large enough to hold application data.

Modifications:
- Take advantage of the new ByteBuffer based BIO provided by netty-tcnative instead of using BIO_read and BIO_write.

Result:
Less copying and lower memory footprint requirement per TLS connection.
2017-02-09 09:50:55 -08:00
Scott Mitchell
6353c229fd SslHandler avoid calling wrap/unwrap when unnecessary
Motivation:
The SSLEngine wrap and unwrap methods can be called in a way that has no side effects, but this could involve costly validation and allocation. The SslHandler should avoid calling into these methods if possible.

Modifications:
- wrapNonAppData should provide additional status which can be used by wrap to breakout early if possible

Result:
SslHandler invokes the SSLEngine less.
2017-02-07 00:12:31 -08:00
Norman Maurer
a7c0ff665c Only use Mockito for mocking.
Motivation:

We used various mocking frameworks. We should only use one...

Modifications:

Make usage of mocking framework consistent by only using Mockito.

Result:

Less dependencies and more consistent mocking usage.
2017-02-07 08:47:22 +01:00
Dmitriy Dumanskiy
b9abd3c9fc Cleanup : for loops for arrays to make code easier to read and removed unnecessary toLowerCase() 2017-02-06 07:47:59 +01:00
Norman Maurer
1a05463c56 More strict testing of handshake behaviour
Motiviation:

We should ensure we not need any extra wrap / unwrap calls during handshake once the handshake was signaled as finished

Modifications:

More strict testing

Result:

Better testing of handshake behaviour
2017-02-03 09:45:09 +01:00
Roger Kapsi
d688e35e70 Fixing argument names
Motivation

Misleading argument names

Modifications

Stripping xMillis suffix from arguments because there's a TimeUnit

Result

Less confusion
2017-02-03 08:39:25 +01:00
Norman Maurer
1d128c7a65 Switch to netty-tcnative 2.0.0 which uses different package names
Motivation:

Previous versions of netty-tcnative used the org.apache.tomcat namespace which could lead to problems when a user tried to use tomcat and netty in the same app.

Modifications:

Use netty-tcnative which now uses a different namespace and adjust code to some API changes.

Result:

Its now possible to use netty-tcnative even when running together with tomcat.
2017-02-02 10:44:38 +01:00
Norman Maurer
735d6dd636 [maven-release-plugin] prepare for next development iteration 2017-01-30 15:14:02 +01:00
Norman Maurer
76e22e63f3 [maven-release-plugin] prepare release netty-4.1.8.Final 2017-01-30 15:12:36 +01:00
Norman Maurer
7736534b34 Ensure tests added in 91f050d2ef work with different openssl installations
Motivation:

Tests were added in 91f050d2ef to run with different protocols / ciphers. These may fail currently when openssl was compiled without support for the protocol / ciphers.

Modifications:

- Refactor tests to easier understand for which protocol / cipher it failed
- Not fail the test if the protocol is not supported with the used openssl version.

Result:

More robust testing.
2017-01-30 13:21:56 +01:00
Norman Maurer
7a39afd031 Correctly detect which protocols are supported when using OpenSSL
Motivation:

We failed to properly test if a protocol is supported on an OpenSSL installation and just always returned all protocols.

Modifications:

- Detect which protocols are supported on a platform.
- Skip protocols in tests when not supported. This fixes a build error on some platforms introduced by [#6276].

Result:

Correctly return only the supported protocols
2017-01-27 23:37:10 +01:00
Norman Maurer
91f050d2ef More precise calculate the maximum record size when using SslProvider.OPENSSL* and so decrease mem usage.
Motivation:

We used ca 2k as maximum overhead for encrypted packets which is a lot more then what is needed in reality by OpenSSL. This could lead to the need of more memory.

Modification:

- Use a lower overhead of 86 bytes as defined by the spec and openssl itself
- Fix unit test to use the correct session to calculate needed buffer size

Result:

Less memory usage.
2017-01-27 19:51:45 +01:00
Norman Maurer
5cd8133477 Add unit test that shows we correctly return BUFFER_UNDERFLOW
Motivation:

We should test that we correctly return BUFFER_UNDERFLOW if the src buffer not contains enough data to unwrap it.

Modification:

Add unit test to verify behaviour.

Result:

Better test coverrage of SSLEngine implementations.
2017-01-26 15:04:10 +01:00
Norman Maurer
640ef615be Allow to configure SslHandler to wait for close_notify response before closing the Channel and fix racy flush close_notify timeout scheduling.
Motivation:

SslHandler closed the channel as soon as it was able to write out the close_notify message. This may not be what the user want as it may make sense to only close it after the actual response to the close_notify was received in order to guarantee a clean-shutdown of the connection in all cases.

Beside this closeNotifyFlushTimeoutMillis is volatile so may change between two reads. We need to cache it in a local variable to ensure it not change int between. Beside this we also need to check if the flush promise was complete the schedule timeout as this may happened but we were not able to cancel the timeout yet. Otherwise we will produce an missleading log message.

Modifications:

- Add new setter / getter to SslHandler which allows to specify the behavior (old behavior is preserved as default)
- Added unit test.
- Cache volatile closeNotifyTimeoutMillis.
- Correctly check if flush promise was complete before we try to forcibly close the Channel and log a warning.
- Add missing javadocs.

Result:

More clean shutdown of connection possible when using SSL and fix racy way of schedule close_notify flush timeouts and javadocs.
2017-01-24 10:51:16 +01:00
Norman Maurer
e9fa40d770 Ensure calling ReferenceCountedOpenSslEngine.wrap(...) after closeOutbound() was called will not throw an SSLException
Motivation:

PR [#6238] added guards to be able to call wrap(...) / unwrap(...) after the engine was shutdown. Unfortunally one case was missed which is when closeOutbound() was called and produced some data while closeInbound() was not called yet.

Modifications:

Correctly guard against SSLException when closeOutbound() was called, produced data and someone calls wrap(...) after it.

Result:

No more SSLException. Fixes [#6260].
2017-01-21 07:36:05 +01:00