Motivation:
e9ce5048df added a testcase to ensure we correctly send the alert in all cases but did use a too strict message matching which did not work for BoringSSL as it not uses whitespaces but underscores.
Modifications:
Make the message matching less strict.
Result:
Test pass also when using BoringSSL.
Motivation:
When the verification of the server cert fails because of the used TrustManager on the client-side we need to ensure we produce the correct alert and send it to the remote peer before closing the connection.
Modifications:
- Use the correct verification mode on the client-side by default.
- Update tests
Result:
Fixes https://github.com/netty/netty/issues/8942.
Motivation:
The SSLSession that is returned by SSLEngine.getHandshakeSession() must be able to provide the local certificates when the TrustManager is invoked on the server-side.
Modifications:
- Correctly return the local certificates
- Add unit test
Result:
Be able to obtain local certificates from handshake SSLSession during verification on the server side.
Motivation:
In the past we found a lot of SSL related bugs because of the interopt tests we have in place between different SSLEngine implementations. We should have as many of these interopt tests as possible for this reason.
Modifications:
- Add interopt tests between Conscrypt and OpenSSL SSLEngine implementations
Result:
More tests for SSL.
Motivation:
SSLEngine API has a notion of tasks that may be expensive and offload these to another thread. We did not support this when using our native implementation but can now for various operations during the handshake.
Modifications:
- Support offloading tasks during the handshake when using our native SSLEngine implementation
- Correctly handle the case when NEED_TASK is returned and nothing was consumed / produced yet
Result:
Be able to offload long running tasks from the EventLoop when using SslHandler with our native SSLEngine.
Motivation:
We must only remove ReferenceCountedOpenSslEngine from OpenSslEngineMap when engine is destroyed as the verifier / certificate callback may be called multiple times when the remote peer did initiate a renegotiation.
If we fail to do so we will cause an NPE like this:
```
13:16:36.750 [testsuite-oio-worker-5-18] DEBUG i.n.h.s.ReferenceCountedOpenSslServerContext - Failed to set the server-side key material
java.lang.NullPointerException: null
at io.netty.handler.ssl.OpenSslKeyMaterialManager.setKeyMaterialServerSide(OpenSslKeyMaterialManager.java:69)
at io.netty.handler.ssl.ReferenceCountedOpenSslServerContext$OpenSslServerCertificateCallback.handle(ReferenceCountedOpenSslServerContext.java:212)
at io.netty.internal.tcnative.SSL.readFromSSL(Native Method)
at io.netty.handler.ssl.ReferenceCountedOpenSslEngine.readPlaintextData(ReferenceCountedOpenSslEngine.java:575)
at io.netty.handler.ssl.ReferenceCountedOpenSslEngine.unwrap(ReferenceCountedOpenSslEngine.java:1124)
at io.netty.handler.ssl.ReferenceCountedOpenSslEngine.unwrap(ReferenceCountedOpenSslEngine.java:1236)
at io.netty.handler.ssl.ReferenceCountedOpenSslEngine.unwrap(ReferenceCountedOpenSslEngine.java:1279)
at io.netty.handler.ssl.SslHandler$SslEngineType$1.unwrap(SslHandler.java:217)
at io.netty.handler.ssl.SslHandler.unwrap(SslHandler.java:1330)
at io.netty.handler.ssl.SslHandler.decodeNonJdkCompatible(SslHandler.java:1237)
at io.netty.handler.ssl.SslHandler.decode(SslHandler.java:1274)
at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:502)
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:441)
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:278)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:359)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:345)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:337)
at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1408)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:359)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:345)
at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:930)
at io.netty.channel.oio.AbstractOioByteChannel.doRead(AbstractOioByteChannel.java:170)
at io.netty.channel.oio.AbstractOioChannel$1.run(AbstractOioChannel.java:40)
at io.netty.channel.ThreadPerChannelEventLoop.run(ThreadPerChannelEventLoop.java:69)
at io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:905)
at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
at java.base/java.lang.Thread.run(Thread.java:834)
```
While the exception is kind of harmless (as we will reject the renegotiation at the end anyway) it produces some noise in the logs.
Modifications:
Don't remove engine from map after handshake is complete but wait for it to be removed until the engine is destroyed.
Result:
No more NPE and less noise in the logs.
Motivation:
ChunkedWriteHandler needs to close both successful and failed
ChunkInputs. It used to never close successful ones.
Modifications:
* ChunkedWriteHandler always closes ChunkInput before completing
the write promise.
* Ensure only ChunkInput#close() is invoked
on a failed input.
* Ensure no methods are invoked on a closed input.
Result:
Fixes https://github.com/netty/netty/issues/8875.
Motivation:
fa6a8cb09c introduced correct dispatching of delegated tasks for SSLEngine but did not correctly handle some cases for resuming wrap / unwrap after the task was executed. This could lead to stales, which showed up during tests when running with Java11 and BoringSSL.
Modifications:
- Correctly resume wrap / unwrap in all cases.
- Fix timeout value which was changed in previous commit by mistake.
Result:
No more stales after task execution.
Motivation:
We use outdated EA releases when building and testing with JDK 12 and 13.
Modifications:
- Update versions.
- Add workaround for possible JDK12+ bug.
Result:
Use latest releases
Motivation:
The SSLEngine does provide a way to signal to the caller that it may need to execute a blocking / long-running task which then can be offloaded to an Executor to ensure the I/O thread is not blocked. Currently how we handle this in SslHandler is not really optimal as while we offload to the Executor we still block the I/O Thread.
Modifications:
- Correctly support offloading the task to the Executor while suspending processing of SSL in the I/O Thread
- Add new methods to SslContext to specify the Executor when creating a SslHandler
- Remove @deprecated annotations from SslHandler constructor that takes an Executor
- Adjust tests to also run with the Executor to ensure all works as expected.
Result:
Be able to offload long running tasks to an Executor when using SslHandler. Partly fixes https://github.com/netty/netty/issues/7862 and https://github.com/netty/netty/issues/7020.
Motivation:
We need to update to a new checkstyle plugin to allow the usage of lambdas.
Modifications:
- Update to new plugin version.
- Fix checkstyle problems.
Result:
Be able to use checkstyle plugin which supports new Java syntax.
Motivation:
ChunkedWriteHandler should report write operation as failed
in case *any* chunked was not written. Right now this is not
true for the last chunk.
Modifications:
* Check if the appropriate write operation was succesfull when
reporting the last chunk
* Skip writing chunks if the write operation was already marked
as "done"
* Test cases to cover write failures when dealing with chunked input
Result:
Fix https://github.com/netty/netty/issues/8700
Motivation:
Users who want to construct a `FlushConsolidationHandler` with a default `explicitFlushAfterFlushes` but non-default `consolidateWhenNoReadInProgress` may benefit from having an easy way to get the default "flush after flushes" count.
Modifications:
- Moved default `explicitFlushAfterFlushes` value to a public constant.
- Adjusted Javadoc accordingly.
Result:
Default `explicitFlushAfterFlushes` is accessible to callers.
Motivation:
During some other work I noticed we do not have any tests to ensure we correctly use SSLSessionBindingEvent. We should add some testing.
Modifications:
- Added unit test to verify we correctly implement it.
- Ignore the test when using Conscrypt as it not correctly implements it.
Result:
More tests for custom SSL impl.
Motivation:
We missed to skip a few tests that depend on the KeyManagerFactory if the used OpenSSL version / flavor not support it.
Modifications:
Add missing overrides.
Result:
Testsuite also passes for example when using LibreSSL.
Motivation:
In windows if the project is in a path that contains whitespace,
resources cannot be accessed and tests fail.
Modifications:
Adds ResourcesUtil.java in netty-common. Tests use ResourcesUtil.java to access a resource.
Result:
Being able to build netty in a path containing whitespace
Motivation:
SSLSession.putValue / getValue / removeValue / getValueNames must be thread-safe as it may be called from multiple threads. This is also the case in the OpenJDK implementation.
Modifications:
Guard with synchronized (this) blocks to keep the memory overhead low as we do not expect to have these called frequently.
Result:
SSLSession implementation is thread-safe.
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:
Most of the maven modules do not explicitly declare their
dependencies and rely on transitivity, which is not always correct.
Modifications:
For all maven modules, add all of their dependencies to pom.xml
Result:
All of the (essentially non-transitive) depepdencies of the modules are explicitly declared in pom.xml
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.
Swallow SSL Exception "closing inbound before receiving peer's close_notify" when running on Java 11 (#8463)
Motivation:
When closing a inbound SSL connection before the remote peer has send a close notify, the Java JDK is trigger happy to throw an exception. This exception can be ignored since the connection is about to be closed.
The exception wasn't printed in Java 8, based on filtering on the exception message. In Java 11 the exception message has been changed.
Modifications:
Update the if statement to also filter/swallow the message on Java 11.
Result:
On Java 11 the exception isn't printed with log levels set to debug. The old behaviour is maintained.
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.
Motivation:
We did not return the pointer to SSL_CTX put to the internal datastructure of tcnative.
Modifications:
Return the correct pointer.
Result:
Methods work as documented in the javadocs.
* 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:
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.
