Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

updates to CRS v4.0.0-rc2, sets equal BodyLimits in default configs #243

Merged
merged 5 commits into from
Nov 24, 2023

Conversation

M4tteoP
Copy link
Member

@M4tteoP M4tteoP commented Oct 25, 2023

This PR:

  • Updates to CRS v4.0.0-rc2
    • Updates accordingly ftw tests. Notably two new tests will require extra care: 934131-5, 934131-7.
  • Deprecates @crs-setup-demo-conf in favour of @crs-setup-conf. @crs-setup-conf is already meant to be specifically edited for coraza-proxy-wasm (early_blocking enabled), two config files providing the same configuration would just be confusing.
    • @demo-conf has some differences compared to the original coraza.conf (first of all enabling the Engine and not keeping it in DetectionOnly mode). This PR does not deprecate it.
  • Sets RequestBodyLimit equal to BodyInMemoryLimit. As stated in Coraza itself: TinyGo MemoryLimit should be equal to Limit

@M4tteoP M4tteoP force-pushed the update_to_crs_4_rc2 branch from da7df91 to 31b7247 Compare October 25, 2023 18:09
@M4tteoP M4tteoP marked this pull request as ready for review October 26, 2023 08:14

SecRequestBodyNoFilesLimit 131072
# SecRequestBodyNoFilesLimit 131072
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think SecRequestBodyNoFilesLimit is not implemented Coraza side, we should at least comment it out, not letting users think that it is enforced.

SecRequestBodyLimit 13107200

SecRequestBodyInMemoryLimit 131072
SecRequestBodyInMemoryLimit 13107200
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change connected to the 1gb?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it is about:

Not being able to split the request between memory and then into a file after a certain limit, for coraza-proxy-wasm the two limits should be the same

@M4tteoP
Copy link
Member Author

M4tteoP commented Nov 3, 2023

Elaborating better the following point:

The issue currently is that in the default configuration, we provide:

  • BodyLimit: 13107200 (12,5Mib)
  • Memorylimit: 131072 (128 Kib)

Sending a request >Memorylimit leads to reaching the memoryLimit reached while writing error followed by Failed to write request body. It ends up with return types.ActionContinue. Therefore, lots of logs are generated, but no action is enforced even if by default we have SecRequestBodyLimitAction Reject.

I think that:

  • It is something that we could also address upstream by enforcing the two values to be the same if no FS can be used, but first of all, we should stick with the recommendations that are about setting TinyGo MemoryLimit equal to BodyLimit.
  • We have to find a proper value to be set for coraza-proxy-wasm in the default configuration. If expanding Memorylimit up to the body limit 13107200 (12,5Mib) is too much, I think that also vice-versa (with the default config Reject any request bigger than 128Kib) is too low

@M4tteoP
Copy link
Member Author

M4tteoP commented Nov 21, 2023

I aligned the limits to the lower threshold: both BodyLimit and Memorylimit are now 131072 (this is effectively the level we are already enforcing). I also moved the body process policy to ProcessPartial. It should allow to avoid service degradation during the initial integration of Coraza. About it, I extended the Readme warning the users to be fully aware of the configurations they are using and pointing to the CRS resources about tuning a CRS deployment.

@M4tteoP
Copy link
Member Author

M4tteoP commented Nov 23, 2023

PTAL @jcchavezs @anuraaga. Also, because the default config and embedded rules changed, I would release 0.4 once this PR gets merged. Are you okay with it?

@M4tteoP
Copy link
Member Author

M4tteoP commented Nov 24, 2023

Just reworded the two failing tests linking to the broader discussion in the Coraza repo. I'm going to merge and tag! 🚀

@M4tteoP M4tteoP merged commit a56db40 into corazawaf:main Nov 24, 2023
2 checks passed
@M4tteoP M4tteoP deleted the update_to_crs_4_rc2 branch December 22, 2023 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants