bookkeeper icon indicating copy to clipboard operation
bookkeeper copied to clipboard

Config option to allow only secured client connections

Open Ghatage opened this issue 5 years ago • 17 comments

(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 avatar Jun 30 '20 02:06 Ghatage

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

eolivelli avatar Jul 01 '20 05:07 eolivelli

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.

Ghatage avatar Jul 02 '20 05:07 Ghatage

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

sijie avatar Jul 21 '20 16:07 sijie

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

Ghatage avatar Jul 22 '20 02:07 Ghatage

@Ghatage do you have time to continue this patch ?

eolivelli avatar Jul 23 '20 07:07 eolivelli

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 [email protected] 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 https://github.com/apache/bookkeeper/pull/2368#issuecomment-662863147, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAX6QQN2H3AL3MG6UPUUQATR47SIRANCNFSM4OLY7IFA .

Ghatage avatar Jul 23 '20 10:07 Ghatage

@Ghatage @eolivelli any updates on this?

sijie avatar Aug 13 '20 08:08 sijie

@Ghatage @eolivelli - Any updates on this?

sijie avatar Sep 10 '20 16:09 sijie

@Ghatage do you need any other hints ?

eolivelli avatar Oct 06 '20 12:10 eolivelli

@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 avatar Nov 03 '20 10:11 eolivelli

@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 avatar Nov 04 '20 07:11 eolivelli

@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 avatar Nov 25 '20 07:11 eolivelli

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

eolivelli avatar Jan 13 '21 07:01 eolivelli

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

cc @nicoloboschi @dianacle

eolivelli avatar Jan 27 '21 07:01 eolivelli

@Ghatage wow, welcome back :)

hezhangjian avatar Jul 12 '24 06:07 hezhangjian

@eolivelli @shoothzj Please merge whenever convenient!

Ghatage avatar Jul 12 '24 07:07 Ghatage

ping @eolivelli

hezhangjian avatar Jul 15 '24 01:07 hezhangjian