I added https://github.com/apache/bookkeeper/issues/2711 for the TLS 1.3 support Unless someone objects in the next 30min, I'll merge Lari's PR. After that I'll start working on the BK 4.14.0 release.
On Thu, May 6, 2021 at 1:25 AM Lari Hotari <l...@hotari.net> wrote: > 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://urldefense.proofpoint.com/v2/url?u=https-3A__stackoverflow.com_a_62465859&d=DwIFaQ&c=adz96Xi0w1RHqtPMowiL2g&r=0B1UvYMwy7dr9qtqFwQCfxUyrozUgZzbOshynTIaYUY&m=76JE79AuinlMNecD5DDFGgg-jXzCGZEh3PANpQOJUoE&s=iZz_eExfeElZI--ooxMmyMABWjailhDc7rKIAZNg59s&e= > . > > 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://urldefense.proofpoint.com/v2/url?u=https-3A__stackoverflow.com_a_62465859&d=DwIFaQ&c=adz96Xi0w1RHqtPMowiL2g&r=0B1UvYMwy7dr9qtqFwQCfxUyrozUgZzbOshynTIaYUY&m=76JE79AuinlMNecD5DDFGgg-jXzCGZEh3PANpQOJUoE&s=iZz_eExfeElZI--ooxMmyMABWjailhDc7rKIAZNg59s&e= > > 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 > > > -- -- Andrey Yegorov
