__Motivation__
`LoggingHandler` misses a constructor variant that only takes `ByteBufFormat`
__Modification__
Added the missing constructor variant.
__Result__
`LoggingHandler` can be constructed with `ByteBufFormat` only.
Co-authored-by: Nitesh Kant <nitesh_kant@apple.com>
Motivation:
We need to ensure we always "consumed" all alerts etc via SSLEngine.wrap(...) before we teardown the engine. Failing to do so may lead to a situation where the remote peer will not be able to see the actual cause of the handshake failure but just see the connection being closed.
Modifications:
Correctly return HandshakeStatus.NEED_WRAP when we need to wrap some data first before we shutdown the engine because of a handshake failure.
Result:
Fixes https://github.com/netty/netty/issues/11388
Motivation:
At the moment BoringSSL doesnt support explicit set the TLSv1.3 ciphers that should be used. If TLSv1.3 should be used it just enables all ciphers. We should better log if the user tries to explicit set a specific ciphers and using BoringSSL to inform the user that what is tried doesnt really work.
Modifications:
Log if the user tries to not use all TLSv1.3 ciphers and use BoringSSL
Result:
Easier for the user to understand why always all TLSv1.3 ciphers are enabled when using BoringSSL
Co-authored-by: Trustin Lee <trustin@gmail.com>
Motivation:
In this issue(https://github.com/netty/netty/issues/11349 ),IpSubnetFilterRule needs to support ipv6 reserved addresses, such as 8000::, but the current implementation does not support
Modification:
Added support for default rule
Result:
Fixes https://github.com/netty/netty/issues/11349
Signed-off-by: xingrufei <xingrufei@sogou-inc.com>
Motivation:
We should have a default case in every switch block.
Modification:
Add default block in IdleStateHandler
Result:
Cleanup
Signed-off-by: xingrufei <xingrufei@sogou-inc.com>
Motivation:
We should better fail the build if we can't load the OpenSSL library to ensure we not introduce a regression at some point related to native library loading
Modifications:
Remove usages of assumeTrue and let the tests fail if we cant load the native lib
Result:
Ensure we not regress
Motivation:
`SelfSignedCertificate` creates a certificate and private key files and store them in a temporary directory. However, if the certificate uses a wildcard hostname that uses asterisk *, e.g. `*.shieldblaze.com`, it'll throw an error because * is not a valid character in the file system.
Modification:
Replace the asterisk with 'x'
Result:
Fixes#11240
Motivation:
We introduced the ability to offload certain operations to an executor that may take some time to complete. At the moment this is not enabled by default when using the openssl based SSL provider. Let's enable it by default as we have this support for some while now and didnt see any issues yet. This will also make things less confusing and more consistent with the JDK based provider.
Modifications:
Use true as default value for io.netty.handler.ssl.openssl.useTasks.
Result:
Offloading works with openssl based SSL provider as well by default
Motivation:
TLSv1 and TLSv1.1 is considered insecure. Let's follow the JDK and disable these by default
Modifications:
- Disable TLSv1 and TLSv1.1 by default when using OpenSSL.
- Add unit tests
Result:
Use only strong TLS versions by default when using OpenSSL
Motivation:
Conscrypt not correctly filters out non support TLS versions which may lead to test failures.
Related to https://github.com/google/conscrypt/issues/1013
Modifications:
- Bump up to latest patch release
- Add workaround
Result:
No more test failures caused by conscrypt
Motivation:
`PlatformDependent#normalizedOs()` already caches normalized variant of
the value of `os.name` system property. Instead of inconsistently
normalizing it in every case, use the utility method.
Modifications:
- `PlatformDependent`: `isWindows0()` and `isOsx0()` use `NORMALIZED_OS`;
- `PlatformDependent#normalizeOs(String)` define `darwin` as `osx`;
- `OpenSsl#loadTcNative()` does not require `equalsIgnoreCase` bcz `os`
is already normalized;
- Epoll and KQueue: `Native#loadNativeLibrary()` use `normalizedOs()`;
- Use consistent `Locale.US` for lower case conversion of `os.name`;
- `MacOSDnsServerAddressStreamProvider#loadNativeLibrary()` uses
`PlatformDependent.isOsx()`;
Result:
Consistent approach for `os.name` parsing.
Motivation:
We should add explicit null checks so its easier for people to understand why it throws.
Modification:
Add explicit checkNotNull(...)
Result:
Easier to understand for users why it fails.
Signed-off-by: xingrufei <xingrufei@sogou-inc.com>
Co-authored-by: xingrufei <xingrufei@sogou-inc.com>
Motivation:
We've seen (very rare) flaky test failures due to timeouts.
They are too rare to analyse properly, but a theory is that on overloaded, small cloud CI instances, it can sometimes take a surprising amount of time to start a thread.
It could be that the event loop thread is getting an unlucky schedule, and takes seconds to start, causing the timeouts to elapse.
Modification:
Increase the initial timeouts in the SSLEngineTest, that could end up waiting for the event loop thread to start.
Also fix a few simple warnings from Intellij.
Result:
Hopefully we will not see these tests be flaky again.
Motivation:
ReferenceCountedOpenSslEngine may unwrap data and complete the handshake
in a single unwrap() call. However it may return HanshakeStatus of
HandshakeStatus of NEED_UNWRAP instead of FINISHED. This may result in
the SslHandler sending the unwrapped data up the pipeline before
notifying that the handshake has completed, and result in out-of-order
events.
Modifications:
- if ReferenceCountedOpenSslEngine handshake status is NEED_UNWRAP and
produced data, or NEED_WRAP and consumed some data, we should call
handshake() to get the current state.
Result:
ReferenceCountedOpenSslEngine correctly indicates when the handshake has
finished if at the same time data was produced or consumed.
Motivation:
It turned out we didnt run the openssl tests on the CI when we used the non-static version of netty-tcnative.
Modifications:
- Upgrade netty-tcnative to fix segfault when using shared openssl
- Adjust tests to only run session cache tests when openssl supports it
- Fix some more tests to only depend on KeyManager if the underlying openssl version supports it
Result:
Run all openssl test on the CI even when shared library is used
Motivation:
In the latest version of BouncyCastle, BCJSSE:'TLSv1.3' is now a supported protocol for both client and server. So should consider enabling TLSv1.3 when TLSv1.3 is available
Modification:
This pr is to enable TLSv1.3 when using BouncyCastle ALPN support, please review this pr,thanks
Result:
Enable TLSv1.3 when using BouncyCastle ALPN support
Signed-off-by: xingrufei <xingrufei@sogou-inc.com>
Co-authored-by: xingrufei <xingrufei@sogou-inc.com>
Motivation:
NullChecks resulting in a NullPointerException or IllegalArgumentException, numeric ranges (>0, >=0) checks, not empty strings/arrays checks must never be anonymous but with the parameter or variable name which is checked. They must be specific and should not be done with an "OR-Logic" (if a == null || b == null) throw new NullPointerEx.
Modifications:
* import static relevant checks
* Replace manual checks with ObjectUtil methods
Result:
All checks needed are done with ObjectUtil, some exception texts are improved.
Fixes#11170
Motivation:
Under Android it was not possible to load a specific web page. It might be related to the (missing?) ALPN of the internal TLS implementation. BouncyCastle as a replacement works but this was not supported so far by Netty.
BouncyCastle also has the benefit to be a pure Java solution, all the other providers (OpenSSL, Conscrypt) require native libraries which are not available under Android at least.
Modification:
BouncyCastleAlpnSslEngine.java and support classes have been added. It is relying on the JDK code, hence some support classes had to be opened to prevent code duplication.
Result:
BouncyCastle can be used as TLS provider.
Co-authored-by: Norman Maurer <norman_maurer@apple.com>
Motivation:
We are increasingly running in environments where Unsafe, setAccessible, etc. are not available.
When debug logging is enabled, we log a complete stack trace every time one of these initialisations fail.
Seeing these stack traces can cause people unnecessary concern.
For instance, people might have alerts that are triggered by a stack trace showing up in logs, regardless of its log level.
Modification:
We continue to print debug log messages on the result of our initialisations, but now we only include the full stack trace is _trace_ logging (or FINEST, or equivalent in whatever logging framework is configured) is enabled.
Result:
We now only log these initialisation stack traces when the lowest possible log level is enabled.
Fixes#7817
Motivation:
SslHandler invokes channel.read() during the handshake process. For some
channel implementations (e.g. LocalChannel) this may result in re-entry
conditions into unwrap. Unwrap currently defers updating the input
buffer indexes until the unwrap method returns to avoid intermediate
updates if not necessary, but this may result in unwrapping the same
contents multiple times which leads to handshake failures [1][2].
[1] ssl3_get_record:decryption failed or bad record mac
[2] ssl3_read_bytes:sslv3 alert bad record mac
Modifications:
- SslHandler#unwrap updates buffer indexes on each iteration so that if
reentry scenario happens the correct indexes will be visible.
Result:
Fixes https://github.com/netty/netty/issues/11146
Motivation:
SslHandler has many independent boolean member variables. They can be
collapsed into a single variable to save memory.
Modifications:
- SslHandler boolean state consolidated into a single short variable.
Result:
Savings of 8 bytes per SslHandler (which is per connection) observed on
OpenJDK.
Motivation:
`SslHandler#unwrap` may produce `SslHandshakeCompletionEvent` if it
receives `close_notify` alert. This alert indicates that the engine is
closed and no more data are expected in the pipeline. However, it fires
the event before the last data chunk. As the result, further handlers
may loose data if they handle `SslHandshakeCompletionEvent`.
This issue was not visible before #11133 because we did not write
`close_notify` alert reliably.
Modifications:
- Add tests to reproduce described behavior;
- Move `notifyClosePromise` after fire of the last `decodeOut`;
Result:
`SslHandshakeCompletionEvent` correctly indicates that the engine is
closed and no more data are expected on the pipeline.
Motivation:
We should avoid blocking in the event loop as much as possible.
The InputStream.read() is a blocking method, and we don't need to call it if available() returns a positive number.
Modification:
Bypass calling InputStream.read() if available() returns a positive number.
Result:
Fewer blocking calls in the event loop, in general, when ChunkedStream is used.
Motivation:
SslHandler's wrap method notifies the handshakeFuture and sends a
SslHandshakeCompletionEvent user event down the pipeline before writing
the plaintext that has just been wrapped. It is possible the application
may write as a result of these events and re-enter into wrap to write
more data. This will result in out of sequence data and result in alerts
such as SSLV3_ALERT_BAD_RECORD_MAC.
Modifications:
- SslHandler wrap should write any pending data before notifying
promises, generating user events, or anything else that may create a
re-entry scenario.
Result:
SslHandler will wrap/write data in the same order.
Motivation:
SslHandler owns the responsibility to flush non-application data
(e.g. handshake, renegotiation, etc.) to the socket. However when
TCP Fast Open is supported but the client_hello cannot be written
in the SYN the client_hello may not always be flushed. SslHandler
may not wrap/flush previously written/flushed data in the event
it was not able to be wrapped due to NEED_UNWRAP state being
encountered in wrap (e.g. peer initiated renegotiation).
Modifications:
- SslHandler to flush in channelActive() if TFO is enabled and
the client_hello cannot be written in the SYN.
- SslHandler to wrap application data after non-application data
wrap and handshake status is FINISHED.
- SocketSslEchoTest only flushes when writes are done, and waits
for the handshake to complete before writing.
Result:
SslHandler flushes handshake data for TFO, and previously flushed
application data after peer initiated renegotiation finishes.
Motivation:
At the moment we don't support session caching on the client side at all when using the native SSL implementation. We should at least allow to enable it.
Modification:
Allow to enable session cache for client side but disable ti by default due a JDK bug atm.
Result:
Be able to cache sessions on the client side when using native SSL implementation .
Motivation:
In WriteTimeoutHandler we did make the assumption that the executor which is used to schedule the timeout is the same that is backing the write promise. This may not be true which will cause concurrency issues
Modifications:
Ensure we are on the right thread when try to modify the doubly-linked-list and if not schedule it on the right thread.
Result:
Fixes https://github.com/netty/netty/issues/11053
Motivation:
We need to ensure that we call queue.remove() before we cal writeAndFlush() as this operation may cause an event that also touches the queue and remove from it. If we miss to do so we may see NoSuchElementExceptions.
Modifications:
- Call queue.remove() before calling writeAndFlush(...)
- Add unit test
Result:
Fixes https://github.com/netty/netty/issues/11046
Motivation:
The `!fastOpen` part of `active || !fastOpen` is always false.
Modification:
- Remove `!fastOpen` and keep only `active` as a `flushAtEnd` flag for
`startHandshakeProcessing`;
- Update comment;
Result:
Simplified `flushAtEnd` flag computation in `SslHandler#handlerAdded`.
Support TCP Fast Open for clients and make SslHandler take advantage
Motivation:
- TCP Fast Open allow us to send a small amount of data along side the initial SYN packet when establishing a TCP connection.
- The TLS Client Hello packet is small enough to fit in there, and is also idempotent (another requirement for using TCP Fast Open), so if we can save a round-trip when establishing TLS connections when using TFO.
Modification:
- Add support for client-side TCP Fast Open for Epoll, and also lowers the Linux kernel version requirements to 3.6.
- When adding the SslHandler to a pipeline, if TCP Fast Open is enabled for the channel (and the channel is not already active) then start the handshake early by writing it to the outbound buffer.
- An important detail to note here, is that the outbound buffer is not flushed at this point, like it would for normal handshakes. The flushing happens later as part of establishing the TCP connection.
Result:
- It is now possible for clients (on epoll) to open connections with TCP Fast Open.
- The SslHandler automatically detects when this is the case, and now send its Client Hello message as part of the initial data in the TCP Fast Open flow when available, saving a round-trip when establishing TLS connections.
Co-authored-by: Colin Godsey <crgodsey@gmail.com>
Motivation:
The testGlobalWriteThrottle is flaky and failed our build multiple times now. Lets disable it for now until we had time to investigate
Modifications:
Disable flaky test
Result:
Less failures during build
Motivation:
At the moment we always set SSL_OP_NO_TICKET when building our context. The problem with this is that this also disables resumption for TLSv1.3 in BoringSSL as it only supports stateless resumption for TLSv1.3 which uses tickets.
We should better clear this option when TLSv1.3 is enabled to be able to resume sessions. This is also inline with the OpenJDK which enables this for TLSv1.3 by default as well.
Modifications:
Check for enabled protocols and if TLSv1.3 is set clear SSL_OP_NO_TICKET.
Result:
Be able to resume sessions for TLSv1.3 when using BoringSSL.
