Motivation:
Some of the features we want to support can only be supported by some of the SslContext implementations. We should allow to configure these in a consistent way the same way as we do it with Channel / ChannelOption
Modifications:
- Add SslContextOption and add builder methods that take these
- Add OpenSslContextOption and define two options there which are specific to openssl
Result:
More flexible configuration and implementation of SslContext
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
This brings forward the build and release automation changes from 4.1 (#10879, #10883, #10884, #10886, #10888, #10889, #10893, #10900, #10933, #10945, #10966, #10968, #11002, and #11019) to 5.0.
Details are as follows:
* Use Github workflows for CI (#10879)
Motivation:
We should just use GitHub Actions for the CI
Modifications:
- Adjust docker / docker compose files
- Add different workflows and jobs to deploy and build the project
Result:
Don't depend on external CI services
* Fix non leak build condition
* Only use build and deploy workflows for 4.1 for now
* Add deploy job for cross compiled aarch64 (#10883)
Motivation:
We should also deploy snapshots for our cross compiled native jars.
Modifications:
- Add job and docker files for deploying cross compiled native jars
- Ensure we map the maven cache into our docker containers
Result:
Deploy aarch64 jars and re-use cache
* Use correct docker-compose file to deploy cross compiled artifacts
* Use correct docker-compose task to deploy for cross compiled artifacts
* Split pr and normal build (#10884)
Motivation:
We should better use seperate workflows for PR and normal builds
Modifications:
- Split workflows
- Better cache reuse
Result:
Cleanup
* Only deploy snapshots for one arch
Motivation:
We need to find a way to deploy SNAPSHOTS for different arch with the same timestamp. Otherwise it will cause problems.
See https://github.com/netty/netty/issues/10887
Modification:
Skip all other deploys then x86_64
Result:
Users are able to use SNAPSHOTS for x86_6
* Use maven cachen when running analyze job (#10888)
Motivation:
To prevent failures to problems while downloading dependencies we shoud cache these
Modifications:
Add maven cache
Result:
No more failures due problems while downloading dependencies
* Also include one PR job that uses boringssl (#10886)
Motivation:
When validating PRs we should also at least run one job that uses boringssl
Modifications:
- Add job that uses boringssl
- Cleanup docker compose files
- Fix buffer leak in test
Result:
Also run with boringssl when PRs are validated
* Use matrix for job configurations (#10889)
Motivation:
We can use the matrix feature to define our jobs. This reduces a lot of config
Modification:
Use job matrix
Result:
Easier to maintain
* Correctly deploy artifacts that are build on different archs (#10893)
Motivation:
We need to take special care when deploying snapshots as we need to generate the jars in multiple steps
Modifications:
- Use the nexus staging pluging to stage jars locally in multiple steps
- Add extra job that will merge these staged jars and deploy these
Result:
Fixes https://github.com/netty/netty/issues/10887
* Dont use cron for PRs
Motivation:
It doesnt make sense to use cron for PRs
Modifications:
Remove cron config
Result:
Cleanup
* We run all combinations when validate the PR, let's just use one type for normal push
Motivation:
Let us just only use one build config when building the 4.1 branch.
Modifications:
As we already do a full validation when doing the PR builds we can just only use one build config for pushes to the "main" branches
Result:
Faster build times
* Update action-docker-layer-caching (#10900)
Motivation:
We are three releases behind.
Modifications:
Update to latest version
Result:
Use up-to-date action-docker-layer-caching version
* Verify we can load native modules and add job that verifies on aarch64 as well (#10933)
Motivation:
As shown in the past we need to verify we actually can load the native as otherwise we may introduce regressions.
Modifications:
- Add new maven module which tests loading of native modules
- Add job that will also test loading on aarch64
Result:
Less likely to introduce regressions related to loading native code in the future
* Let script fail if one command fail (#10945)
Motivation:
We should use `set -e` to ensure we fail the script if one command fails.
Modifications:
Add set -e to script
Result:
Fail fast
* Use action to report unit test errors (#10966)
Motivation:
To make it easier to understand why the build fails lets use an action that will report which unit test failed
Modifications:
- Replace custom script with action-surefire-report
Result:
Easier to understand test failures
* Use custom script to check for build failures (#10968)
Motivation:
It turns out we can't use the action to check for build failures as it can't be used when a PR is done from a fork. Let's just use our simple script.
Modifications:
- Replace action with custom script
Result:
Builds for PRs that are done via forks work again.
* Publish test results after PR run (#11002)
Motivation:
To make it easier to understand why a build failed let us publish the rest results
Modifications:
Use a new workflow to be able to publish the test reports
Result:
Easier to understand why a PR did fail
* Fix test reports name
* Add workflow to cut releases (#11019)
Motivation:
Doing releases manually is error-prone, it would be better if we could do it via a workflow
Modification:
- Add workflow to cut releases
- Add related scripts
Result:
Be able to easily cut a release via a workflow
* Update build for master branch
Motivation:
The build changes were brought forward from 4.1, and contain many things specific to 4.1.
Modification:
Changed baseline Java version from 8 to 11, and changed branch references from "4.1" to "master".
Result:
Builds should now work for the master branch.
Co-authored-by: Norman Maurer <norman_maurer@apple.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:
File.createTempFile(String, String)` will create a temporary file in the system temporary directory if the 'java.io.tmpdir'. The permissions on that file utilize the umask. In a majority of cases, this means that the file that java creates has the permissions: `-rw-r--r--`, thus, any other local user on that system can read the contents of that file.
This can be a security concern if any sensitive data is stored in this file.
This was reported by Jonathan Leitschuh <jonathan.leitschuh@gmail.com> as a security problem.
Modifications:
Use Files.createTempFile(...) which will use safe-defaults when running on java 7 and later. If running on java 6 there isnt much we can do, which is fair enough as java 6 shouldnt be considered "safe" anyway.
Result:
Create temporary files with sane permissions by default.
Motivation:
We didnt correctly filter out TLSv1.3 ciphers when TLSv1.3 is not enabled.
Modifications:
- Filter out ciphers that are not supported due the selected TLS version
- Add unit test
Result:
Fixes https://github.com/netty/netty/issues/10911
Co-authored-by: Bryce Anderson <banderson@twitter.com>
Motivation:
If the given port is already bound, the PcapWriteHandlerTest will sometimes fail.
Modification:
Use a dynamic port using `0`, which is more reliable
Result:
Less Flaky
Motivation:
We should expose some methods as protected to make it easier to write custom SslContext implementations.
This will be reused by the code for https://github.com/netty/netty-incubator-codec-quic/issues/97
Modifications:
- Add protected to some static methods which are useful for sub-classes
- Remove some unused methods
- Move *Wrapper classes to util package and make these public
Result:
Easier to write custom SslContext implementations
Motivation:
We need to ensure we always drain the error stack when a callback throws as otherwise we may pick up the error on a different SSL instance which uses the same thread.
Modifications:
- Correctly drain the error stack if native method throws
- Add a unit test which failed before the change
Result:
Always drain the error stack
Motivation:
When using the JDKs SSLEngineImpl with TLSv1.3 it sometimes returns HandshakeResult.FINISHED multiple times. This can lead to have SslHandshakeCompletionEvents to be fired multiple times.
Modifications:
- Keep track of if we notified before and if so not do so again if we use TLSv1.3
- Add unit test
Result:
Consistent usage of events
Motivation:
FingerprintTrustManagerFactory can only use SHA-1 that is considered
insecure.
Modifications:
- Updated FingerprintTrustManagerFactory to accept a stronger hash algorithm.
- Remove the constructors that still use SHA-1.
- Added a test for FingerprintTrustManagerFactory.
Result:
A user can now configure FingerprintTrustManagerFactory to use a
stronger hash algorithm.
Co-authored-by: Norman Maurer <norman_maurer@apple.com>
Motivation:
HTTP is a plaintext protocol which means that someone may be able
to eavesdrop the data. To prevent this, HTTPS should be used whenever
possible. However, maintaining using https:// in all URLs may be
difficult. The nohttp tool can help here. The tool scans all the files
in a repository and reports where http:// is used.
Modifications:
- Added nohttp (via checkstyle) into the build process.
- Suppressed findings for the websites
that don't support HTTPS or that are not reachable
Result:
- Prevent using HTTP in the future.
- Encourage users to use HTTPS when they follow the links they found in
the code.
Motivation:
junit deprecated Assert.assertThat(...)
Modifications:
Use MatcherAssert.assertThat(...) as replacement for deprecated method
Result:
Less deprecation warnings
Motivation:
We need to take the Provider into account as well when trying to detect if TLSv1.3 is used by default / supported
Modifications:
- Change utility method to respect provider as well
- Change testcode
Result:
Less error-prone tests
Motivation:
Some JDKs dissallow the usage of keysizes < 2048, so we should not use such small keysizes in tests.
This showed up on fedora 32:
```
Caused by: java.security.cert.CertPathValidatorException: Algorithm constraints check failed on keysize limits. RSA 1024bit key used with certificate: CN=tlsclient. Usage was tls client
at sun.security.util.DisabledAlgorithmConstraints$KeySizeConstraint.permits(DisabledAlgorithmConstraints.java:817)
at sun.security.util.DisabledAlgorithmConstraints$Constraints.permits(DisabledAlgorithmConstraints.java:419)
at sun.security.util.DisabledAlgorithmConstraints.permits(DisabledAlgorithmConstraints.java:167)
at sun.security.provider.certpath.AlgorithmChecker.check(AlgorithmChecker.java:326)
at sun.security.provider.certpath.PKIXMasterCertPathValidator.validate(PKIXMasterCertPathValidator.java:125)
... 23 more
```
Modifications:
Replace hardcoded keys / certs with SelfSignedCertificate
Result:
No test-failures related to small key sizes anymore.
Motivation:
We need to let openssl know that we failed to find the key material so it will produce an alert for the remote peer to consume. Beside this we also need to ensure we wrap(...) until we produced everything as otherwise the remote peer may see partial data when an alert is produced in multiple steps.
Modifications:
- Correctly throw if we could not find the keymaterial
- wrap until we produced everything
- Add test
Result:
Correctly handle the case when key material could not be found
Motivation:
Write TCP and UDP packets into Pcap `OutputStream` which helps a lot in debugging.
Modification:
Added TCP and UDP Pcap writer.
Result:
New handler can write packets into an `OutputStream`, e.g. a file that can be opened with Wireshark.
Fixes#10385.
Motivation:
This reverts commit 7bf1ffb2d4ba731feb7b12db1ae1d4148f20c3f4 as it turns out it introduced a big performance regression.
Modifications:
Revert 7bf1ffb2d4ba731feb7b12db1ae1d4148f20c3f4
Result:
Performance of TLS is back to normal
Motivation:
`IpSubnetFilter` uses Binary Search for IP Address search which is fast if we have large set of IP addresses to filter.
Modification:
Added `IpSubnetFilter` which takes `IpSubnetFilterRule` for filtering.
Result:
Faster IP address filter.
Motivation:
This reverts commit b559711f3e1b712c3f1dfc77d5504cd1ca58aa38 due regression introduced by it.
Modification:
Revert commit
Result:
Fixes https://github.com/netty/netty/issues/10464
Motiviation:
When TLSv1.3 was introduced almost 2 years ago, it was decided to disable it by default, even when it's supported by the underlying TLS implementation.
TLSv13 is pretty stable now in Java (out of the box in Java 11, OpenJSSE for Java 8, BoringSSL and OpenSSL) and may be enabled by default.
Modifications:
Ensure TLSv13 is enabled by default when the underyling JDK SSLEngine implementation enables it as well
Result:
TLSv1.3 is now enabled by default, so users don't have to explicitly enable it.
Co-authored-by: Stephane Landelle <slandelle@gatling.io>
Motivation:
TLSv1.3 is not strictly limited to Java11+ anymore as different vendors backported TLSv1.3 to Java8 as well. We should ensure we make the detection of if TLSv1.3 is supported not depend on the Java version that is used.
Modifications:
- Add SslProvider.isTlsv13Supported(...) and use it in tests to detect if we should run tests against TLSv1.3 as well
- Adjust testcase to work on latest JDK 8 release as well
Result:
Correct detection of TLSv1.3 support even if Java version < 11.
Motivation:
At the moment we don't support session caching for client side when using native SSLEngine implementation and our implementation of SSLSessionContext is incomplete.
Modification:
- Consume netty-tcnative changes to be able to cache session in an external cache
- Add and adjust unit tests to test session caching
- Add an in memory session cache that is hooked into native SSLEngine
Result:
Support session caching on the client and server side
Motivation:
There was a new netty-tcnative release which we should use. Beside this the SSLErrorTest was quite fragile and so should be adjusted.
Modifications:
Update netty-tcnative and adjust test
Result:
Use latest netty-tcnative release
Motivation:
BoringSSL behaves differently then OpenSSL and not include any TLS1.3 ciphers in the returned array when calling SSL_get_ciphers(...). This is due the fact that it also not allow to explicit configure which are supported and which not for TLS1.3. To mimic the behaviour expected by the SSLEngine API we should workaround this.
Modifications:
- Add a unit test that verifies enabled protocols / ciphers
- Add special handling for BoringSSL and tls1.3
Result:
Make behaviour consistent
Motivation:
Due a bug in our test we may dropped data on the floor which are generated during handshaking (or slightly after). This could lead to corrupt state in the engine itself and so fail tests. This is especially true for TLS1.3 which generates the sessions on the server after the "actual handshake" is done.
Modifications:
Contine with wrap / unwrap until all data was consumed
Result:
Correctly feed all data to the engine during testing
Motivation:
When ApplicationProtocolNegotiationHandler is in the pipeline we should expect that its handshakeFailure(...) method will be able to completly handle the handshake error. At the moment this is not the case as it only handled SslHandshakeCompletionEvent but not the exceptionCaught(...) that is also triggered in this case
Modifications:
- Call handshakeFailure(...) in exceptionCaught and so fix double notification.
- Add testcases
Result:
Fixes https://github.com/netty/netty/issues/10342
Motivation:
I did not correctly adjust the code before cherry-pick, causing a compilation error in the test
Modifications:
Use ChannelHandler
Result:
No more compilation error
The current FLowControlHandler keeps a flag to track whether a read() call is pending.
This could lead to a scenario where you call read multiple times when the queue is empty,
and when the FlowControlHandler Queue starts getting messages, channelRead will be fired only once,
when we should've fired x many times, once for each time the handlers downstream called read().
Modifications:
Minor change to replace the boolean flag with a counter and adding a unit test for this scenario.
Result:
I used TDD, so I wrote the test, made sure it's failing, then updated the code and re-ran the test
to make sure it's successful after the changes.
Co-authored-by: Kareem Ali <kali@localhost.localdomain>
Motivation:
9b7e091 added a special SSLHandshakeException sub-class to signal handshake timeouts but we missed to update a testcase to directly assert the type of the exception.
Modifications:
Assert directly that SslHandshakeTimeoutException is used
Result:
Test cleanup
Motivation:
It seems that `testSwallowedReadComplete(...)` may fail with an AssertionError sometimes after my tests. The relevant stack trace is as follows:
```
java.lang.AssertionError: expected:<IdleStateEvent(READER_IDLE, first)> but was:<null>
at org.junit.Assert.fail(Assert.java:88)
at org.junit.Assert.failNotEquals(Assert.java:834)
at org.junit.Assert.assertEquals(Assert.java:118)
at org.junit.Assert.assertEquals(Assert.java:144)
at io.netty.handler.flow.FlowControlHandlerTest.testSwallowedReadComplete(FlowControlHandlerTest.java:478)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:89)
at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:41)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:542)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:770)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:464)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:210)
```
Obviously the `readerIdleTime` of `IdleStateHandler` and the thread sleep time before `EmbeddedChannel.runPendingTasks` are both 100ms. And if `userEvents.poll()` happened before `userEvents.add(...)` or no `IdleStateEvent` fired at all, this test case would fail.
Modification:
Sleep for a little more time before running all pending tasks in the `EmbeddedChannel`.
Result:
Fix the problem of small probability of failure.
Motivation:
SslHandler currently throws a general SSLException if a wrap attempt
fails due to the SSLEngine being closed. If writes are queued the
failure rational typically requires more investigation to track down the
original failure from a previous event. We may have more informative
rational for the failure and so we should use it.
Modifications:
- SslHandler#wrap to use failure information from the handshake or prior
transport closure if available
Result:
More informative exceptions from SslHandler#wrap if the SSLEngine has
been previously closed.
Motivation:
The `FlowControlHandler` may cache the received messages in a queue in order to do the flow control. However, if this handler is manually removed from pipeline during runtime, those cached messages might not be passed to the next channel handler forever.
Modification:
Dequeue all these cached messages and call `ChannelHandlerContext.fireChannelRead(...)` in `handlerRemoved(...)` method.
Result:
Avoid losing the received messages.
Motivation:
`SslHandlerTest.testSessionTicketsWithTLSv12AndNoKey` does not require
BoringSSL and works with OpenSSL as well.
Modifications:
- Remove assume statement that expected BoringSSL;
Result:
Test works for any implementation of `OPENSSL` provider.
Motivation:
BoringSSL supports to automatically manage the session tickets to be used and so also rotate them etc. This is often prefered by users as it removed some complexity. We should support to make use of this.
Modifications:
- Allow to have setSessionTickets() called without an argument or an empty array
- Add tests
Result:
Easier usage of session tickets
Motivation:
OpenSslSession.getLocalCertificates() and getLocalPrincipal() must return null on client side if mTLS is not used as stated in the API documentation. At the moment this is not always the case
Modifications:
- Ensure we only return non-null if mTLS is used
- Add unit tests
Result:
Follow SSLSession API contract
Motivation:
We are far behind with the version of Conscrypt we are using during testing. We should ensure we use the latest.
Modifications:
- Update conscrypt dependency
- Ensure we use conscrypt provider in tests
- Add workarounds for conscrypt bugs in testsuite
Result:
Use latest Conscrypt release
Motivation:
Currently calculateSize method in AbstractTrafficShapingHandler calculates size for object of type ByteBuf or ByteBufHolder. Adding a check for FileRegion, makes it possible to do traffic shaping for FileRegion objects as well
Modification:
Check if object to be sent is of type FileRegion, if yes calculate the size using its count() method.
Co-authored-by: Dinesh Joshi <dinesh.joshi@apple.com>
Motivation:
We need to make some slightly changes to be able to build on Java15 as some previous deprecated methods now throw UnsupportedOperationException
Modifications:
- Add code to handle UnsupportedOperationException
- Revert previous applied workaround for bug that was fixed in Java15
- Add maven profile
Result:
Be able to build with latest Java15 EA release
Motivation:
ALPN support was backported to java 8 lately. Ensure we support it if the user uses the latest java 8 release
Modifications:
- Update logic to be able to detect if ALPN is supported out of the box when using Java8
- Update jetty alpn version
Result:
Be able to use ALPN out of the box when using java 8u251
Motivation:
The `AbstractTrafficShapingHandler` caches the `ReopenReadTimerTask` instance in the channel attribute. However, if this handler is removed from the channel pipeline, this `ReopenReadTimerTask` instance may not be released.
Modification:
Release the channel attribute `REOPEN_TASK` in `handlerRemoved` method.
Result:
Avoid a channel attribute leak.
Motivation:
If the SslHandler is removed from the pipeline we also need to ensure we fail the handshake / close promise if it was not notified before as otherwise we may never do so.
Modifications:
- Correctly fail promise and notify pipeline if handshake was not done yet when the SslHandler is removed
- Add unit test
Result:
Fix https://github.com/netty/netty/issues/10158
Motivation:
JDK 14 was released and need some special settings to be able to build with. Also there seems to be one regression that we need to workaround for now.
Modifications:
- Add maven profile for JDK 14
- Update blockhound version to be able to work on JDK 14
- Add workaround for possible JDK 14 regression
Result:
Be able to build on JDK 14
Motivation:
Before release 4.1.23, there was only ONE call to exceptionCaught method when an ssl handshake failure occurs, now we have two when using the JDK provider.
Modifications:
- Ensure we only propagate one exception fi we already failed the handshake and channelInactive(...) produce an exception again
- Add unit test
Result:
Fixes https://github.com/netty/netty/issues/10119
Motivation:
At the moment the next / prev references are not set to "null" in the DefaultChannelHandlerContext once the ChannelHandler is removed. This is bad as it basically let users still use the ChannelHandlerContext of a ChannelHandler after it is removed and may produce very suprising behaviour.
Modifications:
- Fail if someone tries to use the ChannelHandlerContext once the ChannelHandler was removed (for outbound operations fail the promise, for inbound fire the error through the ChannelPipeline)
- Fix some handlers to ensure we not use the ChannelHandlerContext after the handler was removed
- Adjust DefaultChannelPipeline / DefaultChannelHandlerContext to fixes races with removal / replacement of handlers
Result:
Cleanup behaviour and make it more predictable for pipeline modifications
Motivation:
Often it is useful to be able to detect different sorts of SSL errors that cause the handshake to fail. To make this easier we should throw and explicit exception type for handshake timeouts.
Modifications:
- Add SslHandshakeTimeoutException (which extends SSLHandshakeException) and use it for handshake timeouts
- Adjust testcases
Result:
Easier to detect that handshake failed because of a timeout
Motivation:
We did not correctly account for produced bytes when SSL_write(...) returns -1 in all cases. This could lead to lost data and so a corrupt SSL connection.
Modifications:
- Always ensure we calculate the produced bytes correctly
- Add unit tests
Result:
Fixes https://github.com/netty/netty/issues/10041
