[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2368: Config option to allow only secured client connections

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

eolivelli commented on a change in pull request #2368:
URL: https://github.com/apache/bookkeeper/pull/2368#discussion_r460679786



##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/AuthHandler.java
##########
@@ -92,6 +96,24 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
             }
 
             if (authenticated) {
+                if (msg instanceof BookkeeperProtocol.Request) {

Review comment:
       @Ghatage 
   this is probably the wrong place for this check.
   If you allow ONLY secured client you must forbid the auth as well, because the risk is to exchange credentials without encryption.
   I didn't check well, but I hope that startTLS happens before exchanging auth
   
   so your check should be:
   - in `if (msg instanceof BookieProtocol.AuthRequest) { // pre-PB-client` block
   - in `if (msg instanceof BookkeeperProtocol.Request) { // post-PB-client` block
   
   The former is v2 protocol, and you should simply fail the request, because there is no support for TLS
   The latter is v3 protocol and you should your check `c.pipeline().get(SslHandler.class) == null`
   
   O hope that helps




----------------------------------------------------------------
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