Motivation:
SslHandlerTest tried to get access to the SslHandler in the pipeline via pipeline.get(...) which may return null if the channel was already closed and so the pipeline was teared down.
This showed up in a test run as:
```
-------------------------------------------------------------------------------
Test set: io.netty.handler.ssl.SslHandlerTest
-------------------------------------------------------------------------------
Tests run: 17, Failures: 0, Errors: 1, Skipped: 1, Time elapsed: 0.802 sec <<< FAILURE! - in io.netty.handler.ssl.SslHandlerTest
testCloseOnHandshakeFailure(io.netty.handler.ssl.SslHandlerTest) Time elapsed: 0.188 sec <<< ERROR!
java.lang.NullPointerException
at io.netty.handler.ssl.SslHandlerTest.testCloseOnHandshakeFailure(SslHandlerTest.java:640)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:564)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:298)
at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:292)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
at java.base/java.lang.Thread.run(Thread.java:844)
```
Modifications:
Use an AtomicReference to propagate the SslHandler instance to the outer scope.
Result:
No more NPE.
Motivation:
We currently have only interopt tests for Conscrypt, we should also have non-interopt tests.
Modifications:
Add ConscryptSslEngineTest
Result:
More tests
Motivation:
We previously did not correctly take into account when we could not wrap (and so produce) the full SSL record with an alert when the SSLEngine was closed.
There are two problems here:
- If we call wrap(...) with an empty dst buffer after closeOutbound() was called we will not notify the user if we could not store the whole SSLRecord into the dst buffer and so we may produce incomplete SSLRecords
Modifications:
Add unit test which failed before.
Result:
Correctly handle the case when the dst buffer is not big enough and and alert needs to be produced.
Motivation:
https://github.com/netty/netty/pull/7943 had a bug which caused to not have the argument passed to the delegating method.
Modifications:
Add argument to release call.
Result:
Correctly delegate method.
Motivation:
Some of the tests failed when using BoringSSL as some protocol / cipher combinations are not supported and it uses a different alert when the cert is not valid yet.
Modification:
- Remove protocol / cipher combos that are not supported by BoringSSL
- Test for different alert when using BoringSSL
Result:
Not test failures when using BoringSSL.
Motivation:
https://github.com/netty/netty/pull/7941 proved that its easy to not correctly clear the error stack sometimes. We should do carefully test this.
Modifications:
Add a new SSLEngine wrapper that is used during tests, which verifies that the error stack is empty after each method call.
Result:
Better testing.
Motivation:
We sometimes did not correctly detect when a protocol is not enabled when using netty-tcnative as we did not take into account when the option flag is 0 (as for example in BoringSSL for SSLv2).
Modifications:
- Correctly take an option flag with 0 into account.
- Add unit tests.
Result:
Fixes https://github.com/netty/netty/issues/7935.
Motivation:
We missed to correctly clear the error stack in one case when using the ReferenceCountedOpenSslEngine. Because of this it was possible to pick up an error on an unrelated operation.
Modifications:
- Correctly clear the stack
- Add verification of empty error stack to tests.
Result:
Not possible to observe unrelated errors.
Motivation:
We had a bug-report that claimed the src buffer used by OpenSslEngine will be zero out.
Modifications:
Add testcase to ensure correct behaviour
Result:
Testcase for https://github.com/netty/netty/issues/7753
Motivation:
LibreSSL removed support for NPN in its 2.6.1+ releases.
Modifications:
Skip NPN tests in libressl 2.6.1+
Result:
Be able to run netty tests against libressl 2.6.1+ as well.
Motivation:
When SSL handshake fails, the connection should be closed. This is not true anymore after 978a46c.
Modifications:
- Ensure we always flush and close the channel on handshake failure.
- Add testcase.
Result:
Fixes [#7724].
Motivation:
Sometimes, it would be convenient to be able to easily enable all
supported cipher suites, regardless of security.
Currently, the only way it to retrieve all supported ciphers and pass
them explicitly.
Modification:
Introduce a new IdentityCipherSuiteFilter singleton that defaults to
supportedCiphers instead of defaultCiphers when ciphers are null.
Result:
Convenient way to enabled all supported cipher suites.
Motivation:
JdkSslContext builds the list of supported cipher suites, but assumes that ciphers prefixed with SSL_ and TLS_ will be interchangeable. However this is not the case and only applies to a small subset of ciphers. This results in the JdkSslContext attempting to use unsupported ciphers.
Modifications:
- When building the list of ciphers in JdkSslContext we should first check if the engine supports the TLS_ prefix cipher.
Result:
Fixes https://github.com/netty/netty/issues/7673
Motivation:
SslHandler#decode methods catch any exceptions and attempt to wrap
before shutting down the engine. The intention is to write any alerts
which the engine may have pending. However the wrap process may also
attempt to write user data, and may also complete the associated
promises. If this is the case, and a promise listener closes the channel
then SslHandler may later propagate a SslHandshakeCompletionEvent user
event through the pipeline. Since the channel has already been closed
the user may no longer be paying attention to user events.
Modifications:
- Sslhandler#decode should first fail the associated handshake promise
and propagate the SslHandshakeCompletionEvent before attempting to wrap
Result:
Fixes https://github.com/netty/netty/issues/7639
Motivation:
We recently removed support for renegotiation, but there are still some hooks to attempt to allow remote initiated renegotiation to succeed. The remote initated renegotiation can be even more problematic from a security stand point and should also be removed.
Modifications:
- Remove state related to remote iniated renegotiation from OpenSslEngine
Result:
More renegotiation code removed from the OpenSslEngine code path.
Motivation:
SSL.setState() has gone from openssl 1.1. Calling it is, and probably
always has been, incorrect. Doing renogitation in this manner is
potentially insecure. There have been at least two insecure
renegotiation vulnerabilities in users of the OpenSSL library.
Renegotiation is not necessary for correct operation of the TLS protocol.
BoringSSL has already eliminated this functionality, and the tests
(now deleted) were not running on BoringSSL.
Modifications:
If the connection setup has completed, always return that
negotiation is not supported. Previously this was done only if we were
the client.
Remove the tests for this functionality.
Fixes#6320.
Motivation:
We tried to call `select` after we closed the channel (and so removed all the handlers from the pipeline) when we detected a non SSL record. This would cause an exception like this:
```
Caused by: java.util.NoSuchElementException: io.netty.handler.ssl.SniHandler
at io.netty.channel.DefaultChannelPipeline.getContextOrDie(DefaultChannelPipeline.java:1098)
at io.netty.channel.DefaultChannelPipeline.replace(DefaultChannelPipeline.java:506)
at io.netty.handler.ssl.SniHandler.replaceHandler(SniHandler.java:133)
at io.netty.handler.ssl.SniHandler.onLookupComplete(SniHandler.java:113)
at io.netty.handler.ssl.AbstractSniHandler.select(AbstractSniHandler.java:225)
at io.netty.handler.ssl.AbstractSniHandler.decode(AbstractSniHandler.java:218)
at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:489)
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:428)
... 40 more
```
Modifications:
- Ensure we rethrow the NotSslRecordException when detecting it (and closing the channel). This will also ensure we not call `select(...)`
- Not catch `Throwable` but only `Exception`
- Add test case.
Result:
Correctly handle the case of an non SSL record.
* FIX: force a read operation for peer instead of self
Motivation:
When A is in `writeInProgress` and call self close, A should
`finishPeerRead` for B(A' peer).
Modifications:
Call `finishPeerRead` with peer in `LocalChannel#doClose`
Result:
Clear confuse of code logic
* FIX: preserves order of close after write in same event loop
Motivation:
If client and server(client's peer channel) are in same event loop, client writes data to
server in `ChannelActive`. Server receives the data and write it
back. The client's read can't be triggered becasue client's
`ChannelActive` is not finished at this point and its `readInProgress`
is false. Then server closes itself, it will also close the client's
channel. And client has no chance to receive the data.
Modifications:
1. Add a test case to demonstrate the problem
2. When `doClose` peer, we always call
`peer.eventLoop().execute()` and `registerInProgress` is not needed.
3. Remove test case
`testClosePeerInWritePromiseCompleteSameEventLoopPreservesOrder`. This
test case can't pass becasue of this commit. IMHO, I think it is OK,
becasue it is reasonable that the client flushes the data to socket,
then server close the channel without received the data.
4. For mismatch test in SniClientTest, the client should receive server's alert before closed(caused by server's close)
Result:
The problem is gone.
Motivation:
At the moment its a bit "hacky" to retrieve the hostname that was used during SNI as you need to hold a reference to SniHandler and then call hostname() once the selection is done. It would be better to fire an event to let the user know we did the selection.
Modifications:
Add a SniCompletionEvent that can be used to get the hostname that was used to do the selection and was included in the SNI extension.
Result:
Easier usage of SNI.
Motivation:
The default enabled cipher suites of the OpenSsl engine are not set to
SslUtils#DEFAULT_CIPHER_SUITES. Instead all available cipher suites are
enabled. This should happen only as a fallback.
Modifications:
Moved the line in the static initializer in OpenSsl which adds the
SslUtils#DEFAULT_CIPHER_SUITES to the default enabled cipher suites up
before the fallback.
Result:
The default enabled cipher suites of the OpenSsl engine are set to the
available ones of the SslUtils#DEFAULT_CIPHER_SUITES.
The default enabled cipher suites of the OpenSsl engine are only set to
all available cipher suites if no one of the
SslUtils#DEFAULT_CIPHER_SUITES is supported.
Motivation:
We should not fire a SslHandshakeEvent if the channel is closed but the handshake was not started.
Modifications:
- Add a variable to SslHandler which tracks if an handshake was started yet or not and depending on this fire the event.
- Add a unit test
Result:
Fixes [#7262].
Motivation:
At the moment use loops to run SslHandler tests with different SslProviders which is error-prone and also make it difficult to understand with which provider these failed.
Modifications:
- Move unit tests that should run with multiple SslProviders to extra class.
- Use junit Parameterized to run with different SslProvider combinations
Result:
Easier to understand which SslProvider produced test failures
complete
Motivation:
SslHandler removes a Buffer/Promise pair from
AbstractCoalescingBufferQueue when wrapping data. However it is possible
the SSLEngine will not consume the entire buffer. In this case
SslHandler adds the Buffer back to the queue, but doesn't add the
Promise back to the queue. This may result in the promise completing
immediately in finishFlush, and generally not correlating to the
completion of writing the corresponding Buffer
Modifications:
- AbstractCoalescingBufferQueue#addFirst should also support adding the
ChannelPromise
- In the event of a handshake timeout we should immediately fail pending
writes immediately to get a more accurate exception
Result:
Fixes https://github.com/netty/netty/issues/7378.
Motivation:
SslHandler only supports ByteBuf objects, but will not release objects of other types. SslHandler will also not release objects if its internal state is not correctly setup.
Modifications:
- Release non-ByteBuf objects in write
- Release all objects if the SslHandler queue is not setup
Result:
Less leaks in SslHandler.
Motivation:
Bug in capacity calculation: occurs auto convert to string instead of sum up.
Modifications:
Use `eventName.length()` in sum.
Result:
Less trash in logs.
Motivation:
We should also enforce the handshake timeout on the server-side to allow closing connections which will not finish the handshake in an expected amount of time.
Modifications:
- Enforce the timeout on the server and client side
- Add unit test.
Result:
Fixes [#7230].
Motivation:
Even if it's a super micro-optimization (most JVM could optimize such
cases in runtime), in theory (and according to some perf tests) it
may help a bit. It also makes a code more clear and allows you to
access such methods in the test scope directly, without instance of
the class.
Modifications:
Add 'static' modifier for all methods, where it possible. Mostly in
test scope.
Result:
Cleaner code with proper 'static' modifiers.
Motivation:
Without a 'serialVersionUID' field, any change to a class will make
previously serialized versions unreadable.
Modifications:
Add missed 'serialVersionUID' field for all Serializable
classes.
Result:
Proper deserialization of previously serialized objects.
Motivation:
Java9SslEngine did not correctly implement ApplicationProtocolAccessor and so returned an empty String when no extension was used while the interface contract is to return null. This lead to the situation that ApplicationProtocolNegationHandler did not correct work.
Modifications:
- Rename ApplicationProtocolAccessor.getApplicationProtocol() to getNegotiatedApplicationProtocol() which resolves the clash with the method exposed by Java9s SSLEngine.
- Correctly implement getNegotiatedApplicationProtocol() for Java9Sslengine
- Add delegate in Java9Sslengine.getApplicationProtocol() which is provided by Java9
- Adjust tests to test the correct behaviour.
Result:
Fixes [#7251].
Motivation:
A bunch of unit tests are failing due to certificates having expired.
This has to be replaced with a newly generated certificate that has a
longer validity.
Modifications:
- generated a certificate with validity of 100 years from now
Results:
Unit tests are passing again
Motivation:
Netty is unable to use Java9s ALPN support atm.
Modifications:
When running on Java9+ we invoke the correct methods that are exposed on the Java9+ implementation of SSLEngine and so be able to support ALPN.
This patch is based on the work of @rschmitt and so https://github.com/netty/netty/pull/6992.
Result:
Fixes#6933.
Motivation:
When SslHandlerTest#testCompositeBufSizeEstimationGuaranteesSynchronousWrite fails it would be useful to know the SslProvider type
Modifications:
- Print the sever and client SslProvider upon failure
- Increase test timeout to 8 minutes to allow more time to run
Result:
Failures include more info to help diagnose issues.
Motivation:
DelegatingSslContext at the moment intercept newEngine calls and allow to init the SslEngine after it is created. The problem here is that this may not work the SSLEngine that is wrapped in the SslHandler when calling newHandler(...). This is because some SslContext implementations not delegate to newEngine(...) when creating the SslHandler to allow some optimizations. For this we should also allow to init the SslHandler after its creation and by default just delegate to initEngine(...).
Modifications:
Allow the user to also init the SslHandler after creation while by default init its SSLEngine after creation.
Result:
More flexible and correct code.
Motivation:
SslHandlerTest#testCompositeBufSizeEstimationGuaranteesSynchronousWrite has been observed to fail on CI servers, but it is not clear why.
Modifications:
- Add more visibility into what the state was and what the condition that caused the failure was.
Result:
More visibility when the test fails.
Motivation:
We need to ensure we not try to load any conscrypt classes directly (which means without using reflection) in the same class that is used to check if conscrypt is available. This is needed as otherwise we will have the following problem when try to use netty on java7:
java.lang.UnsupportedClassVersionError: org/conscrypt/BufferAllocator : Unsupported major.minor version 52.0
at io.netty.handler.ssl.ConscryptJdkSslEngineInteropTest.checkConscrypt(ConscryptJdkSslEngineInteropTest.java:49)
This regression was introduced by 4448b8f42f and detected on the CI when using:
mvn clean package -DtestJavaHome=$JAVA7_HOME
Modifications:
Move the detection code in an extra class and use it.
Result:
Works correctly also when using Java7.
Motivation:
Starting with 1.0.0.RC9, conscrypt supports a buffer allocator.
Modifications:
- Updated the creation process for the engine to pass through the
ByteBufAllocator.
- Wrap a ByteBufAllocator with an adapter for conscrypt.
- Added a property to optionally control whether conscrypt uses
Netty's buffer allocator.
Result:
Netty+conscrypt will support using Netty's ByteBufAllocator.
Motivation:
We need to ensure we only create the ResourceLeak when the constructor not throws.
Modifications:
Ensure ResourceLeakDetector.track(...) is only called if the constructor of ReferenceCoundedOpenSslEngine not throws.
Result:
No more false-positves.
Motivation:
ByteBuf#ensureWritable(int,boolean) returns an int indicating the status of the resize operation. For buffers that are unmodifiable or cannot be resized this method shouldn't throw but just return 1.
ByteBuf#ensureWriteable(int) should throw unmodifiable buffers.
Modifications:
- ReadOnlyByteBuf should be updated as described above.
- Add a unit test to SslHandler which verifies the read only buffer can be tolerated in the aggregation algorithm.
Result:
Fixes https://github.com/netty/netty/issues/7002.
Motivation:
Lots of usages of SelfSignedCertificates were not deleting the certs at
the end of the test. This includes setupHandlers() which is used by
extending classes. Although these files will be deleted at JVM exit and
deleting them early does not free the JVM from trying to delete them at
shutdown, it's good practice to delete eagerly and since users sometimes
use tests as a form of documentation, it'd be good for them to see the
explicit deletes.
Modifications:
Add missing delete() calls to ½ of the SelfSignedCertificates-using
tests.
Result:
Tests that more clearly communicates which resources are created and
may accumulate without early delete.
Motivation:
Previously filterCipherSuites was being passed the OpenSSL-formatted
cipher names. Commit 43ae974 introduced a regression as it swapped to the
RFC/JDK format, except that user-provided ciphers were not converted and
remained in the OpenSSL format.
This mis-match would cause all user-provided to be thrown away, leading
to failure trying to set zero ciphers:
Exception in thread "main" javax.net.ssl.SSLException: failed to set cipher suite: []
at io.netty.handler.ssl.ReferenceCountedOpenSslContext.<init>(ReferenceCountedOpenSslContext.java:299)
at io.netty.handler.ssl.OpenSslContext.<init>(OpenSslContext.java:43)
at io.netty.handler.ssl.OpenSslServerContext.<init>(OpenSslServerContext.java:347)
at io.netty.handler.ssl.OpenSslServerContext.<init>(OpenSslServerContext.java:335)
at io.netty.handler.ssl.SslContext.newServerContextInternal(SslContext.java:421)
at io.netty.handler.ssl.SslContextBuilder.build(SslContextBuilder.java:441)
Caused by: java.lang.Exception: Unable to configure permitted SSL ciphers (error:100000b1:SSL routines:OPENSSL_internal:NO_CIPHER_MATCH)
at io.netty.internal.tcnative.SSLContext.setCipherSuite(Native Method)
at io.netty.handler.ssl.ReferenceCountedOpenSslContext.<init>(ReferenceCountedOpenSslContext.java:295)
... 7 more
Modifications:
Remove the reformatting of user-provided ciphers, as they are already in
the RFC/JDK format.
Result:
No regression, and the internals stay sane using the RFC/JDK format.
Motivation:
Some ChannelOptions must be set before the Channel is really registered to have the desired effect.
Modifications:
Add another constructor argument which allows to not register the EmbeddedChannel to its EventLoop until the user calls register().
Result:
More flexible usage of EmbeddedChannel. Also Fixes [#6968].
Motivation:
6152990073 introduced a test-case in SSLEngineTest which used OpenSsl.* which should not be done as this is am abstract bass class that is also used for non OpenSsl tests.
Modifications:
Move the protocol definations into SslUtils.
Result:
Cleaner code.
Motivation:
TLS doesn't support a way to advertise non-contiguous versions from the client's perspective, and the client just advertises the max supported version. The TLS protocol also doesn't support all different combinations of discrete protocols, and instead assumes contiguous ranges. OpenSSL has some unexpected behavior (e.g. handshake failures) if non-contiguous protocols are used even where there is a compatible set of protocols and ciphers. For these reasons this method will determine the minimum protocol and the maximum protocol and enabled a contiguous range from [min protocol, max protocol] in OpenSSL.
Modifications:
- ReferenceCountedOpenSslEngine#setEnabledProtocols should determine the min/max protocol versions and enable a contiguous range
Result:
OpenSslEngine is more consistent with the JDK's SslEngineImpl and no more unexpected handshake failures due to protocol selection quirks.
Motivation:
Each call to SSL_write may introduce about ~100 bytes of overhead. The OpenSslEngine (based upon OpenSSL) is not able to do gathering writes so this means each wrap operation will incur the ~100 byte overhead. This commit attempts to increase goodput by aggregating the plaintext in chunks of <a href="https://tools.ietf.org/html/rfc5246#section-6.2">2^14</a>. If many small chunks are written this can increase goodput, decrease the amount of calls to SSL_write, and decrease overall encryption operations.
Modifications:
- Introduce SslHandlerCoalescingBufferQueue in SslHandler which will aggregate up to 2^14 chunks of plaintext by default
- Introduce SslHandler#setWrapDataSize to control how much data should be aggregated for each write. Aggregation can be disabled by setting this value to <= 0.
Result:
Better goodput when using SslHandler and the OpenSslEngine.
Motivation:
The JDK SSLEngine documentation says that a call to wrap/unwrap "will attempt to consume one complete SSL/TLS network packet" [1]. This limitation can result in thrashing in the pipeline to decode and encode data that may be spread amongst multiple SSL/TLS network packets.
ReferenceCountedOpenSslEngine also does not correct account for the overhead introduced by each individual SSL_write call if there are multiple ByteBuffers passed to the wrap() method.
Modifications:
- OpenSslEngine and SslHandler supports a mode to not comply with the limitation to only deal with a single SSL/TLS network packet per call
- ReferenceCountedOpenSslEngine correctly accounts for the overhead of each call to SSL_write
- SslHandler shouldn't cache maxPacketBufferSize as aggressively because this value may change before/after the handshake.
Result:
OpenSslEngine and SslHanadler can handle multiple SSL/TLS network packet per call.
[1] https://docs.oracle.com/javase/7/docs/api/javax/net/ssl/SSLEngine.html
Motivation:
SslHandlerTest#testCompositeBufSizeEstimationGuaranteesSynchronousWrite has been observed to fail on CI servers. Knowing how many bytes were seen by the client would be helpful.
Modifications:
- Add bytesSeen to the exception if the client closes early.
Result:
More debug info available.
Motivation:
PR #6811 introduced a public utility methods to decode hex dump and its parts, but they are not visible from netty-common.
Modifications:
1. Move the `decodeHexByte`, `decodeHexDump` and `decodeHexNibble` methods into `StringUtils`.
2. Apply these methods where applicable.
3. Remove similar methods from other locations (e.g. `HpackHex` test class).
Result:
Less code duplication.
Motivation:
For historical reasons OpenSSL's internal naming convention for CHACHA20 based cipher suites does not include the HMAC algorithm in the cipher name. This will prevent the CHACHA20 cipher suites from being used if the RFC cipher names are specified.
Modifications:
- Add a special case for CHACHA20 cipher name conversions in CipherSuiteConverter
- Update OPENSSL_CIPHERSUITE_PATTERN to accommodate the new naming scheme for CHACHA20 cipher suites
Result:
CipherSuiteConverter now works with CHACHA20 cipher suites.
Motivation
RFC 6066 (https://tools.ietf.org/html/rfc6066#page-6) says that the hostname in the SNI extension is ASCII encoded but Netty decodes it using UTF-8.
Modifications
Use ASCII instead of UTF-8
Result
Fixes#6717
Motivation:
We not correctly handle LE buffers when try to read the packet length out of the buffer and just assume it always is a BE buffer.
Modifications:
Correctly account for the endianess of the buffer when reading the packet lenght.
Result:
Fixes [#6709].
Motivation:
SSL_write requires a fixed amount of bytes for overhead related to the encryption process for each call. OpenSslEngine#wrap(..) will attempt to encrypt multiple input buffers until MAX_PLAINTEXT_LENGTH are consumed, but the size estimation provided by calculateOutNetBufSize may not leave enough room for each call to SSL_write. If SSL_write is not able to completely write results to the destination buffer it will keep state and attempt to write it later. Netty doesn't account for SSL_write keeping state and assumes all writes will complete synchronously (by attempting to allocate enough space to account for the overhead) and feeds the same data to SSL_write again later which results in corrupted data being generated.
Modifications:
- OpenSslEngine#wrap should only produce a single TLS packet according to the SSLEngine API specificaiton [1].
[1] https://docs.oracle.com/javase/8/docs/api/javax/net/ssl/SSLEngine.html#wrap-java.nio.ByteBuffer:A-int-int-java.nio.ByteBuffer-
- OpenSslEngine#wrap should only consider a single buffer when determining if there is enough space to write, because only a single buffer will ever be consumed.
Result:
OpenSslEngine#wrap will no longer produce corrupted data due to incorrect accounting of space required in the destination buffers.
Motivation:
When adding SNIMatcher support we missed to use static delegating methods and so may try to load classes that not exists in Java7. Which will lead to errors.
Modifications:
- Correctly only try to load classes when running on java8+
- Ensure Java8+ related tests only run when using java8+
Result:
Fixes [#6700]
Motivation:
Java8 adds support for SNIMatcher to reject SNI when the hostname not matches what is expected. We not supported doing this when using SslProvider.OPENSSL*.
Modifications:
- Add support for SNIMatcher when using SslProvider.OPENSSL*
- Add unit tests
Result:
SNIMatcher now support with our own SSLEngine as well.
Motivation:
We need to ensure we only try to to test with the SslProviders that are supported when running the SslHandlerTest.testCompositeBufSizeEstimationGuaranteesSynchronousWrite test.
Modifications:
Skip SslProvider.OPENSSL* if not supported.
Result:
No more test-failures if openssl is not installed on the system.
Motivation:
Java8 adds support for SNIMatcher to reject SNI when the hostname not matches what is expected. We not supported doing this when using SslProvider.OPENSSL*.
Modifications:
- Add support for SNIMatcher when using SslProvider.OPENSSL*
- Add unit tests
Result:
SNIMatcher now support with our own SSLEngine as well.
https://github.com/netty/netty-tcnative/pull/215
Motivation
OCSP stapling (formally known as TLS Certificate Status Request extension) is alternative approach for checking the revocation status of X.509 Certificates. Servers can preemptively fetch the OCSP response from the CA's responder, cache it for some period of time, and pass it along during (a.k.a. staple) the TLS handshake. The client no longer has to reach out on its own to the CA to check the validity of a cetitficate. Some of the key benefits are:
1) Speed. The client doesn't have to crosscheck the certificate.
2) Efficiency. The Internet is no longer DDoS'ing the CA's OCSP responder servers.
3) Safety. Less operational dependence on the CA. Certificate owners can sustain short CA outages.
4) Privacy. The CA can lo longer track the users of a certificate.
https://en.wikipedia.org/wiki/OCSP_staplinghttps://letsencrypt.org/2016/10/24/squarespace-ocsp-impl.html
Modifications
https://www.openssl.org/docs/man1.0.2/ssl/SSL_set_tlsext_status_type.html
Result
High-level API to enable OCSP stapling
Motivation:
`io.netty.handler.logging.LoggingHandler` does not log when these
events happen.
Modifiations:
Add overrides with logging to these methods.
Result:
Logging now happens for these two events.
Motivation:
In OpenSslCertificateException we tried to validate the supplied error code but did not correctly account for all different valid error codes and so threw an IllegalArgumentException.
Modifications:
- Fix validation by updating to latest netty-tcnative and use CertificateVerifier.isValid
- Add unit tests
Result:
Validation of error code works as expected.
Motivation:
1419f5b601 added support for conscrypt but the CI started to fail when running tests with java7 as conscrypt is compiled with java8. This was partly fixed in c4832cd9d9 but we also need to ensure we not try to even load the classes.
Modifications:
Only try to load conscrypt classes when on java8+-
Result:
CI not fails anymore.
Motivation:
Conscrypt is a Java Security provider that wraps OpenSSL (specifically BoringSSL). It's a possible alternative to Netty-tcnative that we should explore. So this commit is just to enable us to further investigate its use.
Modifications:
Modifying the SslContext creation path to support the Conscrypt provider.
Result:
Netty will support OpenSSL with conscrypt.
Motivation:
We should limit the size of the allocated outbound buffer to MAX_ENCRYPTED_PACKET_LENGTH to ensure we not cause an OOME when the user tries to encrypt a very big buffer.
Modifications:
Limit the size of the allocated outbound buffer to MAX_ENCRYPTED_PACKET_LENGTH
Result:
Fixes [#6564]
Motivation:
ChunkedWriteHandler queues written messages and actually writes them
when flush is called. In its doFlush method, it needs to flush after
each chunk is written to preserve memory. However, non-chunked messages
(those that aren't of type ChunkedInput) are treated in the same way,
which means that flush is called after each message is written.
Modifications:
Moved the call to flush() inside the if block that tests if the message
is an instance of ChunkedInput. To ensure flush is called at least once,
the existing boolean flushed is checked at the end of doFlush. This
check was previously in ChunkedWriteHandler.flush(), but wasn't checked in
other invocations of doFlush, e.g. in channelInactive.
Result:
When this handler is present in a pipeline, writing a series
of non-chunked messages will be flushed as the developer intended.
Motivation:
Some pipelines require support for both SSL and non-SSL messaging.
Modifications:
Add utility decoder to support both SSL and non-SSL handlers based on the initial message.
Result:
Less boilerplate code to write for developers.
Motivation:
SslContext and SslContextBuilder do not support a way to specify the desired TLS protocols. This currently requires that the user extracts the SSLEngine once a context is built and manually call SSLEngine#setEnabledProtocols(String[]). Something this critical should be supported at the SslContext level.
Modifications:
- SslContextBuilder should accept a list of protocols to configure for each SslEngine
Result:
SslContext consistently sets the supported TLS/SSL protocols.
Motivaiton:
It is possible that if the OpenSSL library supports the interfaces required to use the KeyManagerFactory, but we fail to get the io.netty.handler.ssl.openssl.useKeyManagerFactory system property (or this property is set to false) that SSLEngineTest based unit tests which use a KeyManagerFactory will fail.
Modifications:
- We should check if the OpenSSL library supports the KeyManagerFactory interfaces and if the system property allows them to be used in OpenSslEngineTests
Result:
Unit tests which use OpenSSL and KeyManagerFactory will be skipped instead of failing.
Motivation:
When we do a wrap operation we calculate the maximum size of the destination buffer ahead of time, and return a BUFFER_OVERFLOW exception if the destination buffer is not big enough. However if there is a CompositeByteBuf the wrap operation may consist of multiple ByteBuffers and each incurs its own overhead during the encryption. We currently don't account for the overhead required for encryption if there are multiple ByteBuffers and we assume the overhead will only apply once to the entire input size. If there is not enough room to write an entire encrypted packed into the BIO SSL_write will return -1 despite having actually written content to the BIO. We then attempt to retry the write with a bigger buffer, but because SSL_write is stateful the remaining bytes from the previous operation are put into the BIO. This results in sending the second half of the encrypted data being sent to the peer which is not of proper format and the peer will be confused and ultimately not get the expected data (which may result in a fatal error). In this case because SSL_write returns -1 we have no way to know how many bytes were actually consumed and so the best we can do is ensure that we always allocate a destination buffer with enough space so we are guaranteed to complete the write operation synchronously.
Modifications:
- SslHandler#allocateNetBuf should take into account how many ByteBuffers will be wrapped and apply the encryption overhead for each
- Include the TLS header length in the overhead computation
Result:
Fixes https://github.com/netty/netty/issues/6481
Motivation:
325cc84a2e introduced new tests which uses classes only provided by Java8+. We need to ensure we only try to load classes needed for these when we run the tests on Java8+ so we still can run the testsuite with Java7.
Modifications:
Add extra class which only gets loaded when Java8+ is used and move code there.
Result:
No more class-loader issue when running tests with Java7.
Motivation:
We not support all SSLParameters settings so we should better throw if a user try to use them.
Modifications:
- Check for unsupported parameters
- Add unit test
Result:
Less surprising behavior.
Motivation:
d8e6fbb9c3 attempted to account for the JDK not throwing the expected SSLHandshakeException by allowing a SSLException to also pass the test. However in some situations the SSLException will not be the top level exception and the Throwable must be unwrapped to see if the root cause is an SSLException.
Modifications:
- Unwrap exceptions thrown by the JDK's SSLEngine to check for SSLException.
Result:
SSLEngineTest (and derived classes) are more reliable.
Motivation:
OpenSSL doesn't automatically verify hostnames and requires extract method calls to enable this feature [1]. We should allow this to be configured.
Modifications:
- SSLParamaters#getEndpointIdentificationAlgorithm() should be respected and configured via tcnative interfaces.
Result:
OpenSslEngine respects hostname verification.
[1] https://wiki.openssl.org/index.php/Hostname_validation
Motivation:
We have our own ThreadLocalRandom implementation to support older JDKs . That said we should prefer the JDK provided when running on JDK >= 7
Modification:
Using ThreadLocalRandom implementation of the JDK when possible.
Result:
Make use of JDK implementations when possible.
Motivation:
Commit cd3bf3df58 made netty observe the latest version of netty-tcnative which changed the way how static fields are computed for various SSL.* values. This lead to have SSL_OP_NO_SSLv2 become 0 when using boringssl as boringssl not supports SSLv2 at all. In the logic of ReferenceCountedOpenSslEngine.getEnabledProtocols() we not expect to have a zero value and so our logic fails.
Modifications:
Check we actual support the protocol before return it as enabled.
Result:
SSLEngineTest.testEnablingAnAlreadyDisabledSslProtocol passes again with boringssl
Motivation:
JdkOpenSslEngineInteroptTest.mySetupMutualAuthServerIsValidClientException(...) delegated to the wrong super method.
Modifications:
Fix delegate
Result:
Correct test-code.
Motivation:
Some version of openssl dont support the needed APIs to use a KeyManagerFactory. In this case we should skip the tests.
Modifications:
- Use assumeTrue(...) to skip tests that need a KeyManagerFactory and its not supported.
Result:
Tests pass on all openssl versions we support.
Motivation:
tcnative was moved into an internal package.
Modifications:
Update package for tcnative imports.
Result:
Use correct package names for tcnative.
Motivation:
OpenSslEngineTest has unused imports and SSLEngineTest uses a fixed port which was used for debugging.
Modifications:
- Remove unused imports
- Use ephemeral port
Result:
Cleaner test code.
Modifications:
tcnative made some fixes and API changes related to setVerify. We should absorb these changes in Netty.
Modifications:
- Use tcnatives updated APIs
- Add unit tests to demonstrate correct behavior
Result:
Updated to latest tcnative code and more unit tests to verify expected behavior.
Motivation:
We should remove the restriction to only allow to call unwrap with a ByteBuffer[] whose cumulative length exceeds MAX_ENCRYPTED_PACKET_LENGTH.
Modifications:
Remove guard.
Result:
Fixes [#6335].
Motivation:
CipherSuiteConverter may throw a NPE if a cipher suite from OpenSSL does not match the precomputed regular expression for OpenSSL ciphers. This method shouldn't throw and instead just return null.
Modifications:
- if cacheFromOpenSsl(..) fails the conversion toJava should return null
Result:
Fixes https://github.com/netty/netty/issues/6336.
Motivation:
Currently Netty utilizes BIO_new_bio_pair so we can control all FD lifetime and event notification but delegates to OpenSSL for encryption/decryption. The current implementation sets up a pair of BIO buffers to read/write encrypted/plaintext data. This approach requires copying of data from Java ByteBuffers to native memory BIO buffers, and also requires both BIO buffers to be sufficiently large to hold application data. If direct ByteBuffers are used we can avoid coyping to/from the intermediate BIO buffer and just read/write directly from the direct ByteBuffer memory. We still need an internal buffer because OpenSSL may generate write data as a result of read calls (e.g. handshake, alerts, renegotiation, etc..), but this buffer doesn't have to be be large enough to hold application data.
Modifications:
- Take advantage of the new ByteBuffer based BIO provided by netty-tcnative instead of using BIO_read and BIO_write.
Result:
Less copying and lower memory footprint requirement per TLS connection.
Motivation:
We used various mocking frameworks. We should only use one...
Modifications:
Make usage of mocking framework consistent by only using Mockito.
Result:
Less dependencies and more consistent mocking usage.
Motiviation:
We should ensure we not need any extra wrap / unwrap calls during handshake once the handshake was signaled as finished
Modifications:
More strict testing
Result:
Better testing of handshake behaviour
Motivation:
Tests were added in 91f050d2ef to run with different protocols / ciphers. These may fail currently when openssl was compiled without support for the protocol / ciphers.
Modifications:
- Refactor tests to easier understand for which protocol / cipher it failed
- Not fail the test if the protocol is not supported with the used openssl version.
Result:
More robust testing.
Motivation:
We failed to properly test if a protocol is supported on an OpenSSL installation and just always returned all protocols.
Modifications:
- Detect which protocols are supported on a platform.
- Skip protocols in tests when not supported. This fixes a build error on some platforms introduced by [#6276].
Result:
Correctly return only the supported protocols
Motivation:
We used ca 2k as maximum overhead for encrypted packets which is a lot more then what is needed in reality by OpenSSL. This could lead to the need of more memory.
Modification:
- Use a lower overhead of 86 bytes as defined by the spec and openssl itself
- Fix unit test to use the correct session to calculate needed buffer size
Result:
Less memory usage.
Motivation:
We should test that we correctly return BUFFER_UNDERFLOW if the src buffer not contains enough data to unwrap it.
Modification:
Add unit test to verify behaviour.
Result:
Better test coverrage of SSLEngine implementations.
Motivation:
SslHandler closed the channel as soon as it was able to write out the close_notify message. This may not be what the user want as it may make sense to only close it after the actual response to the close_notify was received in order to guarantee a clean-shutdown of the connection in all cases.
Beside this closeNotifyFlushTimeoutMillis is volatile so may change between two reads. We need to cache it in a local variable to ensure it not change int between. Beside this we also need to check if the flush promise was complete the schedule timeout as this may happened but we were not able to cancel the timeout yet. Otherwise we will produce an missleading log message.
Modifications:
- Add new setter / getter to SslHandler which allows to specify the behavior (old behavior is preserved as default)
- Added unit test.
- Cache volatile closeNotifyTimeoutMillis.
- Correctly check if flush promise was complete before we try to forcibly close the Channel and log a warning.
- Add missing javadocs.
Result:
More clean shutdown of connection possible when using SSL and fix racy way of schedule close_notify flush timeouts and javadocs.
Motivation:
PR [#6238] added guards to be able to call wrap(...) / unwrap(...) after the engine was shutdown. Unfortunally one case was missed which is when closeOutbound() was called and produced some data while closeInbound() was not called yet.
Modifications:
Correctly guard against SSLException when closeOutbound() was called, produced data and someone calls wrap(...) after it.
Result:
No more SSLException. Fixes [#6260].
Motivation:
In commit fc3c9c9523 I changes the way how we calculate the capacity of the needed ByteBuf for wrap operations that happen during writes when the SslHandler is used. This had the effect that the same capacity for ByteBufs is needed for the JDK implementation of SSLEngine but also for our SSLEngine implementation that uses OpenSSL / BoringSSL / LibreSSL. Unfortunally this had the side-effect that applications that used our SSLEngine implementation now need a lot more memory as bascially the JDK implementation always needs a 16kb buffer for each wrap while we can do a lot better for our SSLEngine implementation.
Modification:
- Resurrect code that calculate a better ByteBuf capacity when using our SSLEngine implementation and so be able to safe a lot of memory
- Add test-case to ensure it works as expected and is not removed again later on.
Result:
Memory footprint of applications that uses our SSLEngine implementation based on OpenSSL / BoringSSL / LibreSSL is back to the same amount of before commit fc3c9c9523.
Motivation:
Currently Netty does not wrap socket connect, bind, or accept
operations in doPrivileged blocks. Nor does it wrap cases where a dns
lookup might happen.
This prevents an application utilizing the SecurityManager from
isolating SocketPermissions to Netty.
Modifications:
I have introduced a class (SocketUtils) that wraps operations
requiring SocketPermissions in doPrivileged blocks.
Result:
A user of Netty can grant SocketPermissions explicitly to the Netty
jar, without granting it to the rest of their application.
Motivation:
As we use different execution path in our SSLEngine implementation depending on if heap, direct or mixed buffers are used we should run the tests with all of them.
Modification:
Ensure we run all tests with different buffer types.
Result:
Better test-coverage
Motivation:
For the completion of a handshake we already fire a SslHandshakeCompletionEvent which the user can intercept. We should do the same for the receiving of close_notify.
Modifications:
Add SslCloseCompletionEvent and test-case.
Result:
More consistent API.
Motivation:
The SslHandler.sslCloseFuture() may not be notified when the Channel is closed before a closify_notify is received.
Modifications:
Ensure we try to fail the sslCloseFuture() when the Channel is closed.
Result:
Correctly notify the ssl close future.
Motivation:
The JDK implementation of SSLEngine allows to have unwrap(...) / wrap(...) called even after closeInbound() and closeOutbound() were called. We need to support the same in ReferenceCountedSslEngine.
Modification:
- Allow calling ReferenceCountedSslEngine.unwrap(...) / wrap(...) after the engine was closed
- Modify unit test to ensure correct behaviour.
Result:
Implementation works as expected.
Motivation:
fc3c9c9523 introduced a bug which will have ReferenceCountedSslEngine.unwrap(...) produce an IOOBE when be called with an BŷteBuffer as src that contains multiple SSLRecords and has a position != 0.
Modification:
- Correctly set the limit on the ByteBuffer and so fix the IOOBE.
- Add test-case to verify the fix
Result:
Correctly handle heap buffers as well.
Motivation:
Openssl provider should behave same as JDK provider when mutual authentication is required and a specific set of trusted Certificate Authorities are specified. The SSL handshake should return back to the connected peer the same list of configured Certificate Authorities.
Modifications:
Correctly set the CA list.
Result:
Correct and same behaviour as the JDK implementation.
Motivation:
We need to ensure we not swallow the close_notify that should be send back to the remote peer. See [#6167]
Modifications:
- Only call shutdown() in closeInbound() if there is nothing pending that should be send back to the remote peer.
- Return the correct HandshakeStatus when the close_notify was received.
- Only shutdown() when close_notify was received after closeOutbound() was called.
Result:
close_notify is correctly send back to the remote peer and handled when received.
Motivation
The IdleStateHandler tracks write() idleness on message granularity but does not take into consideration that the client may be just slow and has managed to consume a subset of the message's bytes in the configured period of time.
Modifications
Adding an optional configuration parameter to IdleStateHandler which tells it to observe ChannelOutboundBuffer's state.
Result
Fixes https://github.com/netty/netty/issues/6150
Motivation:
boringssl uses different messages for the ssl alerts which are all uppercase. As we try to match case as well this fails in SSLErrorTest as we expect lower-case.
This test was introduced by 9b7fb2f362.
Modifications:
Ensure we first translate everything to lower-case before doing the assert.
Result:
SSLErrorTest also pass when boringssl is used.
Motivation:
We need to ensure we not call handshake() when the engine is already closed. Beside this our implementation of isOutboundDone() was not correct as it not took the pending data in the outbound buffer into acount (which may be also generated as part of an ssl alert). Beside this we also called SSL_shutdown(...) while we were still in init state which will produce an error and so noise in the log with openssl later versions.
This is also in some extend related to #5931 .
Modifications:
- Ensure we not call handshake() when already closed
- Correctly implement isOutboundDone()
- Not call SSL_shutdown(...) when still in init state
- Added test-cases
Result:
More correct behaviour of our openssl SSLEngine implementation.
Motivation:
When non SSL data is passed into SSLEngine.unwrap(...) we need to throw an SSLException. This was not done at the moment. Even worse we threw an IllegalArgumentException as we tried to allocate a direct buffer with capacity of -1.
Modifications:
- Guard against non SSL data and added an unit test.
- Make code more consistent
Result:
Correct behaving SSLEngine implementation.
Motivation:
Java9 will be released soon so we should ensure we can compile netty with Java9 and run all our tests. This will help to make sure Netty will be usable with Java9.
Modification:
- Add some workarounds to be able to compile with Java9, note that the full profile is not supported with Java9 atm.
- Remove some usage of internal APIs to be able to compile on java9
- Not support Alpn / Npn and so not run the tests when using Java9 for now. We will do a follow up PR to add support.
Result:
Its possible to build netty and run its testsuite with Java9.
Motivation:
We are now more careful to flush alerts that are generated when errors occur. We should also be more careful in unit tests to release any buffers that may be queued due to potential errors wich result in alerts.
Modifications:
- When SslHandlerTest uses EmbeddedChannel we should always call finishAndReleaseAll
Results:
Fixes https://github.com/netty/netty/issues/6057
Motivation:
When the SslHandler.unwrap(...) (which is called via decode(...)) method did produce an SSLException it was possible that the produced alert was not send to the remote peer. This could lead to staling connections if the remote peer did wait for such an alert and the connection was not closed.
Modifications:
- Ensure we try to flush any pending data when a SSLException is thrown during unwrapping.
- Fix SniHandlerTest to correct test this
- Add explicit new test in SslHandlerTest to verify behaviour with all SslProviders.
Result:
The alert is correctly send to the remote peer in all cases.
Motivation:
We tried to detect the correct alert to use depending on the CertificateException that is thrown by the TrustManager. This not worked all the time as depending on the TrustManager implementation it may also wrap a CertPathValidatorException.
Modification:
- Try to unwrap the CertificateException if needed and detect the right alert via the CertPathValidatorException.
- Add unit to verify
Result:
Send the correct alert depending on the CertificateException when using OpenSslEngine.
Motiviation:
We used ReferenceCountUtil.releaseLater(...) in our tests which simplifies a bit the releasing of ReferenceCounted objects. The problem with this is that while it simplifies stuff it increase memory usage a lot as memory may not be freed up in a timely manner.
Modifications:
- Deprecate releaseLater(...)
- Remove usage of releaseLater(...) in tests.
Result:
Less memory needed to build netty while running the tests.
Motivation:
The SniHandlerTest.testServerNameParsing did fail when SslProvider.JDK was used as it the JDK SSLEngineImpl does not send an alert.
Modifications:
Ensure tests pass with JDK and OPENSSL ssl implementations.
Result:
SniHandlerTest will run with all SslProvider and not fail when SslProvider.JDK is used.
Motiviation:
We need to ensure we only consume as much da as we can maximal put in one ssl record to not produce a BUFFER_OVERFLOW when calling wrap(...).
Modification:
- Limit the amount of data that we consume based on the maximal plain text size that can be put in one ssl record
- Add testcase to verify the fix
- Tighten up testcases to ensure the amount of produced and consumed data in SslEngineResult matches the buffers. If not the tests will fail now.
Result:
Correct and conform behavior of OpenSslEngine.wrap(...) and better test coverage during handshaking in general.
Motivation:
OpenSslEngine.wrap(...) and OpenSslEngie.unwrap(...) may consume bytes even if an BUFFER_OVERFLOW / BUFFER_UNDERFLOW is detected. This is not correct as it should only consume bytes if it can process them without storing data between unwrap(...) / wrap(...) calls. Beside this it also should only process one record at a time.
Modifications:
- Correctly detect BUFFER_OVERFLOW / BUFFER_UNDERFLOW and only consume bytes if non of them is detected.
- Only process one record per call.
Result:
OpenSslEngine behaves like stated in the javadocs of SSLEngine.
Motivation:
If the user removes the SslHandler while still in the processing loop we will produce an IllegalReferenceCountException. We should stop looping when the handlerwas removed.
Modifications:
Ensure we stop looping when the handler is removed.
Result:
No more IllegalReferenceCountException.
Motivation:
Add test-case for doing mutal auth with a certificate chain that holds more then one certificate.
Modifications:
Add test case
Result:
more tests.
Motivation:
the build doesnt seem to enforce this, so they piled up
Modifications:
removed unused import lines
Result:
less unused imports
Signed-off-by: radai-rosenblatt <radai.rosenblatt@gmail.com>
Motivation:
Currently FlushConsolidationHandler only consolidates if a read loop is
active for a Channel, otherwise each writeAndFlush(...) call will still
be flushed individually. When these calls are close enough, it can be
beneficial to consolidate them even outside of a read loop.
Modifications:
When we allow a flush to "go through", don't perform it immediately, but
submit it on the channel's executor. Under high pressure, this gives
other writes a chance to enqueue before the task gets executed, and so
we flush multiple writes at once.
Result:
Lower CPU usage and less context switching.
Motivation:
af632278d2 introduced a test which only worked on some jvm versions and specific os'es.
Modifications:
Fix test to work on different java versions and os'es
Result:
No flacky test.
Motivation:
We need to ensure we not set duplicated certificates when using OpenSslEngine.
Modifications:
- Skip first cert in chain when set the chain itself and so not send duplicated certificates
- Add interopt unit tests to ensure no duplicates are send.
Result:
No more duplicates.
Motivation
The SniHandler is currently hiding its replaceHandler() method and everything that comes with it. The user has no easy way of getting a hold onto the SslContext for the purpose of reference counting for example. The SniHandler does have getter methods for the SslContext and hostname but they're not very practical or useful. For one the SniHandler will remove itself from the pipeline and we'd have to track a reference of it externally and as we saw in #5745 it'll possibly leave its internal "selection" object with the "EMPTY_SELECTION" value (i.e. we've just lost track of the SslContext).
Modifications
Expose replaceHandler() and allow the user to override it and get a hold onto the hostname, SslContext and SslHandler that will replace the SniHandler.
Result
It's possible to get a hold onto the SslContext, the hostname and the SslHandler that is about to replace the SniHandler. Users can add additional behavior.
Motivation
SslHandler's handlerRemoved() is supposed to release the SSLEngine (which it does) but there is no Test for it to make sure it really happens and doesn't unexpectedly change in the future.
Modifications
Add a Unit Test that makes sure that SslHandler releases the SSLEngine when the Channel gets closed.
Result
Assurance that SslHandler will not leak (ReferenceCounted) SSLEngines.
Motivation:
ReferenceCountedOpenSslEngine depends upon the the SslContext to cleanup JNI resources. If we don't wait until the ReferenceCountedOpenSslEngine is done with cleanup before cleaning up the SslContext we may crash the JVM.
Modifications:
- Wait for the channels to close (and thus the ReferenceCountedOpenSslEngine to be cleaned up) before cleaning up the associated SslContext.
Result:
Cleanup sequencing is correct and no more JVM crash.
Fixes https://github.com/netty/netty/issues/5692
Motivation:
Commit b963595988 added a unit that will not work when KeyManagerFactory is used.
Modifications:
Only run the test if OpenSsl.useKeyManagerFactory() returns false.
Result:
Builds with boringssl
Motivation:
The private key and certificate that are passed into #serKeyMaterial() could be PemEncoded in which case the #toPEM() methods return the identity of the value.
That in turn will fail in the #toBIO() step because the underlying ByteBuf is not necessarily direct.
Modifications:
- Use toBIO(...) which also works with non direct PemEncoded values
- Add unit test.
Result:
Correct handling of PemEncoded.
Motivation:
OpenSslEngine and OpenSslContext currently rely on finalizers to ensure that native resources are cleaned up. Finalizers require the GC to do extra work, and this extra work can be avoided if the user instead takes responsibility of releasing the native resources.
Modifications:
- Make a base class for OpenSslENgine and OpenSslContext which does not have a finalizer but instead implements ReferenceCounted. If this engine is inserted into the pipeline it will be released by the SslHandler
- Add a new SslProvider which can be used to enable this new feature
Result:
Users can opt-in to a finalizer free OpenSslEngine and OpenSslContext.
Fixes https://github.com/netty/netty/issues/4958
Motivation:
Calling flush() and writeAndFlush(...) are expensive operations in the sense as both will produce a write(...) or writev(...) system call if there are any pending writes in the ChannelOutboundBuffer. Often we can consolidate multiple flush operations into one if currently a read loop is active for a Channel, as we can just flush when channelReadComplete is triggered. Consolidating flushes can give a huge performance win depending on how often is flush is called. The only "downside" may be a bit higher latency in the case of where only one flush is triggered by the user.
Modifications:
Add a FlushConsolidationHandler which will consolidate flushes and so improve the throughput.
Result:
Better performance (throughput). This is especially true for protocols that use some sort of PIPELINING.
Motivation:
Unit test for the OpenSslEngine "OpenSslEngine writePlaintextData WANT_READ with no data in BIO buffer" issue.
Modifications:
- Update SslEngine test to include renegotiation
Result:
More test coverage in OpenSslEngine.
Motivation:
These methods were recently deprecated. However, they remained in use in several locations in Netty's codebase.
Modifications:
Netty's code will now access the bootstrap config to get the group or child group.
Result:
No impact on functionality.
Motivation:
At the moment SSLSession.getId() may always return an empty byte array when OpenSSLEngine is used. This is as we not set SSL_OP_NO_TICKET on the SSLContext and so SSL_SESSION_get_id(...) will return an session id with length of 0 if tickets are not used.
Modifications:
- Set SSL_OP_NO_TICKET by default and only clear it if the user requests the usage of session tickets.
- Add unit test
Result:
Ensure consistent behavior between different SSLEngine implementations.
Motivation:
We missed to skip some tests for OpenSsl when OpenSsl.isAvailable() is false.
Modifications:
- Correctly skip tests when OpenSsl.isAvailable() is false.
- Simplify some code by using @BeforeClass.
Result:
Be able to compile netty even when OpenSsl is not present on the system.
Motivation:
2b65258568 only partially addressed the synchronization issues that are present in FlowControlHandlerTest. A few tests are attempting to validate state changes made across an EventLoop thread and the JUnit thread but are not properly synchronized.
Modifications:
- Ensure that conditions which verify expectations set in another thread have synchronization gates to ensure the event has actually occurred.
- Remove the message counter verification in favor of using individual CountDownLatch objects
Result:
FLowControlHanderTest has less race conditions which may lead to test failures.
Motivation:
DomainMappingBuilder should have been named as DomainNameMappingBuilder
because it builds a DomainNameMapping.
Modifications:
- Add DomainNameMappingBuilder that does the same job with
DomainMappingBuilder
- Deprecate DomainMappingBuilder and delegate its logic to
DomainNameMappingBuilder
- Remove the references to the deprecated methods and classes related
with domain name mapping
- Miscellaneous:
- Fix Javadoc of DomainNameMapping.asMap()
- Pre-create the unmodifiable map in DomainNameMapping
Result:
- Consistent naming
- Less use of deprecated API
Related: #4333#4421#5128
Motivation:
slice(), duplicate() and readSlice() currently create a non-recyclable
derived buffer instance. Under heavy load, an application that creates a
lot of derived buffers can put the garbage collector under pressure.
Modifications:
- Add the following methods which creates a non-recyclable derived buffer
- retainedSlice()
- retainedDuplicate()
- readRetainedSlice()
- Add the new recyclable derived buffer implementations, which has its
own reference count value
- Add ByteBufHolder.retainedDuplicate()
- Add ByteBufHolder.replace(ByteBuf) so that..
- a user can replace the content of the holder in a consistent way
- copy/duplicate/retainedDuplicate() can delegate the holder
construction to replace(ByteBuf)
- Use retainedDuplicate() and retainedSlice() wherever possible
- Miscellaneous:
- Rename DuplicateByteBufTest to DuplicatedByteBufTest (missing 'D')
- Make ReplayingDecoderByteBuf.reject() return an exception instead of
throwing it so that its callers don't need to add dummy return
statement
Result:
Derived buffers are now recycled when created via retainedSlice() and
retainedDuplicate() and derived from a pooled buffer
Motivation:
FlowControlHandlerTest attempts to validate the expected contents of the underlying queue in FlowControlHandler. However the condition which triggers the check is too early and the queue contents may not yet contain all expected objects. For example a CountDownLatch is counted down in a handler's channelRead which is after the FlowControlHandler in the pipeline. At this point if there is a thread context switch the queue may not yet contain all the expected objects and checking the queue contents is not valid.
Modifications:
- Remove checking the queues contents in FLowControlHandlerTest and instead only check the empty condition at the end of the tests
Result:
FlowControlHandlerTest won't fail due to invalid checks of the contents of the queue.
Motivations
The test SniHandlerTest#testSniWithApnHandler() does not actually
involve SNI: given the client setup, the ClientHello in the form of hex
strings is not actually written to the wire, so the server never receives that.
We may need to write in somewhere else (e.g., channelActive()) instead of in
initChannel() in order for the hex strings to reach the server. So here
what's actually going on is an ordinary TLS C/S communication without SNI.
Modifications
The client part is modified to enable SNI by using an SslHandler with an
SSLEngine created by io.netty.handler.ssl.SslContext#newEngine(), where
the server hostname is specified. Also, more clauses are added to verify that
the SNI is indeed successful.
Results
Now the test verifies that both SNI and APN actually happen and succeed.
Motivation:
Some handlers such as HttpObjectDecoder can emit more than one event per read()
which leads to problems in downstream handlers that expect only one event and hope
that ChannelConfig#setAutoRead(false) prevents further events being sent while they're
processing the one they've just received.
Modifications:
A new handler called FlowControlHandler that feeds off read() and isAutoRead() and acts
as a holding buffer if auto reading gets turned off and more events arrive while auto reading
is off.
Result:
Fixes issues such as #4895.
Motivation:
fcbeebf6df introduced a unit test to verify ApplicationProtocolNegotiationHandler is compatible with SniHandler. However only the server attempts ALPN and verifies that it completes and the client doesn't verify the handshake is completed. This can lead to the client side SSL engine to prematurely close and throw an exception.
Modifications:
- The client should wait for the SSL handshake and ALPN to complete before the test exits.
Result:
SniHandlerTest.testSniWithApnHandler is more reliable.
Motivation:
ApplicationProtocolNegotiationHandler attempts to get a reference to an SslHandler in handlerAdded, but when SNI is in use the actual SslHandler will be added to the pipeline dynamically at some later time. When the handshake completes ApplicationProtocolNegotiationHandler throws an IllegalStateException because its reference to SslHandler is null.
Modifications:
- Instead of saving a reference to SslHandler in handlerAdded just search the pipeline when the SslHandler is needed
Result:
ApplicationProtocolNegotiationHandler support SniHandler.
Fixes https://github.com/netty/netty/issues/5066
Motivation:
There are some use cases when a client may only be willing to read from a channel once
its previous write is finished (eg: serial dispatchers in Finagle). In this case, a
connection with SslHandler installed and ctx.channel().config().isAutoRead() == false
will stall in 100% of cases no matter what order of "channel active", "write", "flush"
events was.
The use case is following (how Finagle serial dispatchers work):
1. Client writeAndFlushes and waits on a write-promise to perform read() once it's satisfied.
2. A write-promise will only be satisfied once SslHandler finishes with handshaking and
sends the unencrypted queued message.
3. The handshaking process itself requires a number of read()s done by a client but the
SslHandler doesn't request them explicitly assuming that either auto-read is enabled
or client requested at least one read() already.
4. At this point a client will stall with NEED_UNWRAP status returned from underlying engine.
Modifiations:
Always request a read() on NEED_UNWRAP returned from engine if
a) it's handshaking and
b) auto read is disabled and
c) it wasn't requested already.
Result:
SslHandler is now completely tolerant of whether or not auto-read is enabled and client
is explicitly reading a channel.
Motivation:
SSLContext.buildTrustManagerFactory(...) builds a KeyStore to
initialize the TrustManagerFactory from an array of X509Certificates,
assuming that array is a chain and that each certificate will have a
unique Subject Distinguised Name.
However, the collection of certificates used as trust anchors is generally
not a chain (it is an unordered collection), and it is legitimate for it
to contain multiple certificates with the same Subject DN.
The existing code uses the Subject DN as the alias name when filling in
the `KeyStore`, thereby overwriting other certificates with the same
Subject DN in this collection, so some certificates may be discarded.
In addition, the code related to building trust managers can take an array of
X509Certificate instances to use as trust anchors. The variable name is
usually trustCertChain, and the documentation refers to them as a "chain".
However, while it makes sense to talk about a "chain" from a keymanager
point of view, these certificates are just an unordered collection in a
trust manager. (There is no chaining requirement, having the Subject DN
matching its predecessor's Issuer DN.)
This can create confusion to for users not used with PKI concepts.
Modifications:
SSLContext.buildTrustManagerFactory(...) now uses a distinct alias for each
array (simply using a counter, since this name is never used for reference
later). This patch also includes a unit test with CA certificates using the
same Subject DN.
Also renamed trustCertChain into trustCertCollection, and changed the
references to "chain" in the Javadoc.
Result:
Each loaded certificate now has a unique identifier when loaded, so it is
now possible to use multiple certificates with the same Subject DN as
trust anchors.
Hopefully, renaming the parameter should also reduce confusion around PKI
concepts.
Motivation:
We need to ensure we call ctx.flush() before closing the actual channel when an handshake failure took place. If we miss to do so we may not send all pending data to the remote peer which also include SSL alerts.
Modifications:
Ensure we call ctx.flush() before ctx.close() on a handshake error.
Result:
All pending data (including SSL alerts) are written to the remote peer on a handshake error.
Motivation:
A user may use a private key which is encrypted with an empty password. Because of this we should only handle a null password in a special way.
Modifications:
- Correctly handle private key that is encrypted with empty password.
- Make OpenSsl*Context implementions consistent in terms of initialization in the constructor.
Result:
Correctly support private key that is encrypted with empty password.
Motivation:
netty-tcnative-1.1.33.Fork was released, we should upgrade. Also we should skip renegotiate tests if boringssl is used because boringssl does not support renegotiation.
Modifications:
- Upgrade to netty-tcnative-1.1.33.Fork13
- Skip renegotiate tests if boringssl is used.
Result:
Use newest version of netty-tcnative and be able to build if boringssl is used.
Motivation:
We need to enable SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER when using OpenSslContext as the memory address of the buffer that is passed to OpenSslEngine.wrap(...) may change during calls and retries. This is the case as
if the buffer is a heap-buffer we will need to copy it to a direct buffer to hand it over to the JNI layer. When not enable this mode we may see errors like: 'error:1409F07F:SSL routines:SSL3_WRITE_PENDING: bad write retry'.
Related to https://github.com/netty/netty-tcnative/issues/100.
Modifications:
Explitict set mode to SSL.SSL_MODE_RELEASE_BUFFERS | SSL.SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER . (SSL.SSL_MODE_RELEASE_BUFFERS was used before implicitly).
Result:
No more 'error:1409F07F:SSL routines:SSL3_WRITE_PENDING: bad write retry' possible when writing heap buffers.
Motivation:
When using SslProvider.OPENSSL we currently not handle SNI on the client side.
Modifications:
Correctly enable SNI when using clientMode and peerHost != null.
Result:
SNI works even with SslProvider.OPENSSL.
