Export http helper functions

[redradrat]

The http handler wrapper is great, but when compiling a custom transaction flow doesn't allow access to ProcessRequest and ProcessResponse anymore. The wrapper itself is cool, but in a lot of cases access to the actual transaction flow would be required. An example would be taking actions if an interruption occurred. e.g. producing metrics, notifying something, etc.

So in general to have an easier time writing a custom, wrapper-like flow with the nice Process helpers, would be great to have them exported again.

Thank you for contributing to Coraza WAF, your effort is greatly appreciated Before submitting check if what you want to add to coraza list meets quality standards before sending pull request. Thanks!

Make sure that you've checked the boxes below before you submit PR:

• [ ] My code includes positive and negative tests.
• [ ] I have an appropriate description with correct grammar.
• [ ] I have read Contribution guidelines, maintainers note and Quality standard.
• [ ] My code is properly linted and passes pre-commit tests.

Thanks for your contribution :heart:

[codecov[bot]]

Codecov Report

Patch coverage: 87.50% and no project coverage change.

Comparison is base (6f11f53) 81.85% compared to head (4bf02c7) 81.85%.

:exclamation: Current head 4bf02c7 differs from pull request most recent head 12a90ae. Consider uploading reports for the commit 12a90ae to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##           v3/dev     #774   +/-   ##
=======================================
  Coverage   81.85%   81.85%           
=======================================
  Files         153      153           
  Lines        8188     8188           
=======================================
  Hits         6702     6702           
  Misses       1267     1267           
  Partials      219      219           

Flag	Coverage Δ
default	78.08% <75.00%> (ø)
examples	25.99% <75.00%> (ø)
ftw	49.09% <37.50%> (ø)
ftw-multiphase	49.21% <37.50%> (ø)
tinygo	77.23% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
http/middleware.go	84.95% <83.33%> (ø)
http/interceptor.go	51.28% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[redradrat]

@jcchavezs really our first use case would have been to inject logic when an interruption was triggered. The simple example would be to report what happened with a prometheus gauge for example. I can adapt the PR to utilise functions as args for WrapHandler maybe?

From my perspective the point here is, that coraza is a WAF framework, flexible for integration no? So having hooks at points within the http handler wrapper would make sense to inject logic, instead of replicating the handler.

[jcchavezs]

Yeah exactly, having hook functions would do the trick (see a similar pattern in https://github.com/openzipkin/zipkin-go/blob/master/middleware/http/server.go#L78). We could have one to deal with the interruption, something like func(types.Transaction, types.Interruption) so people can access the transaction and the interruption (although you can access the interruption from the transaction). In the future I also see ourselves writing one which would access the request and the transaction but let's hold it until it is the right time. The wrap method should accept a Options struct.

On Wed, 12 Apr 2023, 15:24 Ralph Kühnert, @.***> wrote:

@jcchavezs https://github.com/jcchavezs really our first use case would have been to inject logic when an interruption was triggered. The simple example would be to report what happened with a prometheus gauge for example. I can adapt the PR to utilise functions as args for WrapHandler maybe?

From my perspective the point here is, that coraza is a WAF framework, flexible for integration no? So having hooks at points within the http handler wrapper would make sense to inject logic, instead of replicating the handler.

— Reply to this email directly, view it on GitHub https://github.com/corazawaf/coraza/pull/774#issuecomment-1505272487, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXOYAVSHSAGQPZGUGQCU4DXA2UHZANCNFSM6AAAAAAW3MDAZ4 . You are receiving this because you were mentioned.Message ID: @.***>

[redradrat]

awesome... thanks for your thoughts. I'll give it a shot @jcchavezs

[jcchavezs]

Giving a second thought maybe we can accept a observer interface which would report metrics for this? Something like

type Observer interface {
  // onRequest is invoked when a request is started
  func onRequest(*http.Request, types.Transaction)
  // onInterruption is invoked when a request is interrupted
  func onInterruption(*http.Request, types.Transaction)
}

Ping @anuraaga

[anuraaga]

Those hooks seem to make sense to me - I'm a bit hesitant on exposing Transaction though as it would give a lot of surface for footgunning. Ideally the information passed to the hook is something immutable - unless we know of use cases that need mutability in hooks?

[jcchavezs]

Good point @anuraaga. Maybe something like TransactionState is what we are looking for? Or just the ID?

On Thu, 13 Apr 2023, 05:01 Anuraag Agrawal, @.***> wrote:

Those hooks seem to make sense to me - I'm a bit hesitant on exposing Transaction though as it would give a lot of surface for footgunning. Ideally the information passed to the hook is something immutable - unless we know of use cases that need mutability in hooks?

— Reply to this email directly, view it on GitHub https://github.com/corazawaf/coraza/pull/774#issuecomment-1506262503, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXOYASSQNKSKBTEBADH4STXA5UBBANCNFSM6AAAAAAW3MDAZ4 . You are receiving this because you were mentioned.Message ID: @.***>

[anuraaga]

TransactionState is also mutable - basically we currently have Transaction as a view for use in middleware and TransactionState as a view for use in plugins. This would be more of a view for use in management or something like that - maybe having yet another view is overengineering though 🤔

[jcchavezs]

Yeah, after looking at the transaction we might not need to access the transaction at all? I was thinking that transaction ID would be useful for logging purposes but you can achieve the same with seclang itself and when it comes to metrics transaction ID is high cardinality so not worth I think (feedback is welcome) so I would just skip transaction for now.

On Thu, 13 Apr 2023, 07:11 Anuraag Agrawal, @.***> wrote:

TransactionState is also mutable - basically we currently have Transaction as a view for use in middleware and TransactionState as a view for use in plugins. This would be more of a view for use in management or something like that - maybe having yet another view is overengineering though 🤔

— Reply to this email directly, view it on GitHub https://github.com/corazawaf/coraza/pull/774#issuecomment-1506355623, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXOYAXQ7RPZCO353HJPHJTXA6DGTANCNFSM6AAAAAAW3MDAZ4 . You are receiving this because you were mentioned.Message ID: @.***>

[jcchavezs]

@redradrat so something like

type TransactionCallback interface {
  // onRequest is invoked when a request is started
  func onRequest(*http.Request)

  // onInterruption is invoked when a request is interrupted
  func onInterruption(*http.Request, *types.Interruption)
}

might do the trick. I wonder if matched rules (hold by transaction) is something we should also pass cc @piyushroshan and finally if we should include the phase in onInterruption method like we do in coraza-proxy-wasm. See https://github.com/corazawaf/coraza-proxy-wasm/pull/29/files#diff-368a9e52513f8700cc50c424de862c537fc5c1a492c4598b97b960f46fc1c877R38

Anyways, any feedback on the metrics needed would be great.

Ping @RedXanadu as he might have insightful feedback.

[jcchavezs]

Any movement on this @redradrat ?

[redradrat]

Hey @jcchavezs. Actually, yes. We did quite some rewriting on the http package internally, I'll be cleaning that up to open another PR here. Hope you agree to the changes. We had to split up the Wrapper into a request and a response section because the interceptor would just not work for us on response... Long story short, I'm still up for pushing sthg here :)

[fzipi]

@jcchavezs @redradrat so... what's next here?