-
Notifications
You must be signed in to change notification settings - Fork 896
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Config option to allow only secured client connections #2368
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
@@ -92,6 +96,24 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception | |||
} | |||
|
|||
if (authenticated) { | |||
if (msg instanceof BookkeeperProtocol.Request) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
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. 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 |
Makes sense. Will rework the change to use |
@Ghatage @eolivelli Any updates on this issue? What is the current blocker? |
Supporting v2 connections is the current blocker. Any suggestions? |
@Ghatage do you have time to continue this patch ? |
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>
.
|
@@ -92,6 +96,24 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception | |||
} | |||
|
|||
if (authenticated) { | |||
if (msg instanceof BookkeeperProtocol.Request) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
@Ghatage @eolivelli any updates on this? |
@Ghatage @eolivelli - Any updates on this? |
@Ghatage do you need any other hints ? |
@Ghatage do you have time to resolve the conflicts and complete this work ? |
@Ghatage probably this is the best time to resume this work. |
@Ghatage do you have cycles to move forward this useful work ? |
@Ghatage if you do not have much time, we could ask for someone to pick up the patch ? |
515e652
to
0d7e7f5
Compare
0d7e7f5
to
8b28bc8
Compare
@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.
8b28bc8
to
d3a938b
Compare
@eolivelli @shoothzj Please merge whenever convenient! |
ping @eolivelli |
(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.