Motivation:
ByteBuf supports “marker indexes”. The intended use case for these is if a speculative operation (e.g. decode) is in process the user can “mark” and interface and refer to it later if the operation isn’t successful (e.g. not enough data). However this is rarely used in practice,
requires extra memory to maintain, and introduces complexity in the state management for derived/pooled buffer initialization, resizing, and other operations which may modify reader/writer indexes.
Modifications:
Remove support for marking and adjust testcases / code.
Result:
Fixes https://github.com/netty/netty/issues/8535.
Motivation:
If two requests from the same IP are reached at the same time, `connected.contains(remoteIp)` may return false in both threads.
Modifications:
Check if there is already a connection with the same IP using return values.
Result:
Become thread safe.
Motivation:
0d2e38d5d6 added supported for detection of peer supported algorithms but we missed to fix the testcase.
Modifications:
Fix test-case.
Result:
No more failing tests with BoringSSL.
Motivation:
The SSLSession.getLocalCertificates() / getLocalPrincipial() methods did not correctly return the local configured certificate / principal if a KeyManagerFactory was used when configure the SslContext.
Modifications:
- Correctly update the local certificates / principial when the key material is selected.
- Add test case that verifies the SSLSession after the handshake to ensure we correctly return all values.
Result:
SSLSession returns correct values also when KeyManagerFactory is used with the OpenSSL provider.
* Correctly convert supported signature algorithms when using BoringSSL
Motivation:
BoringSSL uses different naming schemes for the signature algorithms so we need to adjust the regex to also handle these.
Modifications:
- Adjust SignatureAlgorithmConverter to handle BoringSSL naming scheme
- Ensure we do not include duplicates
- Add unit tests.
Result:
Correctly convert boringssl signature algorithm names.
Motivation:
We did not correctly convert between openssl / boringssl and java ciphers when using TLV1.3 which had different effects when either using openssl or boringssl.
- When using openssl and TLSv1.3 we always returned SSL_NULL_WITH_NULL_NULL as cipher name
- When using boringssl with TLSv1.3 we always returned an incorrect constructed cipher name which does not match what is defined by Java.
Modifications:
- Add correct mappings in CipherSuiteConverter for both openssl and boringssl
- Add unit tests for CipherSuiteConvert
- Add unit in SSLEngine which checks that we do not return SSL_NULL_WITH_NULL_NULL ever and that server and client returns the same cipher name.
Result:
Fixes https://github.com/netty/netty/issues/8477.
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:
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:
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:
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:
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:
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:
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:
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
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:
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.
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.
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:
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:
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:
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:
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:
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:
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
