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:
[email protected]
