You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2020/06/30 02:40:25 UTC
[GitHub] [bookkeeper] Ghatage opened a new pull request #2368: Don't allow non-secure client connections
Ghatage opened a new pull request #2368:
URL: https://github.com/apache/bookkeeper/pull/2368
(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.
----------------------------------------------------------------
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
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2368: Config option to allow only secured client connections
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2368:
URL: https://github.com/apache/bookkeeper/pull/2368#discussion_r447418133
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtocol.java
##########
@@ -180,6 +180,11 @@ public static short getFlags(int packetHeader) {
*/
int ETOOMANYREQUESTS = 106;
+ /**
+ * Client could not be authenticated.
+ */
+ int EAUTH = 107;
Review comment:
Why aren't you using existing code EUA (102)?
If we use EUA this change is compatible with old clients seamlessly
----------------------------------------------------------------
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
[GitHub] [bookkeeper] eolivelli commented on pull request #2368: Config option to allow only secured client connections
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2368:
URL: https://github.com/apache/bookkeeper/pull/2368#issuecomment-721041635
@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
----------------------------------------------------------------
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
[GitHub] [bookkeeper] eolivelli commented on pull request #2368: Config option to allow only secured client connections
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2368:
URL: https://github.com/apache/bookkeeper/pull/2368#issuecomment-652203326
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
----------------------------------------------------------------
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
[GitHub] [bookkeeper] eolivelli commented on pull request #2368: Config option to allow only secured client connections
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2368:
URL: https://github.com/apache/bookkeeper/pull/2368#issuecomment-768100811
@Ghatage if you do not have much time, we could ask for someone to pick up the patch ?
cc @nicoloboschi @dianacle
----------------------------------------------------------------
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
[GitHub] [bookkeeper] Ghatage commented on a change in pull request #2368: Config option to allow only secured client connections
Posted by GitBox <gi...@apache.org>.
Ghatage commented on a change in pull request #2368:
URL: https://github.com/apache/bookkeeper/pull/2368#discussion_r448051873
##########
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:
@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();
```
----------------------------------------------------------------
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
[GitHub] [bookkeeper] sijie commented on pull request #2368: Config option to allow only secured client connections
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #2368:
URL: https://github.com/apache/bookkeeper/pull/2368#issuecomment-673347299
@Ghatage @eolivelli any updates on this?
----------------------------------------------------------------
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
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2368: Config option to allow only secured client connections
Posted by 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
[GitHub] [bookkeeper] Ghatage commented on pull request #2368: Config option to allow only secured client connections
Posted by GitBox <gi...@apache.org>.
Ghatage commented on pull request #2368:
URL: https://github.com/apache/bookkeeper/pull/2368#issuecomment-662933289
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 <no...@github.com>
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>
> .
>
----------------------------------------------------------------
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
[GitHub] [bookkeeper] eolivelli commented on pull request #2368: Config option to allow only secured client connections
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2368:
URL: https://github.com/apache/bookkeeper/pull/2368#issuecomment-704239643
@Ghatage do you need any other hints ?
----------------------------------------------------------------
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
[GitHub] [bookkeeper] sijie commented on pull request #2368: Config option to allow only secured client connections
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #2368:
URL: https://github.com/apache/bookkeeper/pull/2368#issuecomment-661956302
@Ghatage @eolivelli Any updates on this issue? What is the current blocker?
----------------------------------------------------------------
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
[GitHub] [bookkeeper] eolivelli commented on pull request #2368: Config option to allow only secured client connections
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2368:
URL: https://github.com/apache/bookkeeper/pull/2368#issuecomment-662863147
@Ghatage do you have time to continue this patch ?
----------------------------------------------------------------
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
[GitHub] [bookkeeper] eolivelli commented on pull request #2368: Config option to allow only secured client connections
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2368:
URL: https://github.com/apache/bookkeeper/pull/2368#issuecomment-759253535
@Ghatage do you have cycles to move forward this useful work ?
----------------------------------------------------------------
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
[GitHub] [bookkeeper] eolivelli commented on pull request #2368: Config option to allow only secured client connections
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2368:
URL: https://github.com/apache/bookkeeper/pull/2368#issuecomment-733529192
@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
----------------------------------------------------------------
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
[GitHub] [bookkeeper] Ghatage commented on pull request #2368: Config option to allow only secured client connections
Posted by GitBox <gi...@apache.org>.
Ghatage commented on pull request #2368:
URL: https://github.com/apache/bookkeeper/pull/2368#issuecomment-662209666
> @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](https://github.com/apache/bookkeeper/pull/2368/files#diff-684dc84d2a6bfbef7b2e3e1a5ef6766aR99), 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?
----------------------------------------------------------------
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
[GitHub] [bookkeeper] Ghatage commented on pull request #2368: Config option to allow only secured client connections
Posted by GitBox <gi...@apache.org>.
Ghatage commented on pull request #2368:
URL: https://github.com/apache/bookkeeper/pull/2368#issuecomment-652795582
> 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.
----------------------------------------------------------------
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
[GitHub] [bookkeeper] eolivelli commented on pull request #2368: Config option to allow only secured client connections
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2368:
URL: https://github.com/apache/bookkeeper/pull/2368#issuecomment-721571841
@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
----------------------------------------------------------------
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
[GitHub] [bookkeeper] sijie commented on pull request #2368: Config option to allow only secured client connections
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #2368:
URL: https://github.com/apache/bookkeeper/pull/2368#issuecomment-690448086
@Ghatage @eolivelli - Any updates on this?
----------------------------------------------------------------
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