You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by ekovacs <gi...@git.apache.org> on 2018/09/05 15:23:13 UTC
[GitHub] nifi pull request #2991: NIFI-3469: multipart request support added to Handl...
GitHub user ekovacs opened a pull request:
https://github.com/apache/nifi/pull/2991
NIFI-3469: multipart request support added to HandleHttpRequest
Thank you for submitting a contribution to Apache NiFi.
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?
### For code changes:
- [ ] 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?
- [ ] 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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ekovacs/nifi NIFI-3469
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2991.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2991
----
commit 4910b5cbe73b7b93610abfbf0ac3ddc02b2943bf
Author: Endre Zoltan Kovacs <an...@...>
Date: 2018-09-04T14:35:37Z
NIFI-3469 multipart request support added to HandleHttpRequest
----
---
[GitHub] nifi pull request #2991: NIFI-3469: multipart request support added to Handl...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2991#discussion_r218868215
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -521,161 +553,221 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
final long start = System.nanoTime();
final HttpServletRequest request = container.getRequest();
- FlowFile flowFile = session.create();
- try (OutputStream flowFileOut = session.write(flowFile)) {
- StreamUtils.copy(request.getInputStream(), flowFileOut);
- } catch (final IOException e) {
- // There may be many reasons which can produce an IOException on the HTTP stream and in some of them, eg.
- // bad requests, the connection to the client is not closed. In order to address also these cases, we try
- // and answer with a BAD_REQUEST, which lets the client know that the request has not been correctly
- // processed and makes it aware that the connection can be closed.
- getLogger().error("Failed to receive content from HTTP Request from {} due to {}",
- new Object[]{request.getRemoteAddr(), e});
- session.remove(flowFile);
- try {
- HttpServletResponse response = container.getResponse();
- response.sendError(Status.BAD_REQUEST.getStatusCode());
- response.flushBuffer();
- container.getContext().complete();
- } catch (final IOException ioe) {
- getLogger().warn("Failed to send HTTP response to {} due to {}",
- new Object[]{request.getRemoteAddr(), ioe});
+ if (!Strings.isNullOrEmpty(request.getContentType()) && request.getContentType().contains(MIME_TYPE__MULTIPART_FORM_DATA)) {
+ final long maxRequestSize = context.getProperty(MAX_REQUEST_SIZE).asLong();
+ request.setAttribute(Request.__MULTIPART_CONFIG_ELEMENT, new MultipartConfigElement("/tmp", maxRequestSize, maxRequestSize, 0));
--- End diff --
This also opens up a lot of security concerns. We need to be very careful about how we handle, sanitize, trust, store, and display this data.
Some good starting places for reading:
* https://www.owasp.org/index.php/Deserialization_of_untrusted_data
* https://www.owasp.org/index.php/Unrestricted_File_Upload
* https://www.owasp.org/index.php/Insecure_Temporary_File
* https://www.owasp.org/index.php/Protect_FileUpload_Against_Malicious_File
---
[GitHub] nifi pull request #2991: NIFI-3469: multipart request support added to Handl...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2991#discussion_r218799259
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -229,6 +252,14 @@
.name("container-queue-size").displayName("Container Queue Size")
.description("The size of the queue for Http Request Containers").required(true)
.addValidator(StandardValidators.POSITIVE_INTEGER_VALIDATOR).defaultValue("50").build();
+ public static final PropertyDescriptor MAX_REQUEST_SIZE = new PropertyDescriptor.Builder()
--- End diff --
This appears to be used only for multipart/mime requests. This should be made clear in the description.
---
[GitHub] nifi pull request #2991: NIFI-3469: multipart request support added to Handl...
Posted by ekovacs <gi...@git.apache.org>.
Github user ekovacs commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2991#discussion_r223290455
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -229,7 +252,25 @@
.name("container-queue-size").displayName("Container Queue Size")
.description("The size of the queue for Http Request Containers").required(true)
.addValidator(StandardValidators.POSITIVE_INTEGER_VALIDATOR).defaultValue("50").build();
-
+ public static final PropertyDescriptor MAX_REQUEST_SIZE = new PropertyDescriptor.Builder()
+ .name("max-request-size")
+ .displayName("Max Request Size")
+ .description("The max size of the request. Only applies for requests with Content-Type: multipart/form-data, "
+ + "and is used to prevent denial of service type of attacks, to prevent filling up the heap or disk space")
+ .required(true)
+ .addValidator(StandardValidators.DATA_SIZE_VALIDATOR)
+ .defaultValue("1 MB")
+ .build();
+ public static final PropertyDescriptor IN_MEMORY_FILE_SIZE_THRESHOLD = new PropertyDescriptor.Builder()
+ .name("in-memory-file-size-threshold")
+ .displayName("The threshold size, at which the contents of an incoming file would be written to disk. "
+ + "Only applies for requests with Content-Type: multipart/form-data. "
+ + "It is used to prevent denial of service type of attacks, to prevent filling up the heap or disk space.")
--- End diff --
I believe this still has to do with preventing DOS type attacks, tightly coupled with MULTIPART_REQUEST_MAX_SIZE.
If request size is very high (eg.: and your example of 9GB file) and this measure would not be in place, the heap would fill up, and would bring down the JVM with OutOfMemoryError.
---
[GitHub] nifi pull request #2991: NIFI-3469: multipart request support added to Handl...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2991#discussion_r224003310
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -521,161 +565,223 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
final long start = System.nanoTime();
final HttpServletRequest request = container.getRequest();
- FlowFile flowFile = session.create();
- try (OutputStream flowFileOut = session.write(flowFile)) {
- StreamUtils.copy(request.getInputStream(), flowFileOut);
- } catch (final IOException e) {
- // There may be many reasons which can produce an IOException on the HTTP stream and in some of them, eg.
- // bad requests, the connection to the client is not closed. In order to address also these cases, we try
- // and answer with a BAD_REQUEST, which lets the client know that the request has not been correctly
- // processed and makes it aware that the connection can be closed.
- getLogger().error("Failed to receive content from HTTP Request from {} due to {}",
- new Object[]{request.getRemoteAddr(), e});
- session.remove(flowFile);
- try {
- HttpServletResponse response = container.getResponse();
- response.sendError(Status.BAD_REQUEST.getStatusCode());
- response.flushBuffer();
- container.getContext().complete();
- } catch (final IOException ioe) {
- getLogger().warn("Failed to send HTTP response to {} due to {}",
- new Object[]{request.getRemoteAddr(), ioe});
+ if (!Strings.isNullOrEmpty(request.getContentType()) && request.getContentType().contains(MIME_TYPE__MULTIPART_FORM_DATA)) {
+ final long requestMaxSize = context.getProperty(MULTIPART_REQUEST_MAX_SIZE).asDataSize(DataUnit.B).longValue();
+ final int readBufferSize = context.getProperty(MULTIPART_READ_BUFFER_SIZE).asDataSize(DataUnit.B).intValue();
+ String tempDir = System.getProperty("java.io.tmpdir");
+ request.setAttribute(Request.__MULTIPART_CONFIG_ELEMENT, new MultipartConfigElement(tempDir, requestMaxSize, requestMaxSize, readBufferSize));
+ try {
+ List<Part> parts = ImmutableList.copyOf(request.getParts());
+ int allPartsCount = parts.size();
+ final String contextIdentifier = UUID.randomUUID().toString();
+ for (int i = 0; i < allPartsCount; i++) {
+ Part part = parts.get(i);
+ FlowFile flowFile = session.create();
+ try (OutputStream flowFileOut = session.write(flowFile)) {
+ StreamUtils.copy(part.getInputStream(), flowFileOut);
+ } catch (IOException e) {
+ handleFlowContentStreamingError(session, container, request, Optional.of(flowFile), e);
+ return;
+ }
+ flowFile = savePartAttributes(context, session, part, flowFile, i, allPartsCount);
+ flowFile = saveRequestAttributes(context, session, request, flowFile, contextIdentifier);
+ forwardFlowFile(context, session, container, start, request, flowFile, i == 0);
--- End diff --
What if `contextMap.register()` method returns `false`? I haven't test that, but current code seems to keep processing remaining multi-part. The register part should be a separated method and return if the request is successfully registered, so that this `allPartsCount` loop can break.
Adding a unit test case to confirm that situation would be helpful.
---
[GitHub] nifi pull request #2991: NIFI-3469: multipart request support added to Handl...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2991#discussion_r222562031
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -229,7 +252,25 @@
.name("container-queue-size").displayName("Container Queue Size")
.description("The size of the queue for Http Request Containers").required(true)
.addValidator(StandardValidators.POSITIVE_INTEGER_VALIDATOR).defaultValue("50").build();
-
+ public static final PropertyDescriptor MAX_REQUEST_SIZE = new PropertyDescriptor.Builder()
+ .name("max-request-size")
+ .displayName("Max Request Size")
--- End diff --
I'd prefer "Multipart Request Max Size" to clarify its scope.
---
[GitHub] nifi issue #2991: NIFI-3469: multipart request support added to HandleHttpRe...
Posted by ekovacs <gi...@git.apache.org>.
Github user ekovacs commented on the issue:
https://github.com/apache/nifi/pull/2991
@jtstorck @joewitt @mcgilman @YolandaMDavis could any of you please take a look?
---
[GitHub] nifi pull request #2991: NIFI-3469: multipart request support added to Handl...
Posted by ekovacs <gi...@git.apache.org>.
Github user ekovacs commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2991#discussion_r223285500
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -229,7 +252,25 @@
.name("container-queue-size").displayName("Container Queue Size")
.description("The size of the queue for Http Request Containers").required(true)
.addValidator(StandardValidators.POSITIVE_INTEGER_VALIDATOR).defaultValue("50").build();
-
+ public static final PropertyDescriptor MAX_REQUEST_SIZE = new PropertyDescriptor.Builder()
+ .name("max-request-size")
+ .displayName("Max Request Size")
+ .description("The max size of the request. Only applies for requests with Content-Type: multipart/form-data, "
+ + "and is used to prevent denial of service type of attacks, to prevent filling up the heap or disk space")
+ .required(true)
+ .addValidator(StandardValidators.DATA_SIZE_VALIDATOR)
+ .defaultValue("1 MB")
+ .build();
+ public static final PropertyDescriptor IN_MEMORY_FILE_SIZE_THRESHOLD = new PropertyDescriptor.Builder()
+ .name("in-memory-file-size-threshold")
+ .displayName("The threshold size, at which the contents of an incoming file would be written to disk. "
+ + "Only applies for requests with Content-Type: multipart/form-data. "
+ + "It is used to prevent denial of service type of attacks, to prevent filling up the heap or disk space.")
--- End diff --
indeed, thanks!
---
[GitHub] nifi issue #2991: NIFI-3469: multipart request support added to HandleHttpRe...
Posted by ekovacs <gi...@git.apache.org>.
Github user ekovacs commented on the issue:
https://github.com/apache/nifi/pull/2991
thank you @ijokarumawak for your thorough review. pushed the requested changes.
---
[GitHub] nifi issue #2991: NIFI-3469: multipart request support added to HandleHttpRe...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the issue:
https://github.com/apache/nifi/pull/2991
@ekovacs thanks for the improvement! It appears that Koji is happy with everything now, and a quick code review looks good to me as well. I was able to test locally and all works as expected. +1 merged to master. Thanks!
---
[GitHub] nifi pull request #2991: NIFI-3469: multipart request support added to Handl...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2991#discussion_r222560481
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -229,7 +252,25 @@
.name("container-queue-size").displayName("Container Queue Size")
.description("The size of the queue for Http Request Containers").required(true)
.addValidator(StandardValidators.POSITIVE_INTEGER_VALIDATOR).defaultValue("50").build();
-
+ public static final PropertyDescriptor MAX_REQUEST_SIZE = new PropertyDescriptor.Builder()
+ .name("max-request-size")
+ .displayName("Max Request Size")
+ .description("The max size of the request. Only applies for requests with Content-Type: multipart/form-data, "
+ + "and is used to prevent denial of service type of attacks, to prevent filling up the heap or disk space")
+ .required(true)
+ .addValidator(StandardValidators.DATA_SIZE_VALIDATOR)
+ .defaultValue("1 MB")
+ .build();
+ public static final PropertyDescriptor IN_MEMORY_FILE_SIZE_THRESHOLD = new PropertyDescriptor.Builder()
+ .name("in-memory-file-size-threshold")
+ .displayName("The threshold size, at which the contents of an incoming file would be written to disk. "
+ + "Only applies for requests with Content-Type: multipart/form-data. "
+ + "It is used to prevent denial of service type of attacks, to prevent filling up the heap or disk space.")
--- End diff --
This display name is too long to be fit in the UI, please move it to description and user shorter display name. How about "Multipart Read Buffer Size"?
---
[GitHub] nifi pull request #2991: NIFI-3469: multipart request support added to Handl...
Posted by ekovacs <gi...@git.apache.org>.
Github user ekovacs commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2991#discussion_r223290758
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -521,161 +564,223 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
final long start = System.nanoTime();
final HttpServletRequest request = container.getRequest();
- FlowFile flowFile = session.create();
- try (OutputStream flowFileOut = session.write(flowFile)) {
- StreamUtils.copy(request.getInputStream(), flowFileOut);
- } catch (final IOException e) {
- // There may be many reasons which can produce an IOException on the HTTP stream and in some of them, eg.
- // bad requests, the connection to the client is not closed. In order to address also these cases, we try
- // and answer with a BAD_REQUEST, which lets the client know that the request has not been correctly
- // processed and makes it aware that the connection can be closed.
- getLogger().error("Failed to receive content from HTTP Request from {} due to {}",
- new Object[]{request.getRemoteAddr(), e});
- session.remove(flowFile);
- try {
- HttpServletResponse response = container.getResponse();
- response.sendError(Status.BAD_REQUEST.getStatusCode());
- response.flushBuffer();
- container.getContext().complete();
- } catch (final IOException ioe) {
- getLogger().warn("Failed to send HTTP response to {} due to {}",
- new Object[]{request.getRemoteAddr(), ioe});
+ if (!Strings.isNullOrEmpty(request.getContentType()) && request.getContentType().contains(MIME_TYPE__MULTIPART_FORM_DATA)) {
+ final long maxRequestSize = context.getProperty(MAX_REQUEST_SIZE).asDataSize(DataUnit.B).longValue();
+ final int inMemoryFileSizeThreshold = context.getProperty(IN_MEMORY_FILE_SIZE_THRESHOLD).asDataSize(DataUnit.B).intValue()
+ String tempDir = System.getProperty("java.io.tmpdir");
+ request.setAttribute(Request.__MULTIPART_CONFIG_ELEMENT, new MultipartConfigElement(tempDir, maxRequestSize, maxRequestSize, inMemoryFileSizeThreshold));
+ try {
+ List<Part> parts = ImmutableList.copyOf(request.getParts());
+ int allPartsCount = parts.size();
+ final String contextIdentifier = UUID.randomUUID().toString();
+ for (int i = 0; i < allPartsCount; i++) {
+ Part part = parts.get(i);
+ FlowFile flowFile = session.create();
+ try (OutputStream flowFileOut = session.write(flowFile)) {
+ StreamUtils.copy(part.getInputStream(), flowFileOut);
+ } catch (IOException e) {
+ handleFlowContentStreamingError(session, container, request, Optional.of(flowFile), e);
+ return;
+ }
+ flowFile = savePartAttributes(context, session, part, flowFile, i, allPartsCount);
+ flowFile = saveRequestAttributes(context, session, request, flowFile, contextIdentifier);
+ forwardFlowFile(context, session, container, start, request, flowFile, i == 0);
}
+ } catch (IOException | ServletException e) {
+ handleFlowContentStreamingError(session, container, request, Optional.absent(), e);
--- End diff --
indeed! Thank you!
---
[GitHub] nifi pull request #2991: NIFI-3469: multipart request support added to Handl...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2991#discussion_r218798942
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -229,6 +252,14 @@
.name("container-queue-size").displayName("Container Queue Size")
.description("The size of the queue for Http Request Containers").required(true)
.addValidator(StandardValidators.POSITIVE_INTEGER_VALIDATOR).defaultValue("50").build();
+ public static final PropertyDescriptor MAX_REQUEST_SIZE = new PropertyDescriptor.Builder()
+ .name("max-request-size")
+ .displayName("Max Request Size")
+ .description("The maximal size of the request")
+ .required(true)
+ .addValidator(StandardValidators.POSITIVE_LONG_VALIDATOR)
--- End diff --
This should be using a DATA_SIZE_VALIDATOR and use input in the format of "1 MB" instead of a Long value. Default value should be "1 MB" then.
---
[GitHub] nifi pull request #2991: NIFI-3469: multipart request support added to Handl...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2991#discussion_r222542805
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -521,161 +564,223 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
final long start = System.nanoTime();
final HttpServletRequest request = container.getRequest();
- FlowFile flowFile = session.create();
- try (OutputStream flowFileOut = session.write(flowFile)) {
- StreamUtils.copy(request.getInputStream(), flowFileOut);
- } catch (final IOException e) {
- // There may be many reasons which can produce an IOException on the HTTP stream and in some of them, eg.
- // bad requests, the connection to the client is not closed. In order to address also these cases, we try
- // and answer with a BAD_REQUEST, which lets the client know that the request has not been correctly
- // processed and makes it aware that the connection can be closed.
- getLogger().error("Failed to receive content from HTTP Request from {} due to {}",
- new Object[]{request.getRemoteAddr(), e});
- session.remove(flowFile);
- try {
- HttpServletResponse response = container.getResponse();
- response.sendError(Status.BAD_REQUEST.getStatusCode());
- response.flushBuffer();
- container.getContext().complete();
- } catch (final IOException ioe) {
- getLogger().warn("Failed to send HTTP response to {} due to {}",
- new Object[]{request.getRemoteAddr(), ioe});
+ if (!Strings.isNullOrEmpty(request.getContentType()) && request.getContentType().contains(MIME_TYPE__MULTIPART_FORM_DATA)) {
+ final long maxRequestSize = context.getProperty(MAX_REQUEST_SIZE).asDataSize(DataUnit.B).longValue();
+ final int inMemoryFileSizeThreshold = context.getProperty(IN_MEMORY_FILE_SIZE_THRESHOLD).asDataSize(DataUnit.B).intValue()
--- End diff --
Missing import DataUnit, another compile error.
---
[GitHub] nifi pull request #2991: NIFI-3469: multipart request support added to Handl...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2991#discussion_r222560898
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -229,7 +252,25 @@
.name("container-queue-size").displayName("Container Queue Size")
.description("The size of the queue for Http Request Containers").required(true)
.addValidator(StandardValidators.POSITIVE_INTEGER_VALIDATOR).defaultValue("50").build();
-
+ public static final PropertyDescriptor MAX_REQUEST_SIZE = new PropertyDescriptor.Builder()
+ .name("max-request-size")
+ .displayName("Max Request Size")
+ .description("The max size of the request. Only applies for requests with Content-Type: multipart/form-data, "
+ + "and is used to prevent denial of service type of attacks, to prevent filling up the heap or disk space")
+ .required(true)
+ .addValidator(StandardValidators.DATA_SIZE_VALIDATOR)
+ .defaultValue("1 MB")
+ .build();
+ public static final PropertyDescriptor IN_MEMORY_FILE_SIZE_THRESHOLD = new PropertyDescriptor.Builder()
+ .name("in-memory-file-size-threshold")
+ .displayName("The threshold size, at which the contents of an incoming file would be written to disk. "
+ + "Only applies for requests with Content-Type: multipart/form-data. "
+ + "It is used to prevent denial of service type of attacks, to prevent filling up the heap or disk space.")
--- End diff --
"It is used to prevent denial of service type of attacks, to prevent filling up the heap or disk space." is not applicable here I believe. The statement is for "Max Request Size".
---
[GitHub] nifi issue #2991: NIFI-3469: multipart request support added to HandleHttpRe...
Posted by ekovacs <gi...@git.apache.org>.
Github user ekovacs commented on the issue:
https://github.com/apache/nifi/pull/2991
Hi @markap14
I've pushed some changes.
Could you please take a look them?
Thanks & Best regards,
Endre
---
[GitHub] nifi pull request #2991: NIFI-3469: multipart request support added to Handl...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2991#discussion_r222578602
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -521,161 +564,223 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
final long start = System.nanoTime();
final HttpServletRequest request = container.getRequest();
- FlowFile flowFile = session.create();
- try (OutputStream flowFileOut = session.write(flowFile)) {
- StreamUtils.copy(request.getInputStream(), flowFileOut);
- } catch (final IOException e) {
- // There may be many reasons which can produce an IOException on the HTTP stream and in some of them, eg.
- // bad requests, the connection to the client is not closed. In order to address also these cases, we try
- // and answer with a BAD_REQUEST, which lets the client know that the request has not been correctly
- // processed and makes it aware that the connection can be closed.
- getLogger().error("Failed to receive content from HTTP Request from {} due to {}",
- new Object[]{request.getRemoteAddr(), e});
- session.remove(flowFile);
- try {
- HttpServletResponse response = container.getResponse();
- response.sendError(Status.BAD_REQUEST.getStatusCode());
- response.flushBuffer();
- container.getContext().complete();
- } catch (final IOException ioe) {
- getLogger().warn("Failed to send HTTP response to {} due to {}",
- new Object[]{request.getRemoteAddr(), ioe});
+ if (!Strings.isNullOrEmpty(request.getContentType()) && request.getContentType().contains(MIME_TYPE__MULTIPART_FORM_DATA)) {
+ final long maxRequestSize = context.getProperty(MAX_REQUEST_SIZE).asDataSize(DataUnit.B).longValue();
+ final int inMemoryFileSizeThreshold = context.getProperty(IN_MEMORY_FILE_SIZE_THRESHOLD).asDataSize(DataUnit.B).intValue()
+ String tempDir = System.getProperty("java.io.tmpdir");
+ request.setAttribute(Request.__MULTIPART_CONFIG_ELEMENT, new MultipartConfigElement(tempDir, maxRequestSize, maxRequestSize, inMemoryFileSizeThreshold));
+ try {
+ List<Part> parts = ImmutableList.copyOf(request.getParts());
+ int allPartsCount = parts.size();
+ final String contextIdentifier = UUID.randomUUID().toString();
+ for (int i = 0; i < allPartsCount; i++) {
+ Part part = parts.get(i);
+ FlowFile flowFile = session.create();
+ try (OutputStream flowFileOut = session.write(flowFile)) {
+ StreamUtils.copy(part.getInputStream(), flowFileOut);
+ } catch (IOException e) {
+ handleFlowContentStreamingError(session, container, request, Optional.of(flowFile), e);
+ return;
+ }
+ flowFile = savePartAttributes(context, session, part, flowFile, i, allPartsCount);
+ flowFile = saveRequestAttributes(context, session, request, flowFile, contextIdentifier);
+ forwardFlowFile(context, session, container, start, request, flowFile, i == 0);
}
+ } catch (IOException | ServletException e) {
+ handleFlowContentStreamingError(session, container, request, Optional.absent(), e);
--- End diff --
If the uploaded file size exceeds the configured maximum size, IllegalStateException was thrown. And we need to catch that to handleFlowContentStreamingError and to return 400 BAD_REQUEST status code. When I uploaded 9GB file, NiFi logged ERROR, but HTTP client kept sending remaining data.
By returning 400, client stops sending data immediately.
FYI, Jetty threw following exception:
```
2018-10-04 17:18:02,915 ERROR [Timer-Driven Process Thread-10] o.a.n.p.standard.HandleHttpRequest HandleHttpRequest[id=3daec2d8-0166-1000-ea00-52f6b3d7d1a1] HandleHttpRequest[id=3daec2d8-0166-1000-ea00-52f6b3d7d1a1] failed to process session due to java.lang.IllegalStateException: Request exceeds maxRequestSize (1048576); Processor Administratively Yielded for 1 sec: java.lang.IllegalStateException: Request exceeds maxRequestSize (1048576)
java.lang.IllegalStateException: Request exceeds maxRequestSize (1048576)
at org.eclipse.jetty.util.MultiPartInputStreamParser.parse(MultiPartInputStreamParser.java:773)
at org.eclipse.jetty.util.MultiPartInputStreamParser.getParts(MultiPartInputStreamParser.java:493)
at org.eclipse.jetty.server.MultiParts$MultiPartsUtilParser.<init>(MultiParts.java:121)
at org.eclipse.jetty.server.Request.newMultiParts(Request.java:2410)
at org.eclipse.jetty.server.Request.getParts(Request.java:2333)
at org.eclipse.jetty.server.Request.getParts(Request.java:2319)
at org.apache.nifi.processors.standard.HandleHttpRequest.onTrigger(HandleHttpRequest.java:575)
at org.apache.nifi.processor.AbstractProcessor.onTrigger(AbstractProcessor.java:27)
at org.apache.nifi.controller.StandardProcessorNode.onTrigger(StandardProcessorNode.java:1165)
at org.apache.nifi.controller.tasks.ConnectableTask.invoke(ConnectableTask.java:203)
at org.apache.nifi.controller.scheduling.TimerDrivenSchedulingAgent$1.run(TimerDrivenSchedulingAgent.java:117)
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:308)
```
---
[GitHub] nifi issue #2991: NIFI-3469: multipart request support added to HandleHttpRe...
Posted by ekovacs <gi...@git.apache.org>.
Github user ekovacs commented on the issue:
https://github.com/apache/nifi/pull/2991
thanks @ijokarumawak you were right!
added the fix for it, and an UT as well
---
[GitHub] nifi pull request #2991: NIFI-3469: multipart request support added to Handl...
Posted by ekovacs <gi...@git.apache.org>.
Github user ekovacs commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2991#discussion_r219117891
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -521,161 +553,221 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
final long start = System.nanoTime();
final HttpServletRequest request = container.getRequest();
- FlowFile flowFile = session.create();
- try (OutputStream flowFileOut = session.write(flowFile)) {
- StreamUtils.copy(request.getInputStream(), flowFileOut);
- } catch (final IOException e) {
- // There may be many reasons which can produce an IOException on the HTTP stream and in some of them, eg.
- // bad requests, the connection to the client is not closed. In order to address also these cases, we try
- // and answer with a BAD_REQUEST, which lets the client know that the request has not been correctly
- // processed and makes it aware that the connection can be closed.
- getLogger().error("Failed to receive content from HTTP Request from {} due to {}",
- new Object[]{request.getRemoteAddr(), e});
- session.remove(flowFile);
- try {
- HttpServletResponse response = container.getResponse();
- response.sendError(Status.BAD_REQUEST.getStatusCode());
- response.flushBuffer();
- container.getContext().complete();
- } catch (final IOException ioe) {
- getLogger().warn("Failed to send HTTP response to {} due to {}",
- new Object[]{request.getRemoteAddr(), ioe});
+ if (!Strings.isNullOrEmpty(request.getContentType()) && request.getContentType().contains(MIME_TYPE__MULTIPART_FORM_DATA)) {
+ final long maxRequestSize = context.getProperty(MAX_REQUEST_SIZE).asLong();
+ request.setAttribute(Request.__MULTIPART_CONFIG_ELEMENT, new MultipartConfigElement("/tmp", maxRequestSize, maxRequestSize, 0));
--- End diff --
Hi @markap14
Thank you for reviewing my changes.
Please see my responses inline.
Best regards,
Endre
> As-is, this is going to buffer the entire request into memory and then throw an Exception if the request reaches some configured threshold. Unfortunately, I think this is going to be too limiting for many use cases. We should not request that the entire file upload fit into java's heap.
The original ticket: https://issues.apache.org/jira/browse/NIFI-3469
Explicitly mentioned to avoid writing to local disk, thus i took this approach. This was achieved by passing 0 as the last parameter for: `new MultipartConfigElement("/tmp", maxRequestSize, maxRequestSize, 0)`
> I think the best approach (longer term) would be to create our own MIME Parser (or find one that already exists?) that is capable of processing the data in a streaming fashion. I presume that Jetty doesn't allow that because they want to allow you to obtain all Part's and then access them individually. However, we're not doing that and so their API is rather limiting.
Prior to implementing, I read lots of advices/tips on the web. eg.: warns: https://stackoverflow.com/questions/2422468/how-to-upload-files-to-server-using-jsp-servlet/2424824#2424824
As this SO post warns:
* _Don't manually parse it_
* _ You shouldn't try to do this on your own or copypaste some homegrown library-less code found elsewhere on the Internet_
* _You should rather use a real library which is used (and implicitly tested!) by millions of users for years. Such a library has proven its robustness._
I believe the return on asset would not be optimal, as it would open up a world of bugs.
> For the shorter term, though, I think it makes more sense to expose each of these options - a temporary directory to write files to (we cannot use "/tmp" because it may not exist on many operating systems. Should perhaps default to the value of System.getProperty("java.io.tmpdir")), max request size (explain that this is used to prevent Denial of Service attacks that would fill disk space), and a threshold at which point we would stream the data to the temporary file and then re-read it (what jetty refers to as the "fileSizeThreshold", i.e., the last parameter).
I agree.
> While streaming the data to a temporary file on disk is expensive and less than ideal, I think it's much better to give the user the option of doing that than to simply reject the request if it is too large.
Indeed it makes more sense. I'll make the necessary changes for this.
---
[GitHub] nifi pull request #2991: NIFI-3469: multipart request support added to Handl...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2991#discussion_r218816915
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -521,161 +553,221 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
final long start = System.nanoTime();
final HttpServletRequest request = container.getRequest();
- FlowFile flowFile = session.create();
- try (OutputStream flowFileOut = session.write(flowFile)) {
- StreamUtils.copy(request.getInputStream(), flowFileOut);
- } catch (final IOException e) {
- // There may be many reasons which can produce an IOException on the HTTP stream and in some of them, eg.
- // bad requests, the connection to the client is not closed. In order to address also these cases, we try
- // and answer with a BAD_REQUEST, which lets the client know that the request has not been correctly
- // processed and makes it aware that the connection can be closed.
- getLogger().error("Failed to receive content from HTTP Request from {} due to {}",
- new Object[]{request.getRemoteAddr(), e});
- session.remove(flowFile);
- try {
- HttpServletResponse response = container.getResponse();
- response.sendError(Status.BAD_REQUEST.getStatusCode());
- response.flushBuffer();
- container.getContext().complete();
- } catch (final IOException ioe) {
- getLogger().warn("Failed to send HTTP response to {} due to {}",
- new Object[]{request.getRemoteAddr(), ioe});
+ if (!Strings.isNullOrEmpty(request.getContentType()) && request.getContentType().contains(MIME_TYPE__MULTIPART_FORM_DATA)) {
+ final long maxRequestSize = context.getProperty(MAX_REQUEST_SIZE).asLong();
+ request.setAttribute(Request.__MULTIPART_CONFIG_ELEMENT, new MultipartConfigElement("/tmp", maxRequestSize, maxRequestSize, 0));
--- End diff --
As-is, this is going to buffer the entire request into memory and then throw an Exception if the request reaches some configured threshold. Unfortunately, I think this is going to be too limiting for many use cases. We should not request that the entire file upload fit into java's heap.
I think the best approach (longer term) would be to create our own MIME Parser (or find one that already exists?) that is capable of processing the data in a streaming fashion. I presume that Jetty doesn't allow that because they want to allow you to obtain all Part's and then access them individually. However, we're not doing that and so their API is rather limiting.
For the shorter term, though, I think it makes more sense to expose each of these options - a temporary directory to write files to (we cannot use "/tmp" because it may not exist on many operating systems. Should perhaps default to the value of System.getProperty("java.io.tmpdir")), max request size (explain that this is used to prevent Denial of Service attacks that would fill disk space), and a threshold at which point we would stream the data to the temporary file and then re-read it (what jetty refers to as the "fileSizeThreshold", i.e., the last parameter).
While streaming the data to a temporary file on disk is expensive and less than ideal, I think it's much better to give the user the option of doing that than to simply reject the request if it is too large.
---
[GitHub] nifi pull request #2991: NIFI-3469: multipart request support added to Handl...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2991#discussion_r222541582
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -521,161 +564,223 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
final long start = System.nanoTime();
final HttpServletRequest request = container.getRequest();
- FlowFile flowFile = session.create();
- try (OutputStream flowFileOut = session.write(flowFile)) {
- StreamUtils.copy(request.getInputStream(), flowFileOut);
- } catch (final IOException e) {
- // There may be many reasons which can produce an IOException on the HTTP stream and in some of them, eg.
- // bad requests, the connection to the client is not closed. In order to address also these cases, we try
- // and answer with a BAD_REQUEST, which lets the client know that the request has not been correctly
- // processed and makes it aware that the connection can be closed.
- getLogger().error("Failed to receive content from HTTP Request from {} due to {}",
- new Object[]{request.getRemoteAddr(), e});
- session.remove(flowFile);
- try {
- HttpServletResponse response = container.getResponse();
- response.sendError(Status.BAD_REQUEST.getStatusCode());
- response.flushBuffer();
- container.getContext().complete();
- } catch (final IOException ioe) {
- getLogger().warn("Failed to send HTTP response to {} due to {}",
- new Object[]{request.getRemoteAddr(), ioe});
+ if (!Strings.isNullOrEmpty(request.getContentType()) && request.getContentType().contains(MIME_TYPE__MULTIPART_FORM_DATA)) {
+ final long maxRequestSize = context.getProperty(MAX_REQUEST_SIZE).asDataSize(DataUnit.B).longValue();
+ final int inMemoryFileSizeThreshold = context.getProperty(IN_MEMORY_FILE_SIZE_THRESHOLD).asDataSize(DataUnit.B).intValue()
--- End diff --
Compilation fails due to missing the trailing semi-colon.
---
[GitHub] nifi pull request #2991: NIFI-3469: multipart request support added to Handl...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2991
---