[jira] [Commented] (TINKERPOP-3061) Concurrent queries will break authentication on javascript driver
[
https://issues.apache.org/jira/browse/TINKERPOP-3061?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17831591#comment-17831591
]
ASF GitHub Bot commented on TINKERPOP-3061:
---
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?
> Concurrent queries will break authentication on javascript driver
> -
>
> Key: TINKERPOP-3061
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3061
> Project: TinkerPop
> Issue Type: Bug
> Components: javascript
>Affects Versions: 3.6.6, 3.7.1
>Reporter: Yang Xia
>Priority: Major
>
> Reported by tien on Discord:
> {code:java}
> import gremlin from "gremlin";
> const g = gremlin.process.AnonymousTraversalSource.traversal().withRemote(
> new gremlin.driver.DriverRemoteConnection("ws://localhost:8182/gremlin", {
> authenticator: new gremlin.driver.auth.PlainTextSaslAuthenticator(
> "admin",
> "administrator"
> ),
> })
> );
> // This will throws: Failed to authenticate (401)
> await Promise.all([g.V().toList(), g.V().toList()]);
> // This works as expected
> await g.V().toList();
> await g.V().toList(); {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (TINKERPOP-3061) Concurrent queries will break authentication on javascript driver
[
https://issues.apache.org/jira/browse/TINKERPOP-3061?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17831389#comment-17831389
]
ASF GitHub Bot commented on TINKERPOP-3061:
---
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);
-
[jira] [Commented] (TINKERPOP-3063) Concurrent queries will break authentication on .NET driver
[
https://issues.apache.org/jira/browse/TINKERPOP-3063?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17831312#comment-17831312
]
ASF GitHub Bot commented on TINKERPOP-3063:
---
FlorianHockmann closed pull request #2522: TINKERPOP-3063 Fix bug in
Gremlin.Net authentication for parallel requests
URL: https://github.com/apache/tinkerpop/pull/2522
> Concurrent queries will break authentication on .NET driver
> ---
>
> Key: TINKERPOP-3063
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3063
> Project: TinkerPop
> Issue Type: Bug
> Components: dotnet
>Affects Versions: 3.6.6, 3.7.1
>Reporter: Florian Hockmann
>Assignee: Florian Hockmann
>Priority: Major
>
> Executing multiple queries in parallel can lead to authentication failures if
> {{MaxInProcessPerConnection}} is set to a value higher than {{1}} as the
> second request can then be send to the server while the server is still
> waiting for the authentication challenge response from the driver for the
> first query.
> A simple workaround is to set {{MaxInProcessPerConnection=1}} but this means
> of course that connection pooling will be less efficient.
> This issue also exists for other drivers:
> * Java: TINKERPOP-2132
> * JS: TINKERPOP-3061
> (I don't know about the Python and Go drivers.)
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (TINKERPOP-3063) Concurrent queries will break authentication on .NET driver
[
https://issues.apache.org/jira/browse/TINKERPOP-3063?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17831313#comment-17831313
]
ASF GitHub Bot commented on TINKERPOP-3063:
---
FlorianHockmann commented on PR #2522:
URL: https://github.com/apache/tinkerpop/pull/2522#issuecomment-2022583104
Closing this as it was superseded by #2525 which fixes the problem on the
server side so the workaround I implemented here for the .NET driver isn't
needed any more.
> Concurrent queries will break authentication on .NET driver
> ---
>
> Key: TINKERPOP-3063
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3063
> Project: TinkerPop
> Issue Type: Bug
> Components: dotnet
>Affects Versions: 3.6.6, 3.7.1
>Reporter: Florian Hockmann
>Assignee: Florian Hockmann
>Priority: Major
>
> Executing multiple queries in parallel can lead to authentication failures if
> {{MaxInProcessPerConnection}} is set to a value higher than {{1}} as the
> second request can then be send to the server while the server is still
> waiting for the authentication challenge response from the driver for the
> first query.
> A simple workaround is to set {{MaxInProcessPerConnection=1}} but this means
> of course that connection pooling will be less efficient.
> This issue also exists for other drivers:
> * Java: TINKERPOP-2132
> * JS: TINKERPOP-3061
> (I don't know about the Python and Go drivers.)
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (TINKERPOP-3061) Concurrent queries will break authentication on javascript driver
[
https://issues.apache.org/jira/browse/TINKERPOP-3061?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17831310#comment-17831310
]
ASF GitHub Bot commented on TINKERPOP-3061:
---
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();
-
[jira] [Commented] (TINKERPOP-3063) Concurrent queries will break authentication on .NET driver
[
https://issues.apache.org/jira/browse/TINKERPOP-3063?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17831274#comment-17831274
]
ASF GitHub Bot commented on TINKERPOP-3063:
---
FlorianHockmann commented on PR #2522:
URL: https://github.com/apache/tinkerpop/pull/2522#issuecomment-2022386126
> Do you have any thoughts on the server side solution by @tien in
https://github.com/apache/tinkerpop/pull/2525? If the server fix fully
addresses the issue, then it should make the changes here as well as equivalent
GLV changes unnecessary.
I'm just back from vacation which is why I couldn't review the PR yet.
But I agree in general that a server fix is definitely better in general
than implementing workarounds in all GLV drivers. I'll try to review #2525
today.
If it fixes the issue, then we of course don't need this PR any more.
> Concurrent queries will break authentication on .NET driver
> ---
>
> Key: TINKERPOP-3063
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3063
> Project: TinkerPop
> Issue Type: Bug
> Components: dotnet
>Affects Versions: 3.6.6, 3.7.1
>Reporter: Florian Hockmann
>Assignee: Florian Hockmann
>Priority: Major
>
> Executing multiple queries in parallel can lead to authentication failures if
> {{MaxInProcessPerConnection}} is set to a value higher than {{1}} as the
> second request can then be send to the server while the server is still
> waiting for the authentication challenge response from the driver for the
> first query.
> A simple workaround is to set {{MaxInProcessPerConnection=1}} but this means
> of course that connection pooling will be less efficient.
> This issue also exists for other drivers:
> * Java: TINKERPOP-2132
> * JS: TINKERPOP-3061
> (I don't know about the Python and Go drivers.)
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
Community Over Code NA 2024 Travel Assistance Applications now open!
Hello to all users, contributors and Committers! [ You are receiving this email as a subscriber to one or more ASF project dev or user mailing lists and is not being sent to you directly. It is important that we reach all of our users and contributors/committers so that they may get a chance to benefit from this. We apologise in advance if this doesn't interest you but it is on topic for the mailing lists of the Apache Software Foundation; and it is important please that you do not mark this as spam in your email client. Thank You! ] The Travel Assistance Committee (TAC) are pleased to announce that travel assistance applications for Community over Code NA 2024 are now open! We will be supporting Community over Code NA, Denver Colorado in October 7th to the 10th 2024. TAC exists to help those that would like to attend Community over Code events, but are unable to do so for financial reasons. For more info on this years applications and qualifying criteria, please visit the TAC website at < https://tac.apache.org/ >. Applications are already open on https://tac-apply.apache.org/, so don't delay! The Apache Travel Assistance Committee will only be accepting applications from those people that are able to attend the full event. Important: Applications close on Monday 6th May, 2024. Applicants have until the the closing date above to submit their applications (which should contain as much supporting material as required to efficiently and accurately process their request), this will enable TAC to announce successful applications shortly afterwards. As usual, TAC expects to deal with a range of applications from a diverse range of backgrounds; therefore, we encourage (as always) anyone thinking about sending in an application to do so ASAP. For those that will need a Visa to enter the Country - we advise you apply now so that you have enough time in case of interview delays. So do not wait until you know if you have been accepted or not. We look forward to greeting many of you in Denver, Colorado , October 2024! Kind Regards, Gavin (On behalf of the Travel Assistance Committee)
