Motivation:
The code for initiating a TLS handshake or renegotiation process is
currently difficult to reason about.
Modifications:
This commit introduces to clear paths for starting a handshake. The
first path is a normal handshake. The handshake is started and a timeout
is scheduled.
The second path is renegotiation. If the first handshake is incomplete,
the renegotiation promise is added as a listener to the handshake
promise. Otherwise, the renegotiation promise replaces the original
promsie. At that point the handshake is started again and a timeout is
scheduled.
Result:
Cleaner and easier to understand code.
Motivation:
We did not correctly schedule the handshake timeout if the handshake was either started by a flush(...) or if starttls was used.
Modifications:
- Correctly setup timeout in all cases
- Add unit tests.
Result:
Fixes https://github.com/netty/netty/issues/8493.
Motivation:
If you attempt to write to a channel with an SslHandler prior to channelActive being called you can hit an assertion. In particular - if you write to a channel it forces some handshaking (through flush calls) to occur.
The AssertionError only happens on Java11+.
Modifications:
- Replace assert by an "early return" in case of the handshake be done already.
- Add unit test that verifies we do not hit the AssertionError anymore and that the future is correctly failed.
Result:
Fixes https://github.com/netty/netty/issues/8479.
Motivation:
We should call wrapEngine(...) in our SSLEngineTest to correctly detect all errors in case of the OpenSSLEngine.
Modifications:
Add missing wrapEngine(...) calls.
Result:
More correct tests
Motivation:
We did not override all methods in OpenSslX509Certificate and delegate to the internal 509Certificate.
Modifications:
Add missing overrides.
Result:
More correct implementation
Motivation:
Due a bug in our implementation we tried to release the same ByteBuf two times when we failed to parse the X509Certificate as closing the ByteBufInputStream already closed it.
Modifications:
- Don't close the ByteBuf when closing the ByteBufInputStream
- Explicit release all ByteBufs after we are done parsing in a finally block.
- Add testcase.
Result:
Do not produce an IllegalReferenceCountException and throw the correct CertificateException.
Motivation:
SHA1 is a broken hash function and shouldn't be used anymore (see: https://shattered.io/).
Security scanning tools will raise this as an issue and it will reflect badly on netty and I, therefore, recommend to use a SHA2 hash function which is secure and won't be flagged by such tools.
Modifications:
Replaced insecure SHA1 based signing scheme with SHA2.
Result:
Modern and thus secure cryptographic primitives will be in use and won't be flagged by security scanning tools.
Motivation:
https://github.com/netty/netty/issues/8442 reported that we fail to build a SslContext when an invalid cipher is used with netty-tcnative-boringssl-static, while it worked before. This test verifies that this is now consistent with all other SSLEngine implementations.
Modifications:
Add test-case to verify consistent behaviour
Result:
More tests to assert consistent behaviour across SSLEngine implementations
Motivation:
201e984cb3 added support to use native TLSv1.3 support even with Java versions prior to 11. For this we try to detect if we need to wrap the used KeyManager or not. This testing code did create an X509Certificate[1] but does not correctly also set the certficiate on index 0. While this should be harmless we should better do the right thing and set it.
Modifications:
Correctly init the array.
Result:
Cleaner and more correct code.
Motivation:
0ddc62cec0 added support for TLSv1.3 when using openssl 1.1.1. Now that BoringSSL chromium-stable branch supports it as well we can also support it with netty-tcnative-boringssl-static.
During this some unit tests failed with BoringSSL which was caused by not correctly handling flush() while the handshake is still in progress.
Modification:
- Upgrade netty-tcnative version which also supports TLSv1.3 when using BoringSSL
- Correctly handle flush() when done while the handshake is still in progress in all cases.
Result:
Easier for people to enable TLSv1.3 when using native SSL impl.
Ensure flush() while handshake is in progress will always be honored.
Motivation:
OpenSsl used SelfSignedCertificate in its static init block to detect if KeyManagerFactory is supported. Unfortunally this only works when either sun.security.x509.* can be accessed or bouncycastle is on the classpath.
We should not depend on either of it.
This came up in https://github.com/netty/netty-tcnative/issues/404#issuecomment-431551890.
Modifications:
Just directly use the bytes to generate the X509Certificate and so not depend on sun.security.x509.* / bouncycastle.
Result:
Correctly be able to detect if KeyManagerFactory can be supported in all cases.
Motivation:
We had put some workaround in our tests due a bug in the Java11 implementation of TLSv1.3. This was now fixes as part of 11.0.1.
See https://bugs.openjdk.java.net/browse/JDK-8211067.
Modifications:
Remove workaround in SSL tests.
Result:
Run all tests with supported TLS version.
Motivation:
At the moment it's only possible to use TLSv1.3 with netty-tcnative if Java 11 is used. It should be possible to do so even with Java 8, 9 and 10.
Modification:
Add a workaround to be able to use TLSv1.3 also when using Java version prior to Java 11 and the default X509ExtendedTrustManager is used.
Result:
Be able to use TLSv1.3 also with past versions of Java.
Motivation:
When the constructor of OpenSslEngine threw we could end up to self call SSL_free by ourself and then have the finalizer do the same which may lead to double free-ing and so SIGSEV.
Modifications:
Just call shutdown() when the constructor throws and so ensure SSL_free is guarded correctly in the finalizer.
Result:
No more SIGSEV possible.
Motivation:
TLSv1.3 support is included in java11 and is also supported by OpenSSL 1.1.1, so we should support when possible.
Modifications:
- Add support for TLSv1.3 using either the JDK implementation or the native implementation provided by netty-tcnative when compiled against openssl 1.1.1
- Adjust unit tests for semantics provided by TLSv1.3
- Correctly handle custom Provider implementations that not support TLSv1.3
Result:
Be able to use TLSv1.3 with netty.
Motivation:
JdkSslContext provides public constructors to wrap an existing `javax.net.ssl.SSLContext`.
Sadly, some options combinations are not possible with the existing constructors, eg:
* protocols is not exposed and always forced to null, so default protocols are always enforced
* startTls is not exposed and always forced to false
Modification:
Add full constructor that take protocols and startTls parameters.
Result:
It's possible to create a JdkSslContext from an existing SSLContext and still have control over protocols and startTls
Motivation:
It is possible that a client is unable to locate a certificate alias given the list of issuers and key types. In this case the X509KeyManager
will return a null which when past to the OpenSslKeyMaterialProvider implementation may produce a NPE. If no matching alias could be found we should not
call OpenSslKeyMaterialProvider at all which is also consistent what OpenJDK does.
Modifications:
- Add null check before calling OpenSslKeyMaterialProvider
- Add unit test.
Result:
No more NPE caused by passing null as client alias.
Motivation:
Before when on server-side we just called the X509KeyManager methods when handshake() was called the first time which is not quite correct as we may not have received the full SSL hello / handshake and so could not extra for example the SNI hostname that was requested.
OpenSSL exposes the SSL_CTX_set_cert_cb function which allows to set a callback which is executed at the correct moment, so we should use it. This also allows us to support more methods of ExtendedSSLSession easily.
Modifications:
- Make use of new methods exposed by netty-tcnative since https://github.com/netty/netty-tcnative/pull/388 to ensure we select the key material at the correct time.
- Implement more methods of ExtendedOpenSslSession
- Add unit tests to ensure we are able to retrieve various things on server-side in the X509KeyManager and so verify it is called at the correct time.
- Simplify code by using new netty-tcnative methods.
Result:
More correct implementation for server-side usage and more complete implemented of ExtendedSSLSession.
Motivation:
a208f6dc7c added a testcase which uses hostname validation which may not be supported by OpenSSL depending on the version that is used. We should check first before we try to use it.
Modifications:
Add assumeTrue(...) check to ensure hostname validation is supported before trying to run the test.
Result:
No more test-failures on OpenSSL versions < 1.0.2.
Motivation:
When a X509TrustManager is used while configure the SslContext the JDK automatically does some extra checks during validation of provided certs by the remote peer. We should do the same when our native implementation is used.
Modification:
- Automatically wrap a X509TrustManager and so do the same validations as the JDK does.
- Add unit tests.
Result:
More consistent behaviour. Fixes https://github.com/netty/netty/issues/6664.
Motivation:
4d1458604a did fix some leaks in SniClientTest but missed the ones in SniClientJava8TestUtil.
Modifications:
Correctly release SslContext.
Result:
No more leaks in SNI tests.
Motivation:
I noticed that we had some errors showing up in a test (which did not fail it tho) because we tried to full-fill the promise multiples times.
Modifications:
Use trySuccess(...) as we may produce multiple exceptions.
Result:
Less errors during test-run.
Motivation:
Java9 added getStatusResponses() to ExtendedSSLSession which we should correctly support when possible.
Modifications:
Implement the method correctly.
Result:
More complete and correct implementation.
Motivation:
6ed7c6c75d added support for ExtendedOpenSslSession but we did not override getStatusResponses(). This lead to test failures on java9.
Modifications:
Implement ExtendedOpenSslSession.getStatusResponses() so it just returns an empty list.
Result:
Test pass again on Java9.
Motivation:
6ed7c6c75d added a test which blindly assumed we can use a KeyManagerFactory all the time. This is only true if have OpenSSL 1.0.2 or later, which may not be the case.
Modifications:
Only use KeyManagerFactory in test if the OpenSSL version does support it.
Result:
More robust tests.
Motivation:
When an ExtendedSSLSession is used its possible to do more strict checking of the keys during handshake. We should do this whenever possible.
Modification:
- Return an ExtendedSSLSession when using client-mode and Java7+
- Add unit test
- Simplify unit tests
Result:
More consistent behaviour.
* PemPrivateKey.toPem(...) should throw IllegalArgumentException when PrivateKey which does not support encoding is used.
Motivation:
At the moment when a PrivateKey is used that does not support encoding we throw a NPE when trying to convert the key. We should better throw an IllegalArgumentException with the details about what key we tried to encode.
Modifications:
- Check if PrivateKey.getEncoded() returns null and if so throw an IllegalArgumentException
- Add unit test.
Result:
Better handling of non-supported PrivateKey implementations.
Motivation:
5aaa16b24c introduced a testcase for specific ciphersuites and checked if these are supported by our native implementation before running it. Unfortunally this is not good enough as even on the JDK it may not be supported on various JDK versions (like Java7). Beside this the test leaked buffers.
Modifications:
- Correctly check if ciphersuite is supported on each SslProvider before trying to run test.
- Fix buffer leaks.
Result:
Testsuite pass again on Java7 and others when -Pleak is used.
Motivation:
ea626ef8c3 added more debug logging but we can even include a bit more.
Modifications:
Always log the error number as well.
Result:
More informations for debugging SSL errors.
Motivation:
We should log a bit more details about why we shutdown the SSL.
Modifications:
Add the return value of SSL_get_error(...) as well in debug mode.
Result:
More logging to make it easier to understand why an SSL error happened.
Motivation
Ensure classes of cipher suites continue working between releases. Adding just a DHE check for now as it caused #8165 but this test can be expaned to other suites.
Modifications
Adding an unit test that checks for the presence of a cipher suite.
Result
Prevent #8165 from happening in the future.
Motivation:
OpenSSL itself has an abstraction which allows you to customize some things. For example it is possible to load the PrivateKey from the engine. We should support this.
Modifications:
Add two new static methods to OpenSslX509KeyManagerFactory which allow to create an OpenSslX509KeyManagerFactory that loads its PrivateKey via the OpenSSL Engine directly.
Result:
More flexible usage of OpenSSL possible
Motivation:
In OpenSslCertificateException we should ensure we try to load netty-tcnative before trying to use any class from it as otherwise it may throw an error due missing linking of the native libs.
Modifications:
- Ensure we call OpenSsl.isAvailable() before we try to use netty-tcnative for validation
- Add testcase.
Result:
No more errors causing by not loading native libs before trying to use these.
* Rename SslHandler.close(...) to closeOutbound(...) as it is still useful and delegate to the methods.
Motivation:
Sometimes the user may want to send a close_notify without closing the underlying Channel. For this we offered the SslHandler.close(...) methods which were marked as deeprecated. We should offer an way to still do this without the user calling deprecated methods.
See https://stackoverflow.com/questions/51710231/using-nettys-sslhandlerclosechannelhandlercontext-channelpromise/51753742#comment90555949_51753742 .
Modifications:
- Remove deprecation of the SslHandler.close(...) method that exactly allows this and rename these to closeOutbound(...) as this is more clear.
- Add close(...) methods that delegate to these and mark these as deprecated.
Result:
Be able to send close_notify without closing the Channel.
Motivation:
When using the JDK SSL provider in client mode, the SNI host names (called serverNames in SslEngineImpl) is set to the peerHost (if available) that is used to initialize the SSL Engine:
http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/sun/security/ssl/SSLEngineImpl.java#l377
This allows one to call SslEngine.getSSLParameters() and inspect what is the SNI name to be sent. The same should be done in the OpenSSL provider as well. Currently even though the the SNI name is sent by the OpenSSL provider during handshake when the peerHost is specified, it is missing from the parameters.
Modification:
Set the sniHostNames field when SNI is to be used. Also verifies the peer is actually a hostname before setting it as the SNI name, which is consistent with JDK SSL provider's behavior.
Result:
SslEngine using the OpenSSL provider created in client mode with peerHost will initialize sniHostNames with the peerHost.
Calling SslEngine.getSSLParameters().getServerNames() will return a list that contains that name.
Motivation:
We missed to do a null check before trying to destroy the OpenSslSessionContext, which could lead to a NPE.
Modifications:
Add null check and tests.
Result:
Fix https://github.com/netty/netty/issues/8170.
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.
Automatic-Module-Name entry provides a stable JDK9 module name, when Netty is used in a modular JDK9 applications. More info: http://blog.joda.org/2017/05/java-se-9-jpms-automatic-modules.html
When Netty migrates to JDK9 in the future, the entry can be replaced by actual module-info descriptor.
Modification:
The POM-s are configured to put the correct module names to the manifest.
Result:
Fixes#7218.
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.
Motivation:
SslHandler only supports ByteBuf objects, but will not release objects of other types. SslHandler will also not release objects if its internal state is not correctly setup.
Modifications:
- Release non-ByteBuf objects in write
- Release all objects if the SslHandler queue is not setup
Result:
Less leaks in SslHandler.
Motivation:
HTTP/2 allows writes of 0 length data frames. However in some cases EMPTY_BUFFER is used instead of the actual buffer that was written. This may mask writes of released buffers or otherwise invalid buffer objects. It is also possible that if the buffer is invalid AbstractCoalescingBufferQueue will not release the aggregated buffer nor fail the associated promise.
Modifications:
- DefaultHttp2FrameCodec should take care to fail the promise, even if releasing the data throws
- AbstractCoalescingBufferQueue should release any aggregated data and fail the associated promise if something goes wrong during aggregation
Result:
More correct handling of invalid buffers in HTTP/2 code.
infinite loop
Motivation:
If SslHandler sets jdkCompatibilityMode to false and ReferenceCountedOpenSslEngine sets jdkCompatibilityMode to true there is a chance we will get stuck in an infinite loop if the peer sends a TLS packet with length greater than 2^14 (the maximum length allowed in the TLS 1.2 RFC [1]). However there are legacy implementations which actually send larger TLS payloads than 2^14 (e.g. OpenJDK's SSLSessionImpl [2]) and in this case ReferenceCountedOpenSslEngine will return BUFFER_OVERFLOW in an attempt to notify that a larger buffer is to be used, but if the buffer is already at max size this process will repeat indefinitely.
[1] https://tools.ietf.org/html/rfc5246#section-6.2.1
[2] http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/file/d5a00b1e8f78/src/share/classes/sun/security/ssl/SSLSessionImpl.java#l793
Modifications:
- Support TLS payload sizes greater than 2^14 in ReferenceCountedOpenSslEngine
- ReferenceCountedOpenSslEngine should throw an exception if a
BUFFER_OVERFLOW is impossible to rectify
Result:
No more infinite loop in ReferenceCountedOpenSslEngine due to
BUFFER_OVERFLOW and large TLS payload lengths.
Motivation:
ReferenceCountedOpenSslEngine.rejectRemoteInitiatedRenegotiation() is called in a finally block to ensure we always check for renegotiation. The problem here is that sometimes we will already shutdown the engine before we call the method which will lead to an NPE in this case as the ssl pointer was already destroyed.
Modifications:
Check that the engine is not destroyed yet before calling SSL.getHandshakeCount(...)
Result:
Fixes [#7353].
Motivation:
Some SSLEngine implementations (e.g. ReferenceCountedOpenSslContext) support unwrapping/wrapping multiple packets at a time. The SslHandler behaves differently if the SSLEngine supports this feature, but currently requires that the constructor argument between the SSLEngine creation and SslHandler are coordinated. This can be difficult, or require package private access, if extending the SslHandler.
Modifications:
- The SslHandler should inspect the SSLEngine to see if it supports jdkCompatibilityMode instead of relying on getting an extra constructor argument which maybe out of synch with the SSLEngine
Result:
Easier to override SslHandler and have consistent jdkCompatibilityMode between SSLEngine and SslHandler.
Motivation:
We should ensure we only try to load the netty-tcnative version that was compiled for the architecture we are using.
Modifications:
Include architecture into native lib name.
Result:
Only load native lib if the architecture is supported.
Motivation:
We can end up with a buffer leak if SSLEngine.wrap(...) throws.
Modifications:
Correctly release the ByteBuf if SSLEngine.wrap(...) throws.
Result:
Fixes [#7337].
Motivation:
A regression was introduced in 86e653e which had the effect that the writability was not updated for a Channel while queueing data in the SslHandler.
Modifications:
- Factor out code that will increment / decrement pending bytes and use it in AbstractCoalescingBufferQueue and PendingWriteQueue
- Add test-case
Result:
Channel writability changes are triggered again.
Motivation:
Bug in capacity calculation: occurs auto convert to string instead of sum up.
Modifications:
Use `eventName.length()` in sum.
Result:
Less trash in logs.
Motivation:
We should also enforce the handshake timeout on the server-side to allow closing connections which will not finish the handshake in an expected amount of time.
Modifications:
- Enforce the timeout on the server and client side
- Add unit test.
Result:
Fixes [#7230].
Motivation:
Even if it's a super micro-optimization (most JVM could optimize such
cases in runtime), in theory (and according to some perf tests) it
may help a bit. It also makes a code more clear and allows you to
access such methods in the test scope directly, without instance of
the class.
Modifications:
Add 'static' modifier for all methods, where it possible. Mostly in
test scope.
Result:
Cleaner code with proper 'static' modifiers.
Motivation:
Without a 'serialVersionUID' field, any change to a class will make
previously serialized versions unreadable.
Modifications:
Add missed 'serialVersionUID' field for all Serializable
classes.
Result:
Proper deserialization of previously serialized objects.
Motivation: Today when Netty encounters a general error while decoding
it treats this as a decoder exception. However, for fatal causes this
should not be treated as such, instead the fatal error should be carried
up the stack without the callee having to unwind causes. This was
probably done for byte to byte message decoder but is now done for all
decoders.
Modifications: Instead of translating any error to a decoder exception,
we let those unwind out the stack (note that finally blocks still
execute) except in places where an event needs to fire where we fire
with the error instead of wrapping in a decoder exception.
Result: Fatal errors will not be treated as innocent decoder exceptions.
Motivation:
Java9SslEngine did not correctly implement ApplicationProtocolAccessor and so returned an empty String when no extension was used while the interface contract is to return null. This lead to the situation that ApplicationProtocolNegationHandler did not correct work.
Modifications:
- Rename ApplicationProtocolAccessor.getApplicationProtocol() to getNegotiatedApplicationProtocol() which resolves the clash with the method exposed by Java9s SSLEngine.
- Correctly implement getNegotiatedApplicationProtocol() for Java9Sslengine
- Add delegate in Java9Sslengine.getApplicationProtocol() which is provided by Java9
- Adjust tests to test the correct behaviour.
Result:
Fixes [#7251].
Motivation:
Getting the latest Conscrypt goodies.
Modifications:
A few API changes have occurred, specifically in the Conscrypt
class.
Result:
Netty now builds and tests against Conscrypt 1.0.0.RC11
Motivation:
A bunch of unit tests are failing due to certificates having expired.
This has to be replaced with a newly generated certificate that has a
longer validity.
Modifications:
- generated a certificate with validity of 100 years from now
Results:
Unit tests are passing again
Motivation:
We should only try to load the native artifacts if the architecture we are currently running on is the same as the one the native libraries were compiled for.
Modifications:
Include architecture in native lib name and append the current arch when trying to load these. This will fail then if its not the same as the arch of the compiled arch.
Result:
Fixes [#7150].
Motivation:
Netty is unable to use Java9s ALPN support atm.
Modifications:
When running on Java9+ we invoke the correct methods that are exposed on the Java9+ implementation of SSLEngine and so be able to support ALPN.
This patch is based on the work of @rschmitt and so https://github.com/netty/netty/pull/6992.
Result:
Fixes#6933.
Motivation:
netty-tcnative recently change the name of the native libraries from using - to _.
Modifications:
- OpenSsl should use _ instead of - even for the classifiers to be consistent with netty-tcnative
Result:
Loading netty-tcnative works.
Motivation:
We should deprecate ApplicationProtocolNegotiator as the users should use ApplicationProtocolConfig these days.
Modifications:
Add deprecation annotations and javadocs.
Result:
Be able to make package-private in next major release.
Motivation:
When SslHandlerTest#testCompositeBufSizeEstimationGuaranteesSynchronousWrite fails it would be useful to know the SslProvider type
Modifications:
- Print the sever and client SslProvider upon failure
- Increase test timeout to 8 minutes to allow more time to run
Result:
Failures include more info to help diagnose issues.
Motivation:
DelegatingSslContext at the moment intercept newEngine calls and allow to init the SslEngine after it is created. The problem here is that this may not work the SSLEngine that is wrapped in the SslHandler when calling newHandler(...). This is because some SslContext implementations not delegate to newEngine(...) when creating the SslHandler to allow some optimizations. For this we should also allow to init the SslHandler after its creation and by default just delegate to initEngine(...).
Modifications:
Allow the user to also init the SslHandler after creation while by default init its SSLEngine after creation.
Result:
More flexible and correct code.
This reverts commit d63bb4811e as this not covered correctly all cases and so could lead to missing fireChannelReadComplete() calls. We will re-evalute d63bb4811e and resbumit a pr once we are sure all is handled correctly
Motivation:
We recently changed netty-tcnative to use underscores in its native library names.
Modifications:
Update code to use underscores when loading native library.
Result:
More consistent code.
Motivation:
SslHandlerTest#testCompositeBufSizeEstimationGuaranteesSynchronousWrite has been observed to fail on CI servers, but it is not clear why.
Modifications:
- Add more visibility into what the state was and what the condition that caused the failure was.
Result:
More visibility when the test fails.
Motivation:
Commit 4448b8f42f introduced some API breakage which we need to revert before we release.
Modifications:
- Introduce an AllocatorAwareSslEngineWrapperFactory which expose an extra method that takes a ByteBufAllocator as well.
- Revert API changes to SslEngineWrapperFactory.
Result:
API breakage reverted.
Motivation:
Its wasteful and also confusing that channelReadComplete() is called even if there was no message forwarded to the next handler.
Modifications:
- Only call ctx.fireChannelReadComplete() if at least one message was decoded
- Add unit test
Result:
Less confusing behavior. Fixes [#4312].
Motivation:
We need to ensure we not try to load any conscrypt classes directly (which means without using reflection) in the same class that is used to check if conscrypt is available. This is needed as otherwise we will have the following problem when try to use netty on java7:
java.lang.UnsupportedClassVersionError: org/conscrypt/BufferAllocator : Unsupported major.minor version 52.0
at io.netty.handler.ssl.ConscryptJdkSslEngineInteropTest.checkConscrypt(ConscryptJdkSslEngineInteropTest.java:49)
This regression was introduced by 4448b8f42f and detected on the CI when using:
mvn clean package -DtestJavaHome=$JAVA7_HOME
Modifications:
Move the detection code in an extra class and use it.
Result:
Works correctly also when using Java7.
Motivation:
Starting with 1.0.0.RC9, conscrypt supports a buffer allocator.
Modifications:
- Updated the creation process for the engine to pass through the
ByteBufAllocator.
- Wrap a ByteBufAllocator with an adapter for conscrypt.
- Added a property to optionally control whether conscrypt uses
Netty's buffer allocator.
Result:
Netty+conscrypt will support using Netty's ByteBufAllocator.
Motivation:
We need to ensure we only create the ResourceLeak when the constructor not throws.
Modifications:
Ensure ResourceLeakDetector.track(...) is only called if the constructor of ReferenceCoundedOpenSslEngine not throws.
Result:
No more false-positves.
Motivation:
ByteBuf#ensureWritable(int,boolean) returns an int indicating the status of the resize operation. For buffers that are unmodifiable or cannot be resized this method shouldn't throw but just return 1.
ByteBuf#ensureWriteable(int) should throw unmodifiable buffers.
Modifications:
- ReadOnlyByteBuf should be updated as described above.
- Add a unit test to SslHandler which verifies the read only buffer can be tolerated in the aggregation algorithm.
Result:
Fixes https://github.com/netty/netty/issues/7002.
Motivation:
Lots of usages of SelfSignedCertificates were not deleting the certs at
the end of the test. This includes setupHandlers() which is used by
extending classes. Although these files will be deleted at JVM exit and
deleting them early does not free the JVM from trying to delete them at
shutdown, it's good practice to delete eagerly and since users sometimes
use tests as a form of documentation, it'd be good for them to see the
explicit deletes.
Modifications:
Add missing delete() calls to ½ of the SelfSignedCertificates-using
tests.
Result:
Tests that more clearly communicates which resources are created and
may accumulate without early delete.
Motivation:
Previously filterCipherSuites was being passed the OpenSSL-formatted
cipher names. Commit 43ae974 introduced a regression as it swapped to the
RFC/JDK format, except that user-provided ciphers were not converted and
remained in the OpenSSL format.
This mis-match would cause all user-provided to be thrown away, leading
to failure trying to set zero ciphers:
Exception in thread "main" javax.net.ssl.SSLException: failed to set cipher suite: []
at io.netty.handler.ssl.ReferenceCountedOpenSslContext.<init>(ReferenceCountedOpenSslContext.java:299)
at io.netty.handler.ssl.OpenSslContext.<init>(OpenSslContext.java:43)
at io.netty.handler.ssl.OpenSslServerContext.<init>(OpenSslServerContext.java:347)
at io.netty.handler.ssl.OpenSslServerContext.<init>(OpenSslServerContext.java:335)
at io.netty.handler.ssl.SslContext.newServerContextInternal(SslContext.java:421)
at io.netty.handler.ssl.SslContextBuilder.build(SslContextBuilder.java:441)
Caused by: java.lang.Exception: Unable to configure permitted SSL ciphers (error:100000b1:SSL routines:OPENSSL_internal:NO_CIPHER_MATCH)
at io.netty.internal.tcnative.SSLContext.setCipherSuite(Native Method)
at io.netty.handler.ssl.ReferenceCountedOpenSslContext.<init>(ReferenceCountedOpenSslContext.java:295)
... 7 more
Modifications:
Remove the reformatting of user-provided ciphers, as they are already in
the RFC/JDK format.
Result:
No regression, and the internals stay sane using the RFC/JDK format.
Motivation:
Some ChannelOptions must be set before the Channel is really registered to have the desired effect.
Modifications:
Add another constructor argument which allows to not register the EmbeddedChannel to its EventLoop until the user calls register().
Result:
More flexible usage of EmbeddedChannel. Also Fixes [#6968].
Motivation:
6152990073 introduced a test-case in SSLEngineTest which used OpenSsl.* which should not be done as this is am abstract bass class that is also used for non OpenSsl tests.
Modifications:
Move the protocol definations into SslUtils.
Result:
Cleaner code.
Motivation:
Code introduced in 6152990073 can be cleaned up and use array initializer expressions.
Modifications:
Use array initializer expressions.
Result:
Cleaner code.
Motivation:
TLS doesn't support a way to advertise non-contiguous versions from the client's perspective, and the client just advertises the max supported version. The TLS protocol also doesn't support all different combinations of discrete protocols, and instead assumes contiguous ranges. OpenSSL has some unexpected behavior (e.g. handshake failures) if non-contiguous protocols are used even where there is a compatible set of protocols and ciphers. For these reasons this method will determine the minimum protocol and the maximum protocol and enabled a contiguous range from [min protocol, max protocol] in OpenSSL.
Modifications:
- ReferenceCountedOpenSslEngine#setEnabledProtocols should determine the min/max protocol versions and enable a contiguous range
Result:
OpenSslEngine is more consistent with the JDK's SslEngineImpl and no more unexpected handshake failures due to protocol selection quirks.
Motivation:
Currently the default cipher suites are set independently between JDK and OpenSSL. We should use a common approach to setting the default ciphers. Also the OpenSsl default ciphers are expressed in terms of the OpenSSL cipher name conventions, which is not correct and may be exposed to the end user. OpenSSL should also use the RFC cipher names like the JDK defaults.
Modifications:
- Move the default cipher definition to a common location and use it in JDK and OpenSSL initialization
- OpenSSL should not expose OpenSSL cipher names externally
Result:
Common initialization and OpenSSL doesn't expose custom cipher names.
Motivation:
PR https://github.com/netty/netty/pull/6803 corrected an error in the return status of the OpenSslEngine. We should now be able to restore the SslHandler optimization.
Modifications:
- This reverts commit 7f3b75a509.
Result:
SslHandler optimization is restored.
Motivation:
Each call to SSL_write may introduce about ~100 bytes of overhead. The OpenSslEngine (based upon OpenSSL) is not able to do gathering writes so this means each wrap operation will incur the ~100 byte overhead. This commit attempts to increase goodput by aggregating the plaintext in chunks of <a href="https://tools.ietf.org/html/rfc5246#section-6.2">2^14</a>. If many small chunks are written this can increase goodput, decrease the amount of calls to SSL_write, and decrease overall encryption operations.
Modifications:
- Introduce SslHandlerCoalescingBufferQueue in SslHandler which will aggregate up to 2^14 chunks of plaintext by default
- Introduce SslHandler#setWrapDataSize to control how much data should be aggregated for each write. Aggregation can be disabled by setting this value to <= 0.
Result:
Better goodput when using SslHandler and the OpenSslEngine.
Motivation:
The JDK SSLEngine documentation says that a call to wrap/unwrap "will attempt to consume one complete SSL/TLS network packet" [1]. This limitation can result in thrashing in the pipeline to decode and encode data that may be spread amongst multiple SSL/TLS network packets.
ReferenceCountedOpenSslEngine also does not correct account for the overhead introduced by each individual SSL_write call if there are multiple ByteBuffers passed to the wrap() method.
Modifications:
- OpenSslEngine and SslHandler supports a mode to not comply with the limitation to only deal with a single SSL/TLS network packet per call
- ReferenceCountedOpenSslEngine correctly accounts for the overhead of each call to SSL_write
- SslHandler shouldn't cache maxPacketBufferSize as aggressively because this value may change before/after the handshake.
Result:
OpenSslEngine and SslHanadler can handle multiple SSL/TLS network packet per call.
[1] https://docs.oracle.com/javase/7/docs/api/javax/net/ssl/SSLEngine.html
Motivation:
SslHandlerTest#testCompositeBufSizeEstimationGuaranteesSynchronousWrite has been observed to fail on CI servers. Knowing how many bytes were seen by the client would be helpful.
Modifications:
- Add bytesSeen to the exception if the client closes early.
Result:
More debug info available.
Motivation:
ErrorProne warns about missing cases in switch statements that
appear as an oversight.
Modifcation:
Add the last case to statement to ensure all cases are covered.
Result:
Able to enable Error Prone static analysis
Motivation:
Enable static linking for Java 8. These commits are the same as those introduced to netty tcnative. The goal is to allow lots of JNI libraries to be statically linked together without having conflict `JNI_OnLoad` methods.
Modification:
* add JNI_OnLoad suffixes to enable static linking
* Add static names to the list of libraries that try to be loaded
* Enable compiling with JNI 1.8
* Sort includes
Result:
Enable statically linked JNI code.
Motivation:
PR #6811 introduced a public utility methods to decode hex dump and its parts, but they are not visible from netty-common.
Modifications:
1. Move the `decodeHexByte`, `decodeHexDump` and `decodeHexNibble` methods into `StringUtils`.
2. Apply these methods where applicable.
3. Remove similar methods from other locations (e.g. `HpackHex` test class).
Result:
Less code duplication.
Motivation:
For historical reasons OpenSSL's internal naming convention for CHACHA20 based cipher suites does not include the HMAC algorithm in the cipher name. This will prevent the CHACHA20 cipher suites from being used if the RFC cipher names are specified.
Modifications:
- Add a special case for CHACHA20 cipher name conversions in CipherSuiteConverter
- Update OPENSSL_CIPHERSUITE_PATTERN to accommodate the new naming scheme for CHACHA20 cipher suites
Result:
CipherSuiteConverter now works with CHACHA20 cipher suites.
Motivation
It's cleaner to add listeners to returned Futures rather than provided Promises because the latter can have strange side effects in terms of listeners firing and called methods returning. Adding listeners preemtively may yield also to more OPS than necessary when there's an Exception in the to be called method.
Modifications
Add listener to returned ChannelFuture rather than given ChannelPromise
Result
Cleaner completion and exception handling
Motivation:
We had some useless synchronized (ReferenceCountedOpenSslContext.class) blocks in our code which could slow down concurrent collecting and creating of ReferenceCountedOpenSslContext instances. Beside this we missed a few guards.
Modifications:
Use ReadWriteLock to correctly guard. A ReadWriteLock was choosen as SSL.newSSL(...) will be called from multiple threads all the time so using synchronized would be worse and there would be no way for the JIT to optimize it away
Result:
Faster concurrent creating and collecting of ReferenceCountedOpenSslContext instances and correctly guard in all cases.
Motivation:
If the destination buffer is completely filled during a call to OpenSslEngine#wrap(..) we may return NEED_UNWRAP because there is no data pending in the SSL buffers. However during a handshake if the SSL buffers were just drained, and filled up the destination buffer it is possible OpenSSL may produce more data on the next call to SSL_write. This means we should keep trying to call SSL_write as long as the destination buffer is filled and only return NEED_UNWRAP when the destination buffer is not full and there is no data pending in OpenSSL's buffers.
Modifications:
- If the handshake produces data in OpenSslEngine#wrap(..) we should return NEED_WRAP if the destination buffer is completely filled
Result:
OpenSslEngine returns the correct handshake status from wrap().
Fixes https://github.com/netty/netty/issues/6796.
Motivation
RFC 6066 (https://tools.ietf.org/html/rfc6066#page-6) says that the hostname in the SNI extension is ASCII encoded but Netty decodes it using UTF-8.
Modifications
Use ASCII instead of UTF-8
Result
Fixes#6717
Motivation
SslHandler should release any type of SSLEngine if it implements the ReferenceCounted interface
Modifications
Change condition to check for ReferenceCounted interface
Result
Better use of interfaces
Motivation:
We not correctly handle LE buffers when try to read the packet length out of the buffer and just assume it always is a BE buffer.
Modifications:
Correctly account for the endianess of the buffer when reading the packet lenght.
Result:
Fixes [#6709].
Motivation:
SslHandler#wrapNonAppData may be able to return early if it is called from a unwrap method and the status is NEED_UNWRAP. This has been observed to occur while using the OpenSslEngine and can avoid allocation of an extra ByteBuf of size 2048.
Modifications:
- Return early from SslHandler#wrapNonAppData if NEED_UNWRAP and we are called from an unwrap method
Result:
Less buffer allocations and early return from SslHandler#wrapNonAppData.
Motivation:
We only used the openssl version to detect if Ocsp is supported or not which is not good enough as even the version is correct it may be compiled without support for OCSP (like for example on ubuntu).
Modifications:
Try to enable OCSP while static init OpenSsl and based on if this works return true or false when calling OpenSsl.isOcspSupported().
Result:
Correctly detect if OSCP is supported.
Motivation:
In OpenSsl init code we create a SelfSignedCertificate which we not explicitly delete. This can lead to have the deletion delayed.
Modifications:
Delete the SelfSignedCertificate once done with it.
Result:
Fixes [#6716]
Motivation:
SSL_write requires a fixed amount of bytes for overhead related to the encryption process for each call. OpenSslEngine#wrap(..) will attempt to encrypt multiple input buffers until MAX_PLAINTEXT_LENGTH are consumed, but the size estimation provided by calculateOutNetBufSize may not leave enough room for each call to SSL_write. If SSL_write is not able to completely write results to the destination buffer it will keep state and attempt to write it later. Netty doesn't account for SSL_write keeping state and assumes all writes will complete synchronously (by attempting to allocate enough space to account for the overhead) and feeds the same data to SSL_write again later which results in corrupted data being generated.
Modifications:
- OpenSslEngine#wrap should only produce a single TLS packet according to the SSLEngine API specificaiton [1].
[1] https://docs.oracle.com/javase/8/docs/api/javax/net/ssl/SSLEngine.html#wrap-java.nio.ByteBuffer:A-int-int-java.nio.ByteBuffer-
- OpenSslEngine#wrap should only consider a single buffer when determining if there is enough space to write, because only a single buffer will ever be consumed.
Result:
OpenSslEngine#wrap will no longer produce corrupted data due to incorrect accounting of space required in the destination buffers.
Motivation:
When adding SNIMatcher support we missed to use static delegating methods and so may try to load classes that not exists in Java7. Which will lead to errors.
Modifications:
- Correctly only try to load classes when running on java8+
- Ensure Java8+ related tests only run when using java8+
Result:
Fixes [#6700]
Motivation:
Conscrypt is not needed when using the handler module, so it should be marked as optional
Modifications:
Mark conscrypt as optional
Result:
Be able to use handler module without conscrypt
Motivation
SniHandler is "hardcoded" to use hostname -> SslContext mappings but there are use-cases where it's desireable and necessary to return more information than a SslContext. The only option so far has been to use a delegation pattern
Modifications
Extract parts of the existing SniHandler into an abstract base class and extend SniHandler from it. Users can do the same by extending the new abstract base class and implement custom behavior that is possibly very different from the common/default SniHandler.
Touches
- f97866dbc6
- b604a22395
Result
Fixes#6603
Motivation:
Java8 adds support for SNIMatcher to reject SNI when the hostname not matches what is expected. We not supported doing this when using SslProvider.OPENSSL*.
Modifications:
- Add support for SNIMatcher when using SslProvider.OPENSSL*
- Add unit tests
Result:
SNIMatcher now support with our own SSLEngine as well.
Motivation:
We need to ensure we only try to to test with the SslProviders that are supported when running the SslHandlerTest.testCompositeBufSizeEstimationGuaranteesSynchronousWrite test.
Modifications:
Skip SslProvider.OPENSSL* if not supported.
Result:
No more test-failures if openssl is not installed on the system.
Motivation:
Java8 adds support for SNIMatcher to reject SNI when the hostname not matches what is expected. We not supported doing this when using SslProvider.OPENSSL*.
Modifications:
- Add support for SNIMatcher when using SslProvider.OPENSSL*
- Add unit tests
Result:
SNIMatcher now support with our own SSLEngine as well.
https://github.com/netty/netty-tcnative/pull/215
Motivation
OCSP stapling (formally known as TLS Certificate Status Request extension) is alternative approach for checking the revocation status of X.509 Certificates. Servers can preemptively fetch the OCSP response from the CA's responder, cache it for some period of time, and pass it along during (a.k.a. staple) the TLS handshake. The client no longer has to reach out on its own to the CA to check the validity of a cetitficate. Some of the key benefits are:
1) Speed. The client doesn't have to crosscheck the certificate.
2) Efficiency. The Internet is no longer DDoS'ing the CA's OCSP responder servers.
3) Safety. Less operational dependence on the CA. Certificate owners can sustain short CA outages.
4) Privacy. The CA can lo longer track the users of a certificate.
https://en.wikipedia.org/wiki/OCSP_staplinghttps://letsencrypt.org/2016/10/24/squarespace-ocsp-impl.html
Modifications
https://www.openssl.org/docs/man1.0.2/ssl/SSL_set_tlsext_status_type.html
Result
High-level API to enable OCSP stapling
Motivation:
`io.netty.handler.logging.LoggingHandler` does not log when these
events happen.
Modifiations:
Add overrides with logging to these methods.
Result:
Logging now happens for these two events.
Motivation:
In OpenSslCertificateException we tried to validate the supplied error code but did not correctly account for all different valid error codes and so threw an IllegalArgumentException.
Modifications:
- Fix validation by updating to latest netty-tcnative and use CertificateVerifier.isValid
- Add unit tests
Result:
Validation of error code works as expected.
Motivation:
1419f5b601 added support for conscrypt but the CI started to fail when running tests with java7 as conscrypt is compiled with java8. This was partly fixed in c4832cd9d9 but we also need to ensure we not try to even load the classes.
Modifications:
Only try to load conscrypt classes when on java8+-
Result:
CI not fails anymore.
Motivation:
1419f5b601 added support for conscrypt but the CI started to fail when running tests with java7 as conscrypt is compiled with java8.
Modifications:
Only support conscrypt on Java8+
Result:
CI not fails anymore.
Motivation:
Conscrypt is a Java Security provider that wraps OpenSSL (specifically BoringSSL). It's a possible alternative to Netty-tcnative that we should explore. So this commit is just to enable us to further investigate its use.
Modifications:
Modifying the SslContext creation path to support the Conscrypt provider.
Result:
Netty will support OpenSSL with conscrypt.
Motivation:
We should limit the size of the allocated outbound buffer to MAX_ENCRYPTED_PACKET_LENGTH to ensure we not cause an OOME when the user tries to encrypt a very big buffer.
Modifications:
Limit the size of the allocated outbound buffer to MAX_ENCRYPTED_PACKET_LENGTH
Result:
Fixes [#6564]
Motivation:
The widely used SSL Implementation, OpenSSL, already supports Heartbeat Extension; both sending and responding to Heartbeat Messages. But, since Netty is not recognizing that extension as valid packet, peers won't be able to use this extension.
Modification:
Update SslUtils.java to recognize Heartbeat Extension as valid tls packet.
Result:
With this change, softwares using Netty + OpenSSL will be able to respond for TLS Heartbeat requests (actually taken care by OpenSSL - no need of any extra implementation from Clients)
Motivation:
ChunkedWriteHandler queues written messages and actually writes them
when flush is called. In its doFlush method, it needs to flush after
each chunk is written to preserve memory. However, non-chunked messages
(those that aren't of type ChunkedInput) are treated in the same way,
which means that flush is called after each message is written.
Modifications:
Moved the call to flush() inside the if block that tests if the message
is an instance of ChunkedInput. To ensure flush is called at least once,
the existing boolean flushed is checked at the end of doFlush. This
check was previously in ChunkedWriteHandler.flush(), but wasn't checked in
other invocations of doFlush, e.g. in channelInactive.
Result:
When this handler is present in a pipeline, writing a series
of non-chunked messages will be flushed as the developer intended.
Motivation:
Some pipelines require support for both SSL and non-SSL messaging.
Modifications:
Add utility decoder to support both SSL and non-SSL handlers based on the initial message.
Result:
Less boilerplate code to write for developers.
Motivation:
5e64985089 introduced support for the KeyManagerFactory while using OpenSSL. This same commit also introduced 2 calls to SSLContext.setVerify when 1 should be sufficient.
Modifications:
- Remove the duplicate call to SSLContext.setVerify
Result:
Less duplicate code in ReferenceCountedOpenSslServerContext.
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.
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.
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
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.
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.
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.
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.
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.
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
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).
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.
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.
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.
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.
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
Motivation:
ThreadLocalInsecureRandom still referenced ThreadLocalRandom directly, but shouldn't.
Modifications:
ThreadLocalInsecureRandom should reference PlatformDependent#threadLocalRandom() in comments
Result:
Less usage of internal.ThreadLocalRandom.
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.
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
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.
Motivation:
JdkOpenSslEngineInteroptTest.mySetupMutualAuthServerIsValidClientException(...) delegated to the wrong super method.
Modifications:
Fix delegate
Result:
Correct test-code.
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.
Motivation:
tcnative was moved into an internal package.
Modifications:
Update package for tcnative imports.
Result:
Use correct package names for tcnative.
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
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.
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.
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.
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.
