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

feat: implement coraza_rules_add and coraza_rules_add_file #27

Closed

Conversation

potats0
Copy link
Contributor

@potats0 potats0 commented Jun 28, 2023

No description provided.

Copy link

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

As Coraza v3 had many significant API changes, it looks like libcoraza C ABI needs to be updated to match it better. Rather than trying to keep the existing one using internal mechanisms, it could be updated to something like

//export coraza_new_waf_config - points to a WAFConfig
//export coraza_waf_config_add_rule - works on WAFConfig, and any other config knobs should be exposed
//export coraza_new_waf - accepts WAFConfig and returns a fully initialized WAF

@potats0
Copy link
Contributor Author

potats0 commented Jun 28, 2023

As Coraza v3 had many significant API changes, it looks like libcoraza C ABI needs to be updated to match it better. Rather than trying to keep the existing one using internal mechanisms, it could be updated to something like

//export coraza_new_waf_config - points to a WAFConfig
//export coraza_waf_config_add_rule - works on WAFConfig, and any other config knobs should be exposed
//export coraza_new_waf - accepts WAFConfig and returns a fully initialized WAF

How can I find the API documentation for Coraza v3?

@anuraaga
Copy link

The best place to look is probably the Go pkg docs

https://pkg.go.dev/github.com/corazawaf/coraza/v3

@potats0
Copy link
Contributor Author

potats0 commented Jun 28, 2023

If the coraza_waf_config_add_rule returns a config, we can't add/remove rule dynamically in anytime.

@anuraaga
Copy link

Coraza v3 currently doesn't support dynamic rule updates so it should be OK to remove support in connectors as well at least for now. Adding back, we could discuss in coraza repo that real-world use cases and whether dynamic updates are the best option, vs e.g. recreating waf instance etc. I think for now the primary goal of this repo should be to get updated to expose the current coraza v3.

@potats0
Copy link
Contributor Author

potats0 commented Jun 28, 2023

The best place to look is probably the Go pkg docs

https://pkg.go.dev/github.com/corazawaf/coraza/v3

If I use those api , how should I do that add/remove rule dynamically in anytime? In apisix, we must implement add rule in other phase not init_by_lua phrase.

@anuraaga
Copy link

Perhaps it can be a lazy singleton that's initialized once in the phase you need to? It sounds like an issue of how / when to initialize the WAF as a whole, not individual rules dynamically.

@potats0
Copy link
Contributor Author

potats0 commented Jun 28, 2023

recreating waf isn't acceptable. real-world case

https://blog.openresty.com/en/edge-enable-waf/

image

@anuraaga
Copy link

recreating waf isn't acceptable

Rules shouldn't be getting updated in a high traffic manner, there's a UI or API that may be used sometimes for it - it should be acceptable to reinitialize when a configuration update happens, right? This is a pretty common paradigm I think.

There's far more to WAF config than rules, so if we really wanted to support dynamic updates, it would be a larger API adjustment to Coraza, which would need to be understood vs the option of just reinitializing the WAF.

@potats0
Copy link
Contributor Author

potats0 commented Jun 28, 2023

libcoraza can’t be portable to openresty/nginx if not reconstruct the api. Restarting nginx may cause a disruption. Should i recreate a repo that is named libcoraza-nginx ?

@anuraaga
Copy link

Yeah it seems that currently the goal is to implement nginx logic with coraza rather than create a generically usable C library exposing Coraza - having a separate repo, and then going wild with whatever ABI is required, including things like the string wrapping, will be feasible. What do you think @jcchavezs?

@potats0 potats0 closed this Jun 28, 2023
@potats0 potats0 deleted the implement_coraza_rules_add_file branch June 28, 2023 08:23
@jcchavezs
Copy link
Member

Yeah it makes sense @anuraaga. Writing a general purpose library is tough and requires experience, currently all this changes albeit legit and beneficial are biased towards openresty so my suggestion would be to make something work for openresty, nginx and Apache and then come up with food abstractions.

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.

4 participants