From b884469103623f944ca9790ddbdedff092e5590a Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Mon, 15 May 2023 15:04:50 +0200 Subject: [PATCH] FCGI improvements. (#9733) * FCGI improvements. * Better handling for HTTPS parameter on server side. * Better handling of unknown frame types. Signed-off-by: Simone Bordet --- .../java/org/eclipse/jetty/fcgi/FCGI.java | 2 +- .../internal/HttpConnectionOverFCGI.java | 7 +- .../jetty/fcgi/parser/ClientParser.java | 7 +- .../org/eclipse/jetty/fcgi/parser/Parser.java | 111 ++++++++++-------- .../jetty/fcgi/parser/ServerParser.java | 6 +- .../jetty/fcgi/parser/ClientParserTest.java | 72 +++++++++++- .../fcgi/proxy/FastCGIProxyHandlerTest.java | 24 ++++ .../server/internal/HttpStreamOverFCGI.java | 9 +- 8 files changed, 171 insertions(+), 67 deletions(-) diff --git a/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/FCGI.java b/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/FCGI.java index 2b82a9bbe73..70b8185fb48 100644 --- a/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/FCGI.java +++ b/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/FCGI.java @@ -69,7 +69,7 @@ public class FCGI case 8 -> DATA; case 9 -> GET_VALUES; case 10 -> GET_VALUES_RESULT; - default -> throw new IllegalArgumentException(); + default -> throw new IllegalArgumentException("unknown frame type " + code); }; } diff --git a/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/client/transport/internal/HttpConnectionOverFCGI.java b/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/client/transport/internal/HttpConnectionOverFCGI.java index ce829bd0a72..383ee925b7b 100644 --- a/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/client/transport/internal/HttpConnectionOverFCGI.java +++ b/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/client/transport/internal/HttpConnectionOverFCGI.java @@ -412,11 +412,6 @@ public class HttpConnectionOverFCGI extends AbstractConnection implements IConne destroy(); } - protected void close(Throwable failure) - { - HttpConnectionOverFCGI.this.close(failure); - } - @Override public boolean isClosed() { @@ -524,7 +519,7 @@ public class HttpConnectionOverFCGI extends AbstractConnection implements IConne public void onFailure(int request, Throwable failure) { if (LOG.isDebugEnabled()) - LOG.debug("onFailure r={},f={}", request, failure); + LOG.debug("onFailure request={}", request, failure); HttpChannelOverFCGI channel = HttpConnectionOverFCGI.this.channel; if (channel != null) { diff --git a/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/parser/ClientParser.java b/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/parser/ClientParser.java index 96b26b14fcc..7f63bfd2eac 100644 --- a/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/parser/ClientParser.java +++ b/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/parser/ClientParser.java @@ -25,6 +25,7 @@ public class ClientParser extends Parser public ClientParser(Listener listener) { + super(listener); ResponseContentParser stdOutParser = new ResponseContentParser(headerParser, listener); contentParsers.put(FCGI.FrameType.STDOUT, stdOutParser); StreamContentParser stdErrParser = new StreamContentParser(headerParser, FCGI.StreamType.STD_ERR, listener); @@ -35,7 +36,10 @@ public class ClientParser extends Parser @Override protected ContentParser findContentParser(FCGI.FrameType frameType) { - return contentParsers.get(frameType); + ContentParser contentParser = contentParsers.get(frameType); + if (contentParser == null) + throw new IllegalArgumentException("unsupported frame type " + frameType); + return contentParser; } public interface Listener extends Parser.Listener @@ -47,7 +51,6 @@ public class ClientParser extends Parser private record EndRequestListener(Listener listener, StreamContentParser... streamParsers) implements Listener { - @Override public void onBegin(int request, int code, String reason) { diff --git a/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/parser/Parser.java b/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/parser/Parser.java index 25edab7b149..c410c9a9117 100644 --- a/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/parser/Parser.java +++ b/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/parser/Parser.java @@ -59,79 +59,88 @@ public abstract class Parser private static final Logger LOG = LoggerFactory.getLogger(Parser.class); protected final HeaderParser headerParser = new HeaderParser(); + private final Listener listener; private State state = State.HEADER; private int padding; + protected Parser(Listener listener) + { + this.listener = listener; + } + /** * @param buffer the bytes to parse * @return true if the caller should stop parsing, false if the caller should continue parsing */ public boolean parse(ByteBuffer buffer) { - while (true) + try { - switch (state) + while (true) { - case HEADER: + switch (state) { - if (!headerParser.parse(buffer)) - return false; - state = State.CONTENT; - break; - } - case CONTENT: - { - ContentParser contentParser = findContentParser(headerParser.getFrameType()); - if (headerParser.getContentLength() == 0) + case HEADER -> { - padding = headerParser.getPaddingLength(); - state = State.PADDING; - if (contentParser.noContent()) - return true; + if (!headerParser.parse(buffer)) + return false; + state = State.CONTENT; } - else + case CONTENT -> { - ContentParser.Result result = contentParser.parse(buffer); - if (LOG.isDebugEnabled()) - LOG.debug("Parsed request {} content {} result={}", headerParser.getRequest(), headerParser.getFrameType(), result); - - if (result == ContentParser.Result.PENDING) + ContentParser contentParser = findContentParser(headerParser.getFrameType()); + if (headerParser.getContentLength() == 0) { - // Not enough data, signal to read/parse more. + padding = headerParser.getPaddingLength(); + state = State.PADDING; + if (contentParser.noContent()) + return true; + } + else + { + ContentParser.Result result = contentParser.parse(buffer); + if (LOG.isDebugEnabled()) + LOG.debug("Parsed request {} content {} result={}", headerParser.getRequest(), headerParser.getFrameType(), result); + + if (result == ContentParser.Result.PENDING) + { + // Not enough data, signal to read/parse more. + return false; + } + if (result == ContentParser.Result.ASYNC) + { + // The content will be processed asynchronously, signal to stop + // parsing; the async operation will eventually resume parsing. + return true; + } + padding = headerParser.getPaddingLength(); + state = State.PADDING; + } + } + case PADDING -> + { + if (buffer.remaining() >= padding) + { + buffer.position(buffer.position() + padding); + reset(); + } + else + { + padding -= buffer.remaining(); + buffer.position(buffer.limit()); return false; } - if (result == ContentParser.Result.ASYNC) - { - // The content will be processed asynchronously, signal to stop - // parsing; the async operation will eventually resume parsing. - return true; - } - padding = headerParser.getPaddingLength(); - state = State.PADDING; } - break; - } - case PADDING: - { - if (buffer.remaining() >= padding) - { - buffer.position(buffer.position() + padding); - reset(); - break; - } - else - { - padding -= buffer.remaining(); - buffer.position(buffer.limit()); - return false; - } - } - default: - { - throw new IllegalStateException(); + default -> throw new IllegalStateException(); } } } + catch (Throwable x) + { + buffer.position(buffer.limit()); + listener.onFailure(headerParser.getRequest(), x); + return true; + } } protected abstract ContentParser findContentParser(FCGI.FrameType frameType); diff --git a/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/parser/ServerParser.java b/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/parser/ServerParser.java index c83fcb8add2..4abe5236cd1 100644 --- a/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/parser/ServerParser.java +++ b/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/parser/ServerParser.java @@ -23,6 +23,7 @@ public class ServerParser extends Parser public ServerParser(Listener listener) { + super(listener); contentParsers.put(FCGI.FrameType.BEGIN_REQUEST, new BeginRequestContentParser(headerParser, listener)); contentParsers.put(FCGI.FrameType.PARAMS, new ParamsContentParser(headerParser, listener)); contentParsers.put(FCGI.FrameType.STDIN, new StreamContentParser(headerParser, FCGI.StreamType.STD_IN, listener)); @@ -31,7 +32,10 @@ public class ServerParser extends Parser @Override protected ContentParser findContentParser(FCGI.FrameType frameType) { - return contentParsers.get(frameType); + ContentParser contentParser = contentParsers.get(frameType); + if (contentParser == null) + throw new IllegalArgumentException("unsupported frame type " + frameType); + return contentParser; } public interface Listener extends Parser.Listener diff --git a/jetty-core/jetty-fcgi/jetty-fcgi-client/src/test/java/org/eclipse/jetty/fcgi/parser/ClientParserTest.java b/jetty-core/jetty-fcgi/jetty-fcgi-client/src/test/java/org/eclipse/jetty/fcgi/parser/ClientParserTest.java index 4a14c222845..281778a37e1 100644 --- a/jetty-core/jetty-fcgi/jetty-fcgi-client/src/test/java/org/eclipse/jetty/fcgi/parser/ClientParserTest.java +++ b/jetty-core/jetty-fcgi/jetty-fcgi-client/src/test/java/org/eclipse/jetty/fcgi/parser/ClientParserTest.java @@ -15,6 +15,8 @@ package org.eclipse.jetty.fcgi.parser; import java.nio.ByteBuffer; import java.util.Locale; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; @@ -25,6 +27,8 @@ import org.eclipse.jetty.http.HttpFields; import org.eclipse.jetty.io.ArrayByteBufferPool; import org.eclipse.jetty.io.ByteBufferPool; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -102,7 +106,7 @@ public class ClientParserTest } @Test - public void testParseNoResponseContent() throws Exception + public void testParseNoResponseContent() { int id = 13; HttpFields fields = HttpFields.build() @@ -145,7 +149,7 @@ public class ClientParserTest } @Test - public void testParseSmallResponseContent() throws Exception + public void testParseSmallResponseContent() { int id = 13; HttpFields.Mutable fields = HttpFields.build(); @@ -196,7 +200,7 @@ public class ClientParserTest } @Test - public void testParseLargeResponseContent() throws Exception + public void testParseLargeResponseContent() { int id = 13; HttpFields.Mutable fields = HttpFields.build(); @@ -246,4 +250,66 @@ public class ClientParserTest accumulator.release(); } + + @ParameterizedTest + // Frame type 0x01 is BEGIN_REQUEST, cannot be received by clients. + // Frame type 0x7F is unknown to FCGI. + @ValueSource(ints = {0x01, 0x7F}) + public void testClientUnknownFrameType(int frameType) throws Exception + { + CountDownLatch failureLatch = new CountDownLatch(1); + ClientParser parser = new ClientParser(new ClientParser.Listener() + { + @Override + public void onFailure(int request, Throwable failure) + { + failureLatch.countDown(); + } + }); + + // See Parser for the FCGI record structure. + ByteBuffer byteBuffer = ByteBuffer.allocate(8) + .put((byte)1) + .put((byte)frameType) + .putShort((short)13) + .putShort((short)0) + .put((byte)0) + .put((byte)0) + .flip(); + parser.parse(byteBuffer); + assertFalse(byteBuffer.hasRemaining()); + + assertTrue(failureLatch.await(5, TimeUnit.SECONDS)); + } + + @ParameterizedTest + // Frame type 0x06 is STDOUT, cannot be received by servers. + // Frame type 0x7F is unknown to FCGI. + @ValueSource(ints = {0x06, 0x7F}) + public void testServerUnknownFrameType(int frameType) throws Exception + { + CountDownLatch failureLatch = new CountDownLatch(1); + ServerParser parser = new ServerParser(new ServerParser.Listener() + { + @Override + public void onFailure(int request, Throwable failure) + { + failureLatch.countDown(); + } + }); + + // See Parser for the FCGI record structure. + ByteBuffer byteBuffer = ByteBuffer.allocate(8) + .put((byte)1) + .put((byte)frameType) + .putShort((short)13) + .putShort((short)0) + .put((byte)0) + .put((byte)0) + .flip(); + parser.parse(byteBuffer); + assertFalse(byteBuffer.hasRemaining()); + + assertTrue(failureLatch.await(5, TimeUnit.SECONDS)); + } } diff --git a/jetty-core/jetty-fcgi/jetty-fcgi-proxy/src/test/java/org/eclipse/jetty/fcgi/proxy/FastCGIProxyHandlerTest.java b/jetty-core/jetty-fcgi/jetty-fcgi-proxy/src/test/java/org/eclipse/jetty/fcgi/proxy/FastCGIProxyHandlerTest.java index e320f5555c9..c9a3d7d9c89 100644 --- a/jetty-core/jetty-fcgi/jetty-fcgi-proxy/src/test/java/org/eclipse/jetty/fcgi/proxy/FastCGIProxyHandlerTest.java +++ b/jetty-core/jetty-fcgi/jetty-fcgi-proxy/src/test/java/org/eclipse/jetty/fcgi/proxy/FastCGIProxyHandlerTest.java @@ -25,6 +25,7 @@ import org.eclipse.jetty.client.HttpClient; import org.eclipse.jetty.fcgi.FCGI; import org.eclipse.jetty.fcgi.server.ServerFCGIConnectionFactory; import org.eclipse.jetty.http.HttpHeader; +import org.eclipse.jetty.http.HttpScheme; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpURI; import org.eclipse.jetty.server.Connector; @@ -257,4 +258,27 @@ public class FastCGIProxyHandlerTest assertEquals(HttpStatus.OK_200, response.getStatus()); assertArrayEquals(content, response.getContent()); } + + @Test + public void testFCGISecure() throws Exception + { + start(true, new Handler.Abstract() + { + @Override + public boolean handle(Request request, Response response, Callback callback) + { + assertTrue(request.isSecure()); + assertTrue(HttpScheme.HTTPS.is(request.getHttpURI().getScheme())); + callback.succeeded(); + return true; + } + }); + fcgiHandler.setFastCGISecure(true); + + ContentResponse response = client.newRequest("localhost", proxyConnector.getLocalPort()) + .path(proxyContext.getContextPath() + "/index.php") + .send(); + + assertEquals(HttpStatus.OK_200, response.getStatus()); + } } diff --git a/jetty-core/jetty-fcgi/jetty-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/HttpStreamOverFCGI.java b/jetty-core/jetty-fcgi/jetty-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/HttpStreamOverFCGI.java index 34d8558b67a..fb5f299c23a 100644 --- a/jetty-core/jetty-fcgi/jetty-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/HttpStreamOverFCGI.java +++ b/jetty-core/jetty-fcgi/jetty-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/HttpStreamOverFCGI.java @@ -35,6 +35,7 @@ import org.eclipse.jetty.server.HttpStream; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.NanoTime; +import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.URIUtil; import org.eclipse.jetty.util.thread.Invocable; import org.slf4j.Logger; @@ -57,6 +58,7 @@ public class HttpStreamOverFCGI implements HttpStream private String _path; private String _query; private String _version; + private String _secure; private Content.Chunk _chunk; private boolean _committed; private boolean _shutdown; @@ -101,6 +103,8 @@ public class HttpStreamOverFCGI implements HttpStream _query = value; else if (FCGI.Headers.SERVER_PROTOCOL.equalsIgnoreCase(name)) _version = value; + else if (FCGI.Headers.HTTPS.equalsIgnoreCase(name)) + _secure = value; else processField(field); } @@ -108,8 +112,8 @@ public class HttpStreamOverFCGI implements HttpStream public void onHeaders() { String pathQuery = URIUtil.addPathQuery(_path, _query); - // TODO https? - MetaData.Request request = new MetaData.Request(_method, HttpScheme.HTTP.asString(), hostPort, pathQuery, HttpVersion.fromString(_version), _headers, -1); + HttpScheme scheme = StringUtil.isEmpty(_secure) ? HttpScheme.HTTP : HttpScheme.HTTPS; + MetaData.Request request = new MetaData.Request(_method, scheme.asString(), hostPort, pathQuery, HttpVersion.fromString(_version), _headers, -1); Runnable task = _httpChannel.onRequest(request); _allHeaders.forEach(field -> _httpChannel.getRequest().setAttribute(field.getName(), field.getValue())); // TODO: here we just execute the task. @@ -260,7 +264,6 @@ public class HttpStreamOverFCGI implements HttpStream boolean shutdown = _shutdown = info.getHttpFields().contains(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE.asString()); - ByteBufferPool bufferPool = _generator.getByteBufferPool(); ByteBufferPool.Accumulator accumulator = new ByteBufferPool.Accumulator(); Flusher flusher = _connection.getFlusher(); if (head)