Motivation:
SslHandler must generate control data as part of the TLS protocol, for example
to do handshakes. SslHandler doesn't capture the status of the future
corresponding to the writes when writing this control (aka non-application
data). If there is another handler before the SslHandler that wants to fail
these writes the SslHandler will not detect the failure and we must wait until
the handshake timeout to detect a failure.
Modifications:
- SslHandler should detect if non application writes fail, tear down the
channel, and clean up any pending state.
Result:
SslHandler detects non application write failures and cleans up immediately.
Motivation
SSL unit tests started failing for me (RHEL 7.6) after #9162. It looks
like the intention was to prevent disable use of the
io.netty.handler.ssl.openssl.useKeyManagerFactory property when using
BoringSSL, but it now gets set to false in that case rather than the
prior/non-BoringSSL default of true.
Modification
Set useKeyManagerFactory to true rather than false in BoringSSL case
during static init of OpenSSl class.
Result
Tests pass again.
Motivation:
How we tried to detect if KeyManagerFactory is supported was not good enough for OpenSSL 1.1.0+ as it partly provided the API but not all of what is required.
This then lead to failures like:
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 1.102 s <<< FAILURE! - in io.netty.channel.epoll.EpollDomainSocketStartTlsTest
[ERROR] initializationError(io.netty.channel.epoll.EpollDomainSocketStartTlsTest) Time elapsed: 0.016 s <<< ERROR!
javax.net.ssl.SSLException: failed to set certificate and key
at io.netty.handler.ssl.ReferenceCountedOpenSslServerContext.newSessionContext(ReferenceCountedOpenSslServerContext.java:130)
at io.netty.handler.ssl.OpenSslServerContext.<init>(OpenSslServerContext.java:353)
at io.netty.handler.ssl.OpenSslServerContext.<init>(OpenSslServerContext.java:334)
at io.netty.handler.ssl.SslContext.newServerContextInternal(SslContext.java:468)
at io.netty.handler.ssl.SslContextBuilder.build(SslContextBuilder.java:457)
at io.netty.testsuite.transport.socket.SocketStartTlsTest.data(SocketStartTlsTest.java:93)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
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.runners.Parameterized.allParameters(Parameterized.java:280)
at org.junit.runners.Parameterized.<init>(Parameterized.java:248)
at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
at org.junit.internal.builders.AnnotatedBuilder.buildRunner(AnnotatedBuilder.java:104)
at org.junit.internal.builders.AnnotatedBuilder.runnerForClass(AnnotatedBuilder.java:86)
at org.junit.runners.model.RunnerBuilder.safeRunnerForClass(RunnerBuilder.java:59)
at org.junit.internal.builders.AllDefaultPossibilitiesBuilder.runnerForClass(AllDefaultPossibilitiesBuilder.java:26)
at org.junit.runners.model.RunnerBuilder.safeRunnerForClass(RunnerBuilder.java:59)
at org.junit.internal.requests.ClassRequest.getRunner(ClassRequest.java:33)
at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:362)
at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:273)
at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:238)
at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:159)
at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:384)
at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:345)
at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:126)
at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:418)
Caused by: java.lang.Exception: Requires OpenSSL 1.0.2+
at io.netty.internal.tcnative.SSLContext.setCertificateCallback(Native Method)
at io.netty.handler.ssl.ReferenceCountedOpenSslServerContext.newSessionContext(ReferenceCountedOpenSslServerContext.java:126)
... 32 more
Modifications:
Also try to set the certification callback and only if this works as well mark KeyManagerFactory support as enabled.
Result:
Also correctly work when OpenSSL 1.1.0 is used.
Motivation:
When we added support for KeyManagerFactory we also allowed to disable it to make the change less risky. This was done years ago and so there is really no need to use the property anyway.
Unfortunally due a change in netty-tcnative it is even not supported anymore when using BoringSSL.
Modifications:
- Log an info message to tell users that 'io.netty.handler.ssl.openssl.useKeyManagerFactory' is deprecated when it is used
- Ignore 'io.netty.handler.ssl.openssl.useKeyManagerFactory' when BoringSSL is used.
Result:
Fixes https://github.com/netty/netty/issues/9147.
Motivation:
OOME is occurred by increasing suppressedExceptions because other libraries call Throwable#addSuppressed. As we have no control over what other libraries do we need to ensure this can not lead to OOME.
Modifications:
Only use static instances of the Exceptions if we can either dissable addSuppressed or we run on java6.
Result:
Not possible to OOME because of addSuppressed. Fixes https://github.com/netty/netty/issues/9151.
Motivation:
Depending on what OpenSSL library version we use / system property that is set we need to skip tests that use KeyManagerFactory.
Modifications:
Add missing assume checks for tests that use KeyManagerFactory.
Result:
All tests pass even if KeyManagerFactory is not supported
Motivation:
Previously, any 'relative' pipeline operations, such as
ctx.pipeline().replace(), .addBefore(), addAfter(), etc
would fail as the handler was not present in the pipeline.
Modification:
Used the pattern from ChannelInitializer when invoking configurePipeline().
Result:
Fixes#9131
Motivation:
As brought up in https://github.com/netty/netty/issues/8998, JKS can be substantially faster than pkcs12, JDK's new default. Without an option to set the KeyStore type you must change the configuration of the entire JVM which is impractical.
Modification:
- Allow to specify KeyStore type
- Add test case
Result:
Fixes https://github.com/netty/netty/issues/8998.
Motivation:
Bootstrap allows you to set a localAddress for outbound TCP connections, either via the Bootstrap.localAddress(localAddress) or Bootstrap.connect(remoteAddress, localAddress) methods. This works well if you want to bind to just one IP address on an interface. Sometimes you want to bind to a specific address based on the resolved remote address which should be possible.
Modifications:
Add DynamicAddressConnectHandler and tests
Result:
Fixes https://github.com/netty/netty/issues/8940.
Motivation:
Subclasses of `OpenSslKeyMaterial` implement `ReferenceCounted`. This means that a new object should have an initial refcount of 1. An `OpenSslPrivateKey.OpenSslPrivateKeyMaterial` object shares its refcount with the enclosing `OpenSslPrivateKey` object. This means the enclosing object's refcount must be incremented by 1 when an instance of `OpenSslPrivateKey.OpenSslPrivateKeyMaterial` is created. Otherwise, when the key material object is `release()`-ed, the refcount on the enclosing object will drop to 0 while it is still in use.
Modification:
- Increment the refcount in the constructor of `OpenSslPrivateKey.OpenSslPrivateKeyMaterial`
- Ensure we also always release the native certificates as well.
Result:
Refcount is now correct.
Motivation:
While OpenSslPrivateKeyMethod.* should never return null we should still guard against it to prevent any possible segfault.
Modifications:
- Throw SignatureException if null is returned
- Add unit test
Result:
No segfault when user returns null.
Motivation:
We should only try to use OpenSslX509TrustManagerWrapper when using Java 7+ as otherwise it fail to init in it's static block as X509ExtendedTrustManager was only introduced in Java7
Modifications:
Only call OpenSslX509TrustManagerWrapper if we use Java7+
Result:
Fixes https://github.com/netty/netty/issues/9064.
Motivation:
Seems like some analyzer / validation tools scan code to detect if it may produce some security risk because of just blindly accept certificates. Such a tool did tag our code because we have such an implementation (which then is actually never be used). We should just change the impl to not do this as it does not matter for us and it makes such tools happier.
Modifications:
Throw CertificateException
Result:
Fixes https://github.com/netty/netty/issues/9032
Motivation:
IdleStateHandler may trigger unexpected idle events when flushing large entries to slow clients.
Modification:
In netty design, we check the identity hash code and total pending write bytes of the current flush entry to determine whether there is a change in output. But if a large entry has been flushing slowly (for some reason, the network speed is slow, or the client processing speed is too slow to cause the TCP sliding window to be zero), the total pending write bytes size and identity hash code would remain unchanged.
Avoid this issue by adding checks for the current entry flush progress.
Result:
Fixes#8912 .
Motivation:
4079189f6b introduced OpenSslPrivateKeyMethodTest which will only be run when BoringSSL is used. As the assumeTrue(...) also guards the init of the static fields we need to ensure we only try to destroy these if BoringSSL is used as otherwise it will produce a NPE.
Modifications:
Check if BoringSSL is used before trying to destroy the resources.
Result:
No more NPE when BoringSSL is not used.
Motivation:
BoringSSL allows to customize the way how key signing is done an even offload it from the IO thread. We should provide a way to plugin an own implementation when BoringSSL is used.
Modifications:
- Introduce OpenSslPrivateKeyMethod that can be used by the user to implement custom signing by using ReferenceCountedOpenSslContext.setPrivateKeyMethod(...)
- Introduce static methods to OpenSslKeyManagerFactory which allows to create a KeyManagerFactory which supports to do keyless operations by let the use handle everything in OpenSslPrivateKeyMethod.
- Add testcase which verifies that everything works as expected
Result:
A user is able to customize the way how keys are signed.
Motivation:
During OpenSsl.java initialization, a SelfSignedCertificate is created
during the static initialization block to determine if OpenSsl
can be used.
The default key strength for SelfSignedCertificate was too low if FIPS
mode is used and BouncyCastle-FIPS is the only available provider
(necessary for compliance). A simple fix is to just augment the key
strength to the minimum required about by FIPS.
Modification:
Set default key bit length to 2048 but also allow it to be dynamically set via a system property for future proofing to more stricter security compliance.
Result:
Fixes#9018
Signed-off-by: Farid Zakaria <farid.m.zakaria@gmail.com>
Motivation:
Some SslProvider do support different types of keys and chains. We should fail fast if we can not support the type.
Related to https://github.com/netty/netty-tcnative/issues/455.
Modifications:
- Try to parse key / chain first and if if this fails throw and SslException
- Add tests.
Result:
Fail fast.
Motivation:
A callback may already have stored a initial handshake exception in ReferenceCountedOpenSslEngine so we should include it when throwing a SslHandshakeException to ensure the user has all the infos when debugging.
Modifications:
Include initial handshake exception
Result:
Include all erros when throwing the SslHandshakeException.
Motivation:
We do not need to call SSL.setHostNameValidation(...) as it should be done as part of the TrustManager implementation. This is consistent with the JDK implementation of SSLEngine.
Modifications:
Remove call to SSL.setHostNameValidation(...)
Result:
More consistent behaviour between our SSLEngine implementation and the one that comes with the JDK.
Motivation:
We use SSL.setKeyMaterial(...) in our implementation when using the KeyManagerFactory so we should also use it to detect if we can support KeyManagerFactory.
Modifications:
Use SSL.setKeyMaterial(...) as replacement for SSL.setCertificateBio(...)
Result:
Use the same method call to detect if KeyManagerFactory can be supported as we use in the real implementation.
Motivation:
We have multiple places where we store the exception that was produced by a callback in ReferenceCountedOpenSslEngine, and so have a lot of code-duplication.
Modifications:
- Consolidate code into a package-private method that is called from the callbacks if needed
Result:
Less code-duplication and cleaner code.
Motivation:
BoringSSL supports offloading certificate validation to a different thread. This is useful as it may need to do blocking operations and so may block the EventLoop.
Modification:
- Adjust ReferenceCountedOpenSslEngine to correctly handle offloaded certificate validation (just as we already have code for certificate selection).
Result:
Be able to offload certificate validation when using BoringSSL.
Motivation:
BoringSSL supports offloading certificate validation to a different thread. This is useful as it may need to do blocking operations and so may block the EventLoop.
Modification:
- Adjust ReferenceCountedOpenSslEngine to correctly handle offloaded certificate validation (just as we already have code for certificate selection).
Result:
Be able to offload certificate validation when using BoringSSL.
Motivation:
We had a bug which could case ExtendedSSLSession.getPeerSupportedSignatureAlgorithms() return an empty array when using BoringSSL. This testcase verifies we correctly return algorithms after the fix in https://github.com/netty/netty-tcnative/pull/449.
Modifications:
Add testcase to verify behaviour.
Result:
Ensure we correctly retuen the algorithms.
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.
