Re: [PR] [TINKERPOP-3061] fix: failing authentication when multiple initially requests are executed concurrently [tinkerpop]
tien commented on PR #2525: URL: https://github.com/apache/tinkerpop/pull/2525#issuecomment-2044019322 @xiazcy nah I just got back today, thanks for making this available in the next release -- 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. To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [TINKERPOP-3061] fix: failing authentication when multiple initially requests are executed concurrently [tinkerpop]
xiazcy closed pull request #2525: [TINKERPOP-3061] fix: failing authentication when multiple initially requests are executed concurrently URL: https://github.com/apache/tinkerpop/pull/2525 -- 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. To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [TINKERPOP-3061] fix: failing authentication when multiple initially requests are executed concurrently [tinkerpop]
xiazcy commented on PR #2525: URL: https://github.com/apache/tinkerpop/pull/2525#issuecomment-2044008086 Closing this PR as all changes are merged via 22db8cf for the release. Please feel free to re-open if you find additional improvements and/or updates needed. Thanks! -- 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. To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [TINKERPOP-3061] fix: failing authentication when multiple initially requests are executed concurrently [tinkerpop]
xiazcy commented on PR #2525: URL: https://github.com/apache/tinkerpop/pull/2525#issuecomment-2043756862 > Sorry, I'm away this week and don't have access to my work laptop. Will take a look at all the pending comments & resolve them next week No worries, thank you for all the contributions! Just a quick note. Not sure if you have gotten a chance to start looking at the comments, as we'd like to release this with 3.7.2 this week, we will likely be cherry-picking your changes into another PR for the release branch today. If we do proceed with that we'll be closing this PR, and you shouldn't need to do any further work. Now there might still be functionality improvements we miss, so please feel free to add additional changes once the branches re-open. -- 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. To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [TINKERPOP-3061] fix: failing authentication when multiple initially requests are executed concurrently [tinkerpop]
tien commented on PR #2525: URL: https://github.com/apache/tinkerpop/pull/2525#issuecomment-2035957546 Sorry, I'm away this week and don't have access to my work laptop. Will take a look at all the pending comments & resolve them next week -- 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. To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [TINKERPOP-3061] fix: failing authentication when multiple initially requests are executed concurrently [tinkerpop]
FlorianHockmann commented on code in PR #2525: URL: https://github.com/apache/tinkerpop/pull/2525#discussion_r1542632956 ## gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/SaslAuthenticationHandler.java: ## @@ -75,106 +79,159 @@ public SaslAuthenticationHandler(final Authenticator authenticator, final Author @Override public void channelRead(final ChannelHandlerContext ctx, final Object msg) throws Exception { -if (msg instanceof RequestMessage){ -final RequestMessage requestMessage = (RequestMessage) msg; - -final Attribute negotiator = ((AttributeMap) ctx).attr(StateKey.NEGOTIATOR); -final Attribute request = ((AttributeMap) ctx).attr(StateKey.REQUEST_MESSAGE); -if (negotiator.get() == null) { -try { -// First time through so save the request and send an AUTHENTICATE challenge with no data - negotiator.set(authenticator.newSaslNegotiator(getRemoteInetAddress(ctx))); -request.set(requestMessage); -final ResponseMessage authenticate = ResponseMessage.build(requestMessage) -.code(ResponseStatusCode.AUTHENTICATE).create(); -ctx.writeAndFlush(authenticate); -} catch (Exception ex) { -// newSaslNegotiator can cause troubles - if we don't catch and respond nicely the driver seems -// to hang until timeout which isn't so nice. treating this like a server error as it means that -// the Authenticator isn't really ready to deal with requests for some reason. -logger.error(String.format("%s is not ready to handle requests - check its configuration or related services", -authenticator.getClass().getSimpleName()), ex); - -final ResponseMessage error = ResponseMessage.build(requestMessage) -.statusMessage("Authenticator is not ready to handle requests") -.code(ResponseStatusCode.SERVER_ERROR).create(); -ctx.writeAndFlush(error); -} -} else { -if (requestMessage.getOp().equals(Tokens.OPS_AUTHENTICATION) && requestMessage.getArgs().containsKey(Tokens.ARGS_SASL)) { - -final Object saslObject = requestMessage.getArgs().get(Tokens.ARGS_SASL); -final byte[] saslResponse; - -if(saslObject instanceof String) { -saslResponse = BASE64_DECODER.decode((String) saslObject); -} else { -final ResponseMessage error = ResponseMessage.build(request.get()) -.statusMessage("Incorrect type for : " + Tokens.ARGS_SASL + " - base64 encoded String is expected") - .code(ResponseStatusCode.REQUEST_ERROR_MALFORMED_REQUEST).create(); -ctx.writeAndFlush(error); -return; -} - -try { -final byte[] saslMessage = negotiator.get().evaluateResponse(saslResponse); -if (negotiator.get().isComplete()) { -final AuthenticatedUser user = negotiator.get().getAuthenticatedUser(); - ctx.channel().attr(StateKey.AUTHENTICATED_USER).set(user); -// User name logged with the remote socket address and authenticator classname for audit logging -if (settings.enableAuditLog) { -String address = ctx.channel().remoteAddress().toString(); -if (address.startsWith("/") && address.length() > 1) address = address.substring(1); -final String[] authClassParts = authenticator.getClass().toString().split("[.]"); -auditLogger.info("User {} with address {} authenticated by {}", -user.getName(), address, authClassParts[authClassParts.length - 1]); -} -// If we have got here we are authenticated so remove the handler and pass -// the original message down the pipeline for processing -ctx.pipeline().remove(this); -final RequestMessage original = request.get(); -ctx.fireChannelRead(original); -} else { -// not done here - send back the sasl message for next challenge. -final Map metadata = new HashMap<>(); -metadata.put(Tokens.ARGS_SASL,
Re: [PR] [TINKERPOP-3061] fix: failing authentication when multiple initially requests are executed concurrently [tinkerpop]
Cole-Greer commented on code in PR #2525: URL: https://github.com/apache/tinkerpop/pull/2525#discussion_r1542198439 ## gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerAuthIntegrateTest.java: ## @@ -163,6 +171,46 @@ public void shouldFailAuthenticateWithPlainTextBadUsername() throws Exception { } } +@Test +public void shouldFailAuthenticateWithUnAuthenticatedRequestAfterMaxDeferrableDuration() throws Exception { Review Comment: Hi Tien, I want to be absolutely certain that we aren’t going to lose any of these deferred requests in the case of errors. If the server fails to send a response the drivers will just be left hanging indefinitely. If I’m understanding this test right, the first 3 requests are all expected to succeed, and then after a delay the final request is submitted and fails. Would it also be possible to setup a test such that there are multiple pending requests in the server’s deferred requests queue at the time that auth fails, and then we can verify that the correct error message gets sent to all currently pending requests? -- 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. To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [TINKERPOP-3061] fix: failing authentication when multiple initially requests are executed concurrently [tinkerpop]
tien commented on code in PR #2525: URL: https://github.com/apache/tinkerpop/pull/2525#discussion_r1541269520 ## gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/SaslAuthenticationHandler.java: ## @@ -75,106 +79,159 @@ public SaslAuthenticationHandler(final Authenticator authenticator, final Author @Override public void channelRead(final ChannelHandlerContext ctx, final Object msg) throws Exception { -if (msg instanceof RequestMessage){ -final RequestMessage requestMessage = (RequestMessage) msg; - -final Attribute negotiator = ((AttributeMap) ctx).attr(StateKey.NEGOTIATOR); -final Attribute request = ((AttributeMap) ctx).attr(StateKey.REQUEST_MESSAGE); -if (negotiator.get() == null) { -try { -// First time through so save the request and send an AUTHENTICATE challenge with no data - negotiator.set(authenticator.newSaslNegotiator(getRemoteInetAddress(ctx))); -request.set(requestMessage); -final ResponseMessage authenticate = ResponseMessage.build(requestMessage) -.code(ResponseStatusCode.AUTHENTICATE).create(); -ctx.writeAndFlush(authenticate); -} catch (Exception ex) { -// newSaslNegotiator can cause troubles - if we don't catch and respond nicely the driver seems -// to hang until timeout which isn't so nice. treating this like a server error as it means that -// the Authenticator isn't really ready to deal with requests for some reason. -logger.error(String.format("%s is not ready to handle requests - check its configuration or related services", -authenticator.getClass().getSimpleName()), ex); - -final ResponseMessage error = ResponseMessage.build(requestMessage) -.statusMessage("Authenticator is not ready to handle requests") -.code(ResponseStatusCode.SERVER_ERROR).create(); -ctx.writeAndFlush(error); -} -} else { -if (requestMessage.getOp().equals(Tokens.OPS_AUTHENTICATION) && requestMessage.getArgs().containsKey(Tokens.ARGS_SASL)) { - -final Object saslObject = requestMessage.getArgs().get(Tokens.ARGS_SASL); -final byte[] saslResponse; - -if(saslObject instanceof String) { -saslResponse = BASE64_DECODER.decode((String) saslObject); -} else { -final ResponseMessage error = ResponseMessage.build(request.get()) -.statusMessage("Incorrect type for : " + Tokens.ARGS_SASL + " - base64 encoded String is expected") - .code(ResponseStatusCode.REQUEST_ERROR_MALFORMED_REQUEST).create(); -ctx.writeAndFlush(error); -return; -} - -try { -final byte[] saslMessage = negotiator.get().evaluateResponse(saslResponse); -if (negotiator.get().isComplete()) { -final AuthenticatedUser user = negotiator.get().getAuthenticatedUser(); - ctx.channel().attr(StateKey.AUTHENTICATED_USER).set(user); -// User name logged with the remote socket address and authenticator classname for audit logging -if (settings.enableAuditLog) { -String address = ctx.channel().remoteAddress().toString(); -if (address.startsWith("/") && address.length() > 1) address = address.substring(1); -final String[] authClassParts = authenticator.getClass().toString().split("[.]"); -auditLogger.info("User {} with address {} authenticated by {}", -user.getName(), address, authClassParts[authClassParts.length - 1]); -} -// If we have got here we are authenticated so remove the handler and pass -// the original message down the pipeline for processing -ctx.pipeline().remove(this); -final RequestMessage original = request.get(); -ctx.fireChannelRead(original); -} else { -// not done here - send back the sasl message for next challenge. -final Map metadata = new HashMap<>(); -metadata.put(Tokens.ARGS_SASL,
Re: [PR] [TINKERPOP-3061] fix: failing authentication when multiple initially requests are executed concurrently [tinkerpop]
FlorianHockmann commented on code in PR #2525: URL: https://github.com/apache/tinkerpop/pull/2525#discussion_r1540816027 ## gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/simple/AbstractClient.java: ## @@ -27,8 +27,7 @@ import org.apache.tinkerpop.gremlin.util.message.ResponseMessage; import org.apache.tinkerpop.gremlin.util.message.ResponseStatusCode; -import java.util.ArrayList; -import java.util.List; +import java.util.*; Review Comment: Please revert this change. [Our dev docs explicitly mention that TinkerPop doesn't use wildcard imports](https://tinkerpop.apache.org/docs/current/dev/developer/#_code_style). ## gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerAuthIntegrateTest.java: ## @@ -218,9 +266,19 @@ public void shouldAuthenticateAndWorkWithVariablesOverGraphSONV1Serialization() private static void assertConnection(final Cluster cluster, final Client client) throws InterruptedException, ExecutionException { Review Comment: This method is used in 4 different tests, such as `shouldAuthenticateWithPlainText`. These 4 tests will now fail if submitting multiple requests initially in parallel isn't working. I think it would be good if we could keep these tests as simple as possible so they don't include parallelization of initial requests. A test like `shouldAuthenticateWithPlainText` should really only fail if _authenticate with plain text_ isn't working, not if submitting multiple requests in parallel isn't working. Long story short, I think it would be good if you could revert the changes to this method and instead write a new test specifically for the parallelization issue. ## gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/SaslAuthenticationHandler.java: ## @@ -75,106 +79,159 @@ public SaslAuthenticationHandler(final Authenticator authenticator, final Author @Override public void channelRead(final ChannelHandlerContext ctx, final Object msg) throws Exception { -if (msg instanceof RequestMessage){ -final RequestMessage requestMessage = (RequestMessage) msg; - -final Attribute negotiator = ((AttributeMap) ctx).attr(StateKey.NEGOTIATOR); -final Attribute request = ((AttributeMap) ctx).attr(StateKey.REQUEST_MESSAGE); -if (negotiator.get() == null) { -try { -// First time through so save the request and send an AUTHENTICATE challenge with no data - negotiator.set(authenticator.newSaslNegotiator(getRemoteInetAddress(ctx))); -request.set(requestMessage); -final ResponseMessage authenticate = ResponseMessage.build(requestMessage) -.code(ResponseStatusCode.AUTHENTICATE).create(); -ctx.writeAndFlush(authenticate); -} catch (Exception ex) { -// newSaslNegotiator can cause troubles - if we don't catch and respond nicely the driver seems -// to hang until timeout which isn't so nice. treating this like a server error as it means that -// the Authenticator isn't really ready to deal with requests for some reason. -logger.error(String.format("%s is not ready to handle requests - check its configuration or related services", -authenticator.getClass().getSimpleName()), ex); - -final ResponseMessage error = ResponseMessage.build(requestMessage) -.statusMessage("Authenticator is not ready to handle requests") -.code(ResponseStatusCode.SERVER_ERROR).create(); -ctx.writeAndFlush(error); -} -} else { -if (requestMessage.getOp().equals(Tokens.OPS_AUTHENTICATION) && requestMessage.getArgs().containsKey(Tokens.ARGS_SASL)) { - -final Object saslObject = requestMessage.getArgs().get(Tokens.ARGS_SASL); -final byte[] saslResponse; - -if(saslObject instanceof String) { -saslResponse = BASE64_DECODER.decode((String) saslObject); -} else { -final ResponseMessage error = ResponseMessage.build(request.get()) -.statusMessage("Incorrect type for : " + Tokens.ARGS_SASL + " - base64 encoded String is expected") - .code(ResponseStatusCode.REQUEST_ERROR_MALFORMED_REQUEST).create(); -ctx.writeAndFlush(error); -return; -} - -try { -final byte[] saslMessage = negotiator.get().evaluateResponse(saslResponse); -if
Re: [PR] [TINKERPOP-3061] fix: failing authentication when multiple initially requests are executed concurrently [tinkerpop]
tien commented on PR #2525: URL: https://github.com/apache/tinkerpop/pull/2525#issuecomment-2020601411 I've added one test for the unhappy path. -- 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. To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [TINKERPOP-3061] fix: failing authentication when multiple initially requests are executed concurrently [tinkerpop]
Cole-Greer commented on PR #2525: URL: https://github.com/apache/tinkerpop/pull/2525#issuecomment-2019163528 I agree with @kenhuuu that an additional test is warranted here to ensure that the server will always send a response to every request. We are now entering code freeze week in preparation for the 3.6.7 and 3.7.2 releases. I believe it is fair to grant an exception for a few days to give some time for such a test to be implemented and to ensure this PR can be included in the release. -- 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. To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [TINKERPOP-3061] fix: failing authentication when multiple initially requests are executed concurrently [tinkerpop]
kenhuuu commented on PR #2525: URL: https://github.com/apache/tinkerpop/pull/2525#issuecomment-2018983365 Could you also add a test where the authentication fails and you check that all requests get the proper exception in that case? -- 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. To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [TINKERPOP-3061] fix: failing authentication when multiple initially requests are executed concurrently [tinkerpop]
Cole-Greer commented on PR #2525: URL: https://github.com/apache/tinkerpop/pull/2525#issuecomment-2018796622 Thanks @tien, looks great. 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. To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [TINKERPOP-3061] fix: failing authentication when multiple initially requests are executed concurrently [tinkerpop]
vkagamlyk commented on PR #2525: URL: https://github.com/apache/tinkerpop/pull/2525#issuecomment-2010797352 > Have added 1 changelog entry & a test to the JS driver Thank you @tien! 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. To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [TINKERPOP-3061] fix: failing authentication when multiple initially requests are executed concurrently [tinkerpop]
tien commented on PR #2525: URL: https://github.com/apache/tinkerpop/pull/2525#issuecomment-2010727589 Have added 1 changelog entry & a test to the JS driver -- 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. To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [TINKERPOP-3061] fix: failing authentication when multiple initially requests are executed concurrently [tinkerpop]
vkagamlyk commented on PR #2525: URL: https://github.com/apache/tinkerpop/pull/2525#issuecomment-2005644096 I think this is a good solution for concurrent auth issue. Good to have test for js driver to be 100% sure. Also missing `changelog` entry -- 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. To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [TINKERPOP-3061] fix: failing authentication when multiple initially requests are executed concurrently [tinkerpop]
codecov-commenter commented on PR #2525: URL: https://github.com/apache/tinkerpop/pull/2525#issuecomment-2001946536 ## [Codecov](https://app.codecov.io/gh/apache/tinkerpop/pull/2525?dropdown=coverage=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) Report Attention: Patch coverage is `36.90476%` with `53 lines` in your changes are missing coverage. Please review. > Project coverage is 76.46%. Comparing base [(`9b46b67`)](https://app.codecov.io/gh/apache/tinkerpop/commit/9b46b6777d2fa250e41daacf2fa4554605aff53a?dropdown=coverage=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) to head [(`b65004e`)](https://app.codecov.io/gh/apache/tinkerpop/pull/2525?dropdown=coverage=pr=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). > Report is 42 commits behind head on 3.7-dev. | [Files](https://app.codecov.io/gh/apache/tinkerpop/pull/2525?dropdown=coverage=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Patch % | Lines | |---|---|---| | [...mlin/server/handler/SaslAuthenticationHandler.java](https://app.codecov.io/gh/apache/tinkerpop/pull/2525?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-Z3JlbWxpbi1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3RpbmtlcnBvcC9ncmVtbGluL3NlcnZlci9oYW5kbGVyL1Nhc2xBdXRoZW50aWNhdGlvbkhhbmRsZXIuamF2YQ==) | 36.14% | [45 Missing and 8 partials :warning: ](https://app.codecov.io/gh/apache/tinkerpop/pull/2525?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Additional details and impacted files ```diff @@ Coverage Diff @@ ## 3.7-dev#2525 +/- ## = + Coverage 76.14% 76.46% +0.32% - Complexity 1315213167 +15 = Files 1084 1059 -25 Lines 6516061292-3868 Branches7285 7298 +13 = - Hits 4961646868-2748 + Misses 1283911906 -933 + Partials2705 2518 -187 ``` [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/tinkerpop/pull/2525?dropdown=coverage=pr=continue_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). -- 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. To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [TINKERPOP-3061] fix: failing authentication when multiple initially requests are executed concurrently [tinkerpop]
tien opened a new pull request, #2525: URL: https://github.com/apache/tinkerpop/pull/2525 This solution try to resolve the concurrent initial unauthenticated requests problem described in [TINKERPOP-3063](https://issues.apache.org/jira/browse/TINKERPOP-3063), [TINKERPOP-2132](https://issues.apache.org/jira/browse/TINKERPOP-2132) & [TINKERPOP-3061](https://issues.apache.org/jira/browse/TINKERPOP-3061) by batching them for later processing when authentication handshake is in progress. -- 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. To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org