I can confirm that the PR checks pass after excluding TLSv1.3 from enabled protocols: https://github.com/apache/bookkeeper/pull/2696/commits/6003a374d5aec30d7059a21e473ac91417b5cdc3
There should be tests for both TLSv1.2 and TLSv1.3 because of the differences in TLS handshake described in https://stackoverflow.com/a/62465859 . This also impacts some production code in Bookkeeper. The PR already includes a change to catch SSLException instead of SSLHandshakeException ( https://github.com/apache/bookkeeper/pull/2696/commits/fcbd707a633ed1b8cf8290cb5d70a3070e010196). TLSv1.3 doesn't throw SSLHandshakeException for certificate issues because of the differences in the protocols. This change should work for both protocols, but we should have test coverage to ensure that. TLSv1.3 has been enabled by default since Netty 4.1.52.Final (when the JDK contains TLSv1.3). TLSv1.3 support has been available in Java 8 since 8u262 . One of the remaining problems with TLSv1.3 support in BK is the state machine and TLS counters in PerChannelBookieClient . It doesn't properly model the way TLS 1.3 behaves. Currently there's a counter FAILED_TLS_HANDSHAKE_COUNTER which is expected to count also the certificate issues (code: https://github.com/apache/bookkeeper/blob/fcbd707a633ed1b8cf8290cb5d70a3070e010196/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java#L1535-L1543 ). Since TLSv1.3 doesn't detect certificate issues (mutual TLS) during handshake, this counter doesn't count certificate issues. Certificate issues will show up as successfully established connections. The original issue for adding TLS counters was https://github.com/apache/bookkeeper/issues/1103 and PR commit was https://github.com/apache/bookkeeper/commit/fa10b7dcd89c40222ba5f30bb60f785bd21669b2 . How do we revisit the TLS counter solution for TLSv1.3 compatibility? Do we make changes to the code or do we simply skip the test on TLSv1.3 which ensures that a certificate issue is counted in FAILED_TLS_HANDSHAKE_COUNTER ? Skipping the test for TLSv1.3 would be one option. WDYT? -Lari On Wed, May 5, 2021 at 10:32 PM Andrey Yegorov <andrey.yego...@datastax.com> wrote: > Lari and I have looked at the Netty upgrade. > There are some test breaks, and so far everything is related to behavior > changes related to TLS 1.3, see https://stackoverflow.com/a/62465859 > We managed to fix some of the issues > https://github.com/apache/bookkeeper/pull/2696 but "the client won't know > whether the server has accepted the certificate or not until it next reads > data from the server" complicates things. > > Currently we are considering simply setting "java > -Djdk.tls.client.protocols=TLSv1.2" to unbreak the tests and handling > tls1.3 as a separate work item. > > Lari is planning on spending a little bit more time on this tomorrow (his > tomorrow) to see if there is a better way to address this quickly; we'll > hear more then. > > > On Wed, May 5, 2021 at 9:23 AM Henry Saputra <henry.sapu...@gmail.com> > wrote: > > > I am +1 for having next release as 4.14.0 > > > > - Henry > > > > On Tue, May 4, 2021 at 2:51 PM Andrey Yegorov < > andrey.yego...@datastax.com > > > > > wrote: > > > > > Overall +1 for 4.14.0 - the milestone is due May 16th anyway. > > > There is nothing that breaks compatibility with 4.13 so we can skip > > 4.13.1. > > > > > > One thing I'd love to see in 4.14 is > > > https://github.com/apache/bookkeeper/pull/2696 , to fix > > > https://github.com/netty/netty/issues/10986 > > > It looks like there are issues with vertx > > > https://github.com/apache/bookkeeper/pull/2693#issuecomment-823774769 > > > I hope we can upgrade to latest vertx 3.9.7 and netty 4.1.60 > > > > > > Also https://github.com/apache/bookkeeper/pull/2695 upgrades libthrift > > to > > > address security issues. > > > > > > All these PRs are from Lari, I'll follow up with him. > > > > > > I can be a RM if there are no other volunteers. > > > > > > > > > On Tue, May 4, 2021 at 1:57 PM Matteo Merli <mme...@apache.org> wrote: > > > > > > > +1 We should do 4.14, carrying all the fixes that are there in master > > as > > > > well. > > > > > > > > > > > > -- > > > > Matteo Merli > > > > <mme...@apache.org> > > > > > > > > On Tue, May 4, 2021 at 11:57 AM Sijie Guo <guosi...@gmail.com> > wrote: > > > > > > > > > > +1 > > > > > > > > > > On Tue, May 4, 2021 at 2:22 AM Yunze Xu > <y...@streamnative.io.invalid > > > > > > > > wrote: > > > > > > > > > > > Hello, > > > > > > About 10 days ago I found a heap memory copy problem in Apache > > > Pulsar, > > > > see > > > > > > [1]. > > > > > > It’s a problem of BK side because when > `LedgerHandle#asyncAddEntry` > > > > > > accepts a `CompositeByteBuf` or a wrapper, it will finally call > > > > > > `ByteBuf#nioBuffer()`, which would make a heap copy from direct > > > memory. > > > > > > [2] fixed this problem and has been merged for a week. > > > > > > > > > > > > Since it has a significant impact on Pulsar, Pulsar side needs a > > new > > > BK > > > > > > release with [2] merged to fix it. Is there any plan to cut a > > 4.13.1 > > > > > > release or 4.14.0 release so that we can upgrade the dependency > in > > > > Pulsar? > > > > > > > > > > > > Thanks, > > > > > > Yunze > > > > > > > > > > > > [1] https://github.com/apache/pulsar/pull/10330 < > > > > > > https://github.com/apache/pulsar/pull/10330> > > > > > > > > > > > > [2] https://github.com/apache/bookkeeper/pull/2701 < > > > > > > https://github.com/apache/bookkeeper/pull/2701> > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > -- > > > Andrey Yegorov > > > > > > > > -- > > -- > Andrey Yegorov >
