Motivation:
As mentioned in RFC 7692 :
The "server_no_context_takeover" Extension Parameter should be used on server side for compression and on client side for decompression.
The "client_no_context_takeover" Extension Parameter should be used on client side for compression and on server side for decompression.
Right now, in PerMessageDeflateClientExtensionHandshaker, the decoder uses clientNoContext instead of serverNoContext and the encoder uses serverNoContext instead of clientNoContext.
The same inversion is present in PerMessageDeflateServerExtensionHandshaker: the decoder uses
serverNoContext instead of clientNoContext, while the encoder uses serverNoContext instead of clientNoContext. Besides the context inversion, the sliding window sizes seem to be inversed as well.
Modification:
Inverse clientNoContext with serverNoContext and clientWindowSize with serverWindowSize for both the Decoder and Encoder in PerMessageDeflateServerExtensionHandshaker and PerMessageDeflateClientExtensionHandshaker.
Result:
This fixes the decompression fail in the case that one of the contexts is set and the other one is not.
Motivation:
At the moment it's only possible to use TLSv1.3 with netty-tcnative if Java 11 is used. It should be possible to do so even with Java 8, 9 and 10.
Modification:
Add a workaround to be able to use TLSv1.3 also when using Java version prior to Java 11 and the default X509ExtendedTrustManager is used.
Result:
Be able to use TLSv1.3 also with past versions of Java.
Motivation:
We should explicit specify the java version to use to ensure docker will rebuild the image once a new java version was released and we specify it. Also we should use openjdk for testing when possible.
Modifications:
- Explicit specify the java versions to use
- Use openjdk when possible.
Result:
Ensure latest java versions are used during testing
Motivation:
We need to update jetty-alpn-agent to be able to run tests with OpenJDK 8u191
Modifications:
Update to 2.0.8
Result:
Be able to run tests with latest JDK 8 release.
Motivation:
When the constructor of OpenSslEngine threw we could end up to self call SSL_free by ourself and then have the finalizer do the same which may lead to double free-ing and so SIGSEV.
Modifications:
Just call shutdown() when the constructor throws and so ensure SSL_free is guarded correctly in the finalizer.
Result:
No more SIGSEV possible.
Motivation:
We publish an "uber-jar" for netty-tcnative-boringssl-static so we should use it when testing against boringssl.
Modifications:
Ensure we use empty classifier.
Result:
Use uber-jar when testing
Motivation:
TLSv1.3 support is included in java11 and is also supported by OpenSSL 1.1.1, so we should support when possible.
Modifications:
- Add support for TLSv1.3 using either the JDK implementation or the native implementation provided by netty-tcnative when compiled against openssl 1.1.1
- Adjust unit tests for semantics provided by TLSv1.3
- Correctly handle custom Provider implementations that not support TLSv1.3
Result:
Be able to use TLSv1.3 with netty.
Motivation:
JdkSslContext provides public constructors to wrap an existing `javax.net.ssl.SSLContext`.
Sadly, some options combinations are not possible with the existing constructors, eg:
* protocols is not exposed and always forced to null, so default protocols are always enforced
* startTls is not exposed and always forced to false
Modification:
Add full constructor that take protocols and startTls parameters.
Result:
It's possible to create a JdkSslContext from an existing SSLContext and still have control over protocols and startTls
Motivation:
- Version 0.3 would sometimes fail to report failing tests
- New version contains upgraded version of h2spec
Modifications:
- Bump h2spec-maven-plugin to 0.6
- Remove excluded specs that are no passing
- Add failing spec "half closed (remote): Sends a HEADERS frame" to
exclude list
Result:
Build will fail when non excluded specs fails.
Motivation:
Java since version 6 has the wrapper for the ConcurrentHashMap that could be created via Collections.newSetFromMap(map). So no need to create own ConcurrentSet class. Also, since netty plans to switch to Java 8 soon there is another method for that - ConcurrentHashMap.newKeySet().
For now, marking this class @deprecated would be enough, just to warn users who use netty's ConcurrentSet. After switching to Java 8 ConcurrentSet should be removed and replaced with ConcurrentHashMap.newKeySet().
Modification:
ConcurrentSet deprecated.
Motivation:
It has shown that the used test timeout may be too low when the CI is busy.
Modifications:
Increase timeout to 3 seconds.
Result:
Less false-positives.
Motivation:
While looking at the nice optimization done in
https://github.com/netty/netty/pull/8347 I couldn't help noticing the
logic could be simplified further. Apologies if this is just my OCD and
inappropriate!
Modifications:
Reduce amount of code used for ByteBufInputStream.readLine()
Result:
Slightly smaller and simpler code
Motivation:
MACOSX_DEPLOYMENT_TARGET=10.6 needs to be used as everything before is not supported in 10.14 anymore. 10.6 was released 2009 so this should be a safe thing to do.
Modifications:
Use MACOSX_DEPLOYMENT_TARGET=10.6
Result:
Be able to compile on MacOS 10.14
Motivation:
It is possible that a client is unable to locate a certificate alias given the list of issuers and key types. In this case the X509KeyManager
will return a null which when past to the OpenSslKeyMaterialProvider implementation may produce a NPE. If no matching alias could be found we should not
call OpenSslKeyMaterialProvider at all which is also consistent what OpenJDK does.
Modifications:
- Add null check before calling OpenSslKeyMaterialProvider
- Add unit test.
Result:
No more NPE caused by passing null as client alias.
Motivation:
epoll_wait should work in 4.1.30 like it did in 4.1.29.
Modifications:
Revert Integer.MAX_VALUE back to MAX_SCHEDULED_TIMERFD_NS (999,999,999).
Add unit test.
Result:
epoll_wait will no longer throw EINVAL.
Motivation:
ba594bcf4a added a utility to parse searchdomains defined in /etc/resolv.conf but did not correctly handle the case when multiple are defined that are seperated by either whitespace or tab.
Modifications:
- Correctly parse multiple entries
- Add unit test.
Result:
Correctly parse multiple searchdomain entries.
Motivation:
Currently we may end up in the situation that we incremented the pending bytes before submitting the AbstractWriteTask but never decrement these again if the submitting of the task fails. This may result in incorrect watermark handling.
Modifications:
- Correctly decrement pending bytes if subimitting of task fails and also ensure we recycle it correctly.
- Add unit test.
Result:
Fixes https://github.com/netty/netty/issues/8343.
Motivation:
Seems like IntegerHolder counterHashCode field is the very old legacy field that is no longer used. Should be marked as deprecated and removed in the future versions.
Modification:
IntegerHolder class, InternalThreadLocalMap.counterHashCode() and InternalThreadLocalMap.setCounterHashCode(IntegerHolder counterHashCode) are now deprecated.
Motivation:
Avoid creating any StringBuilder instance if
ByteBufInputStream::readLine isn't used
Modifications:
The StringBuilder instance is lazy allocated on demand and
are added new test case branches to address the increased
complexity of ByteBufInputStream::readLine
Result:
Reduced GC activity if ByteBufInputStream::readLine isn't used
Motivation:
Unless the 'io.netty.noKeySetOptimization' system property is set,
registering a SelectableChannel instance to a NioEventLoop results
in a ClassCastException:
io.netty.channel.nio.SelectedSelectionKeySetSelector cannot be cast
to java.nio.channels.spi.AbstractSelector
Modifications:
Instead of 'selector', pass 'unwrappedSelector' to SelectableChannel.
Result:
It is possible to register a SelectableChannel instance without
setting the 'io.netty.noKeySetOptimization' system property.
Motivation:
Add an option (through a SelectStrategy return code) to have the Netty event loop thread to do busy-wait on the epoll.
The reason for this change is to avoid the context switch cost that comes when the event loop thread is blocked on the epoll_wait() call.
On average, the context switch has a penalty of ~13usec.
This benefits both:
The latency when reading from a socket
Scheduling tasks to be executed on the event loop thread.
The tradeoff, when enabling this feature, is that the event loop thread will be using 100% cpu, even when inactive.
Modification:
Added SelectStrategy option to return BUSY_WAIT
Epoll loop will do a epoll_wait() with no timeout
Use pause instruction to hint to processor that we're in a busy loop
Result:
When enabled, minimizes impact of context switch in the critical path
Motivation:
When writing an HTTP/2 HEADERS with END_STREAM=1, the application expects
the stream to be closed afterward. However, the write can fail locally
due to HPACK encoder and similar. When that happens we need to make sure
to issue a RST_STREAM otherwise the stream can be closed locally but
orphaned remotely. The RST_STREAM is typically handled by
Http2ConnectionHandler.onStreamError, which will only send a RST_STREAM
if that stream still exists locally.
There are two possible flows for trailers, one handled immediately and
one going through the flow controller. Previously they behaved
differently, with the immedate code calling the error handler after
closing the stream. The immediate code also used a listener for calling
closeStreamLocal while the flow controlled code did so immediately after
the write.
The two code paths also differed in their VoidChannelPromise handling,
but both were broken. The immediate code path called unvoid() only if
END_STREAM=1, however it could always potentially add a listener via
notifyLifecycleManagerOnError(). And the flow controlled code path
unvoided incorrectly, changing the promise completion behavior. It also
passed the wrong promise to closeStreamLocal() in FlowControlledBase.
Modifications:
Move closeStreamLocal handling after calls to onError. This is the
primary change.
Now call closeStreamLocal immediately instead of when the future
completes. This is the more likely correct behavior as it matches that
of DATA frames.
Fix all the VoidChannelPromise handling.
Result:
Http2ConnectionHandler.onStreamError sees the same state as the remote
and issues a RST_STREAM, properly cleaning up the stream.
Motivation
The EpollChannelConfig (same for KQueues) and its subclasses repeatetly declare their own channel field which leads to a 3x repetition for each config instance. Given the fields are protected or package-private it's exposing the code code to "field hiding" bugs.
Modifications
Use the the existing protected channel field from the DefaultChannelConfig class and simply cast it when needed.
Result
Fixes#8331
Motivation:
Before when on server-side we just called the X509KeyManager methods when handshake() was called the first time which is not quite correct as we may not have received the full SSL hello / handshake and so could not extra for example the SNI hostname that was requested.
OpenSSL exposes the SSL_CTX_set_cert_cb function which allows to set a callback which is executed at the correct moment, so we should use it. This also allows us to support more methods of ExtendedSSLSession easily.
Modifications:
- Make use of new methods exposed by netty-tcnative since https://github.com/netty/netty-tcnative/pull/388 to ensure we select the key material at the correct time.
- Implement more methods of ExtendedOpenSslSession
- Add unit tests to ensure we are able to retrieve various things on server-side in the X509KeyManager and so verify it is called at the correct time.
- Simplify code by using new netty-tcnative methods.
Result:
More correct implementation for server-side usage and more complete implemented of ExtendedSSLSession.
Motivation:
a208f6dc7c added a testcase which uses hostname validation which may not be supported by OpenSSL depending on the version that is used. We should check first before we try to use it.
Modifications:
Add assumeTrue(...) check to ensure hostname validation is supported before trying to run the test.
Result:
No more test-failures on OpenSSL versions < 1.0.2.
Motivation:
When a X509TrustManager is used while configure the SslContext the JDK automatically does some extra checks during validation of provided certs by the remote peer. We should do the same when our native implementation is used.
Modification:
- Automatically wrap a X509TrustManager and so do the same validations as the JDK does.
- Add unit tests.
Result:
More consistent behaviour. Fixes https://github.com/netty/netty/issues/6664.
* Use AuthoritativeDnsServerCache for creating the new redirect stream.
Motivation:
At the moment if a user wants to provide custom sorting of the nameservers used for redirects it needs to be implemented in two places. This is more complicated as it needs to be.
Modifications:
- Just delegate to the AuthoritativeDnsServerCache always as we fill it before we call newRedirectDnsServerStream anyway.
Result:
Easier way for the user to implement custom sorting.
* Add cache for CNAME mappings resolved during lookup of DNS entries.
Motivation:
If the CNAMEd hostname is backed by load balancing component, typically the final A or AAAA DNS records have small TTL. However, the CNAME record itself is setup with longer TTL.
For example:
* x.netty.io could be CNAMEd to y.netty.io with TTL of 5 min
* A / AAAA records for y.netty.io has a TTL of 0.5 min
In current Netty implementation, original hostname is saved in resolved cached with the TTL of final A / AAAA records. When that cache entry expires, Netty recursive resolver sends at least two queries — 1st one to be resolved as CNAME record and the 2nd one to resolve the hostname in CNAME record.
If CNAME record was cached, only the 2nd query would be needed most of the time. 1st query would be needed less frequently.
Modifications:
Add a new CnameCache that will be used to cache CNAMEs and so may reduce queries.
Result:
Less queries needed when CNAME is used.
Motivation:
The first EA builds for Java12 are released so we should allow to run with these in our docker-compose setup.
Modifications:
Add docker-compose configs for Java12.
Result:
Be able to run easily with Java12 as well.
Motivation:
4d1458604a did fix some leaks in SniClientTest but missed the ones in SniClientJava8TestUtil.
Modifications:
Correctly release SslContext.
Result:
No more leaks in SNI tests.
Motivation:
We should add some command to be able to run all tests with leak detection enabled. This will then be used on the CI during PR builds.
Modifications:
Add new docker-compose config to run with leak-detection enabled.
Result:
Easy way to enable leak detection while running tests via docker.
Motivation:
We need to ensure the Cumulator always releases the input buffer if it can not take over the ownership of it as otherwise it may leak.
Modifications:
- Correctly ensure the buffer is always released.
- Add unit tests.
Result:
Ensure buffer is always released.
Motivation
Applications should not depend on internal packages with Java 9 and later. This cause a warning now, but will break in future versions of Java.
Modification
This change adds methods to UnixResolverDnsServerAddressStreamProvider (following after #6844) that parse /etc/resolv.conf for domain and search entries. Then DnsNameResolver does not need to rely on sun.net.dns.ResolverConfiguration to do this.
Result
Fixes#8318. Furthermore, at least in my testing with Java 11, this also makes multiple search entries work properly (previously I was only getting the first entry).
Motivation:
First EA releases of Java12 are out we should be able to compile with these and run tests.
Modifications:
Add maven profile for java12.
Result:
Be able to use Java12
Motivation:
In theory our estimation of the needed buffer could be off and so we need to ensure we grow it if there is no space left.
Modifications:
Ensure we grow the buffer if there is no space left in there but we still have data to deflate.
Result:
Correctly deflate data in all cases.
Motivation:
I noticed that we had some errors showing up in a test (which did not fail it tho) because we tried to full-fill the promise multiples times.
Modifications:
Use trySuccess(...) as we may produce multiple exceptions.
Result:
Less errors during test-run.
Motivation:
Java9 added getStatusResponses() to ExtendedSSLSession which we should correctly support when possible.
Modifications:
Implement the method correctly.
Result:
More complete and correct implementation.
Motivation:
If you encode a SOCKS5 message like new DefaultSocks5CommandResponse(FAILURE, DOMAIN, "", 0)
you correctly get a result of 05010003000000.
But if the bndAddr is null, for example like new DefaultSocks5CommandResponse(FAILURE, DOMAIN)
the encoded result is 0501000301000000 which means the domain name has a length of one and consists of a 0-byte.
Modification:
With this commit it is also correctly encoded as a string of 0 length.
Result:
Correctly encode empty SOCKS5 address
Motivation:
Conscrypt 1.3.0 was just released and adds support for TLSv1.3
Modifications:
Update to 1.3.0
Result:
Use latest conscrypt during build / test.
Motivation:
Get charset from Content-Type header even it contains multiple parameters.
Modification:
Extract charset value from the charset parameter if it is not last.
Result:
Fixes#8273
Motivation:
6ed7c6c75d added support for ExtendedOpenSslSession but we did not override getStatusResponses(). This lead to test failures on java9.
Modifications:
Implement ExtendedOpenSslSession.getStatusResponses() so it just returns an empty list.
Result:
Test pass again on Java9.
Motivation:
6ed7c6c75d added a test which blindly assumed we can use a KeyManagerFactory all the time. This is only true if have OpenSSL 1.0.2 or later, which may not be the case.
Modifications:
Only use KeyManagerFactory in test if the OpenSSL version does support it.
Result:
More robust tests.
Motivation:
When an ExtendedSSLSession is used its possible to do more strict checking of the keys during handshake. We should do this whenever possible.
Modification:
- Return an ExtendedSSLSession when using client-mode and Java7+
- Add unit test
- Simplify unit tests
Result:
More consistent behaviour.
Motivation:
We should not try to cast the Channel to a DatagramChannel as this will cause a ClassCastException.
Modifications:
- Do not cast
- rethrow from constructor if we detect the registration failed.
- Add unit test.
Result:
Propagate correct exception.
