You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by GitBox <gi...@apache.org> on 2020/05/27 03:33:57 UTC
[GitHub] [tinkerpop] javeme opened a new pull request #1289: TINKERPOP-2374 fix missing authorization with SaslAndHttpBasicAuthenticationHandler
javeme opened a new pull request #1289:
URL: https://github.com/apache/tinkerpop/pull/1289
----------------------------------------------------------------
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] [tinkerpop] divijvaidya commented on pull request #1289: TINKERPOP-2374 fix missing authorization with SaslAndHttpBasicAuthenticationHandler
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on pull request #1289:
URL: https://github.com/apache/tinkerpop/pull/1289#issuecomment-637004936
> AFAIK, Netty bind a channel and a thread for each TCP connection, the requests on this connection are executed serially, so there should not be multiple threads modifying a pipeline.
I will try to reproduce this behaviour separately from this PR.
For the scope of this PR, please add the tests and validation I mentioned in the previous comment to catch such problem proactively in future. We would be good to merge this PR once you have added those.
----------------------------------------------------------------
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] [tinkerpop] divijvaidya commented on pull request #1289: TINKERPOP-2374 fix missing authorization with SaslAndHttpBasicAuthenticationHandler
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on pull request #1289:
URL: https://github.com/apache/tinkerpop/pull/1289#issuecomment-636244153
Really appreciate the detailed explanation @javeme. We are on the same page here. While writing my explanation, I missed that step#7 is not adding the http-authenticator back. Your change will definitely fix this issue.
Before we push this change, can you please add a couple of more associated changes that will ensure we catch such bugs proactively in future.
1. Add logic to validate that the pipeline has been setup as expected. You might want to leverage the [finalize method in AbstractChannelizer](https://github.com/apache/tinkerpop/blob/cc3c5cb83e253b9949076628a7cfaade7f86f40e/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/AbstractChannelizer.java#L124). Note that this wouldn't catch pipeline setup issues where it is modified dynamically based on message information while processing the message. For that, we will do #2
2. Add tests to validate the pipeline through which a message is executed. If we had this test, this bug would have been caught earlier.
After you have made the above changes, we can push this change out while separately working on the larger issue as described below.
The reason why I called this change as a workaround is because, while it solves this particular problem of missing http-authenticator, it doesn't address the underlying issue which is dynamic modification of pipeline when more than one request is expected.
Each request is expected to be processed by a deterministic set of handlers. In some cases such as WS and HTTP being served on same connection, this handler chain is created dynamically based on the request headers or content. However, when we execute two requests on the same pipeline (case of keepAlive), both the requests would try to modify the same pipeline at the same time which might lead to a case when the pipeline is misconfigured. e.g. while one message [has removed the PIPELINE_AUTHENTICATOR handler](https://github.com/apache/tinkerpop/blob/cc3c5cb83e253b9949076628a7cfaade7f86f40e/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/WsAndHttpChannelizerHandler.java#L68) (and has not added it back yet in the next line), in a separate thread, another message would reach a [check for PIPELINE_AUTHENTICATOR presence in the same pipeline](https://github.com/apache/tinkerpop/blob/cc3c5cb83e253b9949076628a7cfaade7f86f40e/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/WsAndHttpChannelizerHandler.java#L65) and finds that it isn't present, hence, not taking the actions to configure the pipeline correctly.
Do you agree that this is still a problem even after the fix you proposed and can lead to a similar misconfigured pipeline?
----------------------------------------------------------------
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] [tinkerpop] javeme commented on pull request #1289: TINKERPOP-2374 fix missing authorization with SaslAndHttpBasicAuthenticationHandler
Posted by GitBox <gi...@apache.org>.
javeme commented on pull request #1289:
URL: https://github.com/apache/tinkerpop/pull/1289#issuecomment-636199113
@spmallette @divijvaidya Thanks for your review.
I don’t think this is a workaround, I'm sure there is no race condition here, let me explain the steps to reproduce this bug:
1. Assume authentication is enabled with `WsAndHttpChannelizerHandler` and and `SaslAndHttpBasicAuthenticationHandler`，and keep http connection alive.
2. Login user1 with username1+password1(in http basic auth), and gremlin-server can get user information from http header "Authorization"(through `HttpBasicAuthenticationHandler`).
3. Login user2 with username2+password2 in the same way, the user information cannot be obtained, and this problem occurs when we keep using the same TCP connection.
Detailed analysis:
1. Login user1, the pipeline status is: http-response-encoder -> WebSocketServerProtocolHandshakeHandler -> ... -> request-close-decoder -> authenticator
2. After [WsAndHttpChannelizerHandler](https://github.com/apache/tinkerpop/blob/cc3c5cb83e253b9949076628a7cfaade7f86f40e/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/WsAndHttpChannelizerHandler.java#L66) updated the pipeline: http-response-encoder -> authenticator -> request-handler, note the request-handler is newly added.
3. After [SaslAndHttpBasicAuthenticationHandler](https://github.com/apache/tinkerpop/blob/cc3c5cb83e253b9949076628a7cfaade7f86f40e/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/SaslAndHttpBasicAuthenticationHandler.java#L51) updated the pipeline: http-response-encoder -> authenticator -> http-authentication -> request-handler, note the http-authentication is newly added.
4. Until now this is correct.
5. Login user2, if the TCP connection is the same, netty will use the original channel, so the original pipline is also be used, the pipeline status is: http-response-encoder -> authenticator -> **http-authentication** -> request-handler.
6. After [WsAndHttpChannelizerHandler](https://github.com/apache/tinkerpop/blob/cc3c5cb83e253b9949076628a7cfaade7f86f40e/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/WsAndHttpChannelizerHandler.java#L66) updated the pipeline: http-response-encoder -> authenticator -> request-handler -> **http-authentication**, note http-authentication is behind request-handler.
_(For the detailed process please refer to_ [how to remove request-handler](https://github.com/apache/tinkerpop/blob/cc3c5cb83e253b9949076628a7cfaade7f86f40e/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/WsAndHttpChannelizerHandler.java#L66) and [how to remove authenticator](https://github.com/apache/tinkerpop/blob/cc3c5cb83e253b9949076628a7cfaade7f86f40e/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/WsAndHttpChannelizerHandler.java#L68) and [how to add authenticator again](https://github.com/apache/tinkerpop/blob/cc3c5cb83e253b9949076628a7cfaade7f86f40e/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/WsAndHttpChannelizerHandler.java#L69) and [how to add request-handler again](https://github.com/apache/tinkerpop/blob/cc3c5cb83e253b9949076628a7cfaade7f86f40e/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/WsAndHttpChannelizerHandler.java#L70))
7. [SaslAndHttpBasicAuthenticationHandler](https://github.com/apache/tinkerpop/blob/cc3c5cb83e253b9949076628a7cfaade7f86f40e/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/SaslAndHttpBasicAuthenticationHandler.java#L51) won't update the pipeline any more since http-authentication already exists.
8. This caused us to be unable to get the information of user2 when executing gremlin through request-handler.
Thie PR fix the bug by moving http-authentication in front of request-handler every time.
----------------------------------------------------------------
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] [tinkerpop] divijvaidya commented on a change in pull request #1289: TINKERPOP-2374 fix missing authorization with SaslAndHttpBasicAuthenticationHandler
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on a change in pull request #1289:
URL: https://github.com/apache/tinkerpop/pull/1289#discussion_r432228859
##########
File path: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/SaslAndHttpBasicAuthenticationHandler.java
##########
@@ -47,9 +47,11 @@ public SaslAndHttpBasicAuthenticationHandler(final Authenticator authenticator,
@Override
public void channelRead(final ChannelHandlerContext ctx, final Object obj) throws Exception {
if (obj instanceof HttpMessage && !WebSocketHandlerUtil.isWebSocket((HttpMessage)obj)) {
- if (null == ctx.pipeline().get(HTTP_AUTH)) {
- ctx.pipeline().addAfter(PIPELINE_AUTHENTICATOR, HTTP_AUTH, new HttpBasicAuthenticationHandler(authenticator, this.authenticationSettings));
+ ChannelPipeline pipeline = ctx.pipeline();
+ if (null != pipeline.get(HTTP_AUTH)) {
+ pipeline.remove(HTTP_AUTH);
}
Review comment:
This code change doesn't really solve the root cause and doesn't explain why are having random pipeline behaviour.
Here's my theory:
When keep alive is turned on, multiple HTTP requests use the same pipeline. This causes a race condition where multiple requests are trying to modify the pipeline dynamically in channelRead method of `WsAndHttpChannelizerHandler`. While one message is dynamically modifying the pipeline, let's say executing line 68 (removing the PIPELINE_AUTHENTICATOR), another message might come in and execute line 65 and erroneously jumped to line 71. This causes the unpredictable behaviour you are observing.
The correct fix would be to ensure that the above dynamic modification of pipeline is only done once per connection instead of modifying it with every message.
IMO we should not push this workaround without fixing the underlying root cause. WDYT @spmallette ?
----------------------------------------------------------------
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] [tinkerpop] divijvaidya commented on pull request #1289: TINKERPOP-2374 fix missing authorization with SaslAndHttpBasicAuthenticationHandler
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on pull request #1289:
URL: https://github.com/apache/tinkerpop/pull/1289#issuecomment-638582597
I also created a ticket https://issues.apache.org/jira/browse/TINKERPOP-2380 to add tests separately that will catch such bugs in future.
----------------------------------------------------------------
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] [tinkerpop] divijvaidya commented on a change in pull request #1289: TINKERPOP-2374 fix missing authorization with SaslAndHttpBasicAuthenticationHandler
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on a change in pull request #1289:
URL: https://github.com/apache/tinkerpop/pull/1289#discussion_r432228859
##########
File path: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/SaslAndHttpBasicAuthenticationHandler.java
##########
@@ -47,9 +47,11 @@ public SaslAndHttpBasicAuthenticationHandler(final Authenticator authenticator,
@Override
public void channelRead(final ChannelHandlerContext ctx, final Object obj) throws Exception {
if (obj instanceof HttpMessage && !WebSocketHandlerUtil.isWebSocket((HttpMessage)obj)) {
- if (null == ctx.pipeline().get(HTTP_AUTH)) {
- ctx.pipeline().addAfter(PIPELINE_AUTHENTICATOR, HTTP_AUTH, new HttpBasicAuthenticationHandler(authenticator, this.authenticationSettings));
+ ChannelPipeline pipeline = ctx.pipeline();
+ if (null != pipeline.get(HTTP_AUTH)) {
+ pipeline.remove(HTTP_AUTH);
}
Review comment:
This code change doesn't really solve the root cause and doesn't explain why are having random pipeline behaviour.
Here's my theory:
When keep alive is turned on, multiple HTTP requests use the same pipeline. This causes a race condition where multiple requests are trying to modify the pipeline dynamically in channelRead method of `WsAndHttpChannelizerHandler`. While one message is dynamically modifying the pipeline, let's say executing line 68 (removing the PIPELINE_AUTHENTICATOR), another message might come in and execute line 65 and erroneously jumped to line 71. This causes the unpredictable behaviour you are observing.
The correct fix would be to ensure that the above dynamic modification of pipeline is only done once.
IMO we should not push this workaround without fixing the underlying root cause. WDYT @spmallette ?
----------------------------------------------------------------
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] [tinkerpop] javeme commented on pull request #1289: TINKERPOP-2374 fix missing authorization with SaslAndHttpBasicAuthenticationHandler
Posted by GitBox <gi...@apache.org>.
javeme commented on pull request #1289:
URL: https://github.com/apache/tinkerpop/pull/1289#issuecomment-636665995
> Do you agree that this is still a problem even after the fix you proposed and can lead to a similar misconfigured pipeline?
@divijvaidya I don't think this is still a problem after the fix, and I'm not sure this will happen "both the requests would try to modify the same pipeline at the same time", did you actually encounter this problem?
AFAIK, Netty bind a channel and a thread for each TCP connection, the requests on this connection are executed serially, so there should not be multiple threads modifying a pipeline.
----------------------------------------------------------------
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] [tinkerpop] divijvaidya commented on pull request #1289: TINKERPOP-2374 fix missing authorization with SaslAndHttpBasicAuthenticationHandler
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on pull request #1289:
URL: https://github.com/apache/tinkerpop/pull/1289#issuecomment-638580287
Yes, it seems like there is no direct way today to write tests to assert the order of handlers in the pipeline. We will work towards adding such tests in the future. Meanwhile, the code looks good to me. No remaining comments.
VOTE +1
----------------------------------------------------------------
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] [tinkerpop] spmallette commented on a change in pull request #1289: TINKERPOP-2374 fix missing authorization with SaslAndHttpBasicAuthenticationHandler
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1289:
URL: https://github.com/apache/tinkerpop/pull/1289#discussion_r432432931
##########
File path: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/SaslAndHttpBasicAuthenticationHandler.java
##########
@@ -47,9 +47,11 @@ public SaslAndHttpBasicAuthenticationHandler(final Authenticator authenticator,
@Override
public void channelRead(final ChannelHandlerContext ctx, final Object obj) throws Exception {
if (obj instanceof HttpMessage && !WebSocketHandlerUtil.isWebSocket((HttpMessage)obj)) {
- if (null == ctx.pipeline().get(HTTP_AUTH)) {
- ctx.pipeline().addAfter(PIPELINE_AUTHENTICATOR, HTTP_AUTH, new HttpBasicAuthenticationHandler(authenticator, this.authenticationSettings));
+ ChannelPipeline pipeline = ctx.pipeline();
+ if (null != pipeline.get(HTTP_AUTH)) {
+ pipeline.remove(HTTP_AUTH);
}
Review comment:
I'm fine with not merging a workaround. i trust your judgement with the Java driver - if you'd prefer a root cause fix, then let's shoot for that.
@javeme do you have any thoughts on the theory that @divijvaidya proposed?
----------------------------------------------------------------
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] [tinkerpop] divijvaidya merged pull request #1289: TINKERPOP-2374 fix missing authorization with SaslAndHttpBasicAuthenticationHandler
Posted by GitBox <gi...@apache.org>.
divijvaidya merged pull request #1289:
URL: https://github.com/apache/tinkerpop/pull/1289
----------------------------------------------------------------
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] [tinkerpop] divijvaidya edited a comment on pull request #1289: TINKERPOP-2374 fix missing authorization with SaslAndHttpBasicAuthenticationHandler
Posted by GitBox <gi...@apache.org>.
divijvaidya edited a comment on pull request #1289:
URL: https://github.com/apache/tinkerpop/pull/1289#issuecomment-637004936
> AFAIK, Netty bind a channel and a thread for each TCP connection, the requests on this connection are executed serially, so there should not be multiple threads modifying a pipeline.
I will try to reproduce this behaviour separately from this PR.
For the scope of this PR, please add the tests and validation I mentioned in the previous comment to catch such problem proactively in future. We would be good to merge this PR once you have added those.
----------------------------------------------------------------
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] [tinkerpop] spmallette commented on pull request #1289: TINKERPOP-2374 fix missing authorization with SaslAndHttpBasicAuthenticationHandler
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1289:
URL: https://github.com/apache/tinkerpop/pull/1289#issuecomment-642550469
VOTE +1
@divijvaidya feel free to merge this one (adding a CHANGELOG entry on merge would be good).
----------------------------------------------------------------
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] [tinkerpop] spmallette edited a comment on pull request #1289: TINKERPOP-2374 fix missing authorization with SaslAndHttpBasicAuthenticationHandler
Posted by GitBox <gi...@apache.org>.
spmallette edited a comment on pull request #1289:
URL: https://github.com/apache/tinkerpop/pull/1289#issuecomment-634579215
Interesting. Thanks for the fix. We are currently in code freeze on 3.4-dev in preparation for release of 3.4.7. We can get this merged when that release is done.
~~VOTE +1~~ - dropped my vote given recent discussions on this PR
----------------------------------------------------------------
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] [tinkerpop] javeme commented on pull request #1289: TINKERPOP-2374 fix missing authorization with SaslAndHttpBasicAuthenticationHandler
Posted by GitBox <gi...@apache.org>.
javeme commented on pull request #1289:
URL: https://github.com/apache/tinkerpop/pull/1289#issuecomment-637529704
@divijvaidya added tests for that two consecutive requests expect to throw an exception instead of normal. But I'm not sure how to validate the pipeline, seems we can't get the pipeline object(in server) from client.
----------------------------------------------------------------
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] [tinkerpop] spmallette commented on pull request #1289: TINKERPOP-2374 fix missing authorization with SaslAndHttpBasicAuthenticationHandler
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1289:
URL: https://github.com/apache/tinkerpop/pull/1289#issuecomment-634579215
Interesting. Thanks for the fix. We are currently in code freeze on 3.4-dev in preparation for release of 3.4.7. We can get this merged when that release is done.
VOTE +1
----------------------------------------------------------------
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