[
https://issues.apache.org/jira/browse/TINKERPOP-3061?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=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<Authenticator.SaslNegotiator> negotiator =
((AttributeMap) ctx).attr(StateKey.NEGOTIATOR);
- final Attribute<RequestMessage> 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<String,Object> metadata = new
HashMap<>();
- metadata.put(Tokens.ARGS_SASL,
BASE64_ENCODER.encodeToString(saslMessage));
- final ResponseMessage authenticate =
ResponseMessage.build(requestMessage)
- .statusAttributes(metadata)
-
.code(ResponseStatusCode.AUTHENTICATE).create();
- ctx.writeAndFlush(authenticate);
- }
- } catch (AuthenticationException ae) {
- final ResponseMessage error =
ResponseMessage.build(request.get())
- .statusMessage(ae.getMessage())
-
.code(ResponseStatusCode.UNAUTHORIZED).create();
- ctx.writeAndFlush(error);
- }
- } else {
- final ResponseMessage error =
ResponseMessage.build(requestMessage)
- .statusMessage("Failed to authenticate")
- .code(ResponseStatusCode.UNAUTHORIZED).create();
- ctx.writeAndFlush(error);
- }
- }
- } else {
+ if (!(msg instanceof RequestMessage)) {
logger.warn("{} only processes RequestMessage instances - received
{} - channel closing",
this.getClass().getSimpleName(), msg.getClass());
ctx.close();
+ return;
+ }
+
+ final RequestMessage requestMessage = (RequestMessage) msg;
+
+ final Attribute<Authenticator.SaslNegotiator> negotiator =
ctx.channel().attr(StateKey.NEGOTIATOR);
+ final Attribute<RequestMessage> request =
ctx.channel().attr(StateKey.REQUEST_MESSAGE);
+ final Attribute<Pair<LocalDateTime, List<RequestMessage>>>
deferredRequests = ctx.channel().attr(StateKey.DEFERRED_REQUEST_MESSAGES);
+
+ 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);
+
+ respondWithError(
+ requestMessage,
+ builder -> builder.statusMessage("Authenticator is not
ready to handle requests").code(ResponseStatusCode.SERVER_ERROR),
+ ctx);
+ }
+
+ return;
}
+
+ // If authentication negotiation is pending, store subsequent
non-authentication requests for later processing
+ if (negotiator.get() != null &&
!requestMessage.getOp().equals(Tokens.OPS_AUTHENTICATION)) {
+ deferredRequests.setIfAbsent(new
ImmutablePair<>(LocalDateTime.now(), new ArrayList<>()));
+ deferredRequests.get().getValue().add(requestMessage);
+
+ final Duration deferredDuration =
Duration.between(deferredRequests.get().getKey(), LocalDateTime.now());
+
+ if (deferredDuration.compareTo(MAX_REQUEST_DEFERRABLE_DURATION) >
0) {
+ respondWithError(
+ requestMessage,
+ builder -> builder.statusMessage("Too many unauthenticated
requests").code(ResponseStatusCode.TOO_MANY_REQUESTS),
Review Comment:
I'm stumped on this 2, do you have a recommendation on what status code &
message would make sense here?
> 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)
