-
-
Notifications
You must be signed in to change notification settings - Fork 219
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
Flatters WAF config API for better flexibility in setting memory limits and enabling BodyAccess #503
Conversation
Sorry for the late review. We want all the settings to be programmatically configurable without any text directives so don't think we can lose that ability for body access. I think we could add an |
d5f2cd9
to
0494ee2
Compare
Codecov ReportBase: 78.21% // Head: 78.27% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## v3/dev #503 +/- ##
==========================================
+ Coverage 78.21% 78.27% +0.06%
==========================================
Files 145 145
Lines 6876 6872 -4
==========================================
+ Hits 5378 5379 +1
+ Misses 1264 1256 -8
- Partials 234 237 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
||
type responseBodyConfig struct { | ||
limit int | ||
inMemoryLimit int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
responseBodyConfig had inMemoryLimit
and WithInMemoryLimit
, but then it was not populated nor called, nor the WAF struct
has a corresponding default value and field for it. It is okay to assume that it was not used, and not needed?
0494ee2
to
a611181
Compare
a611181
to
0c97aff
Compare
Thanks @anuraaga! I removed both |
Thanks! Mostly LGTM |
Sorry for the delay! Addressed the review and updated the title to a more coherent one with the PR evolution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, has a lot of methods, but it's easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In essence, why don't we use uint instead of int? I don't expect sizes to be negative in the future, but maybe someone wants reaaaly big files 😄
|
Another consideration here is that we access to those content types that
are being inspected and only buffer when inspection.
…On Mon, 21 Nov 2022, 18:22 Matteo Pace, ***@***.***> wrote:
1. Internally these limits are defined as int64. I can't see a reason
for negative sizes, but to avoid conversions we may stick with int64 also
for the exported function. I went for this solution in the last pushed
commit.
Otherwise, we can swap everything to unsigned, even if I feel that
int64 is already enough in terms of super big files.
2. Done! Added "Bytes" for all the exported function names.
—
Reply to this email directly, view it on GitHub
<#503 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXOYAR3M64YSCGIY6ZCMUDWJOVUNANCNFSM6AAAAAAR3P365A>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
So, in summary:
Can we document these decisions somewhere else where they can be reached by more people? Also, for future Coraza developers :) |
Yup except for this
We basically assume 64-bit usage where Would be good to document but don't think it's needed for this PR |
Moved from |
In general looks good. But... are we considering the practical implications of an integer overflow here? |
@fzipi that's a legit concern. I feel that the fundamental question is: are 32-bit machines a thing (to be supported)? If so, and |
What I want to point is that assumptions are the "enemy" of security, in general. We make things explicit so we are sure. Making this explicit is about:
Hopefully, it is clear now. |
Can we fix conflicts here @M4tteoP? Thx! |
@@ -100,6 +100,7 @@ type WAF struct { | |||
// UploadDir is the directory where the uploaded files will be stored | |||
UploadDir string | |||
|
|||
// Request body in memory limit excluding the size of any files being transported in the request. | |||
RequestBodyNoFilesLimit int64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think that the value is parsed and stored, but never used. It should be implemented or documented (also in the coraza.conf-recommended)
config.go
Outdated
// int is a signed integer type that is at least 32 bits in size (platform-dependent size). | ||
// While, the theoretical settable upper limit for 32-bit machines is 2GiB, | ||
// it is recommended to keep this value as low as possible. | ||
WithRequestBodyBytesLimit(limit int) WAFConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need the work Bytes
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have strong feelings about it. It makes the function more explicit but makes the name even more verbose. It has been added addressing a previous review #503 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think given the directive is SecRequestBodyLimit it is better to align with it, also Bytes seem redundant, specifying the units is something that I like when in doubt but in this case and in general in golang ecosystem, bytes is the unit for bodies.
…ts and enabling BodyAccess (corazawaf#503) * Removes BodyAccess by default when setting WithBodyAccess * flatters config removing RequestBodyConfig and ResponseBodyConfig * sets default waf config limits to -1 * changes to int64, renames exported functions names * address review clarifying config variable initialization * moves memory limits from int64 to int * fix responseBodyLimit * wip merge * wip merge * makes UnsetLimit private * address review, removes world Byte
Following #499, I'm working on avoiding body-related actions when the bodies themself are not accessible (see corazawaf/coraza-proxy-wasm#76). I'm facing a problem with
WithRequestBodyAccess
because it always enforces the body access, not taking into account any related user directive.This PR proposes to remove the enforcement.
WithRequestBodyAccess
andWithResponseBodyAccess
would become a way to set memory limits, demanding to the traditional directives (SecRequestBodyAccess
andSecResponseBodyAccess
) the decision of enabling or not the body access.I see value in slitting the enforcement from the configuration also considering actions like
ctl:requestBodyAccess
: for specific transactions the body could become accessible and would be okay to already have customized configurations in place if it happens.Thanks for the feedback and alternative ideas/proposals!
P.S:
WithRequestBodyAccess
andWithResponseBodyAccess
names would become not completely consistent, should I change them into something likeWithRequestBodyConfig
even if it will break implementations?