chore: improves coraza.conf-recommended comments
#1334
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
M4tteoP
force-pushed
the
recommended_keep_it_real
branch
from
fb293c1 to
58bb175
Compare
M4tteoP
commented
Mar 23, 2025
Comment on lines
-38
to
+48
| # file uploads then the value given on the first line has to be as large | ||
| # as the largest file you are willing to accept. The second value refers | ||
| # to the size of data, with files excluded. You want to keep that value as | ||
| # low as practical. | ||
| # | ||
| # file uploads, this value must has to be as large as the largest file | ||
| # you are willing to accept. | ||
| SecRequestBodyLimit 13107200 | ||
|
|
||
| # Maximum request body size that Coraza will store in memory. If the body | ||
| # size exceeds this value, it will be saved to a temporary file on disk. | ||
| SecRequestBodyInMemoryLimit 131072 | ||
|
|
||
| # SecRequestBodyNoFilesLimit is currently not supported by Coraza | ||
| # Maximum request body size we will accept for buffering, with files excluded. | ||
| # You want to keep that value as low as practical. | ||
| # Note: SecRequestBodyNoFilesLimit is currently NOT supported by Coraza |
There was a problem hiding this comment.
first line,second valueis not clear at all. I think it was referring toSecRequestBodyNoFilesLimitbut then we addedSecRequestBodyInMemoryLimitin between. I split this comment into one comment per directive.SecRequestBodyInMemoryLimitwas uncommented
M4tteoP
commented
Mar 23, 2025
Comment on lines
+55
to
+56
| # Warning: Setting this directive to ProcessPartial introduces a potential bypass | ||
| # risk, as attackers could prepend junk data equal to or greater than the inspected body size. |
There was a problem hiding this comment.
I feel it is worth mentioning this attack here, this is something to consider when setting this directive. See https://github.com/assetnote/nowafpls
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1334 +/- ##
=======================================
Coverage 81.99% 81.99%
=======================================
Files 170 170
Lines 9803 9803
=======================================
Hits 8038 8038
Misses 1518 1518
Partials 247 247
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
M4tteoP
commented
Mar 23, 2025
Comment on lines
-118
to
+125
| # By default, only keep the files that were determined to be unusual | ||
| # in some way (by an external inspection script). For this to work you | ||
| # will also need at least one file inspection rule. | ||
| # If On, the WAF will store the uploaded files in the SecUploadDir | ||
| # directory. | ||
| # Note: SecUploadKeepFiles is currently NOT supported by Coraza | ||
| # | ||
| #SecUploadKeepFiles RelevantOnly | ||
| #SecUploadKeepFiles Off |
There was a problem hiding this comment.
Matching reality here. See #1205
M4tteoP
commented
Mar 23, 2025
|
|
||
| # Uploaded files are by default created with permissions that do not allow | ||
| # any other user to access them. You may need to relax that if you want to | ||
| # interface Coraza to an external program (e.g., an anti-virus). | ||
| # Note: SecUploadFileMode is currently NOT supported by Coraza |
There was a problem hiding this comment.
We are calling os.CreateTemp(storagePath, "crzmp*") , which is coming with hardcoded 0600. So we are reading this value, not effectively using it.
M4tteoP
commented
Mar 23, 2025
Comment on lines
-141
to
-142
| # Most logging has not been implemented because it will be replaced with | ||
| # advanced rule profiling options |
There was a problem hiding this comment.
It is a pretty old comment. We should maybe revisit all the logs printed in the various levels, but we have some logs for each of them, and I don't think there is some traction for advanced rule profiling options. I would remove these lines and not confuse the user reading them
M4tteoP
commented
Mar 23, 2025
| @@ -153,7 +156,7 @@ SecDataDir /tmp/ | |||
| SecAuditEngine RelevantOnly | |||
| SecAuditLogRelevantStatus "^(?:(5|4)(0|1)[0-9])$" | |||
|
|
|||
| # Log everything we know about a transaction. | |||
There was a problem hiding this comment.
ABIJDEFHZ is not even everything
jcchavezs
approved these changes
Mar 25, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Improves
coraza.conf-recommendedcoherency with the current Coraza state, and reduces old copy-pasted messages. See in-line comments for details