Motivation:
At the moment the next / prev references are not set to "null" in the DefaultChannelHandlerContext once the ChannelHandler is removed. This is bad as it basically let users still use the ChannelHandlerContext of a ChannelHandler after it is removed and may produce very suprising behaviour.
Modifications:
- Fail if someone tries to use the ChannelHandlerContext once the ChannelHandler was removed (for outbound operations fail the promise, for inbound fire the error through the ChannelPipeline)
- Fix some handlers to ensure we not use the ChannelHandlerContext after the handler was removed
- Adjust DefaultChannelPipeline / DefaultChannelHandlerContext to fixes races with removal / replacement of handlers
Result:
Cleanup behaviour and make it more predictable for pipeline modifications
Motivation:
Often it is useful to be able to detect different sorts of SSL errors that cause the handshake to fail. To make this easier we should throw and explicit exception type for handshake timeouts.
Modifications:
- Add SslHandshakeTimeoutException (which extends SSLHandshakeException) and use it for handshake timeouts
- Adjust testcases
Result:
Easier to detect that handshake failed because of a timeout
Motivation:
We did not correctly account for produced bytes when SSL_write(...) returns -1 in all cases. This could lead to lost data and so a corrupt SSL connection.
Modifications:
- Always ensure we calculate the produced bytes correctly
- Add unit tests
Result:
Fixes https://github.com/netty/netty/issues/10041
Motivation:
In SniHandlerTest we depended on implementation details of the SSLEngine. We should better not doing this
Modifications:
Just release all outbound data
Result:
Dont depend on implementation details
Motivation:
Due incorrectly handling of reference count of the clientHello ByteBuf we may overrelease the buffer. This did show up in the log of a test:
11:55:16.595 [main] DEBUG i.n.h.ssl.SslClientHelloHandler - Unexpected client hello packet: 16030100bd010000b90303a74225676d1814ba57faff3b3663656ed05ee9dbb2a4dbb1bb1c32d2ea5fc39e0000000100008c0000001700150000164348415434e380824c45414e434c4f5544e38082434e000b000403000102000a00340032000e000d0019000b000c00180009000a00160017000800060007001400150004000500120013000100020003000f0010001100230000000d0020001e060106020603050105020503040104020403030103020303020102020203000f00010133740000
io.netty.util.IllegalReferenceCountException: refCnt: 0, decrement: 1
at io.netty.util.internal.ReferenceCountUpdater.toLiveRealRefCnt(ReferenceCountUpdater.java:74)
at io.netty.util.internal.ReferenceCountUpdater.release(ReferenceCountUpdater.java:138)
at io.netty.buffer.AbstractReferenceCountedByteBuf.release(AbstractReferenceCountedByteBuf.java:100)
at io.netty.handler.ssl.SslClientHelloHandler.releaseIfNotNull(SslClientHelloHandler.java:181)
at io.netty.handler.ssl.SslClientHelloHandler.select(SslClientHelloHandler.java:225)
at io.netty.handler.ssl.SslClientHelloHandler.decode(SslClientHelloHandler.java:149)
at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:498)
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:437)
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:276)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:377)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:363)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:355)
at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:377)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:363)
at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919)
at io.netty.channel.embedded.EmbeddedChannel.writeInbound(EmbeddedChannel.java:343)
at io.netty.handler.ssl.SniHandlerTest.testNonAsciiServerNameParsing(SniHandlerTest.java:297)
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.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.junit.internal.runners.statements.ExpectException.evaluate(ExpectException.java:19)
at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
at org.junit.runners.Suite.runChild(Suite.java:128)
at org.junit.runners.Suite.runChild(Suite.java:27)
at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47)
at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)
Modifications:
Correctly transfer lifetime of buffer and so not over-release it.
Result:
Correctly handle buffer lifecycle and so not swallow the original exception
Motivation:
Sometimes it is useful to do something depending on the Ssl ClientHello (like for example select a SslContext to use). At the moment we only allow to hook into the SNI extension but this is not enough.
Modifications:
Add SslClientHelloHandler which allows to hook into ClientHello messages. This class is now also the super class of AbstractSniHandler
Result:
More flexible processing of SSL handshakes
Motivation:
JDK is the default SSL provider and internally uses blocking IO operations.
Modifications:
Add allowBlockingCallsInside configuration for SslHandler runAllDelegate function.
Result:
When BlockHound is installed, SSL works out of the box with the default SSL provider.
Co-authored-by: violetagg <milesg78@gmail.com>
Motivation:
When SslHandler.finishWrap throws an exception, ensure that the promise and buf is not reused to avoid throwing IllegalArgumentException or IllegalReferenceCountException which causes the original exception to be lost.
Modification:
The change ensures that the values for the promise and bytebuf are nulled before calling finishWrap so that it will not be called again with the same arguments.
Result:
Fixes#9971 .
Co-authored-by: Norman Maurer <norman_maurer@apple.com>
Co-authored-by: Antony T Curtis <atcurtis@gmail.com>
Motivation
LoggingHandler is a very useful tool for debugging and for tracking the
sequence of events in a pipeline. LoggingHandler also includes the
functionality to log a hex dump of all written and received ByteBufs.
This can be useful for small messages, but for large messages, this can
potentially result in extremely large logs. E.g., a 1 MB payload will
result in over a 1 MB log message being recorded. While LoggingHandler
may only be intended for debugging, this can still be too excessive in
some debugging scenarios.
Modifications
* Create a new ByteBufFormat enum that allows users to specify "SIMPLE"
or "HEX_DUMP" logging for ByteBufs.
* For all constructors that currently accept a LogLevel parameter,
create new overloaded constructors that also accept this enum as a
parameter.
* Continue to record hex dumps by default.
Result
Users will be able to opt out of full hex dump recording, if they wish
to.
Motivation:
At the moment we create a new ChannelFutureListener per chunk when trying to write these to the underlying transport. This can be optimized by replacing the seperate write and flush call with writeAndFlush and only allocate the listener if the future is not complete yet.
Modifications:
- Replace seperate write and flush calls with writeAndFlush
- Only create listener if needed, otherwise execute directly
Result:
Less allocations
Motivation:
At the moment we use an extra field in ChunedWriteHandler to hold the current write. This is not needed and makes sense even more error-prone. We can just peek in the queue.
Modifications:
Use Queue.peek() to keep track of current write
Result:
Less error-prone code
Motivation:
In next major version of netty users should use ChannelHandler everywhere. We should ensure we do the same
Modifications:
Replace usage of deprecated classes / interfaces with ChannelHandler
Result:
Use non-deprecated code
Motivation:
At the moment resolving addresses during connect is done via setting an AddressResolverGroup on the Bootstrap. While this works most of the times as expected sometimes the user want to trigger the connect() from the Channel itself and not via the Bootstrap. For this cases we should provide a ChannelHandler that the user can use that will do the resolution.
Modifications:
Add ResolveAddressHandler and tests
Result:
Be able to resolve addresses without Bootstrap
Motivation:
41c47b4 introduced a change in an existing testcase which let the build fail when jdkCompatibilityMode is false.
Modifications:
Fix unit tests
Result:
Build passes when jdkCompatibilityMode is false as well
Motivation
A bug was introduced in #9806 which looks likely to be the cause of
#9919. SniHandler will enter an infinite loop if an SSL record is
received with SSL major version byte != 3 (i.e. something other than TLS
or SSL3.0)
Modifications
- Follow default path as intended for majorVersion != 3 in
AbstractSniHandler#decode(...)
- Add unit test to reproduce the hang
Result
Fixes#9919
Motivation:
When `consolidatedWhenNoReadInProgress` is true, `channel.writeAndFlush (data) .addListener (f-> channel.writeAndFlush (data2))` Will cause data2 to never be flushed.
Because the flush operation will synchronously execute the `channel.writeAndFlush (data2))` in the `listener`, and at this time, since the current execution thread is still an `eventloop`(`executor.inEventLoop()` was true), all handlers will be executed synchronously. At this time, since `nextScheduledFlush` is still not null, the `flush` operation of `data2` will be ignored in `FlushConsolidationHandler#scheduleFlush`.
Modification:
- reset `nextScheduledFlush` before `ctx.flush`
- use `ObjectUtil` to polish code
Result:
Fixes https://github.com/netty/netty/issues/9923
Motivation:
ByteToMessageDecoder requires using an intermediate List to put results into. This intermediate list adds overhead (memory/CPU) which grows as the number of objects increases. This overhead can be avoided by directly propagating events through the ChannelPipeline via ctx.fireChannelRead(...). This also makes the semantics more clear and allows us to keep track if we need to call ctx.read() in all cases.
Modifications:
- Remove List from the method signature of ByteToMessageDecoder.decode(...) and decodeLast(...)
- Adjust all sub-classes
- Adjust unit tests
- Fix javadocs.
Result:
Adjust ByteToMessageDecoder as noted in https://github.com/netty/netty/issues/8525.
Motivation:
We should use Objects.requireNonNull(...) as we require java8
Modifications:
Replace ObjectUtil.checkNonNull(...) with Objects.requireNonNull(...)
Result:
Code cleanup
Motivation:
At the moment our AbstractSniHandler makes the assemption that Handshake messages are not fragmented. This is incorrect as it is completely valid to split these across multiple TLSPlaintext records.
Thanks to @sskrobotov for bringing this to my attentation and to @Lukasa for the help.
Modifications:
- Adjust logic in AbstractSniHandler to handle fragmentation
- Add unit tests
Result:
Correctly handle fragmented Handshake message in AbstractSniHandler (and so SniHandler).
Motivation:
This is a PR to solve the problem described here: https://github.com/netty/netty/issues/9767
Basically this PR is to add two more APIs in SslContextBuilder, for users to directly specify
the KeyManager or TrustManager they want to use when building SslContext. This is very helpful
when users want to pass in some customized implementation of KeyManager or TrustManager.
Modification:
This PR takes the first approach in here:
https://github.com/netty/netty/issues/9767#issuecomment-551927994 (comment)
which is to immediately convert the managers into factories and let factories continue to pass
through Netty.
1. Add in SslContextBuilder the two APIs mentioned above
2. Create a KeyManagerFactoryWrapper and a TrustManagerFactoryWrapper, which take a KeyManager
and a TrustManager respectively. These are two simple wrappers that do the conversion from
XXXManager class to XXXManagerFactory class
3.Create a SimpleKeyManagerFactory class(and internally X509KeyManagerWrapper for compatibility),
which hides the unnecessary details such as KeyManagerFactorySpi. This serves the similar
functionalities with SimpleTrustManagerFactory, which was already inside Netty.
Result:
Easier usage.
Motivation:
MINIMAL_WAIT is the key constant. Thus, When we see the constant, we must read more code logic to see if it is ms or ns. So improving java doc will be better.
Modifications:
Improve java doc by add "10ms" such as DEFAULT_CHECK_INTERVAL with "1s".
Result:
Easy to know it is ms and keep same java doc style with other constants such as DEFAULT_CHECK_INTERVAL.
Motivation:
sun.security.ssl.X509KeyManagerImpl will not use "stable" aliases and so aliases may be changed during invocations. This means caching is useless. Because of this we should disable the cache if its used.
Modifications:
- Disable caching if sun.security.ssl.X509KeyManagerImpl is used
- Add tests
Result:
More protection against https://github.com/netty/netty/issues/9747.
Motivation:
At the moment te cache is not bound and so lead to huge memory consumpation. We should ensure its bound by default.
Modifications:
Ensure cache is bound
Result:
Fixes https://github.com/netty/netty/issues/9747.
Motivation
Per javadoc in 4.1.x SimpleChannelInboundHandler:
"Please keep in mind that channelRead0(ChannelHandlerContext, I) will be
renamed to messageReceived(ChannelHandlerContext, I) in 5.0."
Modifications
Rename aforementioned method and all references/overrides.
Result
Method is renamed.
Motivation:
Easier to debug SelfSignedCertificate failures.
Modifications:
Add first throwable as suppressed to thrown exception.
Result:
Less technical debt.
Motivation:
At the moment we directly extend the Recycler base class in our code which makes it hard to experiment with different Object pool implementation. It would be nice to be able to switch from one to another by using a system property in the future. This would also allow to more easily test things like https://github.com/netty/netty/pull/8052.
Modifications:
- Introduce ObjectPool class with static method that we now use internally to obtain an ObjectPool implementation.
- Wrap the Recycler into an ObjectPool and return it for now
Result:
Preparation for different ObjectPool implementations
Motivation:
It is common, especially in frameworks, for the parameters to `SslContextBuilder` methods to be built up as a `List` or similar `Iterable`. It is currently difficult to use `SslContextBuilder` in this case because it requires a conversion to array.
Modification:
Add overloads for methods that accept varargs to also accept `Iterable`, delegating by copying into an array.
Result:
Fixes#9293
Motivation:
Netty should respect JVM flags to control SSL protocols, eg. `-Djdk.tls.client.protocols`
Modification:
Changed `JdkSslContext` to use `SSLContext.getDefaultSSLParameters().getProtocols()` instead of `engine.getSupportedProtocols()` which is hardcoded as `SSLv2Hello, SSLv3, TLSv1, TLSv1.1, TLSv1.2`.
Result:
Without `-Djdk.tls.client.protocols`, `SSLContext.getDefaultSSLParameters().getProtocols()` returns `TLSv1, TLSv1.1, TLSv1.2`.
With `-Djdk.tls.client.protocols=TLSv1.2`, `SSLContext.getDefaultSSLParameters().getProtocols()` returns `TLSv1.2`.
Fixes#9706
Motivation:
In PR https://github.com/netty/netty/pull/9695 IdleStateEvents
were made to cache their string representation. The reason for this
was to avoid creating garbage as these values would be used frequently.
However, these objects may be used on multiple event loops and this
may cause an unexpected race to occur.
Modification:
Only make the events that Netty creates cache their toString representation.
Result:
No races.
Motivation:
We should aim to always use heap buffers when using the JDK SSLEngine for now as it wants to operate on byte[] and so will do internal memory copies if a non heap buffer is used. Beside this it will always return BUFFER_OVERFLOW when a smaller buffer then 16kb is used when calling wrap(...) (even if a very small amount of bytes should be encrypted). This can lead to excercive direct memory usage and pressure for no good reason.
Modifications:
Refactor internals of SslHandler to ensure we use heap buffers for the JDK SSLEngine impelementation
Result:
Less direct memory usage when JDK SSLEngine implementation is used
FlowControllerHandler currently may swell read-complete events in some situations.
* Fire read-complete event from flow controller, when it previously was swallowed
* New unit test to cover this case
Fixes#9667: FlowControllerHandler swallows read-complete event when auto-read is disabled
IdleStateEvent is very convenient and frequently used type of events. However both in runtime (logs) and in debug you need some manual steps to see their actual content. Default implementation generates worthless trash like this:
io.netty.handler.timeout.IdleStateEvent@27f674d
There are examples already, where event has convenient and useful toString implementation:
* io.netty.handler.proxy.ProxyConnectionEvent
* io.netty.handler.ssl.SslCompletionEvent
* Implement 'IdleStateEvent.toString' method.
* Unit test.
More useful String representation of IdleStateEvent
Motivation:
We have a public utility `OpenSsl.isAlpnSupported()` that helps users to
check if ALPN is available for `SslProvider.OPENSSL`. However, we do not
provide a similar utility for `SslProvider.JDK`. Therefore, users who
configured ALPN with `SslProvider.JDK` will get a runtime exception at
the time when a new connection will be created.
Modifications:
- Add public `SslProvider.isAlpnSupported(SslProvider)` utility method
that returns `true` if the `SslProvider` supports ALPN;
- Deprecate `OpenSsl.isAlpnSupported()`;
Result:
Users can verify if their environment supports ALPN with
`SslProvider` upfront (at bootstrap), instead of failing with
runtime exception when a new connection will be created.
Motivation:
Sometimes it is useful to be able to set attributes on a SslContext.
Modifications:
Add new method that will return a AttributeMap that is tied to a SslContext instance
Result:
Fixes https://github.com/netty/netty/issues/6542.
Motivation:
There is not need to use a CAS as everything is synchronized anyway. We can simplify the code a bit by not using it.
Modifications:
- Just remove the CAS operation
- Change from int to boolean
Result:
Code cleanup
Motivation:
Currently when the SslHandler coalesces outbound bytes it always
allocates a direct byte buffer. This does not make sense if the JDK
engine is being used as the bytes will have to be copied back to heap
bytes for the engine to operate on them.
Modifications:
Inspect engine type when coalescing outbound bytes and allocate heap
buffer if heap bytes are preferred by the engine.
Result:
Improved performance for JDK engine. Better performance in environments
without direct buffer pooling.
Motivation:
031c2e2e88 introduced some change to reduce the risk of have the `ReferenceCountedOpenSslContext` be destroyed while the `ReferenceCountedSslEngine` is still in us. Unfortunaly it missed to adjust a few tests which make assumptions about the refCnt of the context.
Modifications:
Adjust tests to take new semenatics into acount.
Result:
No more tests failures
Motivation:
With the Netty ref-counted OpenSSL implementation the parent SslContext
maintains state necessary for the SslEngine's it produces. However, it's
possible for the parent context to be closed and release those resources
before the child engines are finished which causes problems.
Modification:
Spawned ReferenceCountedOpenSslEngine's retain a reference to their
parent ReferenceCountedOpenSslContext.
Result:
The lifetime of the shared data is extended to include the lifetime of
the dependents.
Motiviation:
A lot of reentrancy bugs and cycles can happen because the DefaultPromise will notify the FutureListener directly when completely in the calling Thread if the Thread is the EventExecutor Thread. To reduce the risk of this we should always notify the listeners via the EventExecutor which basically means that we will put a task into the taskqueue of the EventExecutor and pick it up for execution after the setSuccess / setFailure methods complete the promise.
Modifications:
- Always notify via the EventExecutor
- Adjust test to ensure we correctly account for this
- Adjust tests that use the EmbeddedChannel to ensure we execute the scheduled work.
Result:
Reentrancy bugs related to the FutureListeners cant happen anymore.
Motivation:
Users can reuse the same FileChannel for different ChunkedNioFile
instances without being worried that FileChannel::position will be
changed concurrently by them.
In addition, FileChannel::read with absolute position allows to
use on *nix pread that is more efficient then fread.
Modifications:
Always use absolute FileChannel::read ops
Result:
Faster and more flexible uses of FileChannel for ChunkedNioFile
Motivation:
Due some bug we did endup with ClassCastExceptions in some cases. Beside this we also did not correctly handle the case when ReferenceCountedOpenSslEngineTest did produce tasks to run in on test.
Modifications:
- Correctly unwrap the engine before to fix ClassCastExceptions
- Run delegated tasks when needed.
Result:
All tests pass with different OpenSSL implementations (OpenSSL, BoringSSL etc)
Motivation:
SystemPropertyUtil already uses the AccessController internally so not need to wrap its usage with AccessController as well.
Modifications:
Remove explicit AccessController usage when SystemPropertyUtil is used.
Result:
Code cleanup
Motivation:
We did not correctly handle taskoffloading when using BoringSSL / OpenSSL. This could lead to the situation that we did not write the SSL alert out for the remote peer before closing the connection.
Modifications:
- Correctly handle exceptions when we resume processing on the EventLoop after the task was offloadded
- Ensure we call SSL.doHandshake(...) to flush the alert out to the outboundbuffer when an handshake exception was detected
- Correctly signal back the need to call WRAP again when a handshake exception is pending. This will ensure we flush out the alert in all cases.
Result:
No more failures when task offloading is used.
Motivation:
When using io.netty.handler.ssl.openssl.useTasks=true we may call ReferenceCountedOpenSslEngine.setKeyMaterial(...) from another thread and so need to synchronize and also check if the engine was destroyed in the meantime to eliminate of the possibility of a native crash.
The same is try when trying to access the authentication methods.
Modification:
- Add synchronized and isDestroyed() checks where missing
- Add null checks for the case when a callback is executed by another thread after the engine was destroyed already
- Move code for master key extraction to ReferenceCountedOpenSslEngine to ensure there can be no races.
Result:
No native crash possible anymore when using io.netty.handler.ssl.openssl.useTasks=true
Motivation:
At the moment it is not possible to build netty on a power 8 systems.
Modifications:
- Improve detection of the possibility of using Conscrypt
- Skip testsuite-shading when not on x86_64 as this is the only platform for which we build tcnative atm
- Only include classifier if on x86_64 for tcnative as dependency as this is the only platform for which we build tcnative atm
- Better detect if UDT test can be run
Result:
Fixes https://github.com/netty/netty/issues/9479
Motivation:
The java doc doesn't match the real case: The exception only happen when a write operation
cannot finish in a certain period of time instead of write idle happen.
Modification:
Correct java doc
Result:
java doc matched the real case
Motiviation:
EmbeddedChannel currently is quite differently in terms of semantics to other Channel implementations. We should better change it to be more closely aligned and so have the testing code be more robust.
Modifications:
- Change EmbeddedEventLoop.inEventLoop() to only return true if we currenlty run pending / scheduled tasks
- Change EmbeddedEventLoop.execute(...) to automatically process pending tasks if not already doing so
- Adjust a few tests for the new semantics (which is closer to other Channel implementations)
Result:
EmbeddedChannel works more like other Channel implementations
Motivation:
There are some extra log level checks (logger.isWarnEnabled()).
Modification:
Remove log level checks (logger.isWarnEnabled()) from io.netty.channel.epoll.AbstractEpollStreamChannel, io.netty.channel.DefaultFileRegion, io.netty.channel.nio.AbstractNioChannel, io.netty.util.HashedWheelTimer, io.netty.handler.stream.ChunkedWriteHandler and io.netty.channel.udt.nio.NioUdtMessageConnectorChannel
Result:
Fixes#9456
Motivation:
The Netty classes are initialized at build time by default for GraalVM Native Image compilation. This is configured via the `--initialize-at-build-time=io.netty` option. While this reduces start-up time it can lead to some problems:
- The class initializer of `io.netty.buffer.PooledByteBufAllocator` looks at the maximum memory size to compute the size of internal buffers. If the class initializer runs during image generation, then the buffers are sized according to the very large heap size that the image generator uses, and Netty allocates several arrays that are 16 MByte. The fix is to initialize the following 3 classes at run time: `io.netty.buffer.PooledByteBufAllocator,io.netty.buffer.ByteBufAllocator,io.netty.buffer.ByteBufUtil`. This fix was dependent on a GraalVM Native Image fix that was included in 19.2.0.
- The class initializer of `io.netty.handler.ssl.util.ThreadLocalInsecureRandom` needs to be initialized at runtime to ensure that the generated values are trully random and not fixed for each generated image.
- The class initializers of `io.netty.buffer.AbstractReferenceCountedByteBuf` and `io.netty.util.AbstractReferenceCounted` compute field offsets. While the field offset recomputation is necessary for correct execution as a native image these initializers also have logic that depends on the presence/absence of `sun.misc.Unsafe`, e.g., via the `-Dio.netty.noUnsafe=true` flag. The fix is to push these initializers to runtime so that the field offset lookups (and the logic depending on them) run at run time. This way no manual substitutions are necessary either.
Modifications:
Add `META-INF/native-image` configuration files that correctly trigger the inialization of the above classes at run time via `--initialize-at-run-time=...` flags.
Result:
Fixes the initialisation issues described above for Netty executables built with GraalVM.
Motivation:
14607979f6 added tests for using ACCP but did miss to use the same unwrapping technique of exceptions as JdkSslEngineTest. This can lead to test-failures on specific JDK versions
Modifications:
Add the same unwrapping code
Result:
No more test failures
Motivation:
Amazon lately released Amazon Corretto Crypto Provider, so we should include it in our testsuite
Modifications:
Add tests related to Amazon Corretto Crypto Provider
Result:
Test netty with Amazon Corretto Crypto Provider
Motivation:
If all we need is the FileChannel we should better use RandomAccessFile as FileInputStream and FileOutputStream use a finalizer.
Modifications:
Replace FileInputStream and FileOutputStream with RandomAccessFile when possible.
Result:
Fixes https://github.com/netty/netty/issues/8078.
Motivation:
When using OpenSSL and JDK < 11 is used we need to wrap the user provided X509ExtendedTrustManager to be able to support TLS1.3. We had a check in place that first tried to see if wrapping is needed at all which could lead to missleading calls of the user provided trustmanager. We should remove these calls and just always wrap if needed.
Modifications:
Always wrap if OpenSSL + JDK < 11 and TLS1.3 is supported
Result:
Less missleading calls to user provided trustmanager
Motivation:
Users' runtime systems may have incompatible dynamic libraries to the ones our
tcnative wrappers link to. Unfortunately, we cannot determine and catch these
scenarios (in which the JVM crashes) but we can make a more educated guess on
what library to load and try to find one that works better before crashing.
Modifications:
1) Build dynamically linked openSSL builds for more OSs (netty-tcnative)
2) Load native linux libraries with matching classifier (first)
Result:
More developers / users can use the dynamically-linked native libraries.
Motivation
Debugging SSL/TLS connections through wireshark is a pain -- if the cipher used involves Diffie-Hellman then it is essentially impossible unless you can have the client dump out the master key [1]
This is a work-in-progress change (tests & comments to come!) that introduces a new handler you can set on the SslContext to receive the master key & session id. I'm hoping to get feedback if a change in this vein would be welcomed.
An implementation that conforms to Wireshark's NSS key log[2] file is also included.
Depending on feedback on the PR going forward I am planning to "clean it up" by adding documentation, example server & tests. Implementation will need to be finished as well for retrieving the master key from the OpenSSL context.
[1] https://jimshaver.net/2015/02/11/decrypting-tls-browser-traffic-with-wireshark-the-easy-way/
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/Key_Log_Format
Modification
- Added SslMasterKeyHandler
- An implementation of the handler that conforms to Wireshark's key log format is included.
Result:
Be able to debug SSL / TLS connections more easily.
Signed-off-by: Farid Zakaria <farid.m.zakaria@gmail.com>
Motivation:
Netty homepage(netty.io) serves both "http" and "https".
It's recommended to use https than http.
Modification:
I changed from "http://netty.io" to "https://netty.io"
Result:
No effects.
Motivation:
2c99fc0f12 introduced a change that eagly recycles the queue. Unfortunally it did not correct protect against re-entrance which can cause a NPE.
Modifications:
- Correctly protect against re-entrance by adding null checks
- Add unit test
Result:
Fixes https://github.com/netty/netty/issues/9319.
Motivation:
The AbstractSniHandler previously was willing to tolerate up to three
non-handshake records before a ClientHello that contained an SNI
extension field. This is, so far as I can tell, completely
unnecessary: no TLS implementation will be sending alerts or change
cipher spec messages before ClientHello.
Given that it was not possible to determine why this loop is in
the code to begin with, it's probably just best to remove it.
Modifications:
Remove the for loop.
Result:
The AbstractSniHandler will more rapidly determine whether it should
pass the records on to the default SSL handler.
Co-authored-by: Norman Maurer <norman_maurer@apple.com>
Motivation:
I've introduced netty/netty-tcnative#421 that introduced exposing OpenSSL master key & client/server
random values with the purpose of allowing someone to log them to debug the traffic via auxiliary tools like Wireshark (see also #8653)
Modification:
Augmented OpenSslEngineTest to include a test which manually decrypts the TLS ciphertext
after exposing the masterkey + client/server random. This acts as proof that the tc-native new methods work correctly!
Result:
More tests
Signed-off-by: Farid Zakaria <farid.m.zakaria@gmail.com>
Motivation:
Some methods that either override others or are implemented as part of implementation an interface did miss the `@Override` annotation
Modifications:
Add missing `@Override`s
Result:
Code cleanup
Motivation:
asList should only be used if there are multiple elements.
Modification:
Call to asList with only one argument could be replaced with singletonList
Result:
Cleaner code and a bit of memory savings
Motivation:
FlowControlHandler does use a recyclable ArrayDeque internally but only recycles it when the channel is closed. We should better recycle it once it is empty.
Modifications:
Recycle the deque as fast as possible
Result:
Less RecyclableArrayDeque instances.
Motivation:
Traffic shaping needs more accurate execution than scheduled one. So the
use of FixedRate instead.
Moreover the current implementation tends to create as many threads as
channels use a ChannelTrafficShapingHandlern, which is unnecessary.
Modifications:
Change the executor.schedule to executor.scheduleAtFixedRate in the
start and remove the reschedule call from run monitor thread since it
will be restarted by the Fixed rate executor.
Also fix a minor bug where restart was only doing start() without stop()
before.
Result:
Threads are more stable in number of cached and precision of traffic
shaping is enhanced.
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:
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:
Usafe of io.netty.handler.ssl.openssl.useKeyManagerFactory system property was deprecated in 4.1 so let us remove it.
Modifications:
Remove io.netty.handler.ssl.openssl.useKeyManagerFactory usage.
Result:
Remove support of deprecated system property
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:
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:
We should not throw check exceptions when the user calls sync*() but should better wrap it in a CompletionException to make it easier for people to reason about what happens.
Modifications:
- Change sync*() to throw CompletionException
- Adjust tests
- Add some more tests
Result:
Fixes https://github.com/netty/netty/issues/8521.
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:
DefaultPromise requires an EventExecutor which provides the thread to notify listeners on and this EventExecutor can never change. We can remove the code that supported the possibility of a changing the executor as this is not possible anymore.
Modifications:
- Remove constructor which allowed to construct a *Promise without an EventExecutor
- Remove extra state
- Adjusted SslHandler and ProxyHandler for new code
Result:
Fixes https://github.com/netty/netty/issues/8517.
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:
In 42742e233f we already added default methods to Channel*Handler and deprecated the Adapter classes to simplify the class hierarchy. With this change we go even further and merge everything into just ChannelHandler. This simplifies things even more in terms of class-hierarchy.
Modifications:
- Merge ChannelInboundHandler | ChannelOutboundHandler into ChannelHandler
- Adjust code to just use ChannelHandler
- Deprecate old interfaces.
Result:
Cleaner and simpler code in terms of class-hierarchy.
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:
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:
As we now us java8 as minimum java version we can deprecate ChannelInboundHandlerAdapter / ChannelOutboundHandlerAdapter and just move the default implementations into the interfaces. This makes things a bit more flexible for the end-user and also simplifies the class-hierarchy.
Modifications:
- Mark ChannelInboundHandlerAdapter and ChannelOutboundHandlerAdapter as deprecated
- Add default implementations to ChannelInboundHandler / ChannelOutboundHandler
- Refactor our code to not use ChannelInboundHandlerAdapter / ChannelOutboundHandlerAdapter anymore
Result:
Cleanup class-hierarchy and make things a bit more flexible.
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.
