From ddaca29d5926ca63609dc2f7dfeba94ed700d25c Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 5 Oct 2023 16:27:47 -0400 Subject: [PATCH] Remove createRestRequest changes in favor of new security rest channel in security plugin Signed-off-by: Craig Perkins --- .../http/netty4/Netty4HttpRequest.java | 62 ++++++++-------- .../http/AbstractHttpServerTransport.java | 74 ++----------------- .../org/opensearch/rest/RestController.java | 2 +- .../java/org/opensearch/rest/RestRequest.java | 61 +-------------- .../AbstractHttpServerTransportTests.java | 16 ---- 5 files changed, 39 insertions(+), 176 deletions(-) diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequest.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequest.java index 51a96b180c2c9..7d937157c1034 100644 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequest.java +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequest.java @@ -32,9 +32,9 @@ package org.opensearch.http.netty4; -import org.opensearch.core.common.bytes.BytesArray; import org.opensearch.core.common.bytes.BytesReference; import org.opensearch.core.rest.RestStatus; +import org.opensearch.http.HttpRequest; import org.opensearch.rest.RestRequest; import org.opensearch.transport.netty4.Netty4Utils; @@ -55,47 +55,52 @@ import io.netty.handler.codec.http.HttpHeaderNames; import io.netty.handler.codec.http.HttpHeaders; import io.netty.handler.codec.http.HttpMethod; -import io.netty.handler.codec.http.HttpRequest; import io.netty.handler.codec.http.cookie.Cookie; import io.netty.handler.codec.http.cookie.ServerCookieDecoder; import io.netty.handler.codec.http.cookie.ServerCookieEncoder; -public class Netty4HttpRequest implements org.opensearch.http.HttpRequest { +public class Netty4HttpRequest implements HttpRequest { - private final HttpRequest request; + private final FullHttpRequest request; private final BytesReference content; private final HttpHeadersMap headers; private final AtomicBoolean released; private final Exception inboundException; private final boolean pooled; - public Netty4HttpRequest(HttpRequest request) { + Netty4HttpRequest(FullHttpRequest request) { this( request, new HttpHeadersMap(request.headers()), new AtomicBoolean(false), true, - (request instanceof FullHttpRequest) ? Netty4Utils.toBytesReference(((FullHttpRequest) request).content()) : BytesArray.EMPTY + Netty4Utils.toBytesReference(request.content()) ); } - Netty4HttpRequest(HttpRequest request, Exception inboundException) { + Netty4HttpRequest(FullHttpRequest request, Exception inboundException) { this( request, new HttpHeadersMap(request.headers()), new AtomicBoolean(false), true, - (request instanceof FullHttpRequest) ? Netty4Utils.toBytesReference(((FullHttpRequest) request).content()) : BytesArray.EMPTY, + Netty4Utils.toBytesReference(request.content()), inboundException ); } - private Netty4HttpRequest(HttpRequest request, HttpHeadersMap headers, AtomicBoolean released, boolean pooled, BytesReference content) { + private Netty4HttpRequest( + FullHttpRequest request, + HttpHeadersMap headers, + AtomicBoolean released, + boolean pooled, + BytesReference content + ) { this(request, headers, released, pooled, content, null); } private Netty4HttpRequest( - HttpRequest request, + FullHttpRequest request, HttpHeadersMap headers, AtomicBoolean released, boolean pooled, @@ -157,32 +162,27 @@ public BytesReference content() { @Override public void release() { - assert request instanceof FullHttpRequest : "release can only be called when underlying request object is of type FullHttpRequest"; if (pooled && released.compareAndSet(false, true)) { - FullHttpRequest req = (FullHttpRequest) request; - req.release(); + request.release(); } } @Override - public org.opensearch.http.HttpRequest releaseAndCopy() { - assert request instanceof FullHttpRequest - : "releaseAndCopy can only be called when underlying request object is of type FullHttpRequest"; + public HttpRequest releaseAndCopy() { assert released.get() == false; if (pooled == false) { return this; } - FullHttpRequest req = (FullHttpRequest) request; try { - final ByteBuf copiedContent = Unpooled.copiedBuffer(req.content()); + final ByteBuf copiedContent = Unpooled.copiedBuffer(request.content()); return new Netty4HttpRequest( new DefaultFullHttpRequest( - req.protocolVersion(), - req.method(), - req.uri(), + request.protocolVersion(), + request.method(), + request.uri(), copiedContent, - req.headers(), - req.trailingHeaders() + request.headers(), + request.trailingHeaders() ), headers, new AtomicBoolean(false), @@ -214,29 +214,27 @@ public List strictCookies() { @Override public HttpVersion protocolVersion() { if (request.protocolVersion().equals(io.netty.handler.codec.http.HttpVersion.HTTP_1_0)) { - return org.opensearch.http.HttpRequest.HttpVersion.HTTP_1_0; + return HttpRequest.HttpVersion.HTTP_1_0; } else if (request.protocolVersion().equals(io.netty.handler.codec.http.HttpVersion.HTTP_1_1)) { - return org.opensearch.http.HttpRequest.HttpVersion.HTTP_1_1; + return HttpRequest.HttpVersion.HTTP_1_1; } else { throw new IllegalArgumentException("Unexpected http protocol version: " + request.protocolVersion()); } } @Override - public org.opensearch.http.HttpRequest removeHeader(String header) { + public HttpRequest removeHeader(String header) { HttpHeaders headersWithoutContentTypeHeader = new DefaultHttpHeaders(); headersWithoutContentTypeHeader.add(request.headers()); headersWithoutContentTypeHeader.remove(header); HttpHeaders trailingHeaders = new DefaultHttpHeaders(); - if (request instanceof FullHttpRequest) { - trailingHeaders.add(((FullHttpRequest) request).trailingHeaders()); - trailingHeaders.remove(header); - } + trailingHeaders.add(request.trailingHeaders()); + trailingHeaders.remove(header); FullHttpRequest requestWithoutHeader = new DefaultFullHttpRequest( request.protocolVersion(), request.method(), request.uri(), - (request instanceof FullHttpRequest) ? ((FullHttpRequest) request).content() : Unpooled.EMPTY_BUFFER, + request.content(), headersWithoutContentTypeHeader, trailingHeaders ); @@ -253,7 +251,7 @@ public Exception getInboundException() { return inboundException; } - public HttpRequest nettyRequest() { + public FullHttpRequest nettyRequest() { return request; } diff --git a/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java b/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java index 398810d1ecccb..b8f8abb6c2c23 100644 --- a/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java +++ b/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java @@ -412,13 +412,13 @@ private void handleIncomingRequest(final HttpRequest httpRequest, final HttpChan { RestRequest innerRestRequest; try { - innerRestRequest = RestRequest.request(xContentRegistry, httpRequest, httpChannel, true); + innerRestRequest = RestRequest.request(xContentRegistry, httpRequest, httpChannel); } catch (final RestRequest.ContentTypeHeaderException e) { badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e); - innerRestRequest = requestWithoutContentTypeHeader(httpRequest, httpChannel, badRequestCause, true); + innerRestRequest = requestWithoutContentTypeHeader(httpRequest, httpChannel, badRequestCause); } catch (final RestRequest.BadParameterException e) { badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e); - innerRestRequest = RestRequest.requestWithoutParameters(xContentRegistry, httpRequest, httpChannel, true); + innerRestRequest = RestRequest.requestWithoutParameters(xContentRegistry, httpRequest, httpChannel); } restRequest = innerRestRequest; } @@ -466,75 +466,13 @@ private void handleIncomingRequest(final HttpRequest httpRequest, final HttpChan dispatchRequest(restRequest, channel, badRequestCause); } - public static RestRequest createRestRequest( - final NamedXContentRegistry xContentRegistry, - final HttpRequest httpRequest, - final HttpChannel httpChannel - ) { - Exception badRequestCause = httpRequest.getInboundException(); - - /* - * We want to create a REST request from the incoming request from Netty. However, creating this request could fail if there - * are incorrectly encoded parameters, or the Content-Type header is invalid. If one of these specific failures occurs, we - * attempt to create a REST request again without the input that caused the exception (e.g., we remove the Content-Type header, - * or skip decoding the parameters). Once we have a request in hand, we then dispatch the request as a bad request with the - * underlying exception that caused us to treat the request as bad. - */ - final RestRequest restRequest; - { - RestRequest innerRestRequest; - try { - innerRestRequest = RestRequest.request(xContentRegistry, httpRequest, httpChannel, false); - } catch (final RestRequest.ContentTypeHeaderException e) { - badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e); - innerRestRequest = requestWithoutContentTypeHeader(xContentRegistry, httpRequest, httpChannel, badRequestCause, false); - } catch (final RestRequest.BadParameterException e) { - badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e); - innerRestRequest = RestRequest.requestWithoutParameters(xContentRegistry, httpRequest, httpChannel, false); - } - restRequest = innerRestRequest; - } - return restRequest; - } - - private static RestRequest requestWithoutContentTypeHeader( - NamedXContentRegistry xContentRegistry, - HttpRequest httpRequest, - HttpChannel httpChannel, - Exception badRequestCause, - boolean shouldGenerateRequestId - ) { + private RestRequest requestWithoutContentTypeHeader(HttpRequest httpRequest, HttpChannel httpChannel, Exception badRequestCause) { HttpRequest httpRequestWithoutContentType = httpRequest.removeHeader("Content-Type"); try { - return RestRequest.request(xContentRegistry, httpRequestWithoutContentType, httpChannel, shouldGenerateRequestId); + return RestRequest.request(xContentRegistry, httpRequestWithoutContentType, httpChannel); } catch (final RestRequest.BadParameterException e) { badRequestCause.addSuppressed(e); - return RestRequest.requestWithoutParameters( - xContentRegistry, - httpRequestWithoutContentType, - httpChannel, - shouldGenerateRequestId - ); - } - } - - private RestRequest requestWithoutContentTypeHeader( - HttpRequest httpRequest, - HttpChannel httpChannel, - Exception badRequestCause, - boolean shouldGenerateRequestId - ) { - HttpRequest httpRequestWithoutContentType = httpRequest.removeHeader("Content-Type"); - try { - return RestRequest.request(xContentRegistry, httpRequestWithoutContentType, httpChannel, shouldGenerateRequestId); - } catch (final RestRequest.BadParameterException e) { - badRequestCause.addSuppressed(e); - return RestRequest.requestWithoutParameters( - xContentRegistry, - httpRequestWithoutContentType, - httpChannel, - shouldGenerateRequestId - ); + return RestRequest.requestWithoutParameters(xContentRegistry, httpRequestWithoutContentType, httpChannel); } } diff --git a/server/src/main/java/org/opensearch/rest/RestController.java b/server/src/main/java/org/opensearch/rest/RestController.java index 92f31076a1489..ac30f999d0da7 100644 --- a/server/src/main/java/org/opensearch/rest/RestController.java +++ b/server/src/main/java/org/opensearch/rest/RestController.java @@ -131,7 +131,7 @@ public RestController( this.headersToCopy = headersToCopy; this.usageService = usageService; if (handlerWrapper == null) { - handlerWrapper = (h) -> h; + handlerWrapper = h -> h; // passthrough if no wrapper set } this.handlerWrapper = handlerWrapper; this.client = client; diff --git a/server/src/main/java/org/opensearch/rest/RestRequest.java b/server/src/main/java/org/opensearch/rest/RestRequest.java index 275341cd960b8..f64774686c89d 100644 --- a/server/src/main/java/org/opensearch/rest/RestRequest.java +++ b/server/src/main/java/org/opensearch/rest/RestRequest.java @@ -76,6 +76,7 @@ public class RestRequest implements ToXContent.Params { // tchar pattern as defined by RFC7230 section 3.2.6 private static final Pattern TCHAR_PATTERN = Pattern.compile("[a-zA-z0-9!#$%&'*+\\-.\\^_`|~]+"); + private static final AtomicLong requestIdGenerator = new AtomicLong(); private final NamedXContentRegistry xContentRegistry; @@ -151,7 +152,7 @@ protected RestRequest(RestRequest restRequest) { * with an unpooled copy. This is supposed to be used before passing requests to {@link RestHandler} instances that can not safely * handle http requests that use pooled buffers as determined by {@link RestHandler#allowsUnsafeBuffers()}. */ - protected void ensureSafeBuffers() { + void ensureSafeBuffers() { httpRequest = httpRequest.releaseAndCopy(); } @@ -179,36 +180,6 @@ public static RestRequest request(NamedXContentRegistry xContentRegistry, HttpRe ); } - /** - * Creates a new REST request. This method will throw {@link BadParameterException} if the path cannot be - * decoded - * - * @param xContentRegistry the content registry - * @param httpRequest the http request - * @param httpChannel the http channel - * @param shouldGenerateRequestId should generate a new request id - * @throws BadParameterException if the parameters can not be decoded - * @throws ContentTypeHeaderException if the Content-Type header can not be parsed - */ - public static RestRequest request( - NamedXContentRegistry xContentRegistry, - HttpRequest httpRequest, - HttpChannel httpChannel, - boolean shouldGenerateRequestId - ) { - Map params = params(httpRequest.uri()); - String path = path(httpRequest.uri()); - return new RestRequest( - xContentRegistry, - params, - path, - httpRequest.getHeaders(), - httpRequest, - httpChannel, - shouldGenerateRequestId ? requestIdGenerator.incrementAndGet() : -1 - ); - } - private static Map params(final String uri) { final Map params = new HashMap<>(); int index = uri.indexOf('?'); @@ -257,34 +228,6 @@ public static RestRequest requestWithoutParameters( ); } - /** - * Creates a new REST request. The path is not decoded so this constructor will not throw a - * {@link BadParameterException}. - * - * @param xContentRegistry the content registry - * @param httpRequest the http request - * @param httpChannel the http channel - * @param shouldGenerateRequestId should generate new request id - * @throws ContentTypeHeaderException if the Content-Type header can not be parsed - */ - public static RestRequest requestWithoutParameters( - NamedXContentRegistry xContentRegistry, - HttpRequest httpRequest, - HttpChannel httpChannel, - boolean shouldGenerateRequestId - ) { - Map params = Collections.emptyMap(); - return new RestRequest( - xContentRegistry, - params, - httpRequest.uri(), - httpRequest.getHeaders(), - httpRequest, - httpChannel, - shouldGenerateRequestId ? requestIdGenerator.incrementAndGet() : -1 - ); - } - /** * The method used. * diff --git a/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java b/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java index 7dcea1c206ac3..c34f13041cb11 100644 --- a/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java +++ b/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java @@ -64,7 +64,6 @@ import java.net.InetSocketAddress; import java.net.UnknownHostException; -import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -150,21 +149,6 @@ public void testHttpPublishPort() throws Exception { } } - public void testCreateRestRequestDoesNotGenerateRequestID() { - FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent( - new BytesArray("bar".getBytes(StandardCharsets.UTF_8)), - null - ).withPath("/foo").withHeaders(Collections.singletonMap("Content-Type", Collections.singletonList("text/plain"))).build(); - - RestRequest request = AbstractHttpServerTransport.createRestRequest( - xContentRegistry(), - fakeRestRequest.getHttpRequest(), - fakeRestRequest.getHttpChannel() - ); - - assertEquals("request should not generate id", -1, request.getRequestId()); - } - public void testDispatchDoesNotModifyThreadContext() { final HttpServerTransport.Dispatcher dispatcher = new HttpServerTransport.Dispatcher() {