[GitHub] [bookkeeper] dlg99 commented on issue #2711: BK needs additional test (possibly, fixes) for TLS1.3 support

[GitBox <gi...@apache.org>]

dlg99 commented on issue #2711:
URL: https://github.com/apache/bookkeeper/issues/2711#issuecomment-834577904


   some context from the dev list, courtesy of @lhotari:
   
   --------
   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
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org