Commit Graph

527 Commits

Author SHA1 Message Date
Norman Maurer
705e3f629a Not use InternalThreadLocalMap where access may be done from outside the EventLoop.
Motivation:

We should not use the InternalThreadLocalMap where access may be done from outside the EventLoop as this may create a lot of memory usage while not be reused anyway.

Modifications:

Not use InternalThreadLocalMap in places where the code-path will likely be executed from outside the EventLoop.

Result:

Less memory bloat.
2016-11-10 14:37:16 +01:00
Evgeny Slutsky
4ee361e302 OpenSslSession#initPeerCerts creates too long almost empty arrays.
Motivation:

https://github.com/netty/netty/issues/5945

Modifications:

Refactored initialization of arrays. Fixed arrays length

Result:

Cert arrays have proper length. Testing added
2016-11-03 12:10:39 +01:00
Trustin Lee
0b7bf49c16 Convert X509TrustManager into X509ExtendedTrustManager for Java 7+
Motivation:

Since Java 7, X509TrustManager implementation is wrapped by a JDK class
called AbstractTrustManagerWrapper, which performs an additional
certificate validation for Socket or SSLEngine-backed connections.

This makes the TrustManager implementations provided by
InsecureTrustManagerFactory and FingerprintTrustManagerFactory not
insecure enough, where their certificate validation fails even when it
should pass.

Modifications:

- Add X509TrustManagerWrapper which adapts an X509TrustManager into an
  X509ExtendedTrustManager
- Make SimpleTrustManagerFactory wrap an X509TrustManager with
  X509TrustManagerWrapper is the provided TrustManager does not extend
  X509ExtendedTrustManager

Result:

- InsecureTrustManagerFactory and FingerprintTrustManagerFactory are now
  insecure as expected.
- Fixes #5910
2016-10-12 13:46:02 +02:00
Scott Mitchell
8ba5b5f740 Update Default Cipher List
Motivation:
Our default cipher list has not been updated in a while. We current support some older ciphers not commonly in use and we don't support some newer ciphers which are more commonly used.

Modifications:
- Update the default list of ciphers for JDK and OpenSSL.

Result:
Default cipher list is more likely to connect to peers.
Fixes https://github.com/netty/netty/issues/5859
2016-10-11 07:50:17 -07:00
Norman Maurer
39997814bf [#5860] Ensure removal of SslHandler not produce IllegalReferenceCountException
Motivation:

If the user removes the SslHandler while still in the processing loop we will produce an IllegalReferenceCountException. We should stop looping when the handlerwas removed.

Modifications:

Ensure we stop looping when the handler is removed.

Result:

No more IllegalReferenceCountException.
2016-10-10 11:06:19 +02:00
radai-rosenblatt
15ac6c4a1f Clean-up unused imports
Motivation:

the build doesnt seem to enforce this, so they piled up

Modifications:

removed unused import lines

Result:

less unused imports

Signed-off-by: radai-rosenblatt <radai.rosenblatt@gmail.com>
2016-09-30 09:08:50 +02:00
olim7t
3a8b8c9219 Consolidate flushes even when no read in progress
Motivation:

Currently FlushConsolidationHandler only consolidates if a read loop is
active for a Channel, otherwise each writeAndFlush(...) call will still
be flushed individually. When these calls are close enough, it can be
beneficial to consolidate them even outside of a read loop.

Modifications:

When we allow a flush to "go through", don't perform it immediately, but
submit it on the channel's executor. Under high pressure, this gives
other writes a chance to enqueue before the task gets executed, and so
we flush multiple writes at once.

Result:

Lower CPU usage and less context switching.
2016-09-23 15:27:03 -07:00
Roger Kapsi
9a18159bfe Catch+fire Exceptions thrown from SniHandler's select() method
Motivation

Give the user the ability to back out from SNI negoations.

Modifications

Put a try-catch around the select() call and re-fire any caught Exceptions.

Result

Fixes #5787
2016-09-19 16:10:24 -07:00
Scott Mitchell
0b5e75a614 IdleStateHandler volatile member variables
Motivation:
IdleStateHandler has a number of volatile member variables which are only accessed from the EventLoop thread. These do not have to be volatile. The accessibility of these member variables are not consistent between private and package private. The state variable can also use a byte instead of an int.

Modifications:
- Remove volatile from member variables
- Change access to private for member variables
- Change state from int to byte

Result:
IdleStateHandler member variables cleaned up.
2016-09-15 10:27:29 -07:00
Norman Maurer
4a18a143d2 Only set lastReadTime if an read actually happened before in IdleStateHandler.
Motivation:

IdleStateHandler and ReadTimeoutHandler could mistakely not fire an event even if no channelRead(...) call happened.

Modifications:

Only set lastReadTime if a read happened before.

Result:

More correct IdleStateHandler / ReadTimeoutHandler.
2016-09-13 16:57:29 -07:00
Victor Noël
8566fd1019 Add startTls parameter to SslContextBuilder
Motivation:

There is an incoherence in terms of API when one wants to use
startTls: without startTls one can use the SslContextBuilder's
method newHandler, but with startTls, the developper is forced
to call directly the SslHandler constructor.

Modifications:

Introduce startTls as a SslContextBuilder parameter as well as a
member in SslContext (and thus Jdk and OpenSsl implementations!).
Always use this information to call the SslHandler constructor.
Use false by default, in particular in deprecated constructors of
the SSL implementations.
The client Context use false by default

Results:

Fixes #5170 and more generally homogenise the API so that
everything can be done via SslContextBuilder.
2016-09-06 11:29:10 +02:00
Roger Kapsi
b604a22395 Expose the ChannelHandlerContext from SniHandler's select() step to the user.
Motivation

I'm looking to harden our SSL impl. a little bit and add some guards agaist certain types of abuse. One can think of invalid hostname strings in the SNI extenstion or invalid SNI handshakes altogether. This will require measuring, velocity tracking and other things.

Modifications

Adding a protected `lookup(ctx, hostname)` method that is called from SniHandler's `select(...)` method which users can override and implement custom behaviour. The default implementation will simply call the AsyncMapper.

Result

It's possible to get a hold onto the ChannelHandlerContext. Users can override that method and do something with it right there or they can delegate it to something else. SniHandler is happy as long as a `Future<SslContext>` is being returned.
2016-09-06 07:29:12 +02:00
Norman Maurer
af632278d2 Ensure we not sent duplicate certificates when using OpenSslEngine
Motivation:

We need to ensure we not set duplicated certificates when using OpenSslEngine.

Modifications:

- Skip first cert in chain when set the chain itself and so not send duplicated certificates
- Add interopt unit tests to ensure no duplicates are send.

Result:

No more duplicates.
2016-09-05 15:05:17 +02:00
Norman Maurer
147d427adc [#5712] Allow clients to override userDefinedWritabilityIndex from AbstractTrafficShapingHandler
Motivation:

AbstractTrafficShapingHandler has a package-private method called "userDefinedWritabilityIndex()" which a user may need to override if two sub-classes wants to be used in the ChannelPipeline.

Modifications:

Mark method protected.

Result:

Easier to extend AbstractTrafficShapingHandler.
2016-09-05 12:29:40 +02:00
Norman Maurer
31cbb85073 Cleanup and simplify SslHandler
Motivation:

SslHandler can be cleaned up a bit in terms of naming and duplicated code.

Modifications:

- Fix naming of arguments
- Not schedule timeout event if not really needed
- share some code and simplify

Result:

Cleaner code.
2016-09-01 08:32:35 +02:00
Norman Maurer
3051df8961 Ensure accessing System property is done via AccessController.
Motivation:

When a SecurityManager is in place it may dissallow accessing the property which will lead to not be able to load the application.

Modifications:

Use AccessController.doPrivileged(...)

Result:

No more problems with SecurityManager.
2016-08-31 14:01:05 +02:00
Roger Kapsi
f97866dbc6 Expose SniHandler's replaceHandler() so that users can implement custom behavior.
Motivation

The SniHandler is currently hiding its replaceHandler() method and everything that comes with it. The user has no easy way of getting a hold onto the SslContext for the purpose of reference counting for example. The SniHandler does have getter methods for the SslContext and hostname but they're not very practical or useful. For one the SniHandler will remove itself from the pipeline and we'd have to track a reference of it externally and as we saw in #5745 it'll possibly leave its internal "selection" object with the "EMPTY_SELECTION" value (i.e. we've just lost track of the SslContext).

Modifications

Expose replaceHandler() and allow the user to override it and get a hold onto the hostname, SslContext and SslHandler that will replace the SniHandler.

Result

It's possible to get a hold onto the SslContext, the hostname and the SslHandler that is about to replace the SniHandler. Users can add additional behavior.
2016-08-29 13:18:54 -07:00
Norman Maurer
1fef3872fb Update CertificateRequestCallback usage to match new tcnative
Motiviation:

Previously the way how CertificateRequestCallback was working had some issues which could cause memory leaks and segfaults. Due of this tcnative code was updated to change the signature of the method provided by the interface.

Modifications:

Update CertificateRequestCallback implementations to match new interface signature.

Result:

No more segfaults / memory leaks when using boringssl or openssl >= 1.1.0
2016-08-28 20:26:15 +02:00
Scott Mitchell
acb40a87c3 SniHandler reference count leak if pipeline replace fails
Motivation:
The SniHandler attempts to generate a new SslHandler from the selected SslContext in a and insert that SslHandler into the pipeline. However if the underlying channel has been closed or the pipeline has been modified the pipeline.replace(..) operation may fail. Creating the SslHandler may also create a SSLEngine which is of type ReferenceCounted. The SslHandler states that if it is not inserted into a pipeline that it will not take reference count ownership of the SSLEngine. Under these conditions we will leak the SSLEngine if it is reference counted.

Modifications:
- If the pipeline.replace(..) operation fails we should release the SSLEngine object.

Result:
Fixes https://github.com/netty/netty/issues/5678
2016-08-25 13:08:59 -07:00
Norman Maurer
2d111e0980 Ensure SslHandler.close(...) will not throw exception if flush of pending messages fails
Motivation:

When SslHandler.close(...) is called (as part of Channel.close()). it will also try to flush pending messages. This may fail for various reasons, but we still should propergate the close operation

Modifications:

- Ensure flush(...) itself will not throw an Exception if we was able to at least fail one pending promise (which should always be the case).
- If flush(...) fails as part of close ensure we still close the channel and then rethrow.

Result:

No more lost close operations possible if an exception is thrown during close
2016-08-24 07:33:25 +02:00
Scott Mitchell
9bc3e56647 ReferenceCountedOpenSslEngineTest cleanup sequencing bug
Motivation:
ReferenceCountedOpenSslEngine depends upon the the SslContext to cleanup JNI resources. If we don't wait until the ReferenceCountedOpenSslEngine is done with cleanup before cleaning up the SslContext we may crash the JVM.

Modifications:
- Wait for the channels to close (and thus the ReferenceCountedOpenSslEngine to be cleaned up) before cleaning up the associated SslContext.

Result:
Cleanup sequencing is correct and no more JVM crash.
Fixes https://github.com/netty/netty/issues/5692
2016-08-18 21:03:39 +02:00
Norman Maurer
382f8eacaf Correctly fail all promises SSLENGINE_CLOSED once the engine is closed
Motivation:

We should fail all promises with the correct SSLENGINE_CLOSED exception one the engine is closed. We did not fail the current promise with this exception if the ByteBuf was not readable.

Modifications:

Correctly fail promises.

Result:

More correct handling of promises if the SSLEngine is closed.
2016-08-17 21:47:00 +02:00
Roger Kapsi
b963595988 Fix handling of non direct backed PemEncoded.
Motivation:

The private key and certificate that are passed into #serKeyMaterial() could be PemEncoded in which case the #toPEM() methods return the identity of the value.

That in turn will fail in the #toBIO() step because the underlying ByteBuf is not necessarily direct.

Modifications:

- Use toBIO(...) which also works with non direct PemEncoded values
- Add unit test.

Result:

Correct handling of PemEncoded.
2016-08-16 09:04:38 +02:00
Norman Maurer
00deb2efd5 Remove missleading javadoc
Motivation:

Its completely fine to start writing before the handshake completes when using SslHandler. The writes will be just queued.

Modifications:

Remove the missleading and incorrect javadoc.

Result:

Correct javadoc.
2016-08-14 13:21:34 +02:00
Norman Maurer
c1d5066929 Ensure BIO is only used one time
Motivation:

Its not safe to reuse a BIO, we need to ensure we only use it once.

Modifications:

Only use the BIO once.

Result:

Correctly usage of BIO
2016-08-12 14:41:13 +02:00
Norman Maurer
47d55339c9 [#5648] Detect if netty-tcnative is in classpath or just tcnative
Motivation:

If netty is used in a tomcat container tomcat itself may ship tcnative. Because of this we will try to use OpenSsl in netty and fail because it is different to netty-tcnative.

Modifications:

Ensure if we find tcnative it is really netty-tcnative before using it.

Result:

No more problems when using netty in a tomcat container that also has tcnative installed.
2016-08-11 14:13:05 +02:00
Norman Maurer
cb5f71782e Ensure we only call ReferenceCountUtil.safeRelease(...) in finalize() if the refCnt() > 0
Motivation:

We need to ensure we only call ReferenceCountUtil.safeRelease(...) in finalize() if the refCnt() > 0 as otherwise we will log a message about IllegalReferenceCountException.

Modification:

Check for a refCnt() > 0 before try to release

Result:

No more IllegalReferenceCountException produced when run finalize() on OpenSsl* objects that where explicit released before.
2016-08-11 08:54:19 +02:00
Scott Mitchell
fef2940f32 Update for netty-tcnative API changes
Motivation:
netty-tcnative API has changed to remove a feature that contributed to a memory leak.

Modifications:
- Update to use the modified netty-tcnative API

Result:
Netty can use the latest netty-tcnative.
2016-08-10 21:56:15 -07:00
Norman Maurer
129aee8a92 Remove unused imports and not needed throws declarations.
Motivation:

In latest refeactoring we failed to cleanup imports and also there are some throws declarations which are not needed.

Modifications:

Cleanup imports and throws declarations

Result:

Cleaner code.
2016-08-10 11:46:59 +02:00
Scott Mitchell
7d774584c8 OpenSslEngine with no finalizer
Motivation:
OpenSslEngine and OpenSslContext currently rely on finalizers to ensure that native resources are cleaned up. Finalizers require the GC to do extra work, and this extra work can be avoided if the user instead takes responsibility of releasing the native resources.

Modifications:
- Make a base class for OpenSslENgine and OpenSslContext which does not have a finalizer but instead implements ReferenceCounted. If this engine is inserted into the pipeline it will be released by the SslHandler
- Add a new SslProvider which can be used to enable this new feature

Result:
Users can opt-in to a finalizer free OpenSslEngine and OpenSslContext.
Fixes https://github.com/netty/netty/issues/4958
2016-08-05 00:57:37 -07:00
Norman Maurer
e5b45f120a Allow to explicit disable usage of KeyManagerFactory when using OpenSsl
Motivation:

Sometimes it may be useful to explicit disable the usage of the KeyManagerFactory when using OpenSsl.

Modifications:

Add io.netty.handler.ssl.openssl.useKeyManagerFactory which can be used to explicit disable KeyManagerFactory usage.

Result:

More flexible usage.
2016-08-05 07:15:37 +02:00
Norman Maurer
5513514d08 Take readerIndex into account when write to BIO.
Motivation:

We should take the readerIndex into account whe write into the BIO. Its currently not a problem as we slice before and so the readerIndex is always 0 but we should better not depend on this as this will break easily if we ever refactor the code and not slice anymore.

Modifications:

Take readerIndex into acount.

Result:

More safe and correct use.
2016-08-05 07:14:16 +02:00
Norman Maurer
6bd810210d Servers should not send duplicate intermediate certificates.
Motivation:
Servers sometimes send duplicate intermediate certificates.

Modifications:
OpenSslKeyMaterialManager.setKeyMaterial() dedups aliases before calling SSL.setCertificateChainBio().

Result:
Servers no longer send duplicate itermediate certificates.
2016-08-01 10:52:46 +02:00
Norman Maurer
f585806a74 [#5598] Ensure SslHandler not log false-positives when try to close the channel due timeout.
Motivation:

When we try to close the Channel due a timeout we need to ensure we not log if the notification of the promise fails as it may be completed in the meantime.

Modifications:

Add another constructor to ChannelPromiseNotifier and PromiseNotifier which allows to log on notification failure.

Result:

No more miss-leading logs.
2016-07-30 21:15:09 +02:00
Scott Mitchell
cebf255951 FlushConsolidationHandler remove conditional
Motivation:
FlushConsolidationHandler#flushIfNeeded has a conditional which is fixed based upon code path. This conditional can be removed and instead just manually set in each fixed code path.

Modifications:
- Remove boolean parameter on FlushConsolidationHandler#flushIfNeeded and set readInprogess to false manually when necessary

Result:
Less conditionals in FlushConsolidationHandler
2016-07-27 07:11:47 +02:00
Ian Haken
2ce1d29d4d Elliminated some buggy behavior when using a KeyManagerFactory with OpenSslServerContext.
Motivation:

PR #5493 added support for KeyManagerFactories when using the OpenSsl context. This commit corrects a bug causing a NullPointerException that occurs when using a KeyManagerFactory without a certificate chain and private key.

Modifications:

Removes assertNotNull() assertions which were causing a certificate chain and private key to be required even when using a KeyManagerFactory. Also removed a redundant call to buildKeyManagerFactory() which was also causing a exception when a KeyManagerFactory is provided but a certificate chain and private key is not.

Result:

A KeyManagerFactory can now be used in the OpenSslServerContext without an independent certificate chain and private key.
2016-07-22 09:14:54 +02:00
Norman Maurer
e3c8a92499 Add FlushConsolidationHandler which consolidates flush operations as these are expensive
Motivation:

Calling flush() and writeAndFlush(...) are expensive operations in the sense as both will produce a write(...) or writev(...) system call if there are any pending writes in the ChannelOutboundBuffer. Often we can consolidate multiple flush operations into one if currently a read loop is active for a Channel, as we can just flush when channelReadComplete is triggered. Consolidating flushes can give a huge performance win depending on how often is flush is called. The only "downside" may be a bit higher latency in the case of where only one flush is triggered by the user.

Modifications:

Add a FlushConsolidationHandler which will consolidate flushes and so improve the throughput.

Result:

Better performance (throughput). This is especially true for protocols that use some sort of PIPELINING.
2016-07-07 06:48:23 +02:00
Norman Maurer
987ebb90ec Share code between ReadTimeoutHandler and IdleStateHandler
Motivation:

ReadTimeoutHandler and IdleStateHandler have duplicated code, we should share whatever possible.

Modifications:

Let ReadTimeoutHandler extend IdleStateHandler.

Result:

Remove code duplication.
2016-07-05 20:12:37 +02:00
buchgr
f3dc483c99 Fix NPE in OpenSslEngine
Motivation:

The gRPC interop tests fail due to a NPE in OpenSslEngine.

Caused by: java.lang.NullPointerException
at io.netty.handler.ssl.OpenSslEngine.setSSLParameters(OpenSslEngine.java:1473)

Modifications:

Add a null check

Result:

No more NPE exceptions :-)
2016-06-30 11:12:55 +02:00
Norman Maurer
5e64985089 Add support for KeyManagerFactory when using SslProvider.OpenSsl.
Motivation:

To be able to use SslProvider.OpenSsl with existing java apps that use the JDK SSL API we need to also provide a way to use it with an existing KeyManagerFactory.

Modification:

Make use of new tcnative apis and so hook in KeyManagerFactory.

Result:

SslProvider.OpenSsl can be used with KeyManagerFactory as well.
2016-06-28 09:34:01 +02:00
Norman Maurer
3a69adfefb [#5401] Support -Djdk.tls.ephemeralDHKeySize=num when using OpenSslContext
Motivation:

Java8+ adds support set a DH key size via a System property (jdk.tls.ephemeralDHKeySize). We should respect this when using OpenSSL.

Modifications:

Respect system property.

Result:

More consistent SSL implementation.
2016-06-28 07:07:42 +02:00
Norman Maurer
2546d99864 Expose session ticket statistics.
Motivation:

We recently added support for session ticket statistics which we can expose now.

Modifications:

Expose the statistics.

Result:

Be able to obtain session ticket statistics.
2016-06-27 19:36:45 +02:00
Norman Maurer
77c6d7a672 Correctly implement SSLSession.getLastAccessedTime() for OpenSSLEngine
Motivation:

We need to return a correct time for SSLSession.getLastAccessedTime() so it reflect when the handshake was done when the session was reused.

Modifications:

Correctly reflect handshake time in getLastAccessedTime().

Result:

More conform SSLSession implementation.
2016-06-23 11:30:28 +02:00
Norman Maurer
d495792c48 Allow to wrap another SslContext implementation and do extra init steps on the SSLEngine.
Motivation:

Sometimes its needed to customize the SSLEngine (like setting protocols etc). For this it would be useful if the user could wrap an SslContext and do init steps on the SSLEngine.

Modifications:

Add new SslContext implementation which can wrap another one and allow to customize the SSLEngine

Result:

More flexible usage of SslContext.
2016-06-23 11:28:52 +02:00
Norman Maurer
7c1374dd54 OpenSslEngine.getSupportedCipherSuites() must return java names as well.
Motivation:

At the moment OpenSslEngine.getSupportedCipherSuites() only return the original openssl cipher names and not the java names. We need also include the java names.

Modifications:

Correctly return the java names as well.

Result:

Correct implementation of OpenSslEngine.getSupportedCipherSuites()
2016-06-23 11:27:23 +02:00
Norman Maurer
9687d77b5a Move validation of arguments out of synchronized block
Motivation:

There is no need already use synchronized when validate the args of the methods.

Modifications:

First validate arguments and then use synchronized

Result:

Less code executed in synchronized block.
2016-06-20 14:23:47 +02:00
Norman Maurer
e845670043 Set some StackTraceElement on pre-instantiated static exceptions
Motivation:

We use pre-instantiated exceptions in various places for performance reasons. These exceptions don't include a stacktrace which makes it hard to know where the exception was thrown. This is especially true as we use the same exception type (for example ChannelClosedException) in different places. Setting some StackTraceElements will provide more context as to where these exceptions original and make debugging easier.

Modifications:

Set a generated StackTraceElement on these pre-instantiated exceptions which at least contains the origin class and method name. The filename and linenumber are specified as unkown (as stated in the javadocs of StackTraceElement).

Result:

Easier to find the origin of a pre-instantiated exception.
2016-06-20 11:33:05 +02:00
Norman Maurer
f2efd68c39 Correctly support SSLSession.getId() when using OpenSslEngine
Motivation:

At the moment SSLSession.getId() may always return an empty byte array when OpenSSLEngine is used. This is as we not set SSL_OP_NO_TICKET on the SSLContext and so SSL_SESSION_get_id(...) will return an session id with length of 0 if tickets are not used.

Modifications:

- Set SSL_OP_NO_TICKET by default and only clear it if the user requests the usage of session tickets.
- Add unit test

Result:

Ensure consistent behavior between different SSLEngine implementations.
2016-06-20 09:34:54 +02:00
Norman Maurer
e4d21abfc1 Add support for SSLParameters.setCipherSuiteOrder() when using Java8+
Motivation:

When using java8+ we should support SSLParameters.setCipherSuiteOrder()

Modifications:

Add support of SLParameters.setCipherSuiteOrder() by using reflection, so we can compile with java7 but still support it.

Result:

Users that use java8+ can use SSLParameters.setCipherSuiteOrder()
2016-06-20 09:33:49 +02:00
Norman Maurer
e14a385a88 Add support for SNIHostName when using Java8+
Motivation:

Java8 added support for using SNIHostName with SSLParameters. We currently ignore it in OpenSslEngine.

Modifications:

Use reflection to support SNIHostName.

Result:

People using Java8 can use SNIHostName even when OpenSslEngine is used.
2016-06-20 09:25:12 +02:00