Add a config option to limit request body size #276

andrew4699 · 2024-09-09T20:37:06Z

Description

Adds a config option to limit the request body size. This is useful to prevent DoS-style attacks before the request moves further through the request handlers.

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Added an integration test
Existing tests work
Ran Polaris with no limit, a small limit, and a high limit, and verified that the behavior was as expected

Checklist:

Please delete options that are not relevant.

I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
If adding new functionality, I have discussed my implementation with the community using the linked GitHub issue

...vice/src/main/java/org/apache/polaris/service/throttling/RequestThrottlingErrorResponse.java

...rc/main/java/org/apache/polaris/service/throttling/StreamReadConstraintsExceptionMapper.java

polaris-service/src/test/resources/polaris-server-integrationtest.yml

polaris-server.yml

flyrain · 2024-09-10T17:30:45Z

polaris-service/src/main/java/org/apache/polaris/service/config/PolarisApplicationConfig.java

+  public long getMaxDocumentBytes() {
+    return maxDocumentBytes;
+  }


I think it should have a default value.

Checking a bit more about class StreamReadConstraints. -1 is the right default value, which means no limit.

public Builder maxDocumentLength(long maxDocLen) { if (maxDocLen <= 0L) { maxDocLen = -1L; } this.maxDocLen = maxDocLen; return this; }

Thank you for pulling this up, this actually makes me more strongly interested in 0. Please see my new comment below.

I also find 0-as-unlimited a little confusing. I agree that if we make the default -1 it might imply that 0 is different from -1, but I also think a default of 0 is going to raise some questions.

Maybe we should just disallow 0, since StreamReadConstraints doesn't support what 0 would intuitively mean ("reject all requests")

From the code from StreamReadConstraints, that means both 0 and -1 are considered no limits. But I think it does make sense to have -1 for unlimited and disallow 0.

Ok I updated it to default to -1, explained what -1 is, and restricted 0 with an exception/useful comment. I agree that -1 is a better indicator of "no limit" and restricting 0 solves some ambiguity.

andrew4699 · 2024-09-10T19:38:33Z

I agree with the reasons for wanting to make -1 the default value, but I'd like to offer a counterargument for why it may make sense to keep it at 0 in this case:

As this comment shows, both 0 and -1 disable the limit. Listing -1 may falsely lead the user to believe that 0 is different.
Default value compatibility across JSON/YAML serializers. With the default set to 0, you don't have to worry about whether your serialization library respects your default value nor what it does with unspecified fields.

flyrain · 2024-09-10T20:42:55Z

I'm fine with either -1 or 0. My main concern with using 0 is that there is a chance that it might be misunderstood as a valid limit rather than indicating "no limit." In contrast, -1 is less ambiguous in this context. Additionally, I recommend defining a constant for clarity:

private static final long DOCUMENT_BYTES_NO_LIMIT = -1;

eric-maynard · 2024-09-10T22:59:05Z

polaris-server.yml

@@ -167,3 +167,5 @@ logging:
      archivedLogFilenamePattern: ./logs/polaris-%d.log.gz
      # The maximum number of log files to archive.
      archivedFileCount: 14
+
+maxDocumentBytes: 0 # Don't set a limit


This comment would ideally not be inline

Do we really want to call this maxDocumentBytes? What is a "Document" in the context of a Polaris instance?

Whatever we call it, we might put a small comment explaining what this is

Renamed to maxRequestBodyBytes and updated/moved the comment.

andrew4699 · 2024-09-12T22:35:09Z

Just updated a couple of things:

Renamed maxDocumentBytes to maxRequestBodyBytes
Changed the default to -1 and made it throw an exception if you try to use 0, since the underlying implementation treats 0 and -1 as the same and we don't want to trick the user into thinking 0 actually does something different
Improved comments in the config
Added an additional test validating that there's already an existing limit on total header size

eric-maynard · 2024-09-12T23:30:34Z

polaris-service/src/main/java/org/apache/polaris/service/config/PolarisApplicationConfig.java

+      // we block 0 to prevent ambiguity.
+      throw new IllegalArgumentException(
+          String.format(
+              "maxRequestBodyBytes may not be 0. Use %d to specify no limit.",


nit: "maxRequestBodyBytes must be a positive integer or %d to specify no limit."

eric-maynard · 2024-09-12T23:30:51Z

polaris-service/src/main/java/org/apache/polaris/service/config/PolarisApplicationConfig.java

@@ -146,6 +149,23 @@ public void setFeatureConfiguration(Map<String, Object> featureConfiguration) {
    this.configurationStore = new DefaultConfigurationStore(featureConfiguration);
  }

+  @JsonProperty("maxRequestBodyBytes")
+  public void setMaxRequestBodyBytes(long maxRequestBodyBytes) {
+    if (maxRequestBodyBytes == 0) {


nit: Maybe we should constrain this to a positive integer or -1 while we're at it

Updated to restrict all but -1

flyrain

LGTM with one minor question.

flyrain · 2024-09-12T23:19:17Z

polaris-service/src/main/java/org/apache/polaris/service/config/PolarisApplicationConfig.java

@@ -146,6 +149,23 @@ public void setFeatureConfiguration(Map<String, Object> featureConfiguration) {
    this.configurationStore = new DefaultConfigurationStore(featureConfiguration);
  }

+  @JsonProperty("maxRequestBodyBytes")
+  public void setMaxRequestBodyBytes(long maxRequestBodyBytes) {
+    if (maxRequestBodyBytes == 0) {


Should we consider throwing an exception when the configured value for maxRequestBodyBytes is less than -1 (e.g., -2, -100)? While the underlying library can technically handle these values, I believe we should restrict any negative number except for -1, which is already treated as "unlimited." This is consistent with our approach of throwing an exception for a value of 0.

Updated to restrict all but -1

eric-maynard

LGTM with some optional nits. Thanks for the change!

snazy · 2024-09-16T08:37:28Z

...vice/src/main/java/org/apache/polaris/service/throttling/RequestThrottlingErrorResponse.java

+ * limits
+ */
+public class RequestThrottlingErrorResponse {
+  public enum Error {


Error is a name from java.lang - can you choose something different here?

eric-maynard · 2024-09-16T22:38:07Z

polaris-service/src/main/java/org/apache/polaris/service/config/PolarisApplicationConfig.java

+      // same, so
+      // we block all but -1 to prevent ambiguity.


nit: This formatting looks a bit odd. Can it be 2 lines?

Updated.

I've just been letting spotless take the wheel with these, but I guess it's ok with either style? We should probably update it to prefer a single format if possible.

flyrain

+1 with a minor suggestion

flyrain · 2024-09-16T23:24:24Z

polaris-service/src/main/java/org/apache/polaris/service/config/PolarisApplicationConfig.java

+    if (maxRequestBodyBytes <= 0 && maxRequestBodyBytes != -1) {
+      // The underlying library that we use to implement the limit treats all values <= 0 as the
+      // same, so we block all but -1 to prevent ambiguity.
+      throw new IllegalArgumentException(
+          String.format(
+              "maxRequestBodyBytes must be a positive integer or %d to specify no limit.",
+              REQUEST_BODY_BYTES_NO_LIMIT));
+    }


minor suggestion:

Preconditions.checkArgument(condition, message);

flyrain · 2024-09-17T00:04:01Z

Thanks @andrew4699 for working on this. Thanks @snazy @eric-maynard for the review.

andrew4699 added 3 commits September 9, 2024 11:55

Add a config for limiting request size

3ac7594

Add comment clarifying test setup

431030e

Remove unused description

94dd387

andrew4699 requested review from jbonofre, ashvina, RussellSpitzer, snazy, vvcephei, takidau and jackye1995 as code owners September 9, 2024 20:37

Apply spotless

39cce56

collado-mike reviewed Sep 10, 2024

View reviewed changes

...vice/src/main/java/org/apache/polaris/service/throttling/RequestThrottlingErrorResponse.java Show resolved Hide resolved

...rc/main/java/org/apache/polaris/service/throttling/StreamReadConstraintsExceptionMapper.java Show resolved Hide resolved

eric-maynard reviewed Sep 10, 2024

View reviewed changes

polaris-service/src/test/resources/polaris-server-integrationtest.yml Outdated Show resolved Hide resolved

eric-maynard reviewed Sep 10, 2024

View reviewed changes

polaris-server.yml Outdated Show resolved Hide resolved

eric-maynard reviewed Sep 10, 2024

View reviewed changes

polaris-server.yml Outdated Show resolved Hide resolved

flyrain reviewed Sep 10, 2024

View reviewed changes

Add newline to EOF, javadoc

a147edf

eric-maynard reviewed Sep 10, 2024

View reviewed changes

andrew4699 added 3 commits September 12, 2024 15:28

Rename to maxRequestBodyBytes, restrict 0

9b7f1a9

Add test for header size limit

de66166

spotlessApply

8c3c0ff

andrew4699 requested review from flyrain, aihuaxu, eric-maynard, collado-mike and sfc-gh-aixu September 12, 2024 22:33

eric-maynard reviewed Sep 12, 2024

View reviewed changes

flyrain previously approved these changes Sep 12, 2024

View reviewed changes

eric-maynard approved these changes Sep 12, 2024

View reviewed changes

snazy reviewed Sep 16, 2024

View reviewed changes

Restrict non -1 values, update exception message, rename Error

3dda549

andrew4699 dismissed flyrain’s stale review via 3dda549 September 16, 2024 22:36

eric-maynard reviewed Sep 16, 2024

View reviewed changes

Update weird comment spacing

e35e02e

flyrain previously approved these changes Sep 16, 2024

View reviewed changes

Use Preconditions, allow ProcessingException

d895fd2

andrew4699 dismissed flyrain’s stale review via d895fd2 September 16, 2024 23:42

flyrain approved these changes Sep 17, 2024

View reviewed changes

flyrain merged commit 68b248a into apache:main Sep 17, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a config option to limit request body size #276

Add a config option to limit request body size #276

andrew4699 commented Sep 9, 2024

flyrain Sep 10, 2024

flyrain Sep 10, 2024

andrew4699 Sep 10, 2024

eric-maynard Sep 10, 2024

sfc-gh-aixu Sep 12, 2024

andrew4699 Sep 12, 2024 •

edited

Loading

andrew4699 commented Sep 10, 2024 •

edited

Loading

flyrain commented Sep 10, 2024

eric-maynard Sep 10, 2024

andrew4699 Sep 12, 2024

andrew4699 commented Sep 12, 2024

eric-maynard Sep 12, 2024

andrew4699 Sep 16, 2024

eric-maynard Sep 12, 2024 •

edited

Loading

andrew4699 Sep 16, 2024

flyrain left a comment

flyrain Sep 12, 2024

andrew4699 Sep 16, 2024

eric-maynard left a comment

snazy Sep 16, 2024

andrew4699 Sep 16, 2024

eric-maynard Sep 16, 2024

andrew4699 Sep 16, 2024

flyrain left a comment

flyrain Sep 16, 2024

flyrain commented Sep 17, 2024

Add a config option to limit request body size #276

Add a config option to limit request body size #276

Conversation

andrew4699 commented Sep 9, 2024

Description

Type of change

How Has This Been Tested?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew4699 Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

andrew4699 commented Sep 10, 2024 • edited Loading

flyrain commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew4699 commented Sep 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-maynard Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flyrain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-maynard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flyrain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flyrain commented Sep 17, 2024

andrew4699 Sep 12, 2024 •

edited

Loading

andrew4699 commented Sep 10, 2024 •

edited

Loading

eric-maynard Sep 12, 2024 •

edited

Loading