Motivation:
In some of our tests we not correctly init the SSLEngine before trying to perform a handshake which can cause an IllegalStateException. While this not happened in previous java releases it does now on Java11 (which is "ok" as its even mentioned in the api docs). Beside this how we selected the ciphersuite to test renegotation was not 100 % safe.
Modifications:
- Correctly init SSLEngine before using it
- Correctly select ciphersuite before testing for renegotation.
Result:
More correct tests and also pass on next java11 EA release.
Motivation:
We should allow to also validate sni hostname which contains for example underscore when using our native SSL impl. The JDK implementation does this as well.
Modifications:
- Construct the SNIHostName via byte[] and not String.
- Add unit test
Result:
Fixes https://github.com/netty/netty/issues/8144.
Motivation:
a137291ad1 introduced a way to get the most speed out of OpenSSL by not only caching keymaterial but pre-compute these. The problem was we missed to check for null before doing an instanceof check and then a cast which could lead to a NPE as we tried to cast null to Exception and throw it.
Modifications:
Add null check and unit test.
Result:
No more NPE when keymaterial was not found for requested alias.
* Add OpenSslX509KeyManagerFactory which makes it even easier for people to get the maximum performance when using OpenSSL / LibreSSL / BoringSSL with netty.
Motivation:
To make it even easier for people to get the maximum performance when using native SSL we should provide our own KeyManagerFactory implementation that people can just use to configure their key material.
Modifications:
- Add OpenSslX509KeyManagerFactory which users can use for maximum performance with native SSL
- Refactor some internal code to re-use logic and not duplicate it.
Result:
Easier to get the max performance out of native SSL implementation.
Motivation:
As the used OpenSSL version may not support hostname validation we should only really call SSL.setHostNameValidation(...) if we detect that its needed.
Modifications:
Only call SSL.setHostNameValidation if it was disabled before and now it needs to be enabled or if it was enabled before and it should be disabled now.
Result:
Less risk of an exception when using an OpenSSL version that does not support hostname validation.
Motivation:
OpenSSL allows to use a custom engine for its cryptographic operations. We should allow the user to make use of it if needed.
See also: https://www.openssl.org/docs/man1.0.2/crypto/engine.html.
Modifications:
Add new system property which can be used to specify the engine to use (null is the default and will use the build in default impl).
Result:
More flexible way of using OpenSSL.
Motiviation:
During profiling it showed that a lot of time during the handshake is spent by parsing the key / chain over and over again. We should cache these parsed structures if possible to reduce the overhead during handshake.
Modification:
- Use new APIs provided by https://github.com/netty/netty-tcnative/pull/360.
- Introduce OpensslStaticX509KeyManagerFactory which allows to wrap another KeyManagerFactory and caches the key material provided by it.
Result:
In benchmarks handshake times have improved by 30 %.
Motivation:
Some of the cipher protocol combos that were used are no longer included in more recent OpenSSL releases.
Modifications:
Remove some combos that were used for testing.
Result:
Tests also pass in more recent OpenSSL versions (1.1.0+).
Motivation
There is a cost to concatenating strings and calling methods that will be wasted if the Logger's level is not enabled.
Modifications
Check if Log level is enabled before producing log statement. These are just a few cases found by RegEx'ing in the code.
Result
Tiny bit more efficient code.
Motivation:
SslHandlerTest tried to get access to the SslHandler in the pipeline via pipeline.get(...) which may return null if the channel was already closed and so the pipeline was teared down.
This showed up in a test run as:
```
-------------------------------------------------------------------------------
Test set: io.netty.handler.ssl.SslHandlerTest
-------------------------------------------------------------------------------
Tests run: 17, Failures: 0, Errors: 1, Skipped: 1, Time elapsed: 0.802 sec <<< FAILURE! - in io.netty.handler.ssl.SslHandlerTest
testCloseOnHandshakeFailure(io.netty.handler.ssl.SslHandlerTest) Time elapsed: 0.188 sec <<< ERROR!
java.lang.NullPointerException
at io.netty.handler.ssl.SslHandlerTest.testCloseOnHandshakeFailure(SslHandlerTest.java:640)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:564)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:298)
at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:292)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
at java.base/java.lang.Thread.run(Thread.java:844)
```
Modifications:
Use an AtomicReference to propagate the SslHandler instance to the outer scope.
Result:
No more NPE.
Motivation:
When we build the key-material we should use the ByteBufAllocator used by the ReferenceCountedOpenSslEngine when possible.
Modifications:
Whenever we have access to the ReferenceCountedOpenSslEngine we use its allocator.
Result:
Use correct allocator
Motivation:
We currently have only interopt tests for Conscrypt, we should also have non-interopt tests.
Modifications:
Add ConscryptSslEngineTest
Result:
More tests
Motivation:
We previously did not correctly take into account when we could not wrap (and so produce) the full SSL record with an alert when the SSLEngine was closed.
There are two problems here:
- If we call wrap(...) with an empty dst buffer after closeOutbound() was called we will not notify the user if we could not store the whole SSLRecord into the dst buffer and so we may produce incomplete SSLRecords
Modifications:
Add unit test which failed before.
Result:
Correctly handle the case when the dst buffer is not big enough and and alert needs to be produced.
Motivation:
https://github.com/netty/netty/pull/7943 had a bug which caused to not have the argument passed to the delegating method.
Modifications:
Add argument to release call.
Result:
Correctly delegate method.
Motivation:
Some of the tests failed when using BoringSSL as some protocol / cipher combinations are not supported and it uses a different alert when the cert is not valid yet.
Modification:
- Remove protocol / cipher combos that are not supported by BoringSSL
- Test for different alert when using BoringSSL
Result:
Not test failures when using BoringSSL.
Motivation:
https://github.com/netty/netty/pull/7941 proved that its easy to not correctly clear the error stack sometimes. We should do carefully test this.
Modifications:
Add a new SSLEngine wrapper that is used during tests, which verifies that the error stack is empty after each method call.
Result:
Better testing.
Motivation:
We sometimes did not correctly detect when a protocol is not enabled when using netty-tcnative as we did not take into account when the option flag is 0 (as for example in BoringSSL for SSLv2).
Modifications:
- Correctly take an option flag with 0 into account.
- Add unit tests.
Result:
Fixes https://github.com/netty/netty/issues/7935.
Motivation:
We missed to correctly clear the error stack in one case when using the ReferenceCountedOpenSslEngine. Because of this it was possible to pick up an error on an unrelated operation.
Modifications:
- Correctly clear the stack
- Add verification of empty error stack to tests.
Result:
Not possible to observe unrelated errors.
Motivation:
We had a bug-report that claimed the src buffer used by OpenSslEngine will be zero out.
Modifications:
Add testcase to ensure correct behaviour
Result:
Testcase for https://github.com/netty/netty/issues/7753
Motivation:
Sometimes it's useful to disable native transports / native ssl to debug a problem. We should allow to do so with a system property so people not need to adjust code for this.
Modifications:
Add system properties which allow to disable native transport and native ssl.
Result:
Easier to disable native code usage without code changes.
Motivation:
Some `if` statements contains common parts that can be extracted.
Modifications:
Extract common parts from `if` statements.
Result:
Less code and bytecode. The code is simpler and more clear.
Motivation:
ChunkedWriteHandler.doFlush is called twice from the same write if the channelWritabilityChanged event is invoked during the write. The buffer is already written so no extra data is sent on the socket but it causes the "promise already done" exception to be thrown.
This error happens only when the message is not ChunkedInput.
Modification:
Clear out the currentWrite reference before the ctx.write call, such that next time when the method is invoked the same object is not used twice.
Result:
Fixes#7819
Motivation:
We should only schedule one timeout to wait for the close notify to be done.
Modifications:
Keep track of if we already scheduled a timeout for close notify and if so not schedule another one.
Result:
No duplicated timeouts.
Motivation:
LibreSSL removed support for NPN in its 2.6.1+ releases.
Modifications:
Skip NPN tests in libressl 2.6.1+
Result:
Be able to run netty tests against libressl 2.6.1+ as well.
Motivation:
We had some code duplication in ChunkedWriteHandler.
Modifications:
Factor out duplicated code into private methods and reuse it.
Result:
Less code duplication.
Motivation:
When using the JdkSslEngine, the ALPN class is used keep a reference
to the engine. In the event that the TCP connection fails, the
SSLEngine is not removed from the map, creating a memory leak.
Modification:
Always close the SSLEngine regardless of if the channel became
active. Also, record the SSLEngine was closed in all places.
Result:
Fixes: https://github.com/grpc/grpc-java/issues/3080
Motivation:
Android 5.0 sometimes not correctly update the bytesConsumed of the SSLEngineResult when consuming data from the input ByteBuffer. This will lead to handshake failures.
Modifications:
Add a workaround for Android 5.0
Result:
Be able to use netty on Android 5.0 by fixing https://github.com/netty/netty/issues/7758 .
Motivation:
When SSL handshake fails, the connection should be closed. This is not true anymore after 978a46c.
Modifications:
- Ensure we always flush and close the channel on handshake failure.
- Add testcase.
Result:
Fixes [#7724].
Motivation:
Profiling tcnative SSL code showed a non trivial percentage (1%)
of time spent in JNI code for InstaceOf. This turned out to be
from `Buffer.address` which makes a JNI call, which safely checks
on each call that The ByteBuffer is direct.
Modification:
Prefer using the address field of the pojo rather than looking it
up with JNI. This is the same approach taken by the `OpenSsl`
class.
Result:
Less JNI overhead
Motivation:
The code did reflection every method call which made the code slower and
harder to read with additional cases to consider.
Modifications:
Instead of loading the method and then throwing it away, save the Method
reference instead of the Class reference. Then also use more precise
exception handling for the method invocation.
Result:
Simpler, speedier code.
Motivation:
Sometimes, it would be convenient to be able to easily enable all
supported cipher suites, regardless of security.
Currently, the only way it to retrieve all supported ciphers and pass
them explicitly.
Modification:
Introduce a new IdentityCipherSuiteFilter singleton that defaults to
supportedCiphers instead of defaultCiphers when ciphers are null.
Result:
Convenient way to enabled all supported cipher suites.
Motivation:
In google/conscrypt#313 the Conscrypt.Engines class was removed in favor
of methods directly on Conscrypt and overloading. The Conscrypt-using
code in Netty used reflection to access the old API, that doesn't exist
anymore. And thus recent versions of Conscrypt fail to enable things
like ALPN with Netty.
Modifications:
Instead of calling Conscrypt.Engines.isConscrypt, call
Conscrypt.isConscrypt.
Result:
Conscrypt detected properly at runtime.
Motivation:
JdkSslContext builds the list of supported cipher suites, but assumes that ciphers prefixed with SSL_ and TLS_ will be interchangeable. However this is not the case and only applies to a small subset of ciphers. This results in the JdkSslContext attempting to use unsupported ciphers.
Modifications:
- When building the list of ciphers in JdkSslContext we should first check if the engine supports the TLS_ prefix cipher.
Result:
Fixes https://github.com/netty/netty/issues/7673
Motivation:
SslHandler#decode methods catch any exceptions and attempt to wrap
before shutting down the engine. The intention is to write any alerts
which the engine may have pending. However the wrap process may also
attempt to write user data, and may also complete the associated
promises. If this is the case, and a promise listener closes the channel
then SslHandler may later propagate a SslHandshakeCompletionEvent user
event through the pipeline. Since the channel has already been closed
the user may no longer be paying attention to user events.
Modifications:
- Sslhandler#decode should first fail the associated handshake promise
and propagate the SslHandshakeCompletionEvent before attempting to wrap
Result:
Fixes https://github.com/netty/netty/issues/7639
Motivation:
Will allow easy removal of deprecated methods in future.
Modification:
Replaced ctx.attr(), ctx.hasAttr() with ctx.channel().attr(), ctx.channel().hasAttr().
Result:
No deprecated ctx.attr(), ctx.hasAttr() methods usage.
Motivation:
We recently removed support for renegotiation, but there are still some hooks to attempt to allow remote initiated renegotiation to succeed. The remote initated renegotiation can be even more problematic from a security stand point and should also be removed.
Modifications:
- Remove state related to remote iniated renegotiation from OpenSslEngine
Result:
More renegotiation code removed from the OpenSslEngine code path.
Motivation:
When using netty on android or with for example a IBM JVM it may not be able to build a SslContext as we hardcoded the use of JKS and SunX509 (which both may not be present).
Modifications:
- Use the default algorithm / type which can be override via a System property
- Remove System property check as its redundant with KeyManagerFactory.getDefaultAlgorithm()
Result:
More portable code. Fixes [#7546].
Motivation:
SSL.setState() has gone from openssl 1.1. Calling it is, and probably
always has been, incorrect. Doing renogitation in this manner is
potentially insecure. There have been at least two insecure
renegotiation vulnerabilities in users of the OpenSSL library.
Renegotiation is not necessary for correct operation of the TLS protocol.
BoringSSL has already eliminated this functionality, and the tests
(now deleted) were not running on BoringSSL.
Modifications:
If the connection setup has completed, always return that
negotiation is not supported. Previously this was done only if we were
the client.
Remove the tests for this functionality.
Fixes#6320.
Motivation:
We tried to call `select` after we closed the channel (and so removed all the handlers from the pipeline) when we detected a non SSL record. This would cause an exception like this:
```
Caused by: java.util.NoSuchElementException: io.netty.handler.ssl.SniHandler
at io.netty.channel.DefaultChannelPipeline.getContextOrDie(DefaultChannelPipeline.java:1098)
at io.netty.channel.DefaultChannelPipeline.replace(DefaultChannelPipeline.java:506)
at io.netty.handler.ssl.SniHandler.replaceHandler(SniHandler.java:133)
at io.netty.handler.ssl.SniHandler.onLookupComplete(SniHandler.java:113)
at io.netty.handler.ssl.AbstractSniHandler.select(AbstractSniHandler.java:225)
at io.netty.handler.ssl.AbstractSniHandler.decode(AbstractSniHandler.java:218)
at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:489)
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:428)
... 40 more
```
Modifications:
- Ensure we rethrow the NotSslRecordException when detecting it (and closing the channel). This will also ensure we not call `select(...)`
- Not catch `Throwable` but only `Exception`
- Add test case.
Result:
Correctly handle the case of an non SSL record.
* FIX: force a read operation for peer instead of self
Motivation:
When A is in `writeInProgress` and call self close, A should
`finishPeerRead` for B(A' peer).
Modifications:
Call `finishPeerRead` with peer in `LocalChannel#doClose`
Result:
Clear confuse of code logic
* FIX: preserves order of close after write in same event loop
Motivation:
If client and server(client's peer channel) are in same event loop, client writes data to
server in `ChannelActive`. Server receives the data and write it
back. The client's read can't be triggered becasue client's
`ChannelActive` is not finished at this point and its `readInProgress`
is false. Then server closes itself, it will also close the client's
channel. And client has no chance to receive the data.
Modifications:
1. Add a test case to demonstrate the problem
2. When `doClose` peer, we always call
`peer.eventLoop().execute()` and `registerInProgress` is not needed.
3. Remove test case
`testClosePeerInWritePromiseCompleteSameEventLoopPreservesOrder`. This
test case can't pass becasue of this commit. IMHO, I think it is OK,
becasue it is reasonable that the client flushes the data to socket,
then server close the channel without received the data.
4. For mismatch test in SniClientTest, the client should receive server's alert before closed(caused by server's close)
Result:
The problem is gone.
Motivation:
At the moment its a bit "hacky" to retrieve the hostname that was used during SNI as you need to hold a reference to SniHandler and then call hostname() once the selection is done. It would be better to fire an event to let the user know we did the selection.
Modifications:
Add a SniCompletionEvent that can be used to get the hostname that was used to do the selection and was included in the SNI extension.
Result:
Easier usage of SNI.
Motivation:
We only want to log for the particular case when debug logging is enabled so we not need to try to match the message if this is not the case.
Modifications:
Guard with logger.isDebugEnabled()
Result:
Less overhead when debug logging is not enabled.
Motivation:
SslHandler will do aggregation of writes by default in an attempt to improve goodput and reduce the number of discrete buffers which must be accumulated. However if aggregation is not possible then a CompositeByteBuf is used to accumulate multiple buffers. Using a CompositeByteBuf doesn't provide any of the benefits of better goodput and in the case of small + large writes (e.g. http/2 frame header + data) this can reduce the amount of data that can be passed to writev by about half. This has the impact of increasing latency as well as reducing goodput.
Modifications:
- SslHandler should prefer copying instead of using a CompositeByteBuf
Result:
Better goodput (and potentially improved latency) at the cost of copy operations.
Motivation:
The default enabled cipher suites of the OpenSsl engine are not set to
SslUtils#DEFAULT_CIPHER_SUITES. Instead all available cipher suites are
enabled. This should happen only as a fallback.
Modifications:
Moved the line in the static initializer in OpenSsl which adds the
SslUtils#DEFAULT_CIPHER_SUITES to the default enabled cipher suites up
before the fallback.
Result:
The default enabled cipher suites of the OpenSsl engine are set to the
available ones of the SslUtils#DEFAULT_CIPHER_SUITES.
The default enabled cipher suites of the OpenSsl engine are only set to
all available cipher suites if no one of the
SslUtils#DEFAULT_CIPHER_SUITES is supported.
Motivation:
We should not fire a SslHandshakeEvent if the channel is closed but the handshake was not started.
Modifications:
- Add a variable to SslHandler which tracks if an handshake was started yet or not and depending on this fire the event.
- Add a unit test
Result:
Fixes [#7262].
Motivation:
The behavior for SelectorFailureBehavior and SelectedListenerFailureBehavior enum values are not clear. Additional comments would clarify the expected behavior.
Modifications:
- Add comments for each value in SelectedListenerFailureBehavior and SelectorFailureBehavior which clarify the expected behavior
Result:
The behavior of SelectedListenerFailureBehavior and SelectorFailureBehavior are more clearly communicated.
Motivation:
At the moment use loops to run SslHandler tests with different SslProviders which is error-prone and also make it difficult to understand with which provider these failed.
Modifications:
- Move unit tests that should run with multiple SslProviders to extra class.
- Use junit Parameterized to run with different SslProvider combinations
Result:
Easier to understand which SslProvider produced test failures
complete
Motivation:
SslHandler removes a Buffer/Promise pair from
AbstractCoalescingBufferQueue when wrapping data. However it is possible
the SSLEngine will not consume the entire buffer. In this case
SslHandler adds the Buffer back to the queue, but doesn't add the
Promise back to the queue. This may result in the promise completing
immediately in finishFlush, and generally not correlating to the
completion of writing the corresponding Buffer
Modifications:
- AbstractCoalescingBufferQueue#addFirst should also support adding the
ChannelPromise
- In the event of a handshake timeout we should immediately fail pending
writes immediately to get a more accurate exception
Result:
Fixes https://github.com/netty/netty/issues/7378.
