This is an automated email from the ASF dual-hosted git repository. kenhuuu pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 2ed613e5440b82e4f6586e54bc14338d51cb82e4 Merge: 90abe4fe02 0e743cbc82 Author: Ken Hu <[email protected]> AuthorDate: Mon Aug 31 23:27:08 2026 -0700 Merge branch '3.8-dev' WebSocket authorization is omitted because master removed WebSocket support. Keeping the authenticated user request-local avoids shared mutable state. From the P.typeOf(Class) fix, port only the GraphSON 2/3 serializers with format-specific tests. Those formats still support P through arbitrary object IO and graph property values. Do not port the GraphBinary PSerializer or shared wire round-trip tests because GraphBinary 1 was removed, GraphBinary 4 has no P type, and the remaining PSerializer is unreachable. Do not port the HTTP buffer-release changes because master's MessageToMessageDecoder and SimpleChannelInboundHandler implementations already release inbound messages automatically. Restore changelog entries accidentally removed by later additions. Assisted-by: Codex:gpt-5.6-sol CHANGELOG.asciidoc | 13 +- THREAT_MODEL.md | 25 ++- .../traversal/step/map/VertexProgramStep.java | 14 +- .../io/graphson/TraversalSerializersV2.java | 3 + .../io/graphson/TraversalSerializersV3.java | 3 + .../traversal/step/map/VertexProgramStepTest.java | 178 +++++++++++++++++++++ .../io/graphson/PGraphSONSerializerTest.java | 72 +++++++++ .../tinkerpop/gremlin/server/authz/Authorizer.java | 5 + .../handler/HttpBasicAuthorizationHandler.java | 10 +- .../handler/HttpBasicAuthorizationHandlerTest.java | 130 +++++++++++++++ .../traversal/step/sideEffect/HadoopIoStep.java | 26 +++ .../step/sideEffect/HadoopIoStepTest.java | 173 ++++++++++++++++++++ 12 files changed, 635 insertions(+), 17 deletions(-) diff --cc CHANGELOG.asciidoc index b352b12c8d,037024be88..b03e3cb1c3 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@@ -232,10 -27,12 +232,13 @@@ image::https://raw.githubusercontent.co This release also includes changes from prior 3.7.x releases. + * Enabled building and running with Java 21 and Java 25 (experimental; `spark-gremlin` excluded, as Spark 3.3.x only runs on Java 8 through 17). + * Bumped to Groovy 4.0.32 which adds support for parsing Java 25 bytecode. + * Bumped Hadoop to 3.4.3 (and Kerby to 2.0.3) to enable `hadoop-gremlin` to build and run on Java 25. * Add missing `Configuring` interface to `GraphStepPlaceholder` and `VertexStepPlaceholder` * Fixed bug in `group()` value traversal where keys were retained with stale barrier state instead of being filtered when steps following a `Barrier` in the second `by()` produced no output (e.g. `by(values("age").fold().unfold())` or `by(__.out().fold().count(local).is(P.gt(0)))` for vertices with no out-edges). +* Corrected numerous inaccuracies in the reference documentation, including wrong default values (connection pool sizes, buffer sizes, ports, timeouts), stale serializer class names, removed options documented as available, and broken code examples across the JVM, Python, `.NET`, Go, and JavaScript drivers. - * Fixed bug in `gremlin-javascript` GraphBinary deserialization where `DateTime` values outside the JavaScript `Date` range were silently returned as invalid `Date` objects instead of failing deserialization. + * Fixed bug in `gremlin-javascript` GraphBinary and GraphSON deserialization where `OffsetDateTime` values outside the JavaScript `Date` range were silently returned as invalid `Date` objects instead of failing deserialization. * Added a `propertyMap()` helper to view an element's properties as a map keyed by property key, on the `Element` structure API in `gremlin-core` (inherited by `Vertex`, `Edge`, and `VertexProperty`) and on `Vertex`, `Edge`, and `VertexProperty` in `gremlin-javascript`, `gremlin-python`, `gremlin-dotnet`, and `gremlin-go`. * Fixed a bug in `gremlin-go` where a `VertexProperty` deserialized from GraphBinary did not have its `Key` field populated, causing `Vertex.PropertyMap()` to group properties under an empty key. * Removed `final` from `RepeatStep` to allow providers to extend it. @@@ -474,6 -273,7 +479,8 @@@ image::https://raw.githubusercontent.co * Removed the Mono dependency from the `gremlin-dotnet` build/release process, using `dotnet pack`/`dotnet nuget push` instead of `mono nuget.exe`. * Expanded `gremlin-python` CI matrix to test against Python 3.9, 3.10, 3.11, 3.12, and 3.13. * Add Node 26 support for `gremlin-javascript` and `gremlint`. ++* Corrected numerous inaccuracies in the reference documentation, including wrong default values (connection pool sizes, buffer sizes, ports, timeouts), stale serializer class names, removed options documented as available, and broken code examples across the JVM, Python, `.NET`, Go, and JavaScript drivers. + * Fixed Gremlin Server HTTP requests to release their buffers when request processing fails unexpectedly. * Fixed a panic in `gremlin-go` `PartitionStrategy` when `ReadPartitions` was left unset. * Fixed `gremlin-python` `ProductiveByStrategy` to pass through the `productiveKeys` argument, which was previously accepted but never serialized to the server. * Deprecated `ProductiveByStrategy` which was introduced as a temporary way to mimic pre-3.5.0 null processing behavior. diff --cc THREAT_MODEL.md index 9becade050,c60064092c..a46282bb2c --- a/THREAT_MODEL.md +++ b/THREAT_MODEL.md @@@ -248,6 -237,14 +249,14 @@@ Knobs that change which security proper and defaults to a locked registration allow-list (`registrationRequired=true`). Disabling that lock removes the untrusted-input protection (§9). *(documented — sample configs, `gremlin-applications.asciidoc` "Serialization")* -- **Class names admitted from untrusted bytes** — a class named in request or file bytes resolves only ++- **Class names admitted from untrusted bytes** — a class named in wire or file bytes resolves only + against what has been declared, and an operator can widen or narrow the declared set. Typed GraphSON 1.0 - reads admit further names through `allowedTypeIdNames`. `TraversalStrategies.GlobalCache.denyStrategy()` - permanently removes a strategy from the global name registry. OLAP reads its trust flag and allow-lists - from graph configuration (`gremlin.io.trusted`, `gremlin.io.approvedClasses`, ++ file reads admit further names through `allowedTypeIdNames`. `TraversalStrategies.GlobalCache.denyStrategy()` ++ permanently removes a strategy from the strategy-name registry. OLAP reads its trust flag and allow-lists from ++ graph configuration (`gremlin.io.trusted`, `gremlin.io.approvedClasses`, + `gremlin.io.approvedComputerConfigKeys`, `gremlin.io.approvedGraphConfigKeys`), where + `gremlin.io.trusted` disables the restriction for the deployment. Widening any of these is the operator's + decision and gives up the §8 deserializer-integrity property for whatever it admits (§9). ## §6 Assumptions about inputs @@@ -351,18 -343,16 +360,20 @@@ Per-surface trust table security-critical. - **Parser integrity (`gremlin-language`).** A crafted Gremlin string cannot break out of a string literal to inject additional traversal steps. This is the safe string-to-traversal path, distinct from building a - Groovy string by concatenation, which is the calling application's concern (§9). *Violation symptom:* + Groovy string by concatenation, which is the calling application's concern (§9). The same holds for the + GQL string of a `match(String)` request, parsed server-side by the `gql-gremlin` grammar. That grammar is + reference code active on the default distribution (TinkerGraph registers it), so its robustness (no + crash/hang/OOM on malformed GQL) and integrity (no breakout into the enclosing traversal) are in-model. A provider that does + not register a declarative-match strategy leaves the step non-executable (§3). *Violation symptom:* grammar breakout / step injection from a value that should stay a literal. *Severity:* critical. -- **Deserializer integrity.** The wire deserializers (GraphSON, GraphBinary) and **the hardened Gryo mappers the +- **Deserializer integrity.** The wire deserializer (GraphBinary), GraphSON and **the hardened Gryo mappers the IO paths build** (`registrationRequired=true` plus `javaSerializationAllowed=false`, i.e. `io()`, `GryoReader`, - `GryoWriter`, `GryoIo`, and the Hadoop Gryo input/output formats) reading attacker bytes do not lead to arbitrary - object instantiation / code execution beyond the registered type set. Because `inject()` and value arguments let a - request carry any supported type, a bug in a **registered** type's (de)serializer that crashes/OOMs the reader is also + `GryoWriter`, `GryoIo`, and the Hadoop Gryo input/output formats) reading attacker bytes do not let those bytes + cause code execution by selecting an undeclared class from the classpath for initialization, construction, or + reflective invocation, and do not reach native Java deserialization + (`ObjectInputStream.readObject()`). Because `inject()` and value arguments let a request carry any + supported type, a bug in a **registered** type's (de)serializer that crashes/OOMs the reader is also in-model, on **both** the server (request) and the GLV (response) side. The GraphML reader disables external entities and DTDs by default (XXE-safe). *Violation symptom:* deserialization gadget / RCE / XXE, or a registered-type serializer crashing/OOMing either end. *Severity:* critical. Gryo is not on the wire, diff --cc gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/TraversalSerializersV2.java index 9ce354a876,9daf43f80b..100ea9ff1b --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/TraversalSerializersV2.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/TraversalSerializersV2.java @@@ -124,6 -165,10 +124,9 @@@ final class TraversalSerializersV2 jsonGenerator.writeObject(object); } jsonGenerator.writeEndArray(); + } else if ("typeOf".equals(p.getPredicateName()) && p.getValue() instanceof Class) { - // as in binary/types/PSerializer, typeOf's Class argument travels as the simple name so the read - // resolves it against CompareType.GlobalTypeCache. ++ // Resolve the decoded value through CompareType.GlobalTypeCache. + jsonGenerator.writeObjectField(GraphSONTokens.VALUE, ((Class<?>) p.getValue()).getSimpleName()); } else jsonGenerator.writeObjectField(GraphSONTokens.VALUE, p.getValue()); } diff --cc gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/TraversalSerializersV3.java index 951728ad59,2191e2654e..5d90abef86 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/TraversalSerializersV3.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/TraversalSerializersV3.java @@@ -122,6 -165,10 +122,9 @@@ final class TraversalSerializersV3 jsonGenerator.writeEndArray(); } else if (p instanceof NotP) { jsonGenerator.writeObjectField(GraphSONTokens.VALUE, p.negate()); + } else if ("typeOf".equals(p.getPredicateName()) && p.getValue() instanceof Class) { - // as in binary/types/PSerializer, typeOf's Class argument travels as the simple name so the read - // resolves it against CompareType.GlobalTypeCache. ++ // Resolve the decoded value through CompareType.GlobalTypeCache. + jsonGenerator.writeObjectField(GraphSONTokens.VALUE, ((Class<?>) p.getValue()).getSimpleName()); } else jsonGenerator.writeObjectField(GraphSONTokens.VALUE, p.getValue()); diff --cc gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/computer/traversal/step/map/VertexProgramStepTest.java index 0000000000,efa198c300..8bd4c7ffcc mode 000000,100644..100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/computer/traversal/step/map/VertexProgramStepTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/computer/traversal/step/map/VertexProgramStepTest.java @@@ -1,0 -1,121 +1,178 @@@ + /* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + package org.apache.tinkerpop.gremlin.process.computer.traversal.step.map; + + import org.apache.tinkerpop.gremlin.process.computer.Computer; + import org.apache.tinkerpop.gremlin.process.computer.ComputerResult; + import org.apache.tinkerpop.gremlin.process.computer.GraphComputer; + import org.apache.tinkerpop.gremlin.process.computer.Memory; + import org.apache.tinkerpop.gremlin.process.computer.VertexProgram; + import org.apache.tinkerpop.gremlin.process.traversal.Traversal; + import org.apache.tinkerpop.gremlin.process.traversal.Traverser; + import org.apache.tinkerpop.gremlin.process.traversal.TraverserGenerator; + import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; + import org.apache.tinkerpop.gremlin.process.traversal.traverser.util.TraverserSet; + import org.apache.tinkerpop.gremlin.structure.Graph; + import org.apache.tinkerpop.gremlin.structure.util.empty.EmptyGraph; + import org.junit.Test; + + import java.util.Collections; + import java.util.Optional; + import java.util.concurrent.CompletableFuture; + + import static org.junit.Assert.assertSame; + import static org.mockito.ArgumentMatchers.any; + import static org.mockito.ArgumentMatchers.anyLong; + import static org.mockito.Mockito.mock; + import static org.mockito.Mockito.never; + import static org.mockito.Mockito.verify; + import static org.mockito.Mockito.when; + + public class VertexProgramStepTest { + + // The default seam is identity (returns the graph unchanged); the non-overriding subclasses rely on this to keep + // binding and generating against the graph they are handed. + @Test + public void shouldResolveComputeGraphToSameGraphByDefault() { + final Graph graph = EmptyGraph.instance(); + final VertexProgramStep step = new VertexProgramStep(__.start().asAdmin()) { + @Override + public VertexProgram generateProgram(final Graph graph, final Memory memory) { + return null; + } + }; + assertSame(graph, step.resolveComputeGraph(graph)); + } + + // The graph resolveComputeGraph returns must flow to BOTH getComputer().apply(graph) and generateProgram(graph, ...). + // Driving processNextStart with the seam returning a distinct instance, both the computer binding and the program + // generation must observe that same instance, so a reorder that binds the computer to a different graph than the + // one configured is caught here. + @Test + @SuppressWarnings({"unchecked", "rawtypes"}) - public void shouldBindComputerAndGenerateProgramToTheSameResolvedGraph() { ++ public void shouldBindComputerAndGenerateProgramToTheSameResolvedGraphForInitialExecution() { + final Graph handed = mock(Graph.class); // what the traversal exposes + final Graph resolved = mock(Graph.class); // what the seam returns -- distinct, so we can tell them apart + + // Computer is final and cannot be mocked; use a real Computer.compute() whose apply(graph) calls graph.compute(). + // The GraphComputer is produced by the graph the computer is applied to, so stubbing compute() ONLY on `resolved` + // means the chain completes iff apply() bound to `resolved` -- and we verify that call directly. + final Memory memory = mock(Memory.class); + when(memory.keys()).thenReturn(Collections.emptySet()); + final ComputerResult result = mock(ComputerResult.class); + when(result.memory()).thenReturn(memory); + final GraphComputer graphComputer = mock(GraphComputer.class); + when(graphComputer.program(any())).thenReturn(graphComputer); + when(graphComputer.submit()).thenReturn(CompletableFuture.completedFuture(result)); + when(resolved.compute()).thenReturn(graphComputer); + final Computer computer = Computer.compute(); // graphComputerClass == GraphComputer.class -> apply calls graph.compute() + + // mock the traversal so no strategies/real graph are involved; getGraph() must be present (non-Empty) + final TraverserGenerator generator = mock(TraverserGenerator.class); + when(generator.generate(any(), any(), anyLong())).thenReturn(mock(Traverser.Admin.class)); + final Traversal.Admin traversal = mock(Traversal.Admin.class); + when(traversal.getTraverserSetSupplier()).thenReturn((java.util.function.Supplier) TraverserSet::new); + when(traversal.getGraph()).thenReturn(Optional.of(handed)); + when(traversal.getTraverserGenerator()).thenReturn(generator); + + final Graph[] generatedAgainst = new Graph[1]; + final VertexProgramStep step = new VertexProgramStep(traversal) { + @Override + public Computer getComputer() { + return computer; + } + + @Override + protected Graph resolveComputeGraph(final Graph graph) { + return resolved; + } + + @Override + public VertexProgram generateProgram(final Graph graph, final Memory memory) { + generatedAgainst[0] = graph; + return mock(VertexProgram.class); + } + }; + + step.processNextStart(); + + // the computer was applied to the resolved graph (apply() -> graph.compute() on `resolved`, never on `handed`) + verify(resolved).compute(); + verify(handed, never()).compute(); + // and the program was generated against that same resolved graph + assertSame("the program must be generated against the graph the seam resolved", resolved, generatedAgainst[0]); + } ++ ++ @Test ++ @SuppressWarnings({"unchecked", "rawtypes"}) ++ public void shouldBindComputerAndGenerateProgramToTheSameResolvedGraphForChainedExecution() { ++ final Graph handed = mock(Graph.class); ++ final Graph resolved = mock(Graph.class); ++ final Memory inputMemory = mock(Memory.class); ++ final Memory outputMemory = mock(Memory.class); ++ when(outputMemory.keys()).thenReturn(Collections.emptySet()); ++ ++ final ComputerResult inputResult = mock(ComputerResult.class); ++ when(inputResult.graph()).thenReturn(handed); ++ when(inputResult.memory()).thenReturn(inputMemory); ++ final ComputerResult outputResult = mock(ComputerResult.class); ++ when(outputResult.memory()).thenReturn(outputMemory); ++ ++ final GraphComputer graphComputer = mock(GraphComputer.class); ++ when(graphComputer.program(any())).thenReturn(graphComputer); ++ when(graphComputer.submit()).thenReturn(CompletableFuture.completedFuture(outputResult)); ++ when(resolved.compute()).thenReturn(graphComputer); ++ final Computer computer = Computer.compute(); ++ ++ final Traversal.Admin traversal = mock(Traversal.Admin.class); ++ when(traversal.getTraverserSetSupplier()).thenReturn((java.util.function.Supplier) TraverserSet::new); ++ final Traverser.Admin<ComputerResult> inputTraverser = mock(Traverser.Admin.class); ++ when(inputTraverser.get()).thenReturn(inputResult); ++ ++ final Graph[] generatedAgainst = new Graph[1]; ++ final Memory[] generatedWith = new Memory[1]; ++ final VertexProgramStep step = new VertexProgramStep(traversal) { ++ @Override ++ public Computer getComputer() { ++ return computer; ++ } ++ ++ @Override ++ protected Graph resolveComputeGraph(final Graph graph) { ++ return resolved; ++ } ++ ++ @Override ++ public VertexProgram generateProgram(final Graph graph, final Memory memory) { ++ generatedAgainst[0] = graph; ++ generatedWith[0] = memory; ++ return mock(VertexProgram.class); ++ } ++ }; ++ ++ step.first = false; ++ step.addStart(inputTraverser); ++ step.processNextStart(); ++ ++ verify(resolved).compute(); ++ verify(handed, never()).compute(); ++ assertSame(resolved, generatedAgainst[0]); ++ assertSame(inputMemory, generatedWith[0]); ++ } + } diff --cc gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/structure/io/graphson/PGraphSONSerializerTest.java index 0000000000,0000000000..f87f55d4da new file mode 100644 --- /dev/null +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/structure/io/graphson/PGraphSONSerializerTest.java @@@ -1,0 -1,0 +1,72 @@@ ++/* ++ * Licensed to the Apache Software Foundation (ASF) under one ++ * or more contributor license agreements. See the NOTICE file ++ * distributed with this work for additional information ++ * regarding copyright ownership. The ASF licenses this file ++ * to you under the Apache License, Version 2.0 (the ++ * "License"); you may not use this file except in compliance ++ * with the License. You may obtain a copy of the License at ++ * ++ * http://www.apache.org/licenses/LICENSE-2.0 ++ * ++ * Unless required by applicable law or agreed to in writing, ++ * software distributed under the License is distributed on an ++ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY ++ * KIND, either express or implied. See the License for the ++ * specific language governing permissions and limitations ++ * under the License. ++ */ ++package org.apache.tinkerpop.gremlin.structure.io.graphson; ++ ++import org.apache.tinkerpop.gremlin.process.traversal.GType; ++import org.apache.tinkerpop.gremlin.process.traversal.P; ++import org.apache.tinkerpop.shaded.jackson.databind.ObjectMapper; ++import org.junit.Test; ++import org.junit.runner.RunWith; ++import org.junit.runners.Parameterized; ++ ++import java.util.Arrays; ++ ++import static org.junit.Assert.assertEquals; ++import static org.junit.Assert.assertFalse; ++import static org.junit.Assert.assertTrue; ++ ++@RunWith(Parameterized.class) ++public class PGraphSONSerializerTest { ++ ++ @Parameterized.Parameters(name = "{0}") ++ public static Iterable<Object[]> data() { ++ return Arrays.asList(new Object[][]{ ++ {"v2", GraphSONMapper.build().version(GraphSONVersion.V2_0). ++ typeInfo(TypeInfo.PARTIAL_TYPES).create().createMapper()}, ++ {"v3", GraphSONMapper.build().version(GraphSONVersion.V3_0). ++ typeInfo(TypeInfo.PARTIAL_TYPES).create().createMapper()} ++ }); ++ } ++ ++ @Parameterized.Parameter(0) ++ public String version; ++ ++ @Parameterized.Parameter(1) ++ public ObjectMapper mapper; ++ ++ @Test ++ public void shouldRoundTripTypeOfClassAsRegisteredTypeName() throws Exception { ++ final P<Object> decoded = roundTrip(P.typeOf(Boolean.class)); ++ ++ assertEquals("Boolean", decoded.getValue()); ++ assertTrue(decoded.test(true)); ++ assertFalse(decoded.test("true")); ++ } ++ ++ @Test ++ public void shouldPreserveTypeOfGTypeAndStringValues() throws Exception { ++ assertEquals(GType.BOOLEAN, roundTrip(P.typeOf(GType.BOOLEAN)).getValue()); ++ assertEquals("Boolean", roundTrip(P.typeOf("Boolean")).getValue()); ++ } ++ ++ @SuppressWarnings("unchecked") ++ private P<Object> roundTrip(final P<?> predicate) throws Exception { ++ return mapper.readValue(mapper.writeValueAsString(predicate), P.class); ++ } ++} diff --cc gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpBasicAuthorizationHandler.java index 30787e9fed,048a1c11b7..fb4dea96c9 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpBasicAuthorizationHandler.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpBasicAuthorizationHandler.java @@@ -57,36 -57,45 +56,35 @@@ public class HttpBasicAuthorizationHand @Override public void channelRead(final ChannelHandlerContext ctx, final Object msg) { - if (msg instanceof FullHttpMessage){ - final FullHttpMessage request = (FullHttpMessage) msg; + if (msg instanceof RequestMessage) { + final RequestMessage requestMessage = (RequestMessage) msg; ++ final AuthenticatedUser channelUser = ctx.channel().attr(StateKey.AUTHENTICATED_USER).get(); ++ // channelUser is null when using the AllowAllAuthenticator ++ final AuthenticatedUser user = null == channelUser ? ++ AuthenticatedUser.ANONYMOUS_USER : channelUser; + try { - user = ctx.channel().attr(StateKey.AUTHENTICATED_USER).get(); - if (null == user) { // This is expected when using the AllowAllAuthenticator - user = AuthenticatedUser.ANONYMOUS_USER; - final boolean keepAlive = HttpUtil.isKeepAlive(request); - final RequestMessage requestMessage; - try { - requestMessage = HttpHandlerUtil.getRequestMessageFromHttpRequest((FullHttpRequest) request); - } catch (IllegalArgumentException iae) { - HttpHandlerUtil.sendError(ctx, BAD_REQUEST, iae.getMessage(), keepAlive); - return; -- } - + final String gremlin = requestMessage.getGremlin(); + final Map<String, String> aliases = new HashMap<>(); + aliases.put(Tokens.ARGS_G, requestMessage.getField(Tokens.ARGS_G)); + final String restrictedGremlin = authorizer.authorize(user, gremlin, aliases); + final RequestMessage restrictedMsg = RequestMessage.from(requestMessage, restrictedGremlin).create(); + ctx.fireChannelRead(restrictedMsg); - final AuthenticatedUser channelUser = ctx.channel().attr(StateKey.AUTHENTICATED_USER).get(); - // channelUser is null when using the AllowAllAuthenticator - final AuthenticatedUser user = null == channelUser ? - AuthenticatedUser.ANONYMOUS_USER : channelUser; - try { - authorizer.authorize(user, requestMessage); - ctx.fireChannelRead(ReferenceCountUtil.retain(request)); - } catch (AuthorizationException ex) { // Expected: users can alternate between allowed and disallowed requests - String address = ctx.channel().remoteAddress().toString(); - if (address.startsWith("/") && address.length() > 1) address = address.substring(1); - final String script; - try { - script = HttpHandlerUtil.getRequestMessageFromHttpRequest((FullHttpRequest) request).getArgOrDefault(Tokens.ARGS_GREMLIN, ""); - } catch (IllegalArgumentException iae) { - HttpHandlerUtil.sendError(ctx, BAD_REQUEST, requestMessage.getRequestId(), iae.getMessage(), keepAlive); - return; - } - auditLogger.info("User {} with address {} attempted an unauthorized http request: {}", - user.getName(), address, script); - final String message = String.format("No authorization for script [%s] - check permissions.", script); - HttpHandlerUtil.sendError(ctx, UNAUTHORIZED, requestMessage.getRequestId(), message, keepAlive); - } catch (Exception ex) { - final String message = String.format( - "%s is not ready to handle requests - unknown error", authorizer.getClass().getSimpleName()); - HttpHandlerUtil.sendError(ctx, INTERNAL_SERVER_ERROR, requestMessage.getRequestId(), message, keepAlive); - } - } finally { + } catch (AuthorizationException ex) { // Expected: users can alternate between allowed and disallowed requests + String address = ctx.channel().remoteAddress().toString(); + if (address.startsWith("/") && address.length() > 1) address = address.substring(1); + final String script = requestMessage.getGremlin().toString(); + auditLogger.info("User {} with address {} attempted an unauthorized http request: {}", + user.getName(), address, script); + HttpHandlerUtil.sendError(ctx, UNAUTHORIZED, "Failed to authorize: " + ex.getMessage()); + ReferenceCountUtil.release(msg); + } catch (Exception ex) { + final String message = String.format( + "%s is not ready to handle requests - unknown error", authorizer.getClass().getSimpleName()); + logger.error("Unexpected error authorizing HTTP request for user {} with {}", user.getName(), + authorizer.getClass().getSimpleName(), ex); + HttpHandlerUtil.sendError(ctx, INTERNAL_SERVER_ERROR, message); ReferenceCountUtil.release(msg); } } else { diff --cc gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/handler/HttpBasicAuthorizationHandlerTest.java index 0000000000,992785fa83..dc8efb4902 mode 000000,100644..100644 --- a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/handler/HttpBasicAuthorizationHandlerTest.java +++ b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/handler/HttpBasicAuthorizationHandlerTest.java @@@ -1,0 -1,211 +1,130 @@@ + /* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + package org.apache.tinkerpop.gremlin.server.handler; + -import io.netty.buffer.ByteBuf; -import io.netty.buffer.ByteBufAllocator; + import io.netty.channel.embedded.EmbeddedChannel; -import io.netty.handler.codec.http.DefaultFullHttpRequest; -import io.netty.handler.codec.http.DefaultHttpHeaders; -import io.netty.handler.codec.http.FullHttpRequest; -import io.netty.handler.codec.http.FullHttpResponse; -import io.netty.handler.codec.http.HttpMethod; -import io.netty.handler.codec.http.HttpVersion; -import io.netty.handler.codec.http.QueryStringEncoder; -import io.netty.util.CharsetUtil; -import io.netty.util.ReferenceCountUtil; -import org.apache.tinkerpop.gremlin.process.traversal.Bytecode; + import org.apache.tinkerpop.gremlin.server.auth.AuthenticatedUser; + import org.apache.tinkerpop.gremlin.server.authz.AuthorizationException; + import org.apache.tinkerpop.gremlin.server.authz.Authorizer; -import org.apache.tinkerpop.gremlin.util.Tokens; + import org.apache.tinkerpop.gremlin.util.message.RequestMessage; -import org.apache.tinkerpop.gremlin.util.ser.GraphBinaryMessageSerializerV1; + import org.junit.Test; + + import java.util.Map; + import java.util.concurrent.CountDownLatch; + import java.util.concurrent.ExecutorService; + import java.util.concurrent.Executors; + import java.util.concurrent.Future; + import java.util.concurrent.TimeUnit; + import java.util.concurrent.atomic.AtomicInteger; + -import static io.netty.handler.codec.http.HttpResponseStatus.BAD_REQUEST; + import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertSame; + import static org.junit.Assert.assertTrue; + + public class HttpBasicAuthorizationHandlerTest { + - private final Authorizer authorizer = new Authorizer() { - @Override - public void setup(final Map<String, Object> config) throws AuthorizationException { - } - - @Override - public Bytecode authorize(final AuthenticatedUser user, final Bytecode bytecode, - final Map<String, String> aliases) throws AuthorizationException { - return bytecode; - } - - @Override - public void authorize(final AuthenticatedUser user, final RequestMessage msg) throws AuthorizationException { - } - }; - + @Test + public void shouldHandleRejectedRequestWithTheUserFromItsChannel() throws Exception { + final CountDownLatch firstRequestInAuthorizer = new CountDownLatch(1); + final CountDownLatch continueFirstRequest = new CountDownLatch(1); + final BlockingAuthorizer authorizer = + new BlockingAuthorizer(firstRequestInAuthorizer, continueFirstRequest); + final HttpBasicAuthorizationHandler handler = new HttpBasicAuthorizationHandler(authorizer); + final EmbeddedChannel firstChannel = new EmbeddedChannel(handler); + final EmbeddedChannel secondChannel = new EmbeddedChannel(handler); + final RecordingUser firstUser = new RecordingUser("first"); + final RecordingUser secondUser = new RecordingUser("second"); + firstChannel.attr(StateKey.AUTHENTICATED_USER).set(firstUser); + secondChannel.attr(StateKey.AUTHENTICATED_USER).set(secondUser); + - final FullHttpRequest firstRequest = createRequest("first"); - final FullHttpRequest secondRequest = createRequest("second"); ++ final RequestMessage firstRequest = RequestMessage.build("first").create(); ++ final RequestMessage secondRequest = RequestMessage.build("second").create(); + final ExecutorService executor = Executors.newSingleThreadExecutor(); + try { + final Future<Boolean> firstResult = executor.submit(() -> firstChannel.writeInbound(firstRequest)); + assertTrue(firstRequestInAuthorizer.await(5, TimeUnit.SECONDS)); + + secondChannel.writeInbound(secondRequest); + continueFirstRequest.countDown(); + firstResult.get(5, TimeUnit.SECONDS); + + assertEquals(1, firstUser.getNameCalls()); + assertEquals(0, secondUser.getNameCalls()); + } finally { + continueFirstRequest.countDown(); + executor.shutdownNow(); + executor.awaitTermination(5, TimeUnit.SECONDS); + firstChannel.finishAndReleaseAll(); + secondChannel.finishAndReleaseAll(); + } + } + - private static FullHttpRequest createRequest(final String script) { - final QueryStringEncoder encoder = new QueryStringEncoder("/"); - encoder.addParam(Tokens.ARGS_GREMLIN, script); - return new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, encoder.toString()); - } - + private static class BlockingAuthorizer implements Authorizer { + private final CountDownLatch firstRequestInAuthorizer; + private final CountDownLatch continueFirstRequest; + + private BlockingAuthorizer(final CountDownLatch firstRequestInAuthorizer, + final CountDownLatch continueFirstRequest) { + this.firstRequestInAuthorizer = firstRequestInAuthorizer; + this.continueFirstRequest = continueFirstRequest; + } + + @Override + public void setup(final Map<String, Object> config) { + } + + @Override - public Bytecode authorize(final AuthenticatedUser user, final Bytecode bytecode, - final Map<String, String> aliases) throws AuthorizationException { - return bytecode; - } - - @Override - public void authorize(final AuthenticatedUser user, final RequestMessage msg) throws AuthorizationException { - if (!"first".equals(msg.getArg(Tokens.ARGS_GREMLIN))) - return; ++ public String authorize(final AuthenticatedUser user, final String gremlin, ++ final Map<String, String> aliases) throws AuthorizationException { ++ if (!"first".equals(gremlin)) ++ return gremlin; + + firstRequestInAuthorizer.countDown(); + try { + if (!continueFirstRequest.await(5, TimeUnit.SECONDS)) + throw new AuthorizationException("Timed out waiting for the second request"); + } catch (InterruptedException ex) { + Thread.currentThread().interrupt(); + throw new AuthorizationException("Interrupted while waiting for the second request", ex); + } + throw new AuthorizationException("Request rejected"); + } ++ ++ @Override ++ public void authorize(final AuthenticatedUser user, final RequestMessage msg) { ++ } + } + + private static class RecordingUser extends AuthenticatedUser { + private final AtomicInteger nameCalls = new AtomicInteger(); + + private RecordingUser(final String name) { + super(name); + } + + @Override + public String getName() { + nameCalls.incrementAndGet(); + return super.getName(); + } + + private int getNameCalls() { + return nameCalls.get(); + } + } - - @Test - public void shouldReleaseRejectedGraphBinaryRequest() throws Exception { - final RequestMessage requestMessage = RequestMessage.build("eval").addArg("gremlin", "g.V()").create(); - final FullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/", - new GraphBinaryMessageSerializerV1().serializeRequestAsBinary(requestMessage, ByteBufAllocator.DEFAULT)); - final EmbeddedChannel channel = new EmbeddedChannel(new HttpBasicAuthorizationHandler(authorizer)); - FullHttpResponse response = null; - try { - assertFalse(channel.writeInbound(request)); - - assertEquals(0, request.refCnt()); - response = channel.readOutbound(); - assertNotNull(response); - assertEquals(BAD_REQUEST, response.status()); - } finally { - ReferenceCountUtil.release(response); - channel.finishAndReleaseAll(); - } - } - - @Test - public void shouldTransferAuthorizedRequestOwnership() { - final ByteBuf buffer = ByteBufAllocator.DEFAULT.buffer(); - buffer.writeCharSequence("{\"gremlin\":\"g.V()\"}", CharsetUtil.UTF_8); - final FullHttpRequest request = new DefaultFullHttpRequest( - HttpVersion.HTTP_1_1, HttpMethod.POST, "/", buffer, new DefaultHttpHeaders(), new DefaultHttpHeaders()); - final EmbeddedChannel channel = new EmbeddedChannel(new HttpBasicAuthorizationHandler(authorizer)); - FullHttpRequest forwarded = null; - try { - assertTrue(channel.writeInbound(request)); - - assertEquals(1, request.refCnt()); - forwarded = channel.readInbound(); - assertSame(request, forwarded); - } finally { - ReferenceCountUtil.release(forwarded); - channel.finishAndReleaseAll(); - } - } + } diff --cc hadoop-gremlin/src/test/java/org/apache/tinkerpop/gremlin/hadoop/process/computer/traversal/step/sideEffect/HadoopIoStepTest.java index 09c6043d27,8d4b5f9510..0b43be4a62 --- a/hadoop-gremlin/src/test/java/org/apache/tinkerpop/gremlin/hadoop/process/computer/traversal/step/sideEffect/HadoopIoStepTest.java +++ b/hadoop-gremlin/src/test/java/org/apache/tinkerpop/gremlin/hadoop/process/computer/traversal/step/sideEffect/HadoopIoStepTest.java @@@ -231,4 -243,165 +243,165 @@@ public class HadoopIoStepTest assertEquals("org.provider.CustomOutputFormat", graph.configuration().getString(Constants.GREMLIN_HADOOP_GRAPH_WRITER)); } + + // Request isolation: io() must configure a per-request copy of the graph, never the shared, long-lived HadoopGraph + // configuration reused across requests. The following cover the isolation seam directly. + + @Test + public void shouldIsolateGraphConfigurationIntoADistinctInstance() { + final HadoopGraph shared = HadoopGraph.open(new BaseConfiguration()); + final Graph local = HadoopIoStep.isolateGraphConfiguration(shared); + assertNotSame("io() must run against a request-local graph, not the shared one", shared, local); + assertNotSame("the request-local graph must have its own configuration instance", + shared.configuration(), local.configuration()); + // mutating either configuration must not affect the other (isolation in both directions) + local.configuration().setProperty("only.on.local", "v"); + assertFalse(shared.configuration().containsKey("only.on.local")); + shared.configuration().setProperty("only.on.shared", "v"); + assertFalse(local.configuration().containsKey("only.on.shared")); + } + + @Test + public void shouldCarryPristineKeysIntoIsolatedConfiguration() { + final Configuration config = new BaseConfiguration(); + config.setProperty(OlapClassLoadingPolicy.TRUSTED, true); + config.setProperty(Constants.GREMLIN_HADOOP_GRAPH_READER, "org.provider.CustomInputFormat"); + final HadoopGraph shared = HadoopGraph.open(config); + final Graph local = HadoopIoStep.isolateGraphConfiguration(shared); + // the copy carries every operator key forward, so trust and approved-format seeding stay intact + assertEquals(true, local.configuration().getBoolean(OlapClassLoadingPolicy.TRUSTED)); + assertEquals("org.provider.CustomInputFormat", + local.configuration().getString(Constants.GREMLIN_HADOOP_GRAPH_READER)); + } + + @Test + public void shouldReturnAFreshInstanceForEachIsolationCall() { + // each call yields a brand-new request-local graph/config (no caching or reuse); a structural guard, not + // concurrency coverage -- see shouldIsolateConcurrentRequestsFromEachOther. + final HadoopGraph shared = HadoopGraph.open(new BaseConfiguration()); + final Graph a = HadoopIoStep.isolateGraphConfiguration(shared); + final Graph b = HadoopIoStep.isolateGraphConfiguration(shared); + assertNotSame(a, b); + assertNotSame(a.configuration(), b.configuration()); + } + + @Test + public void shouldIsolateConcurrentRequestsFromEachOther() throws Exception { + // Two io() requests configuring against the SAME shared graph on two threads at once must each see only their + // own reader/input location, and must leave the shared graph unmutated. Isolation is structural (each request + // configures its own copy), so this is deterministic under any interleaving; the barrier forces the two + // requests to configure concurrently, exercising concurrent reads of the shared configuration. + final HadoopGraph shared = HadoopGraph.open(new BaseConfiguration()); + final CyclicBarrier barrier = new CyclicBarrier(2); + final Map<String, String> readerByThread = new ConcurrentHashMap<>(); + final Map<String, String> locationByThread = new ConcurrentHashMap<>(); + final List<Throwable> errors = Collections.synchronizedList(new ArrayList<>()); + + final BiConsumer<String, String> configureRequest = (name, file) -> { + try { + final HadoopIoStep step = new HadoopIoStep(__.start().asAdmin(), file); + step.setMode(ReadWriting.Mode.READING); + barrier.await(); // release both threads together so they configure concurrently + final Graph local = step.resolveComputeGraph(shared); + step.generateProgram(local, null); + readerByThread.put(name, local.configuration().getString(Constants.GREMLIN_HADOOP_GRAPH_READER)); + locationByThread.put(name, local.configuration().getString(Constants.GREMLIN_HADOOP_INPUT_LOCATION)); + } catch (final Throwable t) { + errors.add(t); + } + }; + + final Thread a = new Thread(() -> configureRequest.accept("A", "a.kryo")); + final Thread b = new Thread(() -> configureRequest.accept("B", "b.json")); + a.start(); + b.start(); + a.join(); + b.join(); + + assertTrue("no request should error: " + errors, errors.isEmpty()); + // each request observed only its own reader + input location (Gryo/a.kryo for A, GraphSON/b.json for B) - assertEquals(GryoInputFormatName(), readerByThread.get("A")); ++ assertEquals(gryoInputFormatName(), readerByThread.get("A")); + assertEquals("a.kryo", locationByThread.get("A")); + assertEquals(GraphSONInputFormat.class.getName(), readerByThread.get("B")); + assertEquals("b.json", locationByThread.get("B")); + // and neither concurrent request mutated the shared, long-lived graph configuration + assertFalse(shared.configuration().containsKey(Constants.GREMLIN_HADOOP_GRAPH_READER)); + assertFalse(shared.configuration().containsKey(Constants.GREMLIN_HADOOP_INPUT_LOCATION)); + } + + @Test + public void shouldNotMutateSharedConfigurationWhenConfiguringAnIsolatedCopy() { + // the fix: generateProgram writes onto the request-local copy resolveComputeGraph hands it, leaving the shared + // graph configuration untouched, so a later request inherits none of this request's reader/input location. + final HadoopGraph shared = HadoopGraph.open(new BaseConfiguration()); + final HadoopIoStep step = new HadoopIoStep(__.start().asAdmin(), "graph.kryo"); + step.setMode(ReadWriting.Mode.READING); + + final Graph local = step.resolveComputeGraph(shared); + step.generateProgram(local, null); + + // the request-local copy carries the request's settings ... - assertEquals(GryoInputFormatName(), local.configuration().getString(Constants.GREMLIN_HADOOP_GRAPH_READER)); ++ assertEquals(gryoInputFormatName(), local.configuration().getString(Constants.GREMLIN_HADOOP_GRAPH_READER)); + assertEquals("graph.kryo", local.configuration().getString(Constants.GREMLIN_HADOOP_INPUT_LOCATION)); + // ... while the shared, long-lived configuration is left unmutated + assertFalse(shared.configuration().containsKey(Constants.GREMLIN_HADOOP_GRAPH_READER)); + assertFalse(shared.configuration().containsKey(Constants.GREMLIN_HADOOP_INPUT_LOCATION)); + } + + @Test + public void shouldApplyApprovedWithKeyToTheRequestLocalCopyOnly() { + // an operator-approved with() key is applied to the request-local copy and must not leak onto the shared graph + final Configuration config = new BaseConfiguration(); + config.setProperty(OlapConfigKeyPolicy.APPROVED_GRAPH_CONFIG_KEYS, "my.graph.option"); + final HadoopGraph shared = HadoopGraph.open(config); + final HadoopIoStep step = new HadoopIoStep(__.start().asAdmin(), "graph.kryo"); + step.setMode(ReadWriting.Mode.READING); + step.configure("my.graph.option", "v"); + + final Graph local = step.resolveComputeGraph(shared); + step.generateProgram(local, null); + + // the value landed on the request-local copy ... + assertEquals("v", local.configuration().getString("my.graph.option")); + // ... but the shared graph never received it + assertFalse(shared.configuration().containsKey("my.graph.option")); + } + + @Test + public void shouldLeaveSharedConfigurationCleanWhenARequestFailsPartway() { + // configureForRead writes the reader/input location to the graph before addParametersToConfiguration rejects an + // unapproved with() key, so the request fails partway with those already written -- onto the request-local copy. + final HadoopGraph shared = HadoopGraph.open(new BaseConfiguration()); + final HadoopIoStep step = new HadoopIoStep(__.start().asAdmin(), "graph.kryo"); + step.setMode(ReadWriting.Mode.READING); + step.configure("unapproved.key", "v"); + final Graph local = step.resolveComputeGraph(shared); + try { + step.generateProgram(local, null); + fail("an unapproved with() key must fail the request"); + } catch (final IllegalArgumentException expected) { + // expected: the request failed after the reader/input location were already written to the local copy + } - assertEquals("the partial write must have landed on the request-local copy", GryoInputFormatName(), ++ assertEquals("the partial write must have landed on the request-local copy", gryoInputFormatName(), + local.configuration().getString(Constants.GREMLIN_HADOOP_GRAPH_READER)); + // ... and none of the failed request's partial mutations reached the shared graph + assertFalse(shared.configuration().containsKey(Constants.GREMLIN_HADOOP_GRAPH_READER)); + assertFalse(shared.configuration().containsKey(Constants.GREMLIN_HADOOP_INPUT_LOCATION)); + } + + @Test + public void shouldFailClosedWhenIsolatingANonHadoopGraph() { + // a security-isolation primitive must fail closed: a non-HadoopGraph must be rejected, never returned unchanged + // (which would silently hand back the shared, long-lived graph and reinstate the cross-request leak). + try { + HadoopIoStep.isolateGraphConfiguration(EmptyGraph.instance()); + fail("request isolation must reject a non-HadoopGraph rather than silently returning the shared graph"); + } catch (final IllegalStateException ise) { + assertTrue(ise.getMessage(), ise.getMessage().contains("HadoopGraph")); + } + } + - private static String GryoInputFormatName() { ++ private static String gryoInputFormatName() { + return org.apache.tinkerpop.gremlin.hadoop.structure.io.gryo.GryoInputFormat.class.getName(); + } }
