Config option to allow only secured client connections #2368

Ghatage · 2020-06-30T02:40:17Z

(1) Introduced a new config option onlySecureClientsAllowed in
the Bookie Configuration. Default value is considered false.
(2) If onlySecureClientsAllowed is set to True, the Bookies only
allow the Clients to communicate over TLS. Any requests
from the Clients without TLS enabled will be rejected
and Client gets the Security Exception.

eolivelli

Good work.
Two comments:

why don't you use the existing error code?
we should handle v2 protocol, otherwise clients will be able to connect to the bookie without tls. Something like dropping the connection or sending a generic error response

bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtocol.java

Ghatage · 2020-07-01T00:31:52Z

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


@eolivelli
I can make it use EUA as opposed to creating a new Error code.
However, just using EUA won't work right? The check for older Versions fails here because it tests for message to be an instance of latest version - V3 and it seems incompatible with V2.

If anything I feel we need to add another check here.
If we are in here, we know that the request is already authenticated, so we can cast msg to BookkeeperProtocol.Request and do a .getVersion() on that. If that works, it seems that the message is of type Request (may even be an earlier version)

Something like this:

if (authenticated) { if (((BookieProtocol.Request) msg).getProtocolVersion() || msg instanceof BookkeeperProtocol.Request) { Channel c = ctx.channel();

@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

eolivelli · 2020-07-01T05:40:11Z

Regarding EUA: it is better to use it in order not to change the wire protocol and keep 100% compatibility with all BK clients in the wild.
I do not expect this to fix the v2 protocol problem.

The v2 protocol follows a different code path. I did not check the code but maybe your solution is ok. If you see tests passing then we are on our way

Ghatage · 2020-07-02T05:41:26Z

Regarding EUA: it is better to use it in order not to change the wire protocol and keep 100% compatibility with all BK clients in the wild

Makes sense. Will rework the change to use EUA and my 'fix' for handling v2 as well.

sijie · 2020-07-21T16:13:23Z

@Ghatage @eolivelli Any updates on this issue? What is the current blocker?

Ghatage · 2020-07-22T02:45:17Z

@Ghatage @eolivelli Any updates on this issue? What is the current blocker?

Supporting v2 connections is the current blocker.
Here, we check for the type of the message, but that is false when msg is v2 and hence that check fails and allows the v2 unsecured connections through.
Not sure how to check for v2 in ServerSideHandler.
In ClientSideHandler we pass isUsingV2Protocol, which would be useful in ServerSideHandler but isn't present.

Any suggestions?

eolivelli · 2020-07-23T07:38:33Z

@Ghatage do you have time to continue this patch ?

Ghatage · 2020-07-23T10:31:22Z

Yes, just stuck on the V2 case. Will give it some more thought and post an update soon!

…

On Thu, Jul 23, 2020, 12:38 AM Enrico Olivelli ***@***.***> wrote: @Ghatage <https://github.com/Ghatage> do you have time to continue this patch ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2368 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAX6QQN2H3AL3MG6UPUUQATR47SIRANCNFSM4OLY7IFA> .

eolivelli · 2020-07-27T06:49:36Z

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


@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

sijie · 2020-08-13T08:42:53Z

@Ghatage @eolivelli any updates on this?

sijie · 2020-09-10T16:25:51Z

@Ghatage @eolivelli - Any updates on this?

eolivelli · 2020-10-06T12:37:54Z

@Ghatage do you need any other hints ?

eolivelli · 2020-11-03T10:49:19Z

@Ghatage do you have time to resolve the conflicts and complete this work ?
it would be a super great feature for 4.12.0 users

eolivelli · 2020-11-04T07:54:54Z

@sijie @Ghatage I am not sure it is worth to wait for this patch to land before cutting 4.12, it is a security feature.
We can deliver it with 4.12.1.

I am removing the 4.12 label

eolivelli · 2020-11-25T07:47:55Z

@Ghatage probably this is the best time to resume this work.
We are far away from the new major release and we will have time to let this implementation settle

eolivelli · 2021-01-13T07:10:04Z

@Ghatage do you have cycles to move forward this useful work ?

eolivelli · 2021-01-27T07:44:53Z

@Ghatage if you do not have much time, we could ask for someone to pick up the patch ?

cc @nicoloboschi @dianacle

hezhangjian · 2024-07-12T06:40:10Z

@Ghatage wow, welcome back :)

(1) Introduced a new config option onlySecureClientsAllowed in the Bookie Configuration. Default value is considered false. (2) If onlySecureClientsAllowed is set to True, the Bookies only allow the Clients to communicate over TLS. Any requests from the Clients without TLS enabled will be rejected and Client gets the Security Exception.

Ghatage · 2024-07-12T07:26:06Z

@eolivelli @shoothzj Please merge whenever convenient!

hezhangjian · 2024-07-15T01:14:04Z

ping @eolivelli

Ghatage changed the title ~~Don't allow non-secure client connections~~ Config option to allow only secured client connections Jun 30, 2020

eolivelli requested changes Jun 30, 2020

View reviewed changes

bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtocol.java Outdated Show resolved Hide resolved

Ghatage commented Jul 1, 2020

View reviewed changes

eolivelli requested changes Jul 27, 2020

View reviewed changes

eolivelli added area/bookie area/security priority/important release/4.12.0 type/improvement labels Oct 6, 2020

eolivelli added this to the 4.12.0 milestone Oct 6, 2020

eolivelli removed the release/4.12.0 label Nov 4, 2020

eolivelli removed this from the 4.12.0 milestone Nov 4, 2020

Ghatage force-pushed the onlySecureClientAllowed branch 2 times, most recently from 515e652 to 0d7e7f5 Compare June 28, 2021 03:36

Ghatage force-pushed the onlySecureClientAllowed branch from 0d7e7f5 to 8b28bc8 Compare July 12, 2024 06:37

Ghatage force-pushed the onlySecureClientAllowed branch from 8b28bc8 to d3a938b Compare July 12, 2024 06:44

hezhangjian approved these changes Jul 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Config option to allow only secured client connections #2368

Config option to allow only secured client connections #2368

Ghatage commented Jun 30, 2020

eolivelli left a comment

Ghatage Jul 1, 2020

eolivelli Jul 27, 2020

eolivelli commented Jul 1, 2020

Ghatage commented Jul 2, 2020

sijie commented Jul 21, 2020

Ghatage commented Jul 22, 2020

eolivelli commented Jul 23, 2020

Ghatage commented Jul 23, 2020 via email

eolivelli Jul 27, 2020

sijie commented Aug 13, 2020

sijie commented Sep 10, 2020

eolivelli commented Oct 6, 2020

eolivelli commented Nov 3, 2020

eolivelli commented Nov 4, 2020

eolivelli commented Nov 25, 2020

eolivelli commented Jan 13, 2021

eolivelli commented Jan 27, 2021

hezhangjian commented Jul 12, 2024

Ghatage commented Jul 12, 2024

hezhangjian commented Jul 15, 2024

Config option to allow only secured client connections #2368

Are you sure you want to change the base?

Config option to allow only secured client connections #2368

Conversation

Ghatage commented Jun 30, 2020

eolivelli left a comment

Choose a reason for hiding this comment

Ghatage Jul 1, 2020

Choose a reason for hiding this comment

eolivelli Jul 27, 2020

Choose a reason for hiding this comment

eolivelli commented Jul 1, 2020

Ghatage commented Jul 2, 2020

sijie commented Jul 21, 2020

Ghatage commented Jul 22, 2020

eolivelli commented Jul 23, 2020

Ghatage commented Jul 23, 2020 via email

eolivelli Jul 27, 2020

Choose a reason for hiding this comment

sijie commented Aug 13, 2020

sijie commented Sep 10, 2020

eolivelli commented Oct 6, 2020

eolivelli commented Nov 3, 2020

eolivelli commented Nov 4, 2020

eolivelli commented Nov 25, 2020

eolivelli commented Jan 13, 2021

eolivelli commented Jan 27, 2021

hezhangjian commented Jul 12, 2024

Ghatage commented Jul 12, 2024

hezhangjian commented Jul 15, 2024