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.
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.
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.
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.
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.
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.
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
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.
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.
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.
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
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.
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.
Motivation:
ReadTimeoutHandler and IdleStateHandler have duplicated code, we should share whatever possible.
Modifications:
Let ReadTimeoutHandler extend IdleStateHandler.
Result:
Remove code duplication.
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 :-)
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.
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.
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.
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.
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.
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()
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.
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.
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.
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()
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.
Motivation:
When the OpenSslContext is gc'ed and the user still hold a reference to OpenSslSessionContext / OpenSslSessionStats it is possible to produce a segfault when calling
a method on any of these that tries to pass down the ctx pointer to the native methods. This is because the OpenSslContext finalizer will free the native pointer.
Modifications:
Change OpenSslSessionContext / OpenSslSessionContext to store a reference to OpenSslContext and so prevent the GC to collect it as long as the user has a reference to OpenSslSessionContext / OpenSslSessionContext.
Result:
No more sefault possible.
Motivation
This bug was introduced with #5377 and affects only users who'd like to share/cache/re-use `PemPrivateKey` and `PemX509Certificate` instances.
Modifications
Use `ByteBuf#writeBytes(src, readerIndex, length)` so that the src's readerIndex doesn't change and can consequently be used more than once.
Result
It's possible to share/cache/re-use `PemPrivateKey` and `PemX509Certificate` instances as long as their refCnt remains >= 1.
Motivation
OpenSslContext is expecting Java's PrivateKey and X509Certificate objects as input
(for JdkSslContext API compatibility reasons) but doesn't really use them beyond
turning them into PEM/PKCS#8 strings.
This conversion can be entirely skipped if the user can pass in private keys and
certificates in a format that Netty's OpenSSL code can digest.
Modifications
Two new classes have been added that act as a wrapper around the pre-encoded byte[]
and also retain API compatibility to JdkSslContext.
Result
It's possible to pass PEM encoded bytes straight into OpenSSL without having to
parse them (e.g. File to Java's PrivateKey) and then encode them (i.e. PrivateKey
into PEM/PKCS#8).
File pemPrivateKeyFile;
byte[] pemBytes = readBytes(pemPrivateKeyFile);
PemPrivateKey pemPrivateKey = PemPrivateKey.valueOf(pemBytes);
SslContextBuilder.forServer(pemPrivateKey)
.sslProvider(SslProvider.OPENSSL)
Motivation:
JCTools supports both non-unsafe, unsafe versions of queues and JDK6 which allows us to shade the library in netty-common allowing it to stay "zero dependency".
Modifications:
- Remove copy paste JCTools code and shade the library (dependencies that are shaded should be removed from the <dependencies> section of the generated POM).
- Remove usage of OneTimeTask and remove it all together.
Result:
Less code to maintain and easier to update JCTools and less GC pressure as the queue implementation nt creates so much garbage
Motivation:
OpenSslClientContext / OpenSslServerContext can never be garbage collected as both are part of a reference to a callback that is stored as global reference in jni code.
Modifications:
Ensure the callbacks are static and so not hold the reference.
Result:
No more leak due not collectable OpenSslClientContext / OpenSslServerContext
Motivation:
OpenSslEngine.wrap will only encrypt at most 1 buffer per call. We may be able to encrypt multiple buffers per call.
Modifications:
- OpensslEngine.wrap should continue encrypting data until there is an error, no more data, or until the destination buffer would be overflowed.
Result:
More encryption is done per OpenSslEngine.wrap call
Motivation:
CVE-2016-4970
OpenSslEngine.wrap calls SSL_write which may return SSL_ERROR_WANT_READ, and if in this condition there is nothing to read from the BIO the OpenSslEngine and SslHandler will enter an infinite loop.
Modifications:
- Use the error code provided by OpenSSL and go back to the EventLoop selector to detect if the socket is closed
Result:
OpenSslEngine correctly handles the return codes from OpenSSL and does not enter an infinite loop.
Motivation:
OpenSslEngine calls rejectRemoteInitiatedRenegation in a scenario where the number of handshakes has not been observed to change. The number of handshakes has only been observed to change after readPlaintextData is called.
Modifications:
- Remove the call to rejectRemoteInitiatedRenegation before calls to readPlaintextData
Result:
Less code.
Motivation:
When netty is used with open ssl provider and client authentication the following errors can occur:
error:140D9115:SSL routines:ssl_get_prev_session:session id context uninitialized
error:140A1175:SSL routines:ssl_bytes_to_cipher_list:inappropriate fallback
error:140760FC:SSL routines:SSL23_GET_CLIENT_HELLO:unknown protocol
Modifications:
Set the session id context in OpenSslServerContext so that sessions which use client authentication
which are cached have the same context id value.
Result:
Client authentication now works with open ssl provider.
Motivation:
Sometimes it is useful to include more details in the IdleStateEvents that are produced by the IdleStateHandler. For this users should be able to create their own IdleStateEvents that encapsulate more informations.
Modifications:
- Make IdleStateEvent constructor protected and the class non-final
- Add protected method to IdleStateHandler that users can override and so create their own IdleStateEvents.
Result:
More flexible and customizable IdleStateEvents / IdleStateHandler
Motivation:
We should zero-out the private key as soon as possible when we not need it anymore.
Modifications:
zero out the private key before release the buffer.
Result:
Limit the time the private key resist in memory.
Motivation:
FlowControlHandlerTest attempts to validate the expected contents of the underlying queue in FlowControlHandler. However the condition which triggers the check is too early and the queue contents may not yet contain all expected objects. For example a CountDownLatch is counted down in a handler's channelRead which is after the FlowControlHandler in the pipeline. At this point if there is a thread context switch the queue may not yet contain all the expected objects and checking the queue contents is not valid.
Modifications:
- Remove checking the queues contents in FLowControlHandlerTest and instead only check the empty condition at the end of the tests
Result:
FlowControlHandlerTest won't fail due to invalid checks of the contents of the queue.
Motivation:
The current note reads as if this class is dangerous and advises the reader to "understand what this class does".
Modifications:
Rewrite the Javadoc note to describe what fingerprint checks are and what problems remain.
Result:
Clearer description which no longer causes the impression this class is dangerous.
Motivation:
Some handlers such as HttpObjectDecoder can emit more than one event per read()
which leads to problems in downstream handlers that expect only one event and hope
that ChannelConfig#setAutoRead(false) prevents further events being sent while they're
processing the one they've just received.
Modifications:
A new handler called FlowControlHandler that feeds off read() and isAutoRead() and acts
as a holding buffer if auto reading gets turned off and more events arrive while auto reading
is off.
Result:
Fixes issues such as #4895.
Motivation:
Some applications may use alternative methods of loading the tcnative JNI symbols. We should support this use case.
Modifications:
Separate the loading and initialzation of the tcnative library so that each can fail independently.
Result:
Fixes#5043
Motivation:
Sometimes a user only has access to a preconfigured SSLContext but still would like to use our ssl sub-system. For this situations it would be very useful if the user could create a JdkSslContext instance from an existing SSLContext.
Modifications:
- Create new public constructors in JdkSslContext which allow to wrap an existing SSLContext and make the class non-abstract
- Mark JdkSslServerContext and JdkSslClientContext as deprecated as the user should not directly use these.
Result:
It's now possible to create an JdkSslContext from an existing SSLContext.
Motivation:
ApplicationProtocolNegotiationHandler attempts to get a reference to an SslHandler in handlerAdded, but when SNI is in use the actual SslHandler will be added to the pipeline dynamically at some later time. When the handshake completes ApplicationProtocolNegotiationHandler throws an IllegalStateException because its reference to SslHandler is null.
Modifications:
- Instead of saving a reference to SslHandler in handlerAdded just search the pipeline when the SslHandler is needed
Result:
ApplicationProtocolNegotiationHandler support SniHandler.
Fixes https://github.com/netty/netty/issues/5066
Motivation:
There are some use cases when a client may only be willing to read from a channel once
its previous write is finished (eg: serial dispatchers in Finagle). In this case, a
connection with SslHandler installed and ctx.channel().config().isAutoRead() == false
will stall in 100% of cases no matter what order of "channel active", "write", "flush"
events was.
The use case is following (how Finagle serial dispatchers work):
1. Client writeAndFlushes and waits on a write-promise to perform read() once it's satisfied.
2. A write-promise will only be satisfied once SslHandler finishes with handshaking and
sends the unencrypted queued message.
3. The handshaking process itself requires a number of read()s done by a client but the
SslHandler doesn't request them explicitly assuming that either auto-read is enabled
or client requested at least one read() already.
4. At this point a client will stall with NEED_UNWRAP status returned from underlying engine.
Modifiations:
Always request a read() on NEED_UNWRAP returned from engine if
a) it's handshaking and
b) auto read is disabled and
c) it wasn't requested already.
Result:
SslHandler is now completely tolerant of whether or not auto-read is enabled and client
is explicitly reading a channel.
Motivation:
We should throw a more helpful exception when a non PKCS#8 key is used by the user.
Modifications:
Change exception message to give a hint what is wrong.
Result:
Easier for user to understand whats wrong with their used key.
Motivation:
SSLContext.buildTrustManagerFactory(...) builds a KeyStore to
initialize the TrustManagerFactory from an array of X509Certificates,
assuming that array is a chain and that each certificate will have a
unique Subject Distinguised Name.
However, the collection of certificates used as trust anchors is generally
not a chain (it is an unordered collection), and it is legitimate for it
to contain multiple certificates with the same Subject DN.
The existing code uses the Subject DN as the alias name when filling in
the `KeyStore`, thereby overwriting other certificates with the same
Subject DN in this collection, so some certificates may be discarded.
In addition, the code related to building trust managers can take an array of
X509Certificate instances to use as trust anchors. The variable name is
usually trustCertChain, and the documentation refers to them as a "chain".
However, while it makes sense to talk about a "chain" from a keymanager
point of view, these certificates are just an unordered collection in a
trust manager. (There is no chaining requirement, having the Subject DN
matching its predecessor's Issuer DN.)
This can create confusion to for users not used with PKI concepts.
Modifications:
SSLContext.buildTrustManagerFactory(...) now uses a distinct alias for each
array (simply using a counter, since this name is never used for reference
later). This patch also includes a unit test with CA certificates using the
same Subject DN.
Also renamed trustCertChain into trustCertCollection, and changed the
references to "chain" in the Javadoc.
Result:
Each loaded certificate now has a unique identifier when loaded, so it is
now possible to use multiple certificates with the same Subject DN as
trust anchors.
Hopefully, renaming the parameter should also reduce confusion around PKI
concepts.
Motivation:
We need to ensure we call ctx.flush() before closing the actual channel when an handshake failure took place. If we miss to do so we may not send all pending data to the remote peer which also include SSL alerts.
Modifications:
Ensure we call ctx.flush() before ctx.close() on a handshake error.
Result:
All pending data (including SSL alerts) are written to the remote peer on a handshake error.
Motivation:
We currently not supported using KeyManagerFactory with OpenSslClientContext and so should throw an exception if the user tries to do so. This will at least not give suprising and hard to debug problems later.
Modifications:
Throw exception if a user tries to construct a OpenSslClientContext with a KeyManagerFactory
Result:
Fail fast if the user tries to use something that is not supported.
