You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2020/03/10 04:22:40 UTC
[GitHub] [nifi] natural opened a new pull request #4125: NIFI-7153 Adds
ContentLengthFilter and DoSFilter
natural opened a new pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125
#### Adds ContentLengthFilter and DoSFilter
_Enables filtering HTTP PUT and POST requests by maximum size; enables filtering HTTP requests by DoS attacker behavior; implements behaviors described in NIFI-7153._
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [X] Does your PR title start with **NIFI-XXXX** where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [X] Has your PR been rebased against the latest commit within the target branch (typically `master`)?
- [X] Is your initial contribution a single, squashed commit? _Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### For code changes:
- [ X] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [X] Have you written or updated unit tests to verify your changes?
- [ ] Have you verified that the full build is successful on both JDK 8 and JDK 11?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`?
- [ ] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`?
- [ ] If adding new Properties, have you added `.displayName` in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] natural commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
natural commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r396008619
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/request/ContentLengthFilter.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.nifi.web.security.request;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ReadListener;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+/**
+ * This {@link Filter} rejects HTTP requests that exceed a specific, maximum size.
+ */
+public class ContentLengthFilter implements Filter {
+ private static final Logger logger = LoggerFactory.getLogger(ContentLengthFilter.class);
+ public final static String MAX_LENGTH_INIT_PARAM = "maxContentLength";
+ public final static int MAX_LENGTH_DEFAULT = 10_000_000;
+ private int maxContentLength;
+
+ public ContentLengthFilter() {
+ maxContentLength = MAX_LENGTH_DEFAULT;
+ }
+
+ public ContentLengthFilter(int maxLength) {
+ maxContentLength = maxLength;
+ }
+
+ @Override
+ public void init(FilterConfig config) throws ServletException {
+ String maxLength = config.getInitParameter(MAX_LENGTH_INIT_PARAM);
+ int length = maxLength == null ? MAX_LENGTH_DEFAULT : Integer.parseInt(maxLength);
+ if (length < 0) {
+ throw new ServletException("Invalid max request length.");
+ }
+ maxContentLength = length;
+ logger.info("Max content length set: " + maxLength + "b");
+ }
+
+ @Override
+ public void doFilter(ServletRequest request, ServletResponse response, FilterChain next) throws IOException, ServletException {
+ HttpServletRequest httpRequest = (HttpServletRequest) request;
+ String httpMethod = httpRequest.getMethod();
+
+ // Check the HTTP method because the spec says clients don't have to send a content-length header for methods
+ // that don't use it. So even though an attacker may provide a large body in a GET request, the body should go
+ // unread and a size filter is unneeded at best. See RFC 2616 section 14.13, and RFC 1945 section 10.4.
+ boolean willReadInputStream = maxContentLength > 0 && (httpMethod.equalsIgnoreCase("POST") || httpMethod.equalsIgnoreCase("PUT"));
+ if (!willReadInputStream) {
+ logger.info("No length check of request with method {} and maximum {}", httpMethod, maxContentLength);
Review comment:
Changed this and all others to `debug`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] asfgit closed pull request #4125: NIFI-7153 Adds
ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390686173
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/request/ContentLengthFilter.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.nifi.web.security.request;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ReadListener;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+/**
+ * This {@link Filter} rejects HTTP requests that exceed a specific, maximum size.
+ */
+public class ContentLengthFilter implements Filter {
+ private static final Logger logger = LoggerFactory.getLogger(ContentLengthFilter.class);
+ public final static String MAX_LENGTH_INIT_PARAM = "maxContentLength";
+ public final static int MAX_LENGTH_DEFAULT = 10_000_000;
Review comment:
If the default is defined in `nifi.properties` I don't think it should be repeated here. If it needs to be changed in the future, it will likely lead to confusion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390689380
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/request/ContentLengthFilter.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.nifi.web.security.request;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ReadListener;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+/**
+ * This {@link Filter} rejects HTTP requests that exceed a specific, maximum size.
+ */
+public class ContentLengthFilter implements Filter {
+ private static final Logger logger = LoggerFactory.getLogger(ContentLengthFilter.class);
+ public final static String MAX_LENGTH_INIT_PARAM = "maxContentLength";
+ public final static int MAX_LENGTH_DEFAULT = 10_000_000;
+ private int maxContentLength;
+
+ public ContentLengthFilter() {
+ maxContentLength = MAX_LENGTH_DEFAULT;
+ }
+
+ public ContentLengthFilter(int maxLength) {
+ maxContentLength = maxLength;
+ }
+
+ @Override
+ public void init(FilterConfig config) throws ServletException {
+ String maxLength = config.getInitParameter(MAX_LENGTH_INIT_PARAM);
+ int length = maxLength == null ? MAX_LENGTH_DEFAULT : Integer.parseInt(maxLength);
+ if (length < 0) {
+ throw new ServletException("Invalid max request length.");
+ }
+ maxContentLength = length;
+ logger.info("Max content length set: " + maxLength + "b");
+ }
+
+ @Override
+ public void doFilter(ServletRequest request, ServletResponse response, FilterChain next) throws IOException, ServletException {
+ HttpServletRequest httpRequest = (HttpServletRequest) request;
+ String httpMethod = httpRequest.getMethod();
+
+ // Check the HTTP method because the spec says clients don't have to send a content-length header for methods
+ // that don't use it. So even though an attacker may provide a large body in a GET request, the body should go
+ // unread and a size filter is unneeded at best. See RFC 2616 section 14.13, and RFC 1945 section 10.4.
+ boolean willReadInputStream = maxContentLength > 0 && (httpMethod.equalsIgnoreCase("POST") || httpMethod.equalsIgnoreCase("PUT"));
Review comment:
Does this variable actually determine if the full input stream will be read or just if the request length needs to be examined?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on issue #4125: NIFI-7153 Adds
ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on issue #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#issuecomment-603462810
That sounds like the problem with what I was doing before, but even performing the same action as you described, I did not see the log output:
```
🔒 0s @ 12:19:24 $ openssl rand -hex 30240000 > /tmp/hexdata
...rs/alopresto/Workspace/scratch/templates (master) 😉
🔒 0s @ 12:24:53 $ curl 'http://localhost:8080/nifi-api/processors/0dfe6a6d-0171-1000-b327-3350e76db627' \
> -X PUT \
> --data-binary "@/tmp/hexdata" \
> -vvv
* Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8080 (#0)
> PUT /nifi-api/processors/0dfe6a6d-0171-1000-b327-3350e76db627 HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.54.0
> Accept: */*
> Content-Length: 60480001
> Content-Type: application/x-www-form-urlencoded
> Expect: 100-continue
>
< HTTP/1.1 413 Payload Too Large
< Date: Tue, 24 Mar 2020 19:25:12 GMT
< X-Frame-Options: SAMEORIGIN
< Content-Security-Policy: frame-ancestors 'self'
< X-XSS-Protection: 1; mode=block
< Content-Type: text/plain
< Content-Length: 17
< Connection: close
< Server: Jetty(9.4.19.v20190610)
<
* Closing connection 0
Payload Too large
```
I realized the log output of the filter is going to `nifi-user.log`, the web access request log, by default rather than the main application behavior log in `nifi-app.log` due to this configuration in the `conf/logback.xml` file:
```
<logger name="org.apache.nifi.web.security" level="INFO" additivity="false">
<appender-ref ref="USER_FILE"/>
</logger>
```
I think we should explicitly direct the content length warnings to the application log by adding this configuration to the default `logback.xml`:
```
<logger name="org.apache.nifi.web.security.requests" level="DEBUG" additivity="false">
<appender-ref ref="APP_FILE"/>
</logger>
```
I will make those changes and merge this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on issue #4125: NIFI-7153 Adds
ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on issue #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#issuecomment-602976269
@natural if you can show me what I am doing wrong or resolve the issue with the log output, I will make the minor formatting changes I pointed out and merge this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] ottobackwards commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390943550
##########
File path: nifi-docs/src/main/asciidoc/administration-guide.adoc
##########
@@ -3247,6 +3247,9 @@ For example, when running in a Docker container or behind a proxy (e.g. localhos
host[:port] that NiFi is bound to.
|`nifi.web.proxy.context.path`|A comma separated list of allowed HTTP X-ProxyContextPath, X-Forwarded-Context, or X-Forwarded-Prefix header values to consider. By default, this value is
blank meaning all requests containing a proxy context path are rejected. Configuring this property would allow requests where the proxy path is contained in this listing.
+|`nifi.web.max.content.size`|The maximum size for regular PUT and POST requests. The default value is `10 MB`.
Review comment:
No problem, my confusion was around what "path" meant. I usually think of this as 'route' and associate path with like a download location.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto edited a comment on issue #4125: NIFI-7153 Adds
ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto edited a comment on issue #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#issuecomment-603462810
That sounds like the problem with what I was doing before, but even performing the same action as you described, I did not see the log output:
```
🔒 0s @ 12:19:24 $ openssl rand -hex 30240000 > /tmp/hexdata
...rs/alopresto/Workspace/scratch/templates (master) 😉
🔒 0s @ 12:24:53 $ curl 'http://localhost:8080/nifi-api/processors/0dfe6a6d-0171-1000-b327-3350e76db627' \
> -X PUT \
> --data-binary "@/tmp/hexdata" \
> -vvv
* Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8080 (#0)
> PUT /nifi-api/processors/0dfe6a6d-0171-1000-b327-3350e76db627 HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.54.0
> Accept: */*
> Content-Length: 60480001
> Content-Type: application/x-www-form-urlencoded
> Expect: 100-continue
>
< HTTP/1.1 413 Payload Too Large
< Date: Tue, 24 Mar 2020 19:25:12 GMT
< X-Frame-Options: SAMEORIGIN
< Content-Security-Policy: frame-ancestors 'self'
< X-XSS-Protection: 1; mode=block
< Content-Type: text/plain
< Content-Length: 17
< Connection: close
< Server: Jetty(9.4.19.v20190610)
<
* Closing connection 0
Payload Too large
```
I realized the log output of the filter is going to `nifi-user.log`, the web access request log, by default rather than the main application behavior log in `nifi-app.log` due to this configuration in the `conf/logback.xml` file:
```
<logger name="org.apache.nifi.web.security" level="INFO" additivity="false">
<appender-ref ref="USER_FILE"/>
</logger>
```
I think we should explicitly direct the content length warnings to the application log by adding this configuration to the default `logback.xml`:
```
<logger name="org.apache.nifi.web.security.requests" level="INFO" additivity="false">
<appender-ref ref="APP_FILE"/>
</logger>
```
I will make those changes and merge this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390724601
##########
File path: nifi-docs/src/main/asciidoc/administration-guide.adoc
##########
@@ -3247,6 +3247,9 @@ For example, when running in a Docker container or behind a proxy (e.g. localhos
host[:port] that NiFi is bound to.
|`nifi.web.proxy.context.path`|A comma separated list of allowed HTTP X-ProxyContextPath, X-Forwarded-Context, or X-Forwarded-Prefix header values to consider. By default, this value is
blank meaning all requests containing a proxy context path are rejected. Configuring this property would allow requests where the proxy path is contained in this listing.
+|`nifi.web.max.content.size`|The maximum size for regular PUT and POST requests. The default value is `10 MB`.
Review comment:
Sorry, I thought you had reached an understanding of the settings. @natural this is confusing to people, so we should improve the documentation around it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390707896
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/test/java/org/apache/nifi/web/security/request/ContentLengthFilterTest.java
##########
@@ -0,0 +1,183 @@
+/*
+ * 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.nifi.web.security.request;
+
+import org.apache.commons.lang3.StringUtils;
+import org.eclipse.jetty.server.LocalConnector;
+import org.eclipse.jetty.server.Server;
+import org.eclipse.jetty.servlet.FilterHolder;
+import org.eclipse.jetty.servlet.ServletContextHandler;
+
+import org.eclipse.jetty.servlet.ServletHolder;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Test;
+
+import javax.servlet.DispatcherType;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.http.HttpServlet;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import java.io.IOException;
+import java.util.EnumSet;
+import java.util.concurrent.TimeUnit;
+
+
+/**
+ * This test exercises the {@link ContentLengthFilter} class.
+ *
+ * The approach here is to use a {@link LocalConnector} and raw strings for HTTP requests. The additional complexity
+ * of a complete HTTP client isn't required to determine the behavior, and any client would introduce a new dependency.
+ *
+ */
+public class ContentLengthFilterTest {
+ private static final int MAX_CONTENT_LENGTH = 1000;
+ private static final int SERVER_IDLE_TIMEOUT = 2500; // only one request needed + value large enough for slow systems
+ private static final String POST_REQUEST = "POST / HTTP/1.1\r\nContent-Length: %d\r\nHost: h\r\n\r\n%s";
+ private static final String FORM_REQUEST = "POST / HTTP/1.1\r\nContent-Length: %d\r\nHost: h\r\nContent-Type: application/x-www-form-urlencoded\r\n\r\n%s";
+ public static final int FORM_CONTENT_SIZE = 128;
+
+ private Server serverUnderTest;
+ private LocalConnector localConnector;
+ private ServletContextHandler contextUnderTest;
+
+ @After
+ public void stopServer() throws Exception {
+ if (serverUnderTest != null && serverUnderTest.isRunning()) {
+ serverUnderTest.stop();
+ }
+ }
+
+
+ @Test
+ public void testRequestsWithMissingContentLengthHeader() throws Exception {
+ configureAndStartServer(readFullyAndRespondOK, -1);
+
+ // This shows that the ContentLengthFilter allows a request that does not have a content-length header.
+ String response = localConnector.getResponse("POST / HTTP/1.0\r\n\r\n");
+ Assert.assertFalse(StringUtils.containsIgnoreCase(response, "411 Length Required"));
+ }
+
+
+ @Test
+ public void testRequestsWithContentLengthHeader() throws Exception {
+ configureAndStartServer(readFullyAndRespondOK, -1);
+
+ int smallClaim = 150;
+ int largeClaim = 2000;
+
+ String incompletePayload = StringUtils.repeat("1", 10);
+ String largePayload = StringUtils.repeat("1", largeClaim + 200);
+
+ // This shows that the ContentLengthFilter rejects a request when the client claims more than the max + sends more than the max:
+ String response = localConnector.getResponse(String.format(POST_REQUEST, largeClaim, largePayload));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "413 Payload Too Large"));
+
+ // This shows that the ContentLengthFilter rejects a request when the client claims more than the max + sends less the max:
+ response = localConnector.getResponse(String.format(POST_REQUEST, largeClaim, incompletePayload));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "413 Payload Too Large"));
+
+ // This shows that the ContentLengthFilter allows a request when it claims less than the max + sends more than the max:
+ response = localConnector.getResponse(String.format(POST_REQUEST, smallClaim, largePayload));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "200 OK"));
+
+ // This shows that the server times out when the client claims less than the max + sends less than the max + sends less than it claims to send:
+ response = localConnector.getResponse(String.format(POST_REQUEST, smallClaim, incompletePayload), 500, TimeUnit.MILLISECONDS);
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "500 Server Error"));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "Timeout"));
+ }
+
+
+ @Test
+ public void testJettyMaxFormSize() throws Exception {
+ // This shows that the jetty server option for 'maxFormContentSize' is insufficient for our needs because it
+ // catches requests like this:
+ configureAndStartServer(readFormAttempt, FORM_CONTENT_SIZE);
+ String form = "a=" + StringUtils.repeat("1", FORM_CONTENT_SIZE);
+ String response = localConnector.getResponse(String.format(FORM_REQUEST, form.length(), form));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "413 Payload Too Large"));
+
+
+ // But it does not catch requests like this:
+ response = localConnector.getResponse(String.format(POST_REQUEST, form.length(), form+form));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "417 Read Too Many Bytes"));
+ }
+
+
+
+ private HttpServlet readFullyAndRespondOK = new HttpServlet() {
+ @Override
+ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
+ ServletInputStream input = req.getInputStream();
+ while (!input.isFinished()) {
+ input.read();
+ }
+ resp.setStatus(HttpServletResponse.SC_OK);
+ }
+ };
+
+
+ private HttpServlet readFormAttempt = new HttpServlet() {
+ @Override
+ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
+ try {
+ req.getParameterMap();
+ ServletInputStream input = req.getInputStream();
+ int count = 0;
+ while (!input.isFinished()) {
+ input.read();
+ count += 1;
+ }
+ if (count > FORM_CONTENT_SIZE + "a=\n".length()) {
+ resp.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, "Should not reach this code.");
+ } else {
+ resp.sendError(HttpServletResponse.SC_EXPECTATION_FAILED, "Read Too Many Bytes");
+ }
+
+ } catch (final Exception e) {
+ // This is the jetty context returning a 400 from the maxFormContentSize setting:
+ if (StringUtils.containsIgnoreCase(e.getCause().toString(), "Form Too Large")) {
+ resp.sendError(HttpServletResponse.SC_REQUEST_ENTITY_TOO_LARGE, "Payload Too Large");
+ } else {
+ resp.sendError(HttpServletResponse.SC_FORBIDDEN, "Should not reach this code, either.");
+ }
+ }
+ }
+ };
+
+
+ private void configureAndStartServer(HttpServlet servlet, int maxFormContentSize) throws Exception {
Review comment:
This should be moved above the test methods.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] natural commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
natural commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r396008946
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/test/java/org/apache/nifi/web/security/request/ContentLengthFilterTest.java
##########
@@ -0,0 +1,183 @@
+/*
+ * 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.nifi.web.security.request;
+
+import org.apache.commons.lang3.StringUtils;
+import org.eclipse.jetty.server.LocalConnector;
+import org.eclipse.jetty.server.Server;
+import org.eclipse.jetty.servlet.FilterHolder;
+import org.eclipse.jetty.servlet.ServletContextHandler;
+
+import org.eclipse.jetty.servlet.ServletHolder;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Test;
+
+import javax.servlet.DispatcherType;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.http.HttpServlet;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import java.io.IOException;
+import java.util.EnumSet;
+import java.util.concurrent.TimeUnit;
+
+
+/**
+ * This test exercises the {@link ContentLengthFilter} class.
+ *
+ * The approach here is to use a {@link LocalConnector} and raw strings for HTTP requests. The additional complexity
+ * of a complete HTTP client isn't required to determine the behavior, and any client would introduce a new dependency.
+ *
+ */
+public class ContentLengthFilterTest {
+ private static final int MAX_CONTENT_LENGTH = 1000;
+ private static final int SERVER_IDLE_TIMEOUT = 2500; // only one request needed + value large enough for slow systems
+ private static final String POST_REQUEST = "POST / HTTP/1.1\r\nContent-Length: %d\r\nHost: h\r\n\r\n%s";
+ private static final String FORM_REQUEST = "POST / HTTP/1.1\r\nContent-Length: %d\r\nHost: h\r\nContent-Type: application/x-www-form-urlencoded\r\n\r\n%s";
+ public static final int FORM_CONTENT_SIZE = 128;
+
+ private Server serverUnderTest;
+ private LocalConnector localConnector;
+ private ServletContextHandler contextUnderTest;
+
+ @After
+ public void stopServer() throws Exception {
+ if (serverUnderTest != null && serverUnderTest.isRunning()) {
+ serverUnderTest.stop();
+ }
+ }
+
+
+ @Test
+ public void testRequestsWithMissingContentLengthHeader() throws Exception {
+ configureAndStartServer(readFullyAndRespondOK, -1);
+
+ // This shows that the ContentLengthFilter allows a request that does not have a content-length header.
+ String response = localConnector.getResponse("POST / HTTP/1.0\r\n\r\n");
+ Assert.assertFalse(StringUtils.containsIgnoreCase(response, "411 Length Required"));
+ }
+
+
+ @Test
+ public void testRequestsWithContentLengthHeader() throws Exception {
+ configureAndStartServer(readFullyAndRespondOK, -1);
+
+ int smallClaim = 150;
+ int largeClaim = 2000;
+
+ String incompletePayload = StringUtils.repeat("1", 10);
+ String largePayload = StringUtils.repeat("1", largeClaim + 200);
+
+ // This shows that the ContentLengthFilter rejects a request when the client claims more than the max + sends more than the max:
+ String response = localConnector.getResponse(String.format(POST_REQUEST, largeClaim, largePayload));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "413 Payload Too Large"));
+
+ // This shows that the ContentLengthFilter rejects a request when the client claims more than the max + sends less the max:
+ response = localConnector.getResponse(String.format(POST_REQUEST, largeClaim, incompletePayload));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "413 Payload Too Large"));
+
+ // This shows that the ContentLengthFilter allows a request when it claims less than the max + sends more than the max:
+ response = localConnector.getResponse(String.format(POST_REQUEST, smallClaim, largePayload));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "200 OK"));
+
+ // This shows that the server times out when the client claims less than the max + sends less than the max + sends less than it claims to send:
+ response = localConnector.getResponse(String.format(POST_REQUEST, smallClaim, incompletePayload), 500, TimeUnit.MILLISECONDS);
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "500 Server Error"));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "Timeout"));
+ }
+
+
+ @Test
+ public void testJettyMaxFormSize() throws Exception {
+ // This shows that the jetty server option for 'maxFormContentSize' is insufficient for our needs because it
+ // catches requests like this:
+ configureAndStartServer(readFormAttempt, FORM_CONTENT_SIZE);
+ String form = "a=" + StringUtils.repeat("1", FORM_CONTENT_SIZE);
+ String response = localConnector.getResponse(String.format(FORM_REQUEST, form.length(), form));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "413 Payload Too Large"));
+
+
+ // But it does not catch requests like this:
+ response = localConnector.getResponse(String.format(POST_REQUEST, form.length(), form+form));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "417 Read Too Many Bytes"));
+ }
+
+
+
+ private HttpServlet readFullyAndRespondOK = new HttpServlet() {
+ @Override
+ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
+ ServletInputStream input = req.getInputStream();
+ while (!input.isFinished()) {
+ input.read();
+ }
+ resp.setStatus(HttpServletResponse.SC_OK);
+ }
+ };
+
+
+ private HttpServlet readFormAttempt = new HttpServlet() {
Review comment:
Moved.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390701556
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
##########
@@ -590,11 +591,15 @@ private WebAppContext loadWar(final File warFile, final String contextPath, fina
// add HTTP security headers to all responses
final String ALL_PATHS = "/*";
- ArrayList<Class<? extends Filter>> filters = new ArrayList<>(Arrays.asList(XFrameOptionsFilter.class, ContentSecurityPolicyFilter.class, XSSProtectionFilter.class));
+ ArrayList<Class<? extends Filter>> filters = new ArrayList<>(Arrays.asList(
Review comment:
I notice the max form content size is set to 600KB on line 590 above. If this is set, doesn't the `10 MB` / `100 MB` limit not apply because Jetty overrides it anyway?
https://www.eclipse.org/jetty/documentation/current/setting-form-size.html
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r396871013
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/requests/ContentLengthFilter.java
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.nifi.web.security.requests;
+
+import org.apache.nifi.logging.NiFiLog;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ReadListener;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+
+public class ContentLengthFilter implements Filter {
+ private static final Logger logger = new NiFiLog(LoggerFactory.getLogger(ContentLengthFilter.class));
+ public final static String MAX_LENGTH_INIT_PARAM = "maxContentLength";
+ public final static int MAX_LENGTH_DEFAULT = 10_000_000;
+ private int maxContentLength;
+
+ public void init() {
+ maxContentLength = MAX_LENGTH_DEFAULT;
+ logger.debug("Filter initialized without configuration and set max content length: " + maxContentLength + "B");
+ }
+
+
+ @Override
+ public void init(FilterConfig config) throws ServletException {
+ String maxLength = config.getInitParameter(MAX_LENGTH_INIT_PARAM);
+ int length = maxLength == null ? MAX_LENGTH_DEFAULT : Integer.parseInt(maxLength);
+ if (length < 0) {
+ throw new ServletException("Invalid max request length.");
+ }
+ maxContentLength = length;
+ logger.debug("Filter initialized and set max content length: " + maxContentLength + "B");
+ }
+
+ @Override
+ public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException {
+ HttpServletRequest httpRequest = (HttpServletRequest) request;
+ String httpMethod = httpRequest.getMethod();
+
+ // Check the HTTP method because the spec says clients don't have to send a content-length header for methods
+ // that don't use it. So even though an attacker may provide a large body in a GET request, the body should go
+ // unread and a size filter is unneeded at best. See RFC 2616 section 14.13, and RFC 1945 section 10.4.
+ boolean willExamine = maxContentLength > 0 && (httpMethod.equalsIgnoreCase("POST") || httpMethod.equalsIgnoreCase("PUT"));
+ if (!willExamine) {
+ logger.debug("No length check of request with method {} and maximum {}B", httpMethod, maxContentLength);
+ chain.doFilter(request, response);
+ return;
+ }
+
+ HttpServletResponse httpResponse = (HttpServletResponse) response;
+ int contentLength = request.getContentLength();
+ if (contentLength > maxContentLength) {
+ // Request with a client-specified length greater than our max is rejected:
+ httpResponse.setContentType("text/plain");
+ httpResponse.getOutputStream().write("Payload Too large".getBytes());
+ httpResponse.setStatus(HttpServletResponse.SC_REQUEST_ENTITY_TOO_LARGE);
+ logger.warn("Content length check rejected request with content-length {} greater than maximum {}B", contentLength, maxContentLength);
+ } else {
+ // If or when the request is read, this limits the read to our max:
+ logger.debug("Content length check allowed request with content-length {} less than maximum {}B", contentLength, maxContentLength);
Review comment:
Add space before units.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on issue #4125: NIFI-7153 Adds
ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on issue #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#issuecomment-602970413
I am able to verify some behavior but I am not able to see the log output in `nifi-app.log`. I followed the steps below:
_Successful case:_
1. Start NiFi as normal.
1. Run the following curl command (assume a valid 10KB file containing a valid template in XML at the listed location): `curl 'http://localhost:8080/nifi-api/process-groups/09c4c19d-0171-1000-5cd1-d637f8154dea/templates/upload' \
-F "template=@/Users/alopresto/Workspace/scratch/templates/2020-03-23-NIFI-7153-small-template.xml" \
-vvv`
*Result:* The template is successfully uploaded.
```
🔒 0s @ 18:54:02 $ curl 'http://localhost:8080/nifi-api/process-groups/09c4c19d-0171-1000-5cd1-d637f8154dea/templates/upload' -F "template=@/Users/alopresto/Workspace/scratch/templates/2020-03-23-NIFI-7153-small-template.xml" -vvv
* Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8080 (#0)
> POST /nifi-api/process-groups/09c4c19d-0171-1000-5cd1-d637f8154dea/templates/upload HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.54.0
> Accept: */*
> Content-Length: 10641
> Expect: 100-continue
> Content-Type: multipart/form-data; boundary=------------------------20e80121221a51f6
>
< HTTP/1.1 100 Continue
< HTTP/1.1 201 Created
< Date: Tue, 24 Mar 2020 02:08:16 GMT
< X-Frame-Options: SAMEORIGIN
< Content-Security-Policy: frame-ancestors 'self'
< X-XSS-Protection: 1; mode=block
< Location: http://localhost:8080/nifi-api/templates/253f8230-9046-4e9a-8f8b-ee10acefdab0
< Content-Type: application/xml
< Vary: Accept-Encoding
< Vary: User-Agent
< Content-Length: 433
< Server: Jetty(9.4.19.v20190610)
<
* Connection #0 to host localhost left intact
<?xml version="1.0" encoding="UTF-8" standalone="yes"?><templateEntity><template encoding-version="1.3"><description>GFF -> LA</description><groupId>09c4c19d-0171-1000-5cd1-d637f8154dea</groupId><id>253f8230-9046-4e9a-8f8b-ee10acefdab0</id><name>Simple Template</name><timestamp>03/23/2020 19:08:16 PDT</timestamp><uri>http://localhost:8080/nifi-api/templates/253f8230-9046-4e9a-8f8b-ee10acefdab0</uri></template></templateEntity>
```
_Failing case (default max length is 20 MB):_
1. Modify `conf/logback.xml` to include a `logger` entry for `<logger name="org.apache.nifi.web.security.requests" level="DEBUG" />`
1. Run the following curl command (assume a 21 MB file containing a valid template in XML at the listed location): `curl 'http://localhost:8080/nifi-api/process-groups/09c4c19d-0171-1000-5cd1-d637f8154dea/templates/upload' \
-F "template=@/Users/alopresto/Workspace/scratch/templates/2020-03-23-NIFI-7153-small-template-21MB.xml" \
-vvv`
*Result:* The application replies with HTTP 413 Payload Too Large but there is no warning printed in the log
```
🔓 4s @ 19:13:21 $ curl 'http://localhost:8080/nifi-api/process-groups/09c4c19d-0171-1000-5cd1-d637f8154dea/templates/upload' -F "template=@/Users/alopresto/Workspace/scratch/templates/2020-03-23-NIFI-7153-small-template-21MB.xml" -vvv
* Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8080 (#0)
> POST /nifi-api/process-groups/09c4c19d-0171-1000-5cd1-d637f8154dea/templates/upload HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.54.0
> Accept: */*
> Content-Length: 21235140
> Expect: 100-continue
> Content-Type: multipart/form-data; boundary=------------------------6941ef9b1246d1eb
>
* Done waiting for 100-continue
< HTTP/1.1 413 Payload Too Large
< Date: Tue, 24 Mar 2020 02:14:26 GMT
< X-Frame-Options: SAMEORIGIN
< Content-Security-Policy: frame-ancestors 'self'
< X-XSS-Protection: 1; mode=block
< Content-Type: text/plain
< Content-Length: 17
< Connection: close
< Server: Jetty(9.4.19.v20190610)
<
* we are done reading and this is set to close, stop send
* Closing connection 0
Payload Too large
```
@natural can you reproduce this? Am I missing a required step? Even with the log level set to DEBUG, I do not see the `logger.debug()` lines either. I am able to see the code execution through remote debugging, and it appears to execute those lines.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on issue #4125: NIFI-7153 Adds
ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on issue #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#issuecomment-604057252
Before I made any of these changes, I am now noticing a test failure in `ContentLengthFilterTest#testJettyMaxFormSize()`. I believe this is because the response status code is 400 due to an error parsing the parameter map. I am resolving this.
```
[main] INFO org.eclipse.jetty.util.log - Logging initialized @734ms to org.eclipse.jetty.util.log.Slf4jLog
[main] INFO org.eclipse.jetty.server.Server - jetty-9.4.26.v20200117; built: 2020-01-17T12:35:33.676Z; git: 7b38981d25d14afb4a12ff1f2596756144edf695; jvm 11.0.6+10
[main] INFO org.eclipse.jetty.server.handler.ContextHandler - Started o.e.j.s.ServletContextHandler@5e5d171f{/,null,AVAILABLE}
[main] INFO org.eclipse.jetty.server.AbstractConnector - Started LocalConnector@14dd9eb7{HTTP/1.1,[http/1.1]}
[main] INFO org.eclipse.jetty.server.Server - Started @896ms
[main] INFO org.eclipse.jetty.server.AbstractConnector - Stopped LocalConnector@14dd9eb7{HTTP/1.1,[http/1.1]}
[main] INFO org.eclipse.jetty.server.handler.ContextHandler - Stopped o.e.j.s.ServletContextHandler@5e5d171f{/,null,UNAVAILABLE}
java.lang.AssertionError
at org.junit.Assert.fail(Assert.java:87)
at org.junit.Assert.assertTrue(Assert.java:42)
at org.junit.Assert.assertTrue(Assert.java:53)
at org.apache.nifi.web.filter.ContentLengthFilterTest.testJettyMaxFormSize(ContentLengthFilterTest.java:169)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47)
at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r396868018
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
##########
@@ -655,6 +658,46 @@ private void addDocsServlets(WebAppContext docsContext) {
}
}
+ /**
+ * Adds configurable filters to the given context. Currently, this implementation adds `DosFilter` and `ContentLengthFilter` filters.
+ * @param path path spec for filters
+ * @param webappContext context to which filters will be added
+ */
+ private void addFiltersWithProps(String path, WebAppContext webappContext) {
+ int defaultMaxRequestsPerSecond = Integer.parseInt(NiFiProperties.DEFAULT_WEB_MAX_REQUESTS_PER_SECOND);
+ int configuredMaxRequestsPerSecond = 0;
+ try {
+ configuredMaxRequestsPerSecond = Integer.parseInt(props.getMaxWebRequestsPerSecond());
+ } catch (final NumberFormatException e) {
+ logger.warn("Exception parsing property " + NiFiProperties.WEB_MAX_REQUESTS_PER_SECOND + "; using default value: " + defaultMaxRequestsPerSecond);
+ }
+
+ int maxRequestsPerSecond = configuredMaxRequestsPerSecond > 0 ? configuredMaxRequestsPerSecond : defaultMaxRequestsPerSecond;
+ FilterHolder holder = new FilterHolder(DoSFilter.class);
+ holder.setInitParameters(new HashMap<String, String>(){{
+ put("maxRequestsPerSec", String.valueOf(maxRequestsPerSecond));
+ }});
+ holder.setName(DoSFilter.class.getSimpleName());
+ logger.debug("Adding DoSFilter to context at path: " + path + " with max req/sec: " + configuredMaxRequestsPerSecond);
+ webappContext.addFilter(holder, path, EnumSet.allOf(DispatcherType.class));
+
+ int defaultMaxRequestSize = DataUnit.parseDataSize(NiFiProperties.DEFAULT_WEB_MAX_CONTENT_SIZE, DataUnit.B).intValue();
+ int configuredMaxRequestSize = 0;
+ try {
+ configuredMaxRequestSize = DataUnit.parseDataSize(props.getWebMaxContentSize(), DataUnit.B).intValue();
+ } catch (final IllegalArgumentException e) {
+ logger.warn("Exception parsing property " + NiFiProperties.WEB_MAX_CONTENT_SIZE + "; using default value: " + defaultMaxRequestSize);
+ }
+
+ int maxRequestSize = configuredMaxRequestSize > 0 ? configuredMaxRequestSize : defaultMaxRequestSize;
+ holder = new FilterHolder(ContentLengthFilter.class);
+ holder.setInitParameters(new HashMap<String, String>() {{
+ put("maxContentLength", String.valueOf(maxRequestSize));
+ }});
+ holder.setName(FilterHolder.class.getSimpleName());
+ logger.debug("Adding ContentLengthFilter to context at path: " + path + " with max request size: " + maxRequestSize + "B");
Review comment:
Add space between ordinal and unit (e.g. `maxRequestSize + " B"`).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] natural commented on issue #4125: NIFI-7153 Adds
ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
natural commented on issue #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#issuecomment-603351141
> @natural if you can show me what I am doing wrong or resolve the issue with the log output, I will make the minor formatting changes I pointed out and merge this.
I've made the changes to the formatting and reverted the whitespace-only changes. I have an idea regarding what's causing the missing logs, and I'll update here when I've tested it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] natural commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
natural commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r396011326
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
##########
@@ -607,6 +612,25 @@ private WebAppContext loadWar(final File warFile, final String contextPath, fina
return webappContext;
}
+ private void addContentLengthFilter(String contextPath, String pathSpec, WebAppContext webappContext) {
+ final FilterHolder holder = new FilterHolder(ContentLengthFilter.class);
+ final Map<String, String> largePaths = props.getWebMaxContentSizeLargePaths();
+ int size;
+
+ if (!largePaths.containsValue(contextPath)) {
+ size = DataUnit.parseDataSize(props.getWebMaxContentSize(), DataUnit.B).intValue();
Review comment:
I've updated the method that uses the values to handle parsing exceptions and values out of range.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390701556
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
##########
@@ -590,11 +591,15 @@ private WebAppContext loadWar(final File warFile, final String contextPath, fina
// add HTTP security headers to all responses
final String ALL_PATHS = "/*";
- ArrayList<Class<? extends Filter>> filters = new ArrayList<>(Arrays.asList(XFrameOptionsFilter.class, ContentSecurityPolicyFilter.class, XSSProtectionFilter.class));
+ ArrayList<Class<? extends Filter>> filters = new ArrayList<>(Arrays.asList(
Review comment:
I notice the max form content size is set to 600KB on line 590 above. If this is set, doesn't the `10 MB` / `100 MB` limit not apply because Jetty overrides it anyway?
https://www.eclipse.org/jetty/documentation/current/setting-form-size.html
>Jetty limits the amount of data that can post back from a browser or other client to the server. This helps protect the server against denial of service attacks by malicious clients sending huge amounts of data. The default maximum size Jetty permits is 200000 bytes. You can change this default for a particular webapp, for all webapps on a particular Server instance, or all webapps within the same JVM.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] mcgilman commented on issue #4125: NIFI-7153 Adds
ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
mcgilman commented on issue #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#issuecomment-597299639
Thanks for the PR @natural! In addition to @ottobackwards's comment above..., more clarification about the new feature may be helpful. A couple of comments from my review.
It looks like the path property allows the nifi admin to set which paths the should use the larger limits. And these limits are set per context path. Is this too broad? Should the configurable paths be more specific since there are some endpoints that expect potential large payloads? It's probably ok if not initially as I can appreciate the motivation here. The nifi admin can just ensure that the large value is enough to cover those potential larges payloads.
What are your thoughts on configuring the `DoSFilter` in `JettyServer` as well? My only concern would be maintainability of the codebase going forward and whether folks would know to find that configuration in the future in the `webdefault.xml`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] natural commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
natural commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r396008536
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/request/ContentLengthFilter.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.nifi.web.security.request;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ReadListener;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+/**
+ * This {@link Filter} rejects HTTP requests that exceed a specific, maximum size.
+ */
+public class ContentLengthFilter implements Filter {
+ private static final Logger logger = LoggerFactory.getLogger(ContentLengthFilter.class);
+ public final static String MAX_LENGTH_INIT_PARAM = "maxContentLength";
+ public final static int MAX_LENGTH_DEFAULT = 10_000_000;
+ private int maxContentLength;
+
+ public ContentLengthFilter() {
+ maxContentLength = MAX_LENGTH_DEFAULT;
+ }
+
+ public ContentLengthFilter(int maxLength) {
+ maxContentLength = maxLength;
+ }
+
+ @Override
+ public void init(FilterConfig config) throws ServletException {
+ String maxLength = config.getInitParameter(MAX_LENGTH_INIT_PARAM);
+ int length = maxLength == null ? MAX_LENGTH_DEFAULT : Integer.parseInt(maxLength);
+ if (length < 0) {
+ throw new ServletException("Invalid max request length.");
+ }
+ maxContentLength = length;
+ logger.info("Max content length set: " + maxLength + "b");
Review comment:
Good catch; updated to `B` in all cases.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390685093
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
##########
@@ -607,6 +612,25 @@ private WebAppContext loadWar(final File warFile, final String contextPath, fina
return webappContext;
}
+ private void addContentLengthFilter(String contextPath, String pathSpec, WebAppContext webappContext) {
+ final FilterHolder holder = new FilterHolder(ContentLengthFilter.class);
+ final Map<String, String> largePaths = props.getWebMaxContentSizeLargePaths();
+ int size;
+
+ if (!largePaths.containsValue(contextPath)) {
+ size = DataUnit.parseDataSize(props.getWebMaxContentSize(), DataUnit.B).intValue();
Review comment:
What happens if the max content size is not parseable as a data unit? In the unit tests, these properties accepted "size value" and "large size value" without complaint, so I don't believe there's any internal validation occurring.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] natural commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
natural commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r396008597
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/request/ContentLengthFilter.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.nifi.web.security.request;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ReadListener;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+/**
+ * This {@link Filter} rejects HTTP requests that exceed a specific, maximum size.
+ */
+public class ContentLengthFilter implements Filter {
+ private static final Logger logger = LoggerFactory.getLogger(ContentLengthFilter.class);
+ public final static String MAX_LENGTH_INIT_PARAM = "maxContentLength";
+ public final static int MAX_LENGTH_DEFAULT = 10_000_000;
+ private int maxContentLength;
+
+ public ContentLengthFilter() {
+ maxContentLength = MAX_LENGTH_DEFAULT;
+ }
+
+ public ContentLengthFilter(int maxLength) {
+ maxContentLength = maxLength;
+ }
+
+ @Override
+ public void init(FilterConfig config) throws ServletException {
+ String maxLength = config.getInitParameter(MAX_LENGTH_INIT_PARAM);
+ int length = maxLength == null ? MAX_LENGTH_DEFAULT : Integer.parseInt(maxLength);
+ if (length < 0) {
+ throw new ServletException("Invalid max request length.");
+ }
+ maxContentLength = length;
+ logger.info("Max content length set: " + maxLength + "b");
+ }
+
+ @Override
+ public void doFilter(ServletRequest request, ServletResponse response, FilterChain next) throws IOException, ServletException {
+ HttpServletRequest httpRequest = (HttpServletRequest) request;
+ String httpMethod = httpRequest.getMethod();
+
+ // Check the HTTP method because the spec says clients don't have to send a content-length header for methods
+ // that don't use it. So even though an attacker may provide a large body in a GET request, the body should go
+ // unread and a size filter is unneeded at best. See RFC 2616 section 14.13, and RFC 1945 section 10.4.
+ boolean willReadInputStream = maxContentLength > 0 && (httpMethod.equalsIgnoreCase("POST") || httpMethod.equalsIgnoreCase("PUT"));
Review comment:
Renamed var to `willExamine` to better reflect intent.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390685310
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
##########
@@ -607,6 +612,25 @@ private WebAppContext loadWar(final File warFile, final String contextPath, fina
return webappContext;
}
+ private void addContentLengthFilter(String contextPath, String pathSpec, WebAppContext webappContext) {
+ final FilterHolder holder = new FilterHolder(ContentLengthFilter.class);
+ final Map<String, String> largePaths = props.getWebMaxContentSizeLargePaths();
+ int size;
+
+ if (!largePaths.containsValue(contextPath)) {
+ size = DataUnit.parseDataSize(props.getWebMaxContentSize(), DataUnit.B).intValue();
+ } else {
+ size = DataUnit.parseDataSize(props.getWebMaxContentSizeLarge(), DataUnit.B).intValue();
+ }
+ holder.setInitParameters(new HashMap<String, String>() {{
+ put("maxContentLength", String.valueOf(size));
+ }});
+
+ logger.info("Adding Content Length Filter to context at pathSpec: " + contextPath + " with max size: " + size + "b");
+ // NB: the use of pathSpec vs contextPath
Review comment:
What is the difference between `pathSpec` and `contextPath` here? It appears from the calling method that `pathSpec` will always be `ALL_PATHS` -- is it necessary to parameterize?
How does the `contextPath` hierarchy work here when multiple values are present in `largePaths`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] natural commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
natural commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r396009374
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/nf-error-handler.js
##########
@@ -73,6 +73,8 @@
$('#message-title').text('Insufficient Permissions');
} else if (xhr.status === 409) {
$('#message-title').text('Invalid State');
+ } else if (xhr.status === 413) {
Review comment:
The associated unit tests exercise various cases where the claim does not match the value. When the claim is smaller than the transmitted body, the filter allows the request. When larger, the server will throw an exception and return a 500.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390685310
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
##########
@@ -607,6 +612,25 @@ private WebAppContext loadWar(final File warFile, final String contextPath, fina
return webappContext;
}
+ private void addContentLengthFilter(String contextPath, String pathSpec, WebAppContext webappContext) {
+ final FilterHolder holder = new FilterHolder(ContentLengthFilter.class);
+ final Map<String, String> largePaths = props.getWebMaxContentSizeLargePaths();
+ int size;
+
+ if (!largePaths.containsValue(contextPath)) {
+ size = DataUnit.parseDataSize(props.getWebMaxContentSize(), DataUnit.B).intValue();
+ } else {
+ size = DataUnit.parseDataSize(props.getWebMaxContentSizeLarge(), DataUnit.B).intValue();
+ }
+ holder.setInitParameters(new HashMap<String, String>() {{
+ put("maxContentLength", String.valueOf(size));
+ }});
+
+ logger.info("Adding Content Length Filter to context at pathSpec: " + contextPath + " with max size: " + size + "b");
+ // NB: the use of pathSpec vs contextPath
Review comment:
What is the difference between `pathSpec` and `contextPath` here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on issue #4125: NIFI-7153 Adds
ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on issue #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#issuecomment-604158437
I redirected the log messages to `nifi-app.log` as discussed and included the fixed unit test. Merged.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on issue #4125: NIFI-7153 Adds
ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on issue #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#issuecomment-602904032
The build failures seem to be test failures (see https://github.com/apache/nifi/pull/4125/checks?check_run_id=526023453#step:9:3053).
Reviewing the changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on issue #4125: NIFI-7153 Adds
ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on issue #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#issuecomment-604062889
Through tracing the test, I am seeing `HTTP ERROR 417 Read Too Many Bytes` get returned on the first request/response, rather than `413 Payload Too Large`. I think either the assertions are in the wrong order, or the logic to respond in the overridden server is incorrect.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390697808
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/request/ContentLengthFilter.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.nifi.web.security.request;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ReadListener;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+/**
+ * This {@link Filter} rejects HTTP requests that exceed a specific, maximum size.
+ */
+public class ContentLengthFilter implements Filter {
+ private static final Logger logger = LoggerFactory.getLogger(ContentLengthFilter.class);
+ public final static String MAX_LENGTH_INIT_PARAM = "maxContentLength";
+ public final static int MAX_LENGTH_DEFAULT = 10_000_000;
+ private int maxContentLength;
+
+ public ContentLengthFilter() {
+ maxContentLength = MAX_LENGTH_DEFAULT;
+ }
+
+ public ContentLengthFilter(int maxLength) {
+ maxContentLength = maxLength;
+ }
+
+ @Override
+ public void init(FilterConfig config) throws ServletException {
+ String maxLength = config.getInitParameter(MAX_LENGTH_INIT_PARAM);
+ int length = maxLength == null ? MAX_LENGTH_DEFAULT : Integer.parseInt(maxLength);
+ if (length < 0) {
+ throw new ServletException("Invalid max request length.");
+ }
+ maxContentLength = length;
+ logger.info("Max content length set: " + maxLength + "b");
+ }
+
+ @Override
+ public void doFilter(ServletRequest request, ServletResponse response, FilterChain next) throws IOException, ServletException {
+ HttpServletRequest httpRequest = (HttpServletRequest) request;
+ String httpMethod = httpRequest.getMethod();
+
+ // Check the HTTP method because the spec says clients don't have to send a content-length header for methods
+ // that don't use it. So even though an attacker may provide a large body in a GET request, the body should go
+ // unread and a size filter is unneeded at best. See RFC 2616 section 14.13, and RFC 1945 section 10.4.
+ boolean willReadInputStream = maxContentLength > 0 && (httpMethod.equalsIgnoreCase("POST") || httpMethod.equalsIgnoreCase("PUT"));
+ if (!willReadInputStream) {
+ logger.info("No length check of request with method {} and maximum {}", httpMethod, maxContentLength);
+ next.doFilter(request, response);
+ return;
+ }
+
+ HttpServletResponse httpResponse = (HttpServletResponse) response;
+ int contentLength = request.getContentLength();
+ if (contentLength > maxContentLength) {
+ // Request with a client-specified length greater than our max is rejected:
+ logger.info("Content length check rejected request with content-length {} greater than maximum {}", contentLength, maxContentLength);
Review comment:
Should probably add units to these log statements for clarity.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r396870926
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/requests/ContentLengthFilter.java
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.nifi.web.security.requests;
+
+import org.apache.nifi.logging.NiFiLog;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ReadListener;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+
+public class ContentLengthFilter implements Filter {
+ private static final Logger logger = new NiFiLog(LoggerFactory.getLogger(ContentLengthFilter.class));
+ public final static String MAX_LENGTH_INIT_PARAM = "maxContentLength";
+ public final static int MAX_LENGTH_DEFAULT = 10_000_000;
+ private int maxContentLength;
+
+ public void init() {
+ maxContentLength = MAX_LENGTH_DEFAULT;
+ logger.debug("Filter initialized without configuration and set max content length: " + maxContentLength + "B");
+ }
+
+
+ @Override
+ public void init(FilterConfig config) throws ServletException {
+ String maxLength = config.getInitParameter(MAX_LENGTH_INIT_PARAM);
+ int length = maxLength == null ? MAX_LENGTH_DEFAULT : Integer.parseInt(maxLength);
+ if (length < 0) {
+ throw new ServletException("Invalid max request length.");
+ }
+ maxContentLength = length;
+ logger.debug("Filter initialized and set max content length: " + maxContentLength + "B");
+ }
+
+ @Override
+ public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException {
+ HttpServletRequest httpRequest = (HttpServletRequest) request;
+ String httpMethod = httpRequest.getMethod();
+
+ // Check the HTTP method because the spec says clients don't have to send a content-length header for methods
+ // that don't use it. So even though an attacker may provide a large body in a GET request, the body should go
+ // unread and a size filter is unneeded at best. See RFC 2616 section 14.13, and RFC 1945 section 10.4.
+ boolean willExamine = maxContentLength > 0 && (httpMethod.equalsIgnoreCase("POST") || httpMethod.equalsIgnoreCase("PUT"));
+ if (!willExamine) {
+ logger.debug("No length check of request with method {} and maximum {}B", httpMethod, maxContentLength);
Review comment:
Add space before units.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390708884
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
##########
@@ -590,11 +591,15 @@ private WebAppContext loadWar(final File warFile, final String contextPath, fina
// add HTTP security headers to all responses
final String ALL_PATHS = "/*";
- ArrayList<Class<? extends Filter>> filters = new ArrayList<>(Arrays.asList(XFrameOptionsFilter.class, ContentSecurityPolicyFilter.class, XSSProtectionFilter.class));
+ ArrayList<Class<? extends Filter>> filters = new ArrayList<>(Arrays.asList(
Review comment:
Not a complete solution; see comments on unit test below.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] natural commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
natural commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r396008965
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/test/java/org/apache/nifi/web/security/request/ContentLengthFilterTest.java
##########
@@ -0,0 +1,183 @@
+/*
+ * 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.nifi.web.security.request;
+
+import org.apache.commons.lang3.StringUtils;
+import org.eclipse.jetty.server.LocalConnector;
+import org.eclipse.jetty.server.Server;
+import org.eclipse.jetty.servlet.FilterHolder;
+import org.eclipse.jetty.servlet.ServletContextHandler;
+
+import org.eclipse.jetty.servlet.ServletHolder;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Test;
+
+import javax.servlet.DispatcherType;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.http.HttpServlet;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import java.io.IOException;
+import java.util.EnumSet;
+import java.util.concurrent.TimeUnit;
+
+
+/**
+ * This test exercises the {@link ContentLengthFilter} class.
+ *
+ * The approach here is to use a {@link LocalConnector} and raw strings for HTTP requests. The additional complexity
+ * of a complete HTTP client isn't required to determine the behavior, and any client would introduce a new dependency.
+ *
+ */
+public class ContentLengthFilterTest {
+ private static final int MAX_CONTENT_LENGTH = 1000;
+ private static final int SERVER_IDLE_TIMEOUT = 2500; // only one request needed + value large enough for slow systems
+ private static final String POST_REQUEST = "POST / HTTP/1.1\r\nContent-Length: %d\r\nHost: h\r\n\r\n%s";
+ private static final String FORM_REQUEST = "POST / HTTP/1.1\r\nContent-Length: %d\r\nHost: h\r\nContent-Type: application/x-www-form-urlencoded\r\n\r\n%s";
+ public static final int FORM_CONTENT_SIZE = 128;
+
+ private Server serverUnderTest;
+ private LocalConnector localConnector;
+ private ServletContextHandler contextUnderTest;
+
+ @After
+ public void stopServer() throws Exception {
+ if (serverUnderTest != null && serverUnderTest.isRunning()) {
+ serverUnderTest.stop();
+ }
+ }
+
+
+ @Test
+ public void testRequestsWithMissingContentLengthHeader() throws Exception {
+ configureAndStartServer(readFullyAndRespondOK, -1);
+
+ // This shows that the ContentLengthFilter allows a request that does not have a content-length header.
+ String response = localConnector.getResponse("POST / HTTP/1.0\r\n\r\n");
+ Assert.assertFalse(StringUtils.containsIgnoreCase(response, "411 Length Required"));
+ }
+
+
+ @Test
+ public void testRequestsWithContentLengthHeader() throws Exception {
+ configureAndStartServer(readFullyAndRespondOK, -1);
+
+ int smallClaim = 150;
+ int largeClaim = 2000;
+
+ String incompletePayload = StringUtils.repeat("1", 10);
+ String largePayload = StringUtils.repeat("1", largeClaim + 200);
+
+ // This shows that the ContentLengthFilter rejects a request when the client claims more than the max + sends more than the max:
+ String response = localConnector.getResponse(String.format(POST_REQUEST, largeClaim, largePayload));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "413 Payload Too Large"));
+
+ // This shows that the ContentLengthFilter rejects a request when the client claims more than the max + sends less the max:
+ response = localConnector.getResponse(String.format(POST_REQUEST, largeClaim, incompletePayload));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "413 Payload Too Large"));
+
+ // This shows that the ContentLengthFilter allows a request when it claims less than the max + sends more than the max:
+ response = localConnector.getResponse(String.format(POST_REQUEST, smallClaim, largePayload));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "200 OK"));
+
+ // This shows that the server times out when the client claims less than the max + sends less than the max + sends less than it claims to send:
+ response = localConnector.getResponse(String.format(POST_REQUEST, smallClaim, incompletePayload), 500, TimeUnit.MILLISECONDS);
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "500 Server Error"));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "Timeout"));
+ }
+
+
+ @Test
+ public void testJettyMaxFormSize() throws Exception {
+ // This shows that the jetty server option for 'maxFormContentSize' is insufficient for our needs because it
+ // catches requests like this:
+ configureAndStartServer(readFormAttempt, FORM_CONTENT_SIZE);
+ String form = "a=" + StringUtils.repeat("1", FORM_CONTENT_SIZE);
+ String response = localConnector.getResponse(String.format(FORM_REQUEST, form.length(), form));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "413 Payload Too Large"));
+
+
+ // But it does not catch requests like this:
+ response = localConnector.getResponse(String.format(POST_REQUEST, form.length(), form+form));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "417 Read Too Many Bytes"));
+ }
+
+
+
+ private HttpServlet readFullyAndRespondOK = new HttpServlet() {
+ @Override
+ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
+ ServletInputStream input = req.getInputStream();
+ while (!input.isFinished()) {
+ input.read();
+ }
+ resp.setStatus(HttpServletResponse.SC_OK);
+ }
+ };
+
+
+ private HttpServlet readFormAttempt = new HttpServlet() {
+ @Override
+ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
+ try {
+ req.getParameterMap();
+ ServletInputStream input = req.getInputStream();
+ int count = 0;
+ while (!input.isFinished()) {
+ input.read();
+ count += 1;
+ }
+ if (count > FORM_CONTENT_SIZE + "a=\n".length()) {
+ resp.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, "Should not reach this code.");
+ } else {
+ resp.sendError(HttpServletResponse.SC_EXPECTATION_FAILED, "Read Too Many Bytes");
+ }
+
+ } catch (final Exception e) {
+ // This is the jetty context returning a 400 from the maxFormContentSize setting:
+ if (StringUtils.containsIgnoreCase(e.getCause().toString(), "Form Too Large")) {
+ resp.sendError(HttpServletResponse.SC_REQUEST_ENTITY_TOO_LARGE, "Payload Too Large");
+ } else {
+ resp.sendError(HttpServletResponse.SC_FORBIDDEN, "Should not reach this code, either.");
+ }
+ }
+ }
+ };
+
+
+ private void configureAndStartServer(HttpServlet servlet, int maxFormContentSize) throws Exception {
Review comment:
So moved.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] natural commented on issue #4125: NIFI-7153 Adds
ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
natural commented on issue #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#issuecomment-604075071
> > Content-Type: application/x-www-form-urlencoded
The commands are similar but not the same, and that's the key difference. Could you please try without sending the header? The `data*` parameters to curl set the header for you, so you can add `-H "Content-Type: application/octet-stream"` or similar to change that (per `man curl`).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on issue #4125: NIFI-7153 Adds
ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on issue #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#issuecomment-604147708
I was able to resolve the failing test by changing the string error message it was looking for in the stubbed server control block.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] natural commented on issue #4125: NIFI-7153 Adds
ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
natural commented on issue #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#issuecomment-602071371
> Thanks for the PR @natural! In addition to @ottobackwards's comment above..., more clarification about the new feature may be helpful. A couple of comments from my review.
>
> It looks like the path property allows the nifi admin to set which paths the should use the larger limits. And these limits are set per context path. Is this too broad? Should the configurable paths be more specific since there are some endpoints that expect potential large payloads? It's probably ok if not initially as I can appreciate the motivation here. The nifi admin can just ensure that the large value is enough to cover those potential larges payloads.
>
> What are your thoughts on configuring the `DoSFilter` in `JettyServer` as well? My only concern would be maintainability of the codebase going forward and whether folks would know to find that configuration in the future in the `webdefault.xml`.
thanks for the thoughtful feedback @mcgilman
i had the two filters in different spots because that was the first way i could get them to function properly. based on your feedback, i've put them both in the jetty server class and removed the xml parts.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r396870872
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/requests/ContentLengthFilter.java
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.nifi.web.security.requests;
+
+import org.apache.nifi.logging.NiFiLog;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ReadListener;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+
+public class ContentLengthFilter implements Filter {
+ private static final Logger logger = new NiFiLog(LoggerFactory.getLogger(ContentLengthFilter.class));
+ public final static String MAX_LENGTH_INIT_PARAM = "maxContentLength";
+ public final static int MAX_LENGTH_DEFAULT = 10_000_000;
+ private int maxContentLength;
+
+ public void init() {
+ maxContentLength = MAX_LENGTH_DEFAULT;
+ logger.debug("Filter initialized without configuration and set max content length: " + maxContentLength + "B");
+ }
+
+
+ @Override
+ public void init(FilterConfig config) throws ServletException {
+ String maxLength = config.getInitParameter(MAX_LENGTH_INIT_PARAM);
+ int length = maxLength == null ? MAX_LENGTH_DEFAULT : Integer.parseInt(maxLength);
+ if (length < 0) {
+ throw new ServletException("Invalid max request length.");
+ }
+ maxContentLength = length;
+ logger.debug("Filter initialized and set max content length: " + maxContentLength + "B");
Review comment:
Add space before units.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] ottobackwards commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390612015
##########
File path: nifi-docs/src/main/asciidoc/administration-guide.adoc
##########
@@ -3247,6 +3247,9 @@ For example, when running in a Docker container or behind a proxy (e.g. localhos
host[:port] that NiFi is bound to.
|`nifi.web.proxy.context.path`|A comma separated list of allowed HTTP X-ProxyContextPath, X-Forwarded-Context, or X-Forwarded-Prefix header values to consider. By default, this value is
blank meaning all requests containing a proxy context path are rejected. Configuring this property would allow requests where the proxy path is contained in this listing.
+|`nifi.web.max.content.size`|The maximum size for regular PUT and POST requests. The default value is `10 MB`.
Review comment:
Ok, looking again I understand what path is, sorry.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on issue #4125: NIFI-7153 Adds
ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on issue #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#issuecomment-597366292
Reviewing...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] natural commented on issue #4125: NIFI-7153 Adds
ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
natural commented on issue #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#issuecomment-602715388
I think the build failures are caused by caching, force pushing, or both.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on issue #4125: NIFI-7153 Adds
ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on issue #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#issuecomment-604139581
My most recent comments are with a Java unit test, not running an external `curl` command.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] natural commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
natural commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r396008646
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/request/ContentLengthFilter.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.nifi.web.security.request;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ReadListener;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+/**
+ * This {@link Filter} rejects HTTP requests that exceed a specific, maximum size.
+ */
+public class ContentLengthFilter implements Filter {
+ private static final Logger logger = LoggerFactory.getLogger(ContentLengthFilter.class);
+ public final static String MAX_LENGTH_INIT_PARAM = "maxContentLength";
+ public final static int MAX_LENGTH_DEFAULT = 10_000_000;
+ private int maxContentLength;
+
+ public ContentLengthFilter() {
+ maxContentLength = MAX_LENGTH_DEFAULT;
+ }
+
+ public ContentLengthFilter(int maxLength) {
+ maxContentLength = maxLength;
+ }
+
+ @Override
+ public void init(FilterConfig config) throws ServletException {
+ String maxLength = config.getInitParameter(MAX_LENGTH_INIT_PARAM);
+ int length = maxLength == null ? MAX_LENGTH_DEFAULT : Integer.parseInt(maxLength);
+ if (length < 0) {
+ throw new ServletException("Invalid max request length.");
+ }
+ maxContentLength = length;
+ logger.info("Max content length set: " + maxLength + "b");
+ }
+
+ @Override
+ public void doFilter(ServletRequest request, ServletResponse response, FilterChain next) throws IOException, ServletException {
+ HttpServletRequest httpRequest = (HttpServletRequest) request;
+ String httpMethod = httpRequest.getMethod();
+
+ // Check the HTTP method because the spec says clients don't have to send a content-length header for methods
+ // that don't use it. So even though an attacker may provide a large body in a GET request, the body should go
+ // unread and a size filter is unneeded at best. See RFC 2616 section 14.13, and RFC 1945 section 10.4.
+ boolean willReadInputStream = maxContentLength > 0 && (httpMethod.equalsIgnoreCase("POST") || httpMethod.equalsIgnoreCase("PUT"));
+ if (!willReadInputStream) {
+ logger.info("No length check of request with method {} and maximum {}", httpMethod, maxContentLength);
+ next.doFilter(request, response);
+ return;
+ }
+
+ HttpServletResponse httpResponse = (HttpServletResponse) response;
+ int contentLength = request.getContentLength();
+ if (contentLength > maxContentLength) {
+ // Request with a client-specified length greater than our max is rejected:
+ logger.info("Content length check rejected request with content-length {} greater than maximum {}", contentLength, maxContentLength);
Review comment:
Changed to `warn`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390709410
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/nf-error-handler.js
##########
@@ -73,6 +73,8 @@
$('#message-title').text('Insufficient Permissions');
} else if (xhr.status === 409) {
$('#message-title').text('Invalid State');
+ } else if (xhr.status === 413) {
Review comment:
Can response 417 actually be returned by `LimitedContentLengthRequest` in the NiFi server if the content-length header value is different than the actual content length?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] natural commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
natural commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r396008514
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/request/ContentLengthFilter.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.nifi.web.security.request;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ReadListener;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+/**
+ * This {@link Filter} rejects HTTP requests that exceed a specific, maximum size.
+ */
+public class ContentLengthFilter implements Filter {
+ private static final Logger logger = LoggerFactory.getLogger(ContentLengthFilter.class);
+ public final static String MAX_LENGTH_INIT_PARAM = "maxContentLength";
+ public final static int MAX_LENGTH_DEFAULT = 10_000_000;
Review comment:
Not sure how to remove the duplication without introducing a module dependancy.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390698146
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/request/ContentLengthFilter.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.nifi.web.security.request;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ReadListener;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+/**
+ * This {@link Filter} rejects HTTP requests that exceed a specific, maximum size.
+ */
+public class ContentLengthFilter implements Filter {
+ private static final Logger logger = LoggerFactory.getLogger(ContentLengthFilter.class);
+ public final static String MAX_LENGTH_INIT_PARAM = "maxContentLength";
+ public final static int MAX_LENGTH_DEFAULT = 10_000_000;
+ private int maxContentLength;
+
+ public ContentLengthFilter() {
+ maxContentLength = MAX_LENGTH_DEFAULT;
+ }
+
+ public ContentLengthFilter(int maxLength) {
+ maxContentLength = maxLength;
+ }
+
+ @Override
+ public void init(FilterConfig config) throws ServletException {
+ String maxLength = config.getInitParameter(MAX_LENGTH_INIT_PARAM);
+ int length = maxLength == null ? MAX_LENGTH_DEFAULT : Integer.parseInt(maxLength);
+ if (length < 0) {
+ throw new ServletException("Invalid max request length.");
+ }
+ maxContentLength = length;
+ logger.info("Max content length set: " + maxLength + "b");
+ }
+
+ @Override
+ public void doFilter(ServletRequest request, ServletResponse response, FilterChain next) throws IOException, ServletException {
+ HttpServletRequest httpRequest = (HttpServletRequest) request;
+ String httpMethod = httpRequest.getMethod();
+
+ // Check the HTTP method because the spec says clients don't have to send a content-length header for methods
+ // that don't use it. So even though an attacker may provide a large body in a GET request, the body should go
+ // unread and a size filter is unneeded at best. See RFC 2616 section 14.13, and RFC 1945 section 10.4.
+ boolean willReadInputStream = maxContentLength > 0 && (httpMethod.equalsIgnoreCase("POST") || httpMethod.equalsIgnoreCase("PUT"));
+ if (!willReadInputStream) {
+ logger.info("No length check of request with method {} and maximum {}", httpMethod, maxContentLength);
+ next.doFilter(request, response);
+ return;
+ }
+
+ HttpServletResponse httpResponse = (HttpServletResponse) response;
+ int contentLength = request.getContentLength();
+ if (contentLength > maxContentLength) {
+ // Request with a client-specified length greater than our max is rejected:
+ logger.info("Content length check rejected request with content-length {} greater than maximum {}", contentLength, maxContentLength);
Review comment:
And this should probably be `warn`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390697644
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/request/ContentLengthFilter.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.nifi.web.security.request;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ReadListener;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+/**
+ * This {@link Filter} rejects HTTP requests that exceed a specific, maximum size.
+ */
+public class ContentLengthFilter implements Filter {
+ private static final Logger logger = LoggerFactory.getLogger(ContentLengthFilter.class);
+ public final static String MAX_LENGTH_INIT_PARAM = "maxContentLength";
+ public final static int MAX_LENGTH_DEFAULT = 10_000_000;
+ private int maxContentLength;
+
+ public ContentLengthFilter() {
+ maxContentLength = MAX_LENGTH_DEFAULT;
+ }
+
+ public ContentLengthFilter(int maxLength) {
+ maxContentLength = maxLength;
+ }
+
+ @Override
+ public void init(FilterConfig config) throws ServletException {
+ String maxLength = config.getInitParameter(MAX_LENGTH_INIT_PARAM);
+ int length = maxLength == null ? MAX_LENGTH_DEFAULT : Integer.parseInt(maxLength);
+ if (length < 0) {
+ throw new ServletException("Invalid max request length.");
+ }
+ maxContentLength = length;
+ logger.info("Max content length set: " + maxLength + "b");
+ }
+
+ @Override
+ public void doFilter(ServletRequest request, ServletResponse response, FilterChain next) throws IOException, ServletException {
+ HttpServletRequest httpRequest = (HttpServletRequest) request;
+ String httpMethod = httpRequest.getMethod();
+
+ // Check the HTTP method because the spec says clients don't have to send a content-length header for methods
+ // that don't use it. So even though an attacker may provide a large body in a GET request, the body should go
+ // unread and a size filter is unneeded at best. See RFC 2616 section 14.13, and RFC 1945 section 10.4.
+ boolean willReadInputStream = maxContentLength > 0 && (httpMethod.equalsIgnoreCase("POST") || httpMethod.equalsIgnoreCase("PUT"));
+ if (!willReadInputStream) {
+ logger.info("No length check of request with method {} and maximum {}", httpMethod, maxContentLength);
Review comment:
I think this should be set to `debug` so it doesn't flood the logs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390697974
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/request/ContentLengthFilter.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.nifi.web.security.request;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ReadListener;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+/**
+ * This {@link Filter} rejects HTTP requests that exceed a specific, maximum size.
+ */
+public class ContentLengthFilter implements Filter {
+ private static final Logger logger = LoggerFactory.getLogger(ContentLengthFilter.class);
+ public final static String MAX_LENGTH_INIT_PARAM = "maxContentLength";
+ public final static int MAX_LENGTH_DEFAULT = 10_000_000;
+ private int maxContentLength;
+
+ public ContentLengthFilter() {
+ maxContentLength = MAX_LENGTH_DEFAULT;
+ }
+
+ public ContentLengthFilter(int maxLength) {
+ maxContentLength = maxLength;
+ }
+
+ @Override
+ public void init(FilterConfig config) throws ServletException {
+ String maxLength = config.getInitParameter(MAX_LENGTH_INIT_PARAM);
+ int length = maxLength == null ? MAX_LENGTH_DEFAULT : Integer.parseInt(maxLength);
+ if (length < 0) {
+ throw new ServletException("Invalid max request length.");
+ }
+ maxContentLength = length;
+ logger.info("Max content length set: " + maxLength + "b");
+ }
+
+ @Override
+ public void doFilter(ServletRequest request, ServletResponse response, FilterChain next) throws IOException, ServletException {
+ HttpServletRequest httpRequest = (HttpServletRequest) request;
+ String httpMethod = httpRequest.getMethod();
+
+ // Check the HTTP method because the spec says clients don't have to send a content-length header for methods
+ // that don't use it. So even though an attacker may provide a large body in a GET request, the body should go
+ // unread and a size filter is unneeded at best. See RFC 2616 section 14.13, and RFC 1945 section 10.4.
+ boolean willReadInputStream = maxContentLength > 0 && (httpMethod.equalsIgnoreCase("POST") || httpMethod.equalsIgnoreCase("PUT"));
+ if (!willReadInputStream) {
+ logger.info("No length check of request with method {} and maximum {}", httpMethod, maxContentLength);
+ next.doFilter(request, response);
+ return;
+ }
+
+ HttpServletResponse httpResponse = (HttpServletResponse) response;
+ int contentLength = request.getContentLength();
+ if (contentLength > maxContentLength) {
+ // Request with a client-specified length greater than our max is rejected:
+ logger.info("Content length check rejected request with content-length {} greater than maximum {}", contentLength, maxContentLength);
+ httpResponse.setContentType("text/plain");
+ httpResponse.getOutputStream().write("Payload Too large".getBytes());
+ httpResponse.setStatus(HttpServletResponse.SC_REQUEST_ENTITY_TOO_LARGE);
+ } else {
+ // If or when the request is read, this limits the read to our max:
+ logger.info("Content length check allowed request with content-length {} less than maximum {}", contentLength, maxContentLength);
Review comment:
I think this should be set to `debug` to avoid flooding the logs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r396868280
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/resources/org/apache/nifi/web/webdefault.xml
##########
@@ -551,6 +551,4 @@
<http-method-omission>TRACE</http-method-omission>
</web-resource-collection>
</security-constraint>
-
Review comment:
I don't think any non-whitespace changes were made to this file.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390697974
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/request/ContentLengthFilter.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.nifi.web.security.request;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ReadListener;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+/**
+ * This {@link Filter} rejects HTTP requests that exceed a specific, maximum size.
+ */
+public class ContentLengthFilter implements Filter {
+ private static final Logger logger = LoggerFactory.getLogger(ContentLengthFilter.class);
+ public final static String MAX_LENGTH_INIT_PARAM = "maxContentLength";
+ public final static int MAX_LENGTH_DEFAULT = 10_000_000;
+ private int maxContentLength;
+
+ public ContentLengthFilter() {
+ maxContentLength = MAX_LENGTH_DEFAULT;
+ }
+
+ public ContentLengthFilter(int maxLength) {
+ maxContentLength = maxLength;
+ }
+
+ @Override
+ public void init(FilterConfig config) throws ServletException {
+ String maxLength = config.getInitParameter(MAX_LENGTH_INIT_PARAM);
+ int length = maxLength == null ? MAX_LENGTH_DEFAULT : Integer.parseInt(maxLength);
+ if (length < 0) {
+ throw new ServletException("Invalid max request length.");
+ }
+ maxContentLength = length;
+ logger.info("Max content length set: " + maxLength + "b");
+ }
+
+ @Override
+ public void doFilter(ServletRequest request, ServletResponse response, FilterChain next) throws IOException, ServletException {
+ HttpServletRequest httpRequest = (HttpServletRequest) request;
+ String httpMethod = httpRequest.getMethod();
+
+ // Check the HTTP method because the spec says clients don't have to send a content-length header for methods
+ // that don't use it. So even though an attacker may provide a large body in a GET request, the body should go
+ // unread and a size filter is unneeded at best. See RFC 2616 section 14.13, and RFC 1945 section 10.4.
+ boolean willReadInputStream = maxContentLength > 0 && (httpMethod.equalsIgnoreCase("POST") || httpMethod.equalsIgnoreCase("PUT"));
+ if (!willReadInputStream) {
+ logger.info("No length check of request with method {} and maximum {}", httpMethod, maxContentLength);
+ next.doFilter(request, response);
+ return;
+ }
+
+ HttpServletResponse httpResponse = (HttpServletResponse) response;
+ int contentLength = request.getContentLength();
+ if (contentLength > maxContentLength) {
+ // Request with a client-specified length greater than our max is rejected:
+ logger.info("Content length check rejected request with content-length {} greater than maximum {}", contentLength, maxContentLength);
+ httpResponse.setContentType("text/plain");
+ httpResponse.getOutputStream().write("Payload Too large".getBytes());
+ httpResponse.setStatus(HttpServletResponse.SC_REQUEST_ENTITY_TOO_LARGE);
+ } else {
+ // If or when the request is read, this limits the read to our max:
+ logger.info("Content length check allowed request with content-length {} less than maximum {}", contentLength, maxContentLength);
Review comment:
I think this should be set to `debug` to avoid flooding the logs.
Should probably add units to these log statements for clarity.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390704136
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/test/java/org/apache/nifi/web/security/request/ContentLengthFilterTest.java
##########
@@ -0,0 +1,183 @@
+/*
+ * 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.nifi.web.security.request;
+
+import org.apache.commons.lang3.StringUtils;
+import org.eclipse.jetty.server.LocalConnector;
+import org.eclipse.jetty.server.Server;
+import org.eclipse.jetty.servlet.FilterHolder;
+import org.eclipse.jetty.servlet.ServletContextHandler;
+
+import org.eclipse.jetty.servlet.ServletHolder;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Test;
+
+import javax.servlet.DispatcherType;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.http.HttpServlet;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import java.io.IOException;
+import java.util.EnumSet;
+import java.util.concurrent.TimeUnit;
+
+
+/**
+ * This test exercises the {@link ContentLengthFilter} class.
+ *
+ * The approach here is to use a {@link LocalConnector} and raw strings for HTTP requests. The additional complexity
+ * of a complete HTTP client isn't required to determine the behavior, and any client would introduce a new dependency.
+ *
+ */
+public class ContentLengthFilterTest {
+ private static final int MAX_CONTENT_LENGTH = 1000;
+ private static final int SERVER_IDLE_TIMEOUT = 2500; // only one request needed + value large enough for slow systems
+ private static final String POST_REQUEST = "POST / HTTP/1.1\r\nContent-Length: %d\r\nHost: h\r\n\r\n%s";
+ private static final String FORM_REQUEST = "POST / HTTP/1.1\r\nContent-Length: %d\r\nHost: h\r\nContent-Type: application/x-www-form-urlencoded\r\n\r\n%s";
+ public static final int FORM_CONTENT_SIZE = 128;
+
+ private Server serverUnderTest;
+ private LocalConnector localConnector;
+ private ServletContextHandler contextUnderTest;
+
+ @After
+ public void stopServer() throws Exception {
+ if (serverUnderTest != null && serverUnderTest.isRunning()) {
+ serverUnderTest.stop();
+ }
+ }
+
+
+ @Test
+ public void testRequestsWithMissingContentLengthHeader() throws Exception {
+ configureAndStartServer(readFullyAndRespondOK, -1);
+
+ // This shows that the ContentLengthFilter allows a request that does not have a content-length header.
+ String response = localConnector.getResponse("POST / HTTP/1.0\r\n\r\n");
+ Assert.assertFalse(StringUtils.containsIgnoreCase(response, "411 Length Required"));
+ }
+
+
+ @Test
+ public void testRequestsWithContentLengthHeader() throws Exception {
+ configureAndStartServer(readFullyAndRespondOK, -1);
+
+ int smallClaim = 150;
+ int largeClaim = 2000;
+
+ String incompletePayload = StringUtils.repeat("1", 10);
+ String largePayload = StringUtils.repeat("1", largeClaim + 200);
+
+ // This shows that the ContentLengthFilter rejects a request when the client claims more than the max + sends more than the max:
+ String response = localConnector.getResponse(String.format(POST_REQUEST, largeClaim, largePayload));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "413 Payload Too Large"));
+
+ // This shows that the ContentLengthFilter rejects a request when the client claims more than the max + sends less the max:
+ response = localConnector.getResponse(String.format(POST_REQUEST, largeClaim, incompletePayload));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "413 Payload Too Large"));
+
+ // This shows that the ContentLengthFilter allows a request when it claims less than the max + sends more than the max:
+ response = localConnector.getResponse(String.format(POST_REQUEST, smallClaim, largePayload));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "200 OK"));
+
+ // This shows that the server times out when the client claims less than the max + sends less than the max + sends less than it claims to send:
+ response = localConnector.getResponse(String.format(POST_REQUEST, smallClaim, incompletePayload), 500, TimeUnit.MILLISECONDS);
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "500 Server Error"));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "Timeout"));
+ }
+
+
+ @Test
+ public void testJettyMaxFormSize() throws Exception {
+ // This shows that the jetty server option for 'maxFormContentSize' is insufficient for our needs because it
+ // catches requests like this:
+ configureAndStartServer(readFormAttempt, FORM_CONTENT_SIZE);
+ String form = "a=" + StringUtils.repeat("1", FORM_CONTENT_SIZE);
+ String response = localConnector.getResponse(String.format(FORM_REQUEST, form.length(), form));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "413 Payload Too Large"));
+
+
+ // But it does not catch requests like this:
+ response = localConnector.getResponse(String.format(POST_REQUEST, form.length(), form+form));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "417 Read Too Many Bytes"));
+ }
+
+
+
+ private HttpServlet readFullyAndRespondOK = new HttpServlet() {
Review comment:
This should be moved above the test methods.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] ottobackwards commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390216615
##########
File path: nifi-docs/src/main/asciidoc/administration-guide.adoc
##########
@@ -3247,6 +3247,9 @@ For example, when running in a Docker container or behind a proxy (e.g. localhos
host[:port] that NiFi is bound to.
|`nifi.web.proxy.context.path`|A comma separated list of allowed HTTP X-ProxyContextPath, X-Forwarded-Context, or X-Forwarded-Prefix header values to consider. By default, this value is
blank meaning all requests containing a proxy context path are rejected. Configuring this property would allow requests where the proxy path is contained in this listing.
+|`nifi.web.max.content.size`|The maximum size for regular PUT and POST requests. The default value is `10 MB`.
Review comment:
These could use better descriptions. Why 2 size settings? When does each get used? Do they both need to be set? Should only one be set?
Path to what? what is it used for? Why not /tmp?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] natural commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
natural commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r396008996
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/nf-error-handler.js
##########
@@ -89,7 +91,7 @@
}
// status code 400, 404, and 409 are expected response codes for nfCommon errors.
Review comment:
Comment updated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390709146
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/nf-error-handler.js
##########
@@ -89,7 +91,7 @@
}
// status code 400, 404, and 409 are expected response codes for nfCommon errors.
Review comment:
This comment should be updated to include 413.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390707633
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/test/java/org/apache/nifi/web/security/request/ContentLengthFilterTest.java
##########
@@ -0,0 +1,183 @@
+/*
+ * 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.nifi.web.security.request;
+
+import org.apache.commons.lang3.StringUtils;
+import org.eclipse.jetty.server.LocalConnector;
+import org.eclipse.jetty.server.Server;
+import org.eclipse.jetty.servlet.FilterHolder;
+import org.eclipse.jetty.servlet.ServletContextHandler;
+
+import org.eclipse.jetty.servlet.ServletHolder;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Test;
+
+import javax.servlet.DispatcherType;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.http.HttpServlet;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import java.io.IOException;
+import java.util.EnumSet;
+import java.util.concurrent.TimeUnit;
+
+
+/**
+ * This test exercises the {@link ContentLengthFilter} class.
+ *
+ * The approach here is to use a {@link LocalConnector} and raw strings for HTTP requests. The additional complexity
+ * of a complete HTTP client isn't required to determine the behavior, and any client would introduce a new dependency.
+ *
+ */
+public class ContentLengthFilterTest {
+ private static final int MAX_CONTENT_LENGTH = 1000;
+ private static final int SERVER_IDLE_TIMEOUT = 2500; // only one request needed + value large enough for slow systems
+ private static final String POST_REQUEST = "POST / HTTP/1.1\r\nContent-Length: %d\r\nHost: h\r\n\r\n%s";
+ private static final String FORM_REQUEST = "POST / HTTP/1.1\r\nContent-Length: %d\r\nHost: h\r\nContent-Type: application/x-www-form-urlencoded\r\n\r\n%s";
+ public static final int FORM_CONTENT_SIZE = 128;
+
+ private Server serverUnderTest;
+ private LocalConnector localConnector;
+ private ServletContextHandler contextUnderTest;
+
+ @After
+ public void stopServer() throws Exception {
+ if (serverUnderTest != null && serverUnderTest.isRunning()) {
+ serverUnderTest.stop();
+ }
+ }
+
+
+ @Test
+ public void testRequestsWithMissingContentLengthHeader() throws Exception {
+ configureAndStartServer(readFullyAndRespondOK, -1);
+
+ // This shows that the ContentLengthFilter allows a request that does not have a content-length header.
+ String response = localConnector.getResponse("POST / HTTP/1.0\r\n\r\n");
+ Assert.assertFalse(StringUtils.containsIgnoreCase(response, "411 Length Required"));
+ }
+
+
+ @Test
+ public void testRequestsWithContentLengthHeader() throws Exception {
+ configureAndStartServer(readFullyAndRespondOK, -1);
+
+ int smallClaim = 150;
+ int largeClaim = 2000;
+
+ String incompletePayload = StringUtils.repeat("1", 10);
+ String largePayload = StringUtils.repeat("1", largeClaim + 200);
+
+ // This shows that the ContentLengthFilter rejects a request when the client claims more than the max + sends more than the max:
+ String response = localConnector.getResponse(String.format(POST_REQUEST, largeClaim, largePayload));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "413 Payload Too Large"));
+
+ // This shows that the ContentLengthFilter rejects a request when the client claims more than the max + sends less the max:
+ response = localConnector.getResponse(String.format(POST_REQUEST, largeClaim, incompletePayload));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "413 Payload Too Large"));
+
+ // This shows that the ContentLengthFilter allows a request when it claims less than the max + sends more than the max:
+ response = localConnector.getResponse(String.format(POST_REQUEST, smallClaim, largePayload));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "200 OK"));
+
+ // This shows that the server times out when the client claims less than the max + sends less than the max + sends less than it claims to send:
+ response = localConnector.getResponse(String.format(POST_REQUEST, smallClaim, incompletePayload), 500, TimeUnit.MILLISECONDS);
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "500 Server Error"));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "Timeout"));
+ }
+
+
+ @Test
+ public void testJettyMaxFormSize() throws Exception {
+ // This shows that the jetty server option for 'maxFormContentSize' is insufficient for our needs because it
+ // catches requests like this:
+ configureAndStartServer(readFormAttempt, FORM_CONTENT_SIZE);
+ String form = "a=" + StringUtils.repeat("1", FORM_CONTENT_SIZE);
+ String response = localConnector.getResponse(String.format(FORM_REQUEST, form.length(), form));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "413 Payload Too Large"));
+
+
+ // But it does not catch requests like this:
+ response = localConnector.getResponse(String.format(POST_REQUEST, form.length(), form+form));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "417 Read Too Many Bytes"));
+ }
+
+
+
+ private HttpServlet readFullyAndRespondOK = new HttpServlet() {
+ @Override
+ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
+ ServletInputStream input = req.getInputStream();
+ while (!input.isFinished()) {
+ input.read();
+ }
+ resp.setStatus(HttpServletResponse.SC_OK);
+ }
+ };
+
+
+ private HttpServlet readFormAttempt = new HttpServlet() {
Review comment:
All of these private variables should be moved above the test methods that reference them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390685901
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/resources/org/apache/nifi/web/webdefault.xml
##########
@@ -551,6 +551,24 @@
<http-method-omission>TRACE</http-method-omission>
</web-resource-collection>
</security-constraint>
-
+ <filter>
Review comment:
I agree with @mcgilman 's question about why we can't define and associate this filter in the `JettyServer` class for consistency.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390697821
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/request/ContentLengthFilter.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.nifi.web.security.request;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ReadListener;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+/**
+ * This {@link Filter} rejects HTTP requests that exceed a specific, maximum size.
+ */
+public class ContentLengthFilter implements Filter {
+ private static final Logger logger = LoggerFactory.getLogger(ContentLengthFilter.class);
+ public final static String MAX_LENGTH_INIT_PARAM = "maxContentLength";
+ public final static int MAX_LENGTH_DEFAULT = 10_000_000;
+ private int maxContentLength;
+
+ public ContentLengthFilter() {
+ maxContentLength = MAX_LENGTH_DEFAULT;
+ }
+
+ public ContentLengthFilter(int maxLength) {
+ maxContentLength = maxLength;
+ }
+
+ @Override
+ public void init(FilterConfig config) throws ServletException {
+ String maxLength = config.getInitParameter(MAX_LENGTH_INIT_PARAM);
+ int length = maxLength == null ? MAX_LENGTH_DEFAULT : Integer.parseInt(maxLength);
+ if (length < 0) {
+ throw new ServletException("Invalid max request length.");
+ }
+ maxContentLength = length;
+ logger.info("Max content length set: " + maxLength + "b");
+ }
+
+ @Override
+ public void doFilter(ServletRequest request, ServletResponse response, FilterChain next) throws IOException, ServletException {
+ HttpServletRequest httpRequest = (HttpServletRequest) request;
+ String httpMethod = httpRequest.getMethod();
+
+ // Check the HTTP method because the spec says clients don't have to send a content-length header for methods
+ // that don't use it. So even though an attacker may provide a large body in a GET request, the body should go
+ // unread and a size filter is unneeded at best. See RFC 2616 section 14.13, and RFC 1945 section 10.4.
+ boolean willReadInputStream = maxContentLength > 0 && (httpMethod.equalsIgnoreCase("POST") || httpMethod.equalsIgnoreCase("PUT"));
+ if (!willReadInputStream) {
+ logger.info("No length check of request with method {} and maximum {}", httpMethod, maxContentLength);
Review comment:
Should probably add units to these log statements for clarity.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] natural commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
natural commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r396008323
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
##########
@@ -607,6 +612,25 @@ private WebAppContext loadWar(final File warFile, final String contextPath, fina
return webappContext;
}
+ private void addContentLengthFilter(String contextPath, String pathSpec, WebAppContext webappContext) {
+ final FilterHolder holder = new FilterHolder(ContentLengthFilter.class);
+ final Map<String, String> largePaths = props.getWebMaxContentSizeLargePaths();
+ int size;
+
+ if (!largePaths.containsValue(contextPath)) {
+ size = DataUnit.parseDataSize(props.getWebMaxContentSize(), DataUnit.B).intValue();
+ } else {
+ size = DataUnit.parseDataSize(props.getWebMaxContentSizeLarge(), DataUnit.B).intValue();
+ }
+ holder.setInitParameters(new HashMap<String, String>() {{
+ put("maxContentLength", String.valueOf(size));
+ }});
+
+ logger.info("Adding Content Length Filter to context at pathSpec: " + contextPath + " with max size: " + size + "b");
+ // NB: the use of pathSpec vs contextPath
Review comment:
simplified and removed the extra argument.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] ottobackwards commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390722033
##########
File path: nifi-docs/src/main/asciidoc/administration-guide.adoc
##########
@@ -3247,6 +3247,9 @@ For example, when running in a Docker container or behind a proxy (e.g. localhos
host[:port] that NiFi is bound to.
|`nifi.web.proxy.context.path`|A comma separated list of allowed HTTP X-ProxyContextPath, X-Forwarded-Context, or X-Forwarded-Prefix header values to consider. By default, this value is
blank meaning all requests containing a proxy context path are rejected. Configuring this property would allow requests where the proxy path is contained in this listing.
+|`nifi.web.max.content.size`|The maximum size for regular PUT and POST requests. The default value is `10 MB`.
Review comment:
I know you resolved this @alopresto, but this doesn't tell when each large setting should be used, or if they can be used together or what not. Still confusing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] natural commented on issue #4125: NIFI-7153 Adds
ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
natural commented on issue #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#issuecomment-603408254
> @natural if you can show me what I am doing wrong or resolve the issue with the log output, I will make the minor formatting changes I pointed out and merge this.
I believe you tripped the internal form-length filter because `curl` is sending a multipart form.
When I PUT json directly:
```$ openssl rand -hex 30240000 > /tmp/hexdata
$ http -p Hh PUT :8080/nifi-api/processors/0da1088c-0171-1000-da90-5ddb7f242ee1 foo=@/tmp/hexdata
PUT /nifi-api/processors/0da1088c-0171-1000-da90-5ddb7f242ee1 HTTP/1.1
Accept: application/json, */*
Accept-Encoding: gzip, deflate
Connection: keep-alive
Content-Length: 60480013
Content-Type: application/json
Host: localhost:8080
User-Agent: HTTPie/1.0.2
HTTP/1.1 413 Payload Too Large
Content-Length: 17
Content-Security-Policy: frame-ancestors 'self'
Content-Type: text/plain
Date: Tue, 24 Mar 2020 17:51:08 GMT
Server: Jetty(9.4.19.v20190610)
X-Frame-Options: SAMEORIGIN
X-XSS-Protection: 1; mode=block
```
I have the log entries as expected:
`2020-03-24 09:51:08,932 WARN [NiFi Web Server-18] o.a.n.w.s.requests.ContentLengthFilter Content length check rejected request with content-length 60480013 greater than maximum 20971520 B`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r396870963
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/requests/ContentLengthFilter.java
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.nifi.web.security.requests;
+
+import org.apache.nifi.logging.NiFiLog;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ReadListener;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+
+public class ContentLengthFilter implements Filter {
+ private static final Logger logger = new NiFiLog(LoggerFactory.getLogger(ContentLengthFilter.class));
+ public final static String MAX_LENGTH_INIT_PARAM = "maxContentLength";
+ public final static int MAX_LENGTH_DEFAULT = 10_000_000;
+ private int maxContentLength;
+
+ public void init() {
+ maxContentLength = MAX_LENGTH_DEFAULT;
+ logger.debug("Filter initialized without configuration and set max content length: " + maxContentLength + "B");
+ }
+
+
+ @Override
+ public void init(FilterConfig config) throws ServletException {
+ String maxLength = config.getInitParameter(MAX_LENGTH_INIT_PARAM);
+ int length = maxLength == null ? MAX_LENGTH_DEFAULT : Integer.parseInt(maxLength);
+ if (length < 0) {
+ throw new ServletException("Invalid max request length.");
+ }
+ maxContentLength = length;
+ logger.debug("Filter initialized and set max content length: " + maxContentLength + "B");
+ }
+
+ @Override
+ public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException {
+ HttpServletRequest httpRequest = (HttpServletRequest) request;
+ String httpMethod = httpRequest.getMethod();
+
+ // Check the HTTP method because the spec says clients don't have to send a content-length header for methods
+ // that don't use it. So even though an attacker may provide a large body in a GET request, the body should go
+ // unread and a size filter is unneeded at best. See RFC 2616 section 14.13, and RFC 1945 section 10.4.
+ boolean willExamine = maxContentLength > 0 && (httpMethod.equalsIgnoreCase("POST") || httpMethod.equalsIgnoreCase("PUT"));
+ if (!willExamine) {
+ logger.debug("No length check of request with method {} and maximum {}B", httpMethod, maxContentLength);
+ chain.doFilter(request, response);
+ return;
+ }
+
+ HttpServletResponse httpResponse = (HttpServletResponse) response;
+ int contentLength = request.getContentLength();
+ if (contentLength > maxContentLength) {
+ // Request with a client-specified length greater than our max is rejected:
+ httpResponse.setContentType("text/plain");
+ httpResponse.getOutputStream().write("Payload Too large".getBytes());
+ httpResponse.setStatus(HttpServletResponse.SC_REQUEST_ENTITY_TOO_LARGE);
+ logger.warn("Content length check rejected request with content-length {} greater than maximum {}B", contentLength, maxContentLength);
Review comment:
Add space before units.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] natural commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
natural commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r396008724
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/request/ContentLengthFilter.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.nifi.web.security.request;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ReadListener;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+/**
+ * This {@link Filter} rejects HTTP requests that exceed a specific, maximum size.
+ */
+public class ContentLengthFilter implements Filter {
+ private static final Logger logger = LoggerFactory.getLogger(ContentLengthFilter.class);
+ public final static String MAX_LENGTH_INIT_PARAM = "maxContentLength";
+ public final static int MAX_LENGTH_DEFAULT = 10_000_000;
+ private int maxContentLength;
+
+ public ContentLengthFilter() {
+ maxContentLength = MAX_LENGTH_DEFAULT;
+ }
+
+ public ContentLengthFilter(int maxLength) {
+ maxContentLength = maxLength;
+ }
+
+ @Override
+ public void init(FilterConfig config) throws ServletException {
+ String maxLength = config.getInitParameter(MAX_LENGTH_INIT_PARAM);
+ int length = maxLength == null ? MAX_LENGTH_DEFAULT : Integer.parseInt(maxLength);
+ if (length < 0) {
+ throw new ServletException("Invalid max request length.");
+ }
+ maxContentLength = length;
+ logger.info("Max content length set: " + maxLength + "b");
+ }
+
+ @Override
+ public void doFilter(ServletRequest request, ServletResponse response, FilterChain next) throws IOException, ServletException {
+ HttpServletRequest httpRequest = (HttpServletRequest) request;
+ String httpMethod = httpRequest.getMethod();
+
+ // Check the HTTP method because the spec says clients don't have to send a content-length header for methods
+ // that don't use it. So even though an attacker may provide a large body in a GET request, the body should go
+ // unread and a size filter is unneeded at best. See RFC 2616 section 14.13, and RFC 1945 section 10.4.
+ boolean willReadInputStream = maxContentLength > 0 && (httpMethod.equalsIgnoreCase("POST") || httpMethod.equalsIgnoreCase("PUT"));
+ if (!willReadInputStream) {
+ logger.info("No length check of request with method {} and maximum {}", httpMethod, maxContentLength);
Review comment:
Added `B` to maximum value in logs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] natural commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
natural commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r396008357
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/resources/org/apache/nifi/web/webdefault.xml
##########
@@ -551,6 +551,24 @@
<http-method-omission>TRACE</http-method-omission>
</web-resource-collection>
</security-constraint>
-
+ <filter>
Review comment:
it was an earlier difficulty, now overcome.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] natural commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
natural commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r396008101
##########
File path: nifi-docs/src/main/asciidoc/administration-guide.adoc
##########
@@ -3247,6 +3247,9 @@ For example, when running in a Docker container or behind a proxy (e.g. localhos
host[:port] that NiFi is bound to.
|`nifi.web.proxy.context.path`|A comma separated list of allowed HTTP X-ProxyContextPath, X-Forwarded-Context, or X-Forwarded-Prefix header values to consider. By default, this value is
blank meaning all requests containing a proxy context path are rejected. Configuring this property would allow requests where the proxy path is contained in this listing.
+|`nifi.web.max.content.size`|The maximum size for regular PUT and POST requests. The default value is `10 MB`.
Review comment:
@ottobackwards thanks for the great feedback!
i've gone ahead and simplified this into a single property, `nifi.web.max.content.size`, no paths needed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] natural commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
natural commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r396008763
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/request/ContentLengthFilter.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.nifi.web.security.request;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ReadListener;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+/**
+ * This {@link Filter} rejects HTTP requests that exceed a specific, maximum size.
+ */
+public class ContentLengthFilter implements Filter {
+ private static final Logger logger = LoggerFactory.getLogger(ContentLengthFilter.class);
+ public final static String MAX_LENGTH_INIT_PARAM = "maxContentLength";
+ public final static int MAX_LENGTH_DEFAULT = 10_000_000;
+ private int maxContentLength;
+
+ public ContentLengthFilter() {
+ maxContentLength = MAX_LENGTH_DEFAULT;
+ }
+
+ public ContentLengthFilter(int maxLength) {
+ maxContentLength = maxLength;
+ }
+
+ @Override
+ public void init(FilterConfig config) throws ServletException {
+ String maxLength = config.getInitParameter(MAX_LENGTH_INIT_PARAM);
+ int length = maxLength == null ? MAX_LENGTH_DEFAULT : Integer.parseInt(maxLength);
+ if (length < 0) {
+ throw new ServletException("Invalid max request length.");
+ }
+ maxContentLength = length;
+ logger.info("Max content length set: " + maxLength + "b");
+ }
+
+ @Override
+ public void doFilter(ServletRequest request, ServletResponse response, FilterChain next) throws IOException, ServletException {
+ HttpServletRequest httpRequest = (HttpServletRequest) request;
+ String httpMethod = httpRequest.getMethod();
+
+ // Check the HTTP method because the spec says clients don't have to send a content-length header for methods
+ // that don't use it. So even though an attacker may provide a large body in a GET request, the body should go
+ // unread and a size filter is unneeded at best. See RFC 2616 section 14.13, and RFC 1945 section 10.4.
+ boolean willReadInputStream = maxContentLength > 0 && (httpMethod.equalsIgnoreCase("POST") || httpMethod.equalsIgnoreCase("PUT"));
+ if (!willReadInputStream) {
+ logger.info("No length check of request with method {} and maximum {}", httpMethod, maxContentLength);
+ next.doFilter(request, response);
+ return;
+ }
+
+ HttpServletResponse httpResponse = (HttpServletResponse) response;
+ int contentLength = request.getContentLength();
+ if (contentLength > maxContentLength) {
+ // Request with a client-specified length greater than our max is rejected:
+ logger.info("Content length check rejected request with content-length {} greater than maximum {}", contentLength, maxContentLength);
+ httpResponse.setContentType("text/plain");
+ httpResponse.getOutputStream().write("Payload Too large".getBytes());
+ httpResponse.setStatus(HttpServletResponse.SC_REQUEST_ENTITY_TOO_LARGE);
+ } else {
+ // If or when the request is read, this limits the read to our max:
+ logger.info("Content length check allowed request with content-length {} less than maximum {}", contentLength, maxContentLength);
Review comment:
Changed to `debug` and added units.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390706399
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/test/java/org/apache/nifi/web/security/request/ContentLengthFilterTest.java
##########
@@ -0,0 +1,183 @@
+/*
+ * 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.nifi.web.security.request;
+
+import org.apache.commons.lang3.StringUtils;
+import org.eclipse.jetty.server.LocalConnector;
+import org.eclipse.jetty.server.Server;
+import org.eclipse.jetty.servlet.FilterHolder;
+import org.eclipse.jetty.servlet.ServletContextHandler;
+
+import org.eclipse.jetty.servlet.ServletHolder;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Test;
+
+import javax.servlet.DispatcherType;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.http.HttpServlet;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import java.io.IOException;
+import java.util.EnumSet;
+import java.util.concurrent.TimeUnit;
+
+
+/**
+ * This test exercises the {@link ContentLengthFilter} class.
+ *
+ * The approach here is to use a {@link LocalConnector} and raw strings for HTTP requests. The additional complexity
+ * of a complete HTTP client isn't required to determine the behavior, and any client would introduce a new dependency.
+ *
+ */
+public class ContentLengthFilterTest {
+ private static final int MAX_CONTENT_LENGTH = 1000;
+ private static final int SERVER_IDLE_TIMEOUT = 2500; // only one request needed + value large enough for slow systems
+ private static final String POST_REQUEST = "POST / HTTP/1.1\r\nContent-Length: %d\r\nHost: h\r\n\r\n%s";
+ private static final String FORM_REQUEST = "POST / HTTP/1.1\r\nContent-Length: %d\r\nHost: h\r\nContent-Type: application/x-www-form-urlencoded\r\n\r\n%s";
+ public static final int FORM_CONTENT_SIZE = 128;
+
+ private Server serverUnderTest;
+ private LocalConnector localConnector;
+ private ServletContextHandler contextUnderTest;
+
+ @After
+ public void stopServer() throws Exception {
+ if (serverUnderTest != null && serverUnderTest.isRunning()) {
+ serverUnderTest.stop();
+ }
+ }
+
+
+ @Test
+ public void testRequestsWithMissingContentLengthHeader() throws Exception {
+ configureAndStartServer(readFullyAndRespondOK, -1);
+
+ // This shows that the ContentLengthFilter allows a request that does not have a content-length header.
+ String response = localConnector.getResponse("POST / HTTP/1.0\r\n\r\n");
+ Assert.assertFalse(StringUtils.containsIgnoreCase(response, "411 Length Required"));
+ }
+
+
+ @Test
+ public void testRequestsWithContentLengthHeader() throws Exception {
+ configureAndStartServer(readFullyAndRespondOK, -1);
+
+ int smallClaim = 150;
+ int largeClaim = 2000;
+
+ String incompletePayload = StringUtils.repeat("1", 10);
+ String largePayload = StringUtils.repeat("1", largeClaim + 200);
+
+ // This shows that the ContentLengthFilter rejects a request when the client claims more than the max + sends more than the max:
+ String response = localConnector.getResponse(String.format(POST_REQUEST, largeClaim, largePayload));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "413 Payload Too Large"));
+
+ // This shows that the ContentLengthFilter rejects a request when the client claims more than the max + sends less the max:
+ response = localConnector.getResponse(String.format(POST_REQUEST, largeClaim, incompletePayload));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "413 Payload Too Large"));
+
+ // This shows that the ContentLengthFilter allows a request when it claims less than the max + sends more than the max:
+ response = localConnector.getResponse(String.format(POST_REQUEST, smallClaim, largePayload));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "200 OK"));
+
+ // This shows that the server times out when the client claims less than the max + sends less than the max + sends less than it claims to send:
+ response = localConnector.getResponse(String.format(POST_REQUEST, smallClaim, incompletePayload), 500, TimeUnit.MILLISECONDS);
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "500 Server Error"));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "Timeout"));
+ }
+
+
+ @Test
+ public void testJettyMaxFormSize() throws Exception {
+ // This shows that the jetty server option for 'maxFormContentSize' is insufficient for our needs because it
+ // catches requests like this:
+ configureAndStartServer(readFormAttempt, FORM_CONTENT_SIZE);
+ String form = "a=" + StringUtils.repeat("1", FORM_CONTENT_SIZE);
+ String response = localConnector.getResponse(String.format(FORM_REQUEST, form.length(), form));
+ Assert.assertTrue(StringUtils.containsIgnoreCase(response, "413 Payload Too Large"));
+
+
+ // But it does not catch requests like this:
+ response = localConnector.getResponse(String.format(POST_REQUEST, form.length(), form+form));
Review comment:
Interesting catch. It appears it is only set for form elements.
>The problem with maxFormContentSize it only limit the request size for "form" data and not for other content types like text/xml or text/json ... and also only if Content-Length [is sent]. So not really a strong security improvement.
From: https://stackoverflow.com/questions/26181892/configure-max-post-size-for-a-external-webapp-in-jetty#comment50923561_30646189
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390688868
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/request/ContentLengthFilter.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.nifi.web.security.request;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ReadListener;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+/**
+ * This {@link Filter} rejects HTTP requests that exceed a specific, maximum size.
+ */
+public class ContentLengthFilter implements Filter {
+ private static final Logger logger = LoggerFactory.getLogger(ContentLengthFilter.class);
+ public final static String MAX_LENGTH_INIT_PARAM = "maxContentLength";
+ public final static int MAX_LENGTH_DEFAULT = 10_000_000;
+ private int maxContentLength;
+
+ public ContentLengthFilter() {
+ maxContentLength = MAX_LENGTH_DEFAULT;
+ }
+
+ public ContentLengthFilter(int maxLength) {
+ maxContentLength = maxLength;
+ }
+
+ @Override
+ public void init(FilterConfig config) throws ServletException {
+ String maxLength = config.getInitParameter(MAX_LENGTH_INIT_PARAM);
+ int length = maxLength == null ? MAX_LENGTH_DEFAULT : Integer.parseInt(maxLength);
+ if (length < 0) {
+ throw new ServletException("Invalid max request length.");
+ }
+ maxContentLength = length;
+ logger.info("Max content length set: " + maxLength + "b");
+ }
+
+ @Override
+ public void doFilter(ServletRequest request, ServletResponse response, FilterChain next) throws IOException, ServletException {
+ HttpServletRequest httpRequest = (HttpServletRequest) request;
+ String httpMethod = httpRequest.getMethod();
+
+ // Check the HTTP method because the spec says clients don't have to send a content-length header for methods
Review comment:
This is a good and helpful comment.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] alopresto commented on a change in pull request #4125:
NIFI-7153 Adds ContentLengthFilter and DoSFilter
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4125: NIFI-7153 Adds ContentLengthFilter and DoSFilter
URL: https://github.com/apache/nifi/pull/4125#discussion_r390686447
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/request/ContentLengthFilter.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.nifi.web.security.request;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ReadListener;
+import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+/**
+ * This {@link Filter} rejects HTTP requests that exceed a specific, maximum size.
+ */
+public class ContentLengthFilter implements Filter {
+ private static final Logger logger = LoggerFactory.getLogger(ContentLengthFilter.class);
+ public final static String MAX_LENGTH_INIT_PARAM = "maxContentLength";
+ public final static int MAX_LENGTH_DEFAULT = 10_000_000;
+ private int maxContentLength;
+
+ public ContentLengthFilter() {
+ maxContentLength = MAX_LENGTH_DEFAULT;
+ }
+
+ public ContentLengthFilter(int maxLength) {
+ maxContentLength = maxLength;
+ }
+
+ @Override
+ public void init(FilterConfig config) throws ServletException {
+ String maxLength = config.getInitParameter(MAX_LENGTH_INIT_PARAM);
+ int length = maxLength == null ? MAX_LENGTH_DEFAULT : Integer.parseInt(maxLength);
+ if (length < 0) {
+ throw new ServletException("Invalid max request length.");
+ }
+ maxContentLength = length;
+ logger.info("Max content length set: " + maxLength + "b");
Review comment:
Is the max content length in _bytes_ (`B`) or _bits_ (`b`)?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services