From c0ae910d237f9e3c50a75abc01b82b0505c3c588 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Sun, 22 Nov 2020 13:35:45 +0100 Subject: [PATCH] Implement more robust socket hook protocol --- .../repository/hooks/DefaultHookHandler.java | 33 ++++--- .../sonia/scm/repository/hooks/Sockets.java | 60 ++++++++++-- .../resources/sonia/scm/python/scmhooks.py | 26 ++--- .../hooks/DefaultHookHandlerTest.java | 4 +- .../scm/repository/hooks/HookServerTest.java | 4 +- .../scm/repository/hooks/SocketsTest.java | 94 +++++++++++++++++++ 6 files changed, 185 insertions(+), 36 deletions(-) create mode 100644 scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/hooks/SocketsTest.java diff --git a/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/hooks/DefaultHookHandler.java b/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/hooks/DefaultHookHandler.java index fce7d5dfae..c189299d72 100644 --- a/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/hooks/DefaultHookHandler.java +++ b/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/hooks/DefaultHookHandler.java @@ -79,37 +79,47 @@ class DefaultHookHandler implements HookHandler { LOG.warn("failed to read hook request", e); } finally { LOG.trace("close client socket"); + TransactionId.clear(); close(); } } private void handleHookRequest(InputStream input, OutputStream output) throws IOException { - Request request = Sockets.read(input, Request.class); + Request request = Sockets.receive(input, Request.class); + TransactionId.set(request.getTransactionId()); Response response = handleHookRequest(request); Sockets.send(output, response); } private Response handleHookRequest(Request request) { LOG.trace("process {} hook for node {}", request.getType(), request.getNode()); - TransactionId.set(request.getTransactionId()); + if (!environment.isAcceptAble(request.getChallenge())) { + LOG.warn("received hook with invalid challenge: {}", request.getChallenge()); + return error("invalid hook challenge"); + } + + try { + authenticate(request); + + return fireHook(request); + } catch (AuthenticationException ex) { + LOG.warn("hook authentication failed", ex); + return error("hook authentication failed"); + } + } + + @Nonnull + private Response fireHook(Request request) { HgHookContextProvider context = hookContextProviderFactory.create(request.getRepositoryId(), request.getNode()); try { - if (!environment.isAcceptAble(request.getChallenge())) { - LOG.warn("received hook with invalid challenge: {}", request.getChallenge()); - return error("invalid hook challenge"); - } - - authenticate(request); environment.setPending(request.getType() == RepositoryHookType.PRE_RECEIVE); hookEventFacade.handle(request.getRepositoryId()).fireHookEvent(request.getType(), context); return new Response(context.getHgMessageProvider().getMessages(), false); - } catch (AuthenticationException ex) { - LOG.warn("hook authentication failed", ex); - return error("hook authentication failed"); + } catch (NotFoundException ex) { LOG.warn("could not find repository with id {}", request.getRepositoryId(), ex); return error("repository not found"); @@ -121,7 +131,6 @@ class DefaultHookHandler implements HookHandler { return error(context, "unknown error"); } finally { environment.clearPendingState(); - TransactionId.clear(); } } diff --git a/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/hooks/Sockets.java b/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/hooks/Sockets.java index 9791e99c31..76fe3f9f88 100644 --- a/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/hooks/Sockets.java +++ b/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/hooks/Sockets.java @@ -25,14 +25,20 @@ package sonia.scm.repository.hooks; import com.fasterxml.jackson.databind.ObjectMapper; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; -import java.io.ByteArrayOutputStream; +import java.io.EOFException; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; class Sockets { + private static final Logger LOG = LoggerFactory.getLogger(Sockets.class); + + private static final int READ_LIMIT = 8192; + private static final ObjectMapper objectMapper = new ObjectMapper(); private Sockets() { @@ -40,18 +46,54 @@ class Sockets { static void send(OutputStream out, Object object) throws IOException { byte[] bytes = objectMapper.writeValueAsBytes(object); + LOG.trace("send message length of {} to socket", bytes.length); + writeInt(out, bytes.length); + LOG.trace("send message to socket"); out.write(bytes); - out.write('\0'); + LOG.trace("flush socket"); out.flush(); } - static T read(InputStream in, Class type) throws IOException { - ByteArrayOutputStream buffer = new ByteArrayOutputStream(); - int c = in.read(); - while (c != '\0') { - buffer.write(c); - c = in.read(); + static T receive(InputStream in, Class type) throws IOException { + LOG.trace("read {} from socket", type); + int length = readInt(in); + LOG.trace("read message length of {} from socket", length); + if (length > READ_LIMIT) { + String message = String.format("received length of %d, which exceeds the limit of %d", length, READ_LIMIT); + throw new IOException(message); } - return objectMapper.readValue(buffer.toByteArray(), type); + byte[] data = read(in, length); + LOG.trace("convert message to {}", type); + return objectMapper.readValue(data, type); } + + private static void writeInt(OutputStream out, int value) throws IOException { + out.write((value >>> 24) & 0xFF); + out.write((value >>> 16) & 0xFF); + out.write((value >>> 8) & 0xFF); + out.write(value & 0xFF); + } + + private static int readInt(InputStream in) throws IOException { + int b1 = in.read(); + int b2 = in.read(); + int b3 = in.read(); + int b4 = in.read(); + + if ((b1 | b2 | b3 | b4) < 0) { + throw new EOFException("failed to read int from socket"); + } + + return ((b1 << 24) + (b2 << 16) + (b3 << 8) + b4); + } + + private static byte[] read(InputStream in, int length) throws IOException { + byte[] buffer = new byte[length]; + int read = in.read(buffer); + if (read < length) { + throw new EOFException("failed to read bytes from socket"); + } + return buffer; + } + } diff --git a/scm-plugins/scm-hg-plugin/src/main/resources/sonia/scm/python/scmhooks.py b/scm-plugins/scm-hg-plugin/src/main/resources/sonia/scm/python/scmhooks.py index 1fdbc07b15..1e337317e7 100644 --- a/scm-plugins/scm-hg-plugin/src/main/resources/sonia/scm/python/scmhooks.py +++ b/scm-plugins/scm-hg-plugin/src/main/resources/sonia/scm/python/scmhooks.py @@ -29,7 +29,7 @@ # changegroup.scm = python:scmhooks.callback # -import os, sys, json, socket +import os, sys, json, socket, struct # read environment port = os.environ['SCM_HOOK_PORT'] @@ -54,17 +54,19 @@ def fire_hook(ui, repo, hooktype, node): values = {'token': token, 'type': hooktype, 'repositoryId': repositoryId, 'transactionId': transactionId, 'challenge': challenge, 'node': node.decode('utf8') } connection.connect(("127.0.0.1", int(port))) - connection.send(json.dumps(values).encode('utf-8')) - connection.sendall(b'\0') - received = [] - byte = connection.recv(1) - while byte != b'\0': - received.append(byte) - byte = connection.recv(1) + data = json.dumps(values).encode('utf-8') + connection.send(struct.pack('>i', len(data))) + connection.sendall(data) - message = b''.join(received).decode('utf-8') - response = json.loads(message) + d = connection.recv(4, socket.MSG_WAITALL) + length = struct.unpack('>i', bytearray(d))[0] + if length > 8192: + ui.warn( b"scm-hook received message with exceeds the limit of 8192\n" ) + return True + + d = connection.recv(length, socket.MSG_WAITALL) + response = json.loads(d.decode("utf-8")) abort = response['abort'] print_messages(ui, response['messages']) @@ -94,7 +96,7 @@ def pre_hook(ui, repo, hooktype, node=None, source=None, pending=None, **kwargs) # newer mercurial version # we have to make in-memory changes visible to external process - # this does not happen automatically, because mercurial treat our hooks as internal hooks + # this does not happen automatically, because mercurial treat our hooks as internal hook # see hook.py at mercurial sources _exthook try: if repo is not None: @@ -103,7 +105,7 @@ def pre_hook(ui, repo, hooktype, node=None, source=None, pending=None, **kwargs) if tr and not tr.writepending(): ui.warn(b"no pending write transaction found") except AttributeError: - ui.debug(b"mercurial does not support currenttransation") + ui.debug(b"mercurial does not support currenttransaction") # do nothing return callback(ui, repo, "PRE_RECEIVE", node) diff --git a/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/hooks/DefaultHookHandlerTest.java b/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/hooks/DefaultHookHandlerTest.java index 1be67c4c44..eed006b6ce 100644 --- a/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/hooks/DefaultHookHandlerTest.java +++ b/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/hooks/DefaultHookHandlerTest.java @@ -291,14 +291,16 @@ class DefaultHookHandlerTest { private DefaultHookHandler.Response send(DefaultHookHandler.Request request) throws IOException { ByteArrayOutputStream buffer = new ByteArrayOutputStream(); Sockets.send(buffer, request); + ByteArrayInputStream input = new ByteArrayInputStream(buffer.toByteArray()); when(socket.getInputStream()).thenReturn(input); + ByteArrayOutputStream output = new ByteArrayOutputStream(); when(socket.getOutputStream()).thenReturn(output); handler.run(); - return Sockets.read(new ByteArrayInputStream(output.toByteArray()), DefaultHookHandler.Response.class); + return Sockets.receive(new ByteArrayInputStream(output.toByteArray()), DefaultHookHandler.Response.class); } private static class TestingException extends ExceptionWithContext { diff --git a/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/hooks/HookServerTest.java b/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/hooks/HookServerTest.java index 95467edc25..ccffd36b04 100644 --- a/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/hooks/HookServerTest.java +++ b/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/hooks/HookServerTest.java @@ -82,7 +82,7 @@ class HookServerTest { OutputStream output = socket.getOutputStream() ) { Sockets.send(output, request); - return Sockets.read(input, Response.class); + return Sockets.receive(input, Response.class); } catch (IOException ex) { throw new RuntimeException("failed", ex); } @@ -100,7 +100,7 @@ class HookServerTest { @Override public void run() { try (InputStream input = socket.getInputStream(); OutputStream output = socket.getOutputStream()) { - Request request = Sockets.read(input, Request.class); + Request request = Sockets.receive(input, Request.class); Subject subject = SecurityUtils.getSubject(); Sockets.send(output, new Response("Hello " + request.getName(), subject.getPrincipal().toString())); } catch (IOException ex) { diff --git a/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/hooks/SocketsTest.java b/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/hooks/SocketsTest.java new file mode 100644 index 0000000000..f20c2e93db --- /dev/null +++ b/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/hooks/SocketsTest.java @@ -0,0 +1,94 @@ +/* + * MIT License + * + * Copyright (c) 2020-present Cloudogu GmbH and Contributors + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +package sonia.scm.repository.hooks; + +import lombok.AllArgsConstructor; +import lombok.Data; +import org.junit.jupiter.api.Test; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; + +class SocketsTest { + + @Test + void shouldSendAndReceive() throws IOException { + ByteArrayOutputStream output = new ByteArrayOutputStream(); + Sockets.send(output, new TestValue("awesome")); + ByteArrayInputStream input = new ByteArrayInputStream(output.toByteArray()); + TestValue value = Sockets.receive(input, TestValue.class); + assertThat(value.value).isEqualTo("awesome"); + } + + @Test + void shouldFailWithTooFewBytesForLength() { + ByteArrayOutputStream output = new ByteArrayOutputStream(); + output.write((512 >>> 24) & 0xFF); + output.write((512 >>> 16) & 0xFF); + + ByteArrayInputStream input = new ByteArrayInputStream(output.toByteArray()); + IOException ex = assertThrows(IOException.class, () -> Sockets.receive(input, TestValue.class)); + assertThat(ex.getMessage()).contains("int"); + } + + @Test + void shouldFailWithTooFewBytesForData() { + ByteArrayOutputStream output = new ByteArrayOutputStream(); + output.write((16 >>> 24) & 0xFF); + output.write((16 >>> 16) & 0xFF); + output.write((16 >>> 8) & 0xFF); + output.write(16 & 0xFF); + + ByteArrayInputStream input = new ByteArrayInputStream(output.toByteArray()); + IOException ex = assertThrows(IOException.class, () -> Sockets.receive(input, TestValue.class)); + assertThat(ex.getMessage()).contains("bytes"); + } + + @Test + void shouldFailIfLimitIsExceeded() { + ByteArrayOutputStream output = new ByteArrayOutputStream(); + output.write((9216 >>> 24) & 0xFF); + output.write((9216 >>> 16) & 0xFF); + output.write((9216 >>> 8) & 0xFF); + output.write(9216 & 0xFF); + + ByteArrayInputStream input = new ByteArrayInputStream(output.toByteArray()); + IOException ex = assertThrows(IOException.class, () -> Sockets.receive(input, TestValue.class)); + assertThat(ex.getMessage()).contains("9216"); + } + + @Data + @AllArgsConstructor + public static class TestValue { + + private String value; + + } + +}