diff options
author | Bjørn Christian Seime <bjorncs@verizonmedia.com> | 2020-11-19 11:36:59 +0100 |
---|---|---|
committer | Bjørn Christian Seime <bjorncs@verizonmedia.com> | 2020-11-19 11:36:59 +0100 |
commit | e8dae5ac60da5fee6d84ddaf055ee4834a178c0d (patch) | |
tree | ea100c409011063459a2ee4a6c67efa90fcb96b5 | |
parent | 301252bba0c1c17d5e962091191ed4187be038c1 (diff) |
Add strict filtering mode to jdisc
Jdisc will reject requests that does not match a request filter (chain) if strict filtering is enabled.
8 files changed, 115 insertions, 9 deletions
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/http/Http.java b/config-model/src/main/java/com/yahoo/vespa/model/container/http/Http.java index 1ed043857e2..b5c3cac1879 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/http/Http.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/http/Http.java @@ -24,6 +24,7 @@ public class Http extends AbstractConfigProducer<AbstractConfigProducer<?>> impl private final List<FilterBinding> bindings = new CopyOnWriteArrayList<>(); private volatile JettyHttpServer httpServer; private volatile AccessControl accessControl; + private volatile boolean strictFiltering = false; // TODO Vespa 8: Enable strict filtering by default if filtering is enabled public Http(FilterChains chains) { super("http"); @@ -72,6 +73,8 @@ public class Http extends AbstractConfigProducer<AbstractConfigProducer<?>> impl return Optional.ofNullable(accessControl); } + public void setStrictFiltering(boolean enabled) { this.strictFiltering = enabled; } + @Override public void getConfig(ServerConfig.Builder builder) { for (FilterBinding binding : bindings) { @@ -80,6 +83,7 @@ public class Http extends AbstractConfigProducer<AbstractConfigProducer<?>> impl .binding(binding.binding().patternString())); } populateDefaultFiltersConfig(builder, httpServer); + builder.strictFiltering(strictFiltering); } @Override diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/HttpBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/HttpBuilder.java index 9b9ebedda6d..5417a522d6a 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/HttpBuilder.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/HttpBuilder.java @@ -40,11 +40,14 @@ public class HttpBuilder extends VespaDomBuilder.DomConfigProducerBuilder<Http> FilterChains filterChains; List<FilterBinding> bindings = new ArrayList<>(); AccessControl accessControl = null; + Optional<Boolean> strictFiltering = Optional.empty(); Element filteringElem = XML.getChild(spec, "filtering"); if (filteringElem != null) { filterChains = new FilterChainsBuilder().build(deployState, ancestor, filteringElem); bindings = readFilterBindings(filteringElem); + strictFiltering = XmlHelper.getOptionalAttribute(filteringElem, "strict-mode") + .map(Boolean::valueOf); Element accessControlElem = XML.getChild(filteringElem, "access-control"); if (accessControlElem != null) { @@ -55,6 +58,7 @@ public class HttpBuilder extends VespaDomBuilder.DomConfigProducerBuilder<Http> } Http http = new Http(filterChains); + strictFiltering.ifPresent(http::setStrictFiltering); http.getBindings().addAll(bindings); ApplicationContainerCluster cluster = getContainerCluster(ancestor).orElse(null); http.setHttpServer(new JettyHttpServerBuilder(cluster).build(deployState, ancestor, spec)); diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/http/StrictFilteringTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/http/StrictFilteringTest.java new file mode 100644 index 00000000000..98383e77324 --- /dev/null +++ b/config-model/src/test/java/com/yahoo/vespa/model/container/http/StrictFilteringTest.java @@ -0,0 +1,41 @@ +// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.model.container.http; + +import com.yahoo.config.model.builder.xml.test.DomBuilderTest; +import com.yahoo.config.model.deploy.DeployState; +import com.yahoo.jdisc.http.ServerConfig; +import com.yahoo.vespa.model.container.xml.ContainerModelBuilder; +import org.junit.Test; +import org.w3c.dom.Element; + +import static org.junit.Assert.assertTrue; + +/** + * @author bjorncs + */ +public class StrictFilteringTest extends DomBuilderTest { + + @Test + public void default_request_and_response_filters_in_services_xml_are_listen_in_server_config() { + Element xml = parse( + "<container version='1.0'>", + " <http>", + " <filtering strict-mode=\"true\">", + " <request-chain id='request-chain-with-binding'>", + " <filter id='my-filter' class='MyFilter'/>", + " <binding>http://*/my-chain-binding</binding>", + " </request-chain>", + " </filtering>", + " <server id='server1' port='8000' />", + " </http>", + "</container>"); + buildContainerCluster(xml); + ServerConfig config = root.getConfig(ServerConfig.class, "container/http/jdisc-jetty/server1"); + assertTrue(config.strictFiltering()); + } + + private void buildContainerCluster(Element containerElem) { + new ContainerModelBuilder(true, ContainerModelBuilder.Networking.enable).build(DeployState.createTestState(), null, null, root, containerElem); + root.freezeModelTopology(); + } +} diff --git a/jdisc_http_service/abi-spec.json b/jdisc_http_service/abi-spec.json index 8bf7f30964a..8b48631d4aa 100644 --- a/jdisc_http_service/abi-spec.json +++ b/jdisc_http_service/abi-spec.json @@ -742,6 +742,7 @@ "public com.yahoo.jdisc.http.ServerConfig$Builder filter(java.util.List)", "public com.yahoo.jdisc.http.ServerConfig$Builder defaultFilters(com.yahoo.jdisc.http.ServerConfig$DefaultFilters$Builder)", "public com.yahoo.jdisc.http.ServerConfig$Builder defaultFilters(java.util.List)", + "public com.yahoo.jdisc.http.ServerConfig$Builder strictFiltering(boolean)", "public com.yahoo.jdisc.http.ServerConfig$Builder maxWorkerThreads(int)", "public com.yahoo.jdisc.http.ServerConfig$Builder minWorkerThreads(int)", "public com.yahoo.jdisc.http.ServerConfig$Builder stopTimeout(double)", @@ -930,6 +931,7 @@ "public com.yahoo.jdisc.http.ServerConfig$Filter filter(int)", "public java.util.List defaultFilters()", "public com.yahoo.jdisc.http.ServerConfig$DefaultFilters defaultFilters(int)", + "public boolean strictFiltering()", "public int maxWorkerThreads()", "public int minWorkerThreads()", "public double stopTimeout()", diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/FilterResolver.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/FilterResolver.java index b80dd216b04..1e2686aa184 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/FilterResolver.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/FilterResolver.java @@ -2,6 +2,12 @@ package com.yahoo.jdisc.http.server.jetty; import com.yahoo.jdisc.Metric; +import com.yahoo.jdisc.NoopSharedResource; +import com.yahoo.jdisc.Response; +import com.yahoo.jdisc.handler.FastContentWriter; +import com.yahoo.jdisc.handler.ResponseDispatch; +import com.yahoo.jdisc.handler.ResponseHandler; +import com.yahoo.jdisc.http.HttpRequest; import com.yahoo.jdisc.http.filter.RequestFilter; import com.yahoo.jdisc.http.filter.ResponseFilter; import com.yahoo.jdisc.http.servlet.ServletRequest; @@ -22,10 +28,12 @@ class FilterResolver { private final FilterBindings bindings; private final Metric metric; + private final boolean strictFiltering; - FilterResolver(FilterBindings bindings, Metric metric) { + FilterResolver(FilterBindings bindings, Metric metric, boolean strictFiltering) { this.bindings = bindings; this.metric = metric; + this.strictFiltering = strictFiltering; } Optional<RequestFilter> resolveRequestFilter(HttpServletRequest servletRequest, URI jdiscUri) { @@ -33,8 +41,13 @@ class FilterResolver { if (maybeFilterId.isPresent()) { metric.add(MetricDefinitions.FILTERING_REQUEST_HANDLED, 1L, createMetricContext(servletRequest, maybeFilterId.get())); servletRequest.setAttribute(ServletRequest.JDISC_REQUEST_CHAIN, maybeFilterId.get()); - } else { + } else if (!strictFiltering) { metric.add(MetricDefinitions.FILTERING_REQUEST_UNHANDLED, 1L, createMetricContext(servletRequest, null)); + } else { + String syntheticFilterId = RejectingRequestFilter.SYNTHETIC_FILTER_CHAIN_ID; + metric.add(MetricDefinitions.FILTERING_REQUEST_HANDLED, 1L, createMetricContext(servletRequest, syntheticFilterId)); + servletRequest.setAttribute(ServletRequest.JDISC_REQUEST_CHAIN, syntheticFilterId); + return Optional.of(RejectingRequestFilter.INSTANCE); } return maybeFilterId.map(bindings::getRequestFilter); } @@ -56,4 +69,20 @@ class FilterResolver { : Map.of(); return JDiscHttpServlet.getConnector(request).createRequestMetricContext(request, extraDimensions); } + + private static class RejectingRequestFilter extends NoopSharedResource implements RequestFilter { + + private static final RejectingRequestFilter INSTANCE = new RejectingRequestFilter(); + private static final String SYNTHETIC_FILTER_CHAIN_ID = "strict-reject"; + + @Override + public void filter(HttpRequest request, ResponseHandler handler) { + Response response = new Response(Response.Status.FORBIDDEN); + response.headers().add("Content-Type", "text/plain"); + try (FastContentWriter writer = ResponseDispatch.newInstance(response).connectFastWriter(handler)) { + writer.write("Request did not match any request filter chain"); + } + } + } + } diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscContext.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscContext.java index 66471587bd5..b37a7352dc6 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscContext.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscContext.java @@ -20,7 +20,7 @@ public class JDiscContext { Metric metric, ServerConfig serverConfig) { - this.filterResolver = new FilterResolver(filterBindings, metric); + this.filterResolver = new FilterResolver(filterBindings, metric, serverConfig.strictFiltering()); this.container = container; this.janitor = janitor; this.metric = metric; diff --git a/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.jdisc.http.server.def b/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.jdisc.http.server.def index f33dc35ea0b..f75a4aaa441 100644 --- a/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.jdisc.http.server.def +++ b/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.jdisc.http.server.def @@ -33,6 +33,9 @@ defaultFilters[].filterId string # The local port which the default filter should be applied to defaultFilters[].localPort int +# Reject all requests not handled by a request filter (chain) +strictFiltering bool default = false + # Max number of threads in underlying Jetty pool maxWorkerThreads int default = 200 diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/FilterTestCase.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/FilterTestCase.java index fd929b3e037..9c5c4027ae3 100644 --- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/FilterTestCase.java +++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/FilterTestCase.java @@ -35,6 +35,7 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; +import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; import static org.mockito.ArgumentMatchers.any; @@ -495,7 +496,7 @@ public class FilterTestCase { .build(); MetricConsumerMock metricConsumerMock = new MetricConsumerMock(); MyRequestHandler requestHandler = new MyRequestHandler(); - TestDriver testDriver = newDriver(requestHandler, filterBindings, metricConsumerMock); + TestDriver testDriver = newDriver(requestHandler, filterBindings, metricConsumerMock, false); testDriver.client().get("/status.html"); assertThat(requestHandler.awaitInvocation(), is(true)); @@ -510,25 +511,47 @@ public class FilterTestCase { assertThat(testDriver.close(), is(true)); } + @Test + public void requireThatStrictFilteringRejectsRequestsNotMatchingFilterChains() throws IOException { + RequestFilter filter = mock(RequestFilter.class); + FilterBindings filterBindings = new FilterBindings.Builder() + .addRequestFilter("my-request-filter", filter) + .addRequestFilterBinding("my-request-filter", "http://*/filtered/*") + .build(); + MyRequestHandler requestHandler = new MyRequestHandler(); + TestDriver testDriver = newDriver(requestHandler, filterBindings, new MetricConsumerMock(), true); + + testDriver.client().get("/unfiltered/") + .expectStatusCode(is(Response.Status.FORBIDDEN)) + .expectContent(containsString("Request did not match any request filter chain")); + verify(filter, never()).filter(any(), any()); + assertThat(testDriver.close(), is(true)); + } + private static TestDriver newDriver(MyRequestHandler requestHandler, FilterBindings filterBindings) { - return newDriver(requestHandler, filterBindings, new MetricConsumerMock()); + return newDriver(requestHandler, filterBindings, new MetricConsumerMock(), false); } - private static TestDriver newDriver(MyRequestHandler requestHandler, FilterBindings filterBindings, MetricConsumerMock metricConsumer) { + private static TestDriver newDriver( + MyRequestHandler requestHandler, + FilterBindings filterBindings, + MetricConsumerMock metricConsumer, + boolean strictFiltering) { return TestDriver.newInstance( JettyHttpServer.class, requestHandler, - newFilterModule(filterBindings, metricConsumer)); + newFilterModule(filterBindings, metricConsumer, strictFiltering)); } - private static com.google.inject.Module newFilterModule(FilterBindings filterBindings, MetricConsumerMock metricConsumer) { + private static com.google.inject.Module newFilterModule( + FilterBindings filterBindings, MetricConsumerMock metricConsumer, boolean strictFiltering) { return Modules.combine( new AbstractModule() { @Override protected void configure() { bind(FilterBindings.class).toInstance(filterBindings); - bind(ServerConfig.class).toInstance(new ServerConfig(new ServerConfig.Builder())); + bind(ServerConfig.class).toInstance(new ServerConfig(new ServerConfig.Builder().strictFiltering(strictFiltering))); bind(ConnectorConfig.class).toInstance(new ConnectorConfig(new ConnectorConfig.Builder())); bind(ServletPathsConfig.class).toInstance(new ServletPathsConfig(new ServletPathsConfig.Builder())); } |