Exports IsRuleEngineOff #504
Conversation
There was a problem hiding this comment.
Sometimes it is easy to get a too broad API because of this kind of changes where we expose an internal state and expect clients to deal with the expected behaviour so before we merge this I would expect to exercise this API in the http package and prove what can every value do for us.
|
Also if finally export this we should document what are the expectations for connectors as for example, |
Codecov ReportBase: 78.04% // Head: 78.08% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## v3/dev #504 +/- ##
==========================================
+ Coverage 78.04% 78.08% +0.03%
==========================================
Files 145 145
Lines 6861 6876 +15
==========================================
+ Hits 5355 5369 +14
- Misses 1268 1270 +2
+ Partials 238 237 -1
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. |
|
Thanks @jcchavezs for the review. I see your concern. I tried to properly document the exported methods and the expected use cases. Happy to keep the conversation going, and eventually revert the changes if we feel that there is too much risk of being misused. |
|
I think there are more tweaks that could be applied to middlewares/interceptors based on this. My main concern here would be: do we want to expose |
I agree, I'm thinking about them, but they are tricky to be integrated with the interceptor logic.
I see both values in |
|
Relevant #505 |
|
I think what I am chasing here is: do we want to expose the state or just let connectors know if engine is on/off. I was playing with it in the http middleware and ended up using three states:
I guess only on/off works for proxy/wasm? how about caddy @jptosso ? |
This is Detection only status, isn't it? I'm still wondering about the corner case of a rule triggered in |
|
If that is the case then we definitively expose a binary value
`EngineRuleOff` or something like that as that is all conectors can know
about whitout dealing with internal logic cc @jptosso @fzipi
…On Tue, 15 Nov 2022, 12:50 Matteo Pace, ***@***.***> wrote:
analysing the request/response body in a separate goroutine (as it won't
change the response)
This is Detection only status, isn't it? I'm still wondering about the
corner case of a rule triggered in detection only with ctl:ruleEngine=On.
In that case, we should consider that Detection only may still end up
with disruptive action, any other early assumption may be wrong.
—
Reply to this email directly, view it on GitHub
<#504 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXOYAQWA32XRXFZTCJU3A3WIN2HDANCNFSM6AAAAAAR5VJMHM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I think it makes sense to expose the value or any method doing the same will do also. |
|
@jcchavezs on Caddy the connector will still fill all variables even if the engine is off. I'm not sure if it makes sense to control that behavior from the connector side, but maybe boolean methods would be more developer-friendly, like |
|
So I guess we only need to expose |
|
Mostly reverted this PR.
The main use case in my mind was about Sorry for the noise and many thanks for the feedback |
|
LGTM
…On Fri, 18 Nov 2022, 13:00 Matteo Pace, ***@***.***> wrote:
Mostly reverted this PR.
- The exported function is now IsEngineRuleOff.
- I revisited the comments about the proper usage of
RequestBodyAccessible() and ResponseBodyAccessible().
The main use case in my mind was about DetectionOnly status that would
have permitted the connectors to send information to Coraza and at the same
time let the request continue. But I came up with that it is a wrong early
assumption and it would impact at least one use case
<#504 (comment)>.
Sorry for the noise and many thanks for the feedback
—
Reply to this email directly, view it on GitHub
<#504 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXOYAWRTGNOLSGAPDTEW2DWI5VWBANCNFSM6AAAAAAR5VJMHM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Do you mind using the condition in the HTTP middleware to return same response writer when engine is off? |
|
Done @jcchavezs, I hope this is what you expected. I felt was right to just keep the transaction generation thinking that Coraza is actually aware that a new request is coming, but then no rules are processed. I also inverted "Rule" and "Engine" inside the function name being everywhere called RuleEngine (e.g. |
|
I think it is not correct as of now. If you return when the engine is off
then you don't even call downstream to do the response. Also worth adding a
test on this.
…On Sat, 19 Nov 2022, 01:11 Matteo Pace, ***@***.***> wrote:
Done @jcchavezs <https://github.com/jcchavezs>, I hope this is what you
expected. I felt was right to just keep the transaction generation thinking
that Coraza is actually aware that a new request is coming, but then no
rules are processed.
I also inverted "Rule" and "Engine" inside the function name being
everywhere called RuleEngine (e.g. SecRuleEngine)
—
Reply to this email directly, view it on GitHub
<#504 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXOYAVFHF5ONSBCBUVOW53WJALJZANCNFSM6AAAAAAR5VJMHM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
My bad. I added the call to downstream to do the response. I'm thinking about anticipating, even more, the check to return the same response writer, but we need the transaction to check for engine off. I added |
Follows #499, Just like
RequestBodyAccessandResponseBodyAccess, connectors could benefit from retrieving the current status of the engine. I also added a note to explicitly tell that it is the current status per-transaction basis in order to be aware of using it without making wrong assumptions.