JC 12:02:32 UTC
Aloha, are we having the montly meeting?
Juan Pablo Tosso 12:08:51 UTC
hello everyone ! Sorry for the delay 😅
Juan Pablo Tosso 12:09:10 UTC
Welcome to the monthly meeting agenda corazawaf/coraza#388
Juan Pablo Tosso 12:09:21 UTC
There are not many topics to discuss today, but we can go deep into them
Juan Pablo Tosso 12:10:16 UTC
First of all I want to greet again @Anuraag Agrawal and @JC who are now part of. the core team
Juan Pablo Tosso 12:11:16 UTC
Also I would like to specially notice @Anuraag Agrawal great work on the immutability pattern implementation corazawaf/coraza#397
Juan Pablo Tosso 12:12:07 UTC
In the past month, we have merged more than 30 PRs, mostly enhancing performance and fixing bugs
Juan Pablo Tosso 12:12:41 UTC
Great performances for libinjection-go too!
Juan Pablo Tosso 12:13:38 UTC
Regarding libcoraza (C exports), we have had issues with the log callback, it’s working better now, but we still have to update some headers in order to make it work
Juan Pablo Tosso 12:14:07 UTC
Coraza for nginx is compiling, but we have to write a function to “clone” waf instances in order to make it execute rules
↳ JC 12:15:26 UTC
This can be done with the new immutable API
↳ Juan Pablo Tosso 12:15:50 UTC
Yes, we should document the specific path though
Juan Pablo Tosso 12:14:54 UTC
that’s because in nginx a location coraza instance should inherit rules from the server coraza instance
Juan Pablo Tosso 12:15:22 UTC
Our first topic for this meeting is: corazawaf/community#1
Juan Pablo Tosso 12:16:38 UTC
Thanks @JC for the draft, this is a great opportunity to discus what you expect on the community guidelines
Juan Pablo Tosso 12:17:33 UTC
Personally I think we should keep some level of flexibility in order to avoid entering complex company bureaucracies for our contributors and users
Juan Pablo Tosso 12:18:10 UTC
But I also think we must keep a zero trust approach on how we handle our resources as a cybersecurity project
JC 12:19:12 UTC
Agree. Notice this document documents practices that are already in place tacitly. The main change is the introduction of github issues for certain decisions but that is mainly because we make decisions over slack and as a free plan we have no retention.
Anuraag Agrawal 12:20:08 UTC
Don’t think it’s free plan 😄
JC 12:20:36 UTC
Nice finding @Anuraag Agrawal I somehow assumed it was free because OSS org.
Juan Pablo Tosso 12:21:12 UTC
I think slack can be used to maintain important decisions but we would have to implement some good practices like pinning and creating bookmarks
↳ Juan Pablo Tosso 12:27:03 UTC
Btw I prefer github discussions, but nobody uses them
JC 12:21:17 UTC
Still no way to point to specific decision made in the slack channel. Probably we need to make that visible somewhere.
Juan Pablo Tosso 12:22:09 UTC
Usually what we do during meetings is we wait for a few votes in a message. We don’t wait for enough votes, we just wait for counter-arguments
Juan Pablo Tosso 12:22:58 UTC
Decisions can be taking during meetings because everyone was notified that there is an ongoing meeting, but what about decisions outside meetings
JC 12:23:29 UTC
Yep, also what about counter-arguments post meeting? maybe a github issue istn’t a terrible idea?
Juan Pablo Tosso 12:23:57 UTC
Of course, all proposals should have a github issue
Juan Pablo Tosso 12:24:37 UTC
The thing is how do we make decisions fast enough
Juan Pablo Tosso 12:24:51 UTC
during v3 development decisions has to be made fast until we reach a stable point
Juan Pablo Tosso 12:25:42 UTC
That’s why I think we should have a decision making protocol for stable and for development
Juan Pablo Tosso 12:27:03 UTC
Btw I prefer github discussions, but nobody uses them
↳ Juan Pablo Tosso 12:27:03 UTC
Btw I prefer github discussions, but nobody uses them
JC 12:29:24 UTC
I think it should be linked to our values. For me there are only two important decisiones: those which can be resolved by 2-3 team members and can be easily reverted if general concern (e.g. PRs) and those which has to be decided by majority (which usually take time to digest) and reverting them would be harmful for the project: e.g. membership
JC 12:30:15 UTC
For the former one trust is the key word, I trust you guys will make a good decision in simple changes and you would ping the right people and wait for feedback for those complex ones.
JC 12:30:31 UTC
Without trust, a community is hard to build.
JC 12:31:08 UTC
The other ones should be decoupled from trust and more stuck to rules. 2 people can decide a new membership, we need rules for that.
JC 12:31:51 UTC
So for trust based decisions, we don’t have formal process more than common sense. For rule base decisions we have formal process and rules.
Juan Pablo Tosso 12:32:41 UTC
Yes, and we are our own moderators, that’s why the approvals system
Juan Pablo Tosso 12:33:15 UTC
Regarding membership, I think we should have unanimous consensus from the core team
Juan Pablo Tosso 12:33:45 UTC
Not explicit approvals from everyone, but also no rejections
JC 12:34:07 UTC
That is good but then we need a deadline because we can’t wait for too long.
Juan Pablo Tosso 12:34:23 UTC
50% team + 1 votes
Juan Pablo Tosso 12:34:39 UTC
We are already 6, we can easily get 4 votes
JC 12:35:17 UTC
Yeah, that is good.
Juan Pablo Tosso 12:37:01 UTC
Any other comments on the community doc? I think we will be discussing it for a few weeks in its repository
Juan Pablo Tosso 12:37:16 UTC
Feel free to comment corazawaf/community#2
JC 12:37:45 UTC
Yeah please, let’s have an active discussion for this. It’s going to be the first version and hopefully the first of many.
Juan Pablo Tosso 12:38:42 UTC
Ok! So @Anuraag Agrawal after thousands of changes has managed to merge immutability
Juan Pablo Tosso 12:39:11 UTC
My first impressions after running tests and benchmarking is “its working”
Juan Pablo Tosso 12:39:34 UTC
now we have to validate that plugins can still be added for actions, operators, transformations and directives
Juan Pablo Tosso 12:40:02 UTC
Also we have to review important use-cases like libcoraza, coraza-caddy and coraza-nginx
Juan Pablo Tosso 12:40:22 UTC
I think this minimal api exposure is brilliant, and it will make development much easier
Juan Pablo Tosso 12:40:52 UTC
We will also have to update coraza.io 😜 I will upload my previous v3 branch
Juan Pablo Tosso 12:41:34 UTC
Other thing we have to handle is quality control, we are currently not running pre-commit so we need to make sure we add validations for the old controls
↳ JC 12:42:13 UTC
Where?
↳ Juan Pablo Tosso 12:52:32 UTC
Github actions, I think our current lint validations are not working fine. I will write an issue
Juan Pablo Tosso 12:41:57 UTC
- id: go-fmt - id: go-vet - id: go-lint - id: go-imports - id: golangci-lint - id: go-critic - id: go-unit-tests - id: go-mod-tidy
Anuraag Agrawal 12:42:28 UTC
These should all be checked by go run mage.go check now
↳ Juan Pablo Tosso 12:43:01 UTC
is it part of CI?
↳ Anuraag Agrawal 12:43:06 UTC
Yeah
↳ Juan Pablo Tosso 12:43:21 UTC
I will take a look, I think we are missing some validations
↳ Juan Pablo Tosso 12:43:32 UTC
we should have a lot of errors from some declarations
↳ Anuraag Agrawal 12:44:01 UTC
We might need to check the lint config, it doesn’t include cyclo right now
https://github.com/corazawaf/coraza/blob/v3/dev/.golangci.yml
↳ Juan Pablo Tosso 12:44:18 UTC
Cyclo is problematic because of the modsecurity C ports
Juan Pablo Tosso 12:42:37 UTC
Also we need https://goreportcard.com/report/github.com/corazawaf/coraza/v3 A+ 100% in everything
↳ JC 12:43:25 UTC
Mind filling an issue?
↳ Juan Pablo Tosso 12:43:43 UTC
Of course
Juan Pablo Tosso 12:45:00 UTC
Ok, we might have enough time to review everything
Juan Pablo Tosso 12:45:08 UTC
any other comments or questions on immutability?
Juan Pablo Tosso 12:45:37 UTC
Ok moving forward
Juan Pablo Tosso 12:45:43 UTC
What about GraphQL support?
Juan Pablo Tosso 12:45:48 UTC
Should it be part of the core?
Juan Pablo Tosso 12:46:12 UTC
JC 12:46:28 UTC
By core you mean coraza repo?
Juan Pablo Tosso 12:46:31 UTC
yes
JC 12:47:18 UTC
I think not. library should not take into account specifics IMHO. We can always write a connector/adapter.
JC 12:47:59 UTC
Also, and most importantly, who requested this? I see it is mainly listed in monthly meeting agenda.
Roshan Piyush 12:48:24 UTC
Yeah. Even corerulset tried to move all specifics to plugins.
Juan Pablo Tosso 12:48:36 UTC
I personally think we should add some features just to differentiate from modsecurity, nothing specific
Juan Pablo Tosso 12:48:42 UTC
But it can be implemented as a plugin
Juan Pablo Tosso 12:49:15 UTC
cool
Juan Pablo Tosso 12:49:18 UTC
Regarding persistence
Juan Pablo Tosso 12:49:30 UTC
we have had this discussion with @fzipitria for more than a year
Juan Pablo Tosso 12:49:44 UTC
modsecurity persistence doesn’t work, it’s a walking race condition
Juan Pablo Tosso 12:50:00 UTC
we still get a lot of questions regarding DDOS though
Juan Pablo Tosso 12:50:37 UTC
should we consider persistence as part of the project? or should we directly implement plugins to perform actions like ddos protection
Juan Pablo Tosso 12:50:57 UTC
the other persistence feature is user tracking, you can assign risk scores to users based on their IP or session ID
Juan Pablo Tosso 12:51:38 UTC
it seems like there is not much interest in the community, people just want to run CRS 😅
JC 12:53:12 UTC
I think DDoS is important but given the current capacity and focus it might be part of 3.1 maybe?
JC 12:53:51 UTC
For example, when it comes to wasm filter there is very likely some approaches and work additional to support whatever persistance we design.
Juan Pablo Tosso 12:54:01 UTC
lgtm, we must make sure to keep the persistent variables (like User, Session) open so we don’t have to make breaking changes
Roshan Piyush 12:54:20 UTC
You need the give users the taste before they demand for such features
JC 12:54:53 UTC
I mean, we are interested on it (tetrate is a contributor and also a user) but no time to look into that now.
Juan Pablo Tosso 12:55:23 UTC
cool, let’s keep the feature waiting
JC 12:55:23 UTC
and definitively no time to look at the implicances of it in WASM
Juan Pablo Tosso 12:55:48 UTC
Yes, we also need a default persistence engine, that could be redis or something like that. And it might break tinygo compatibility
Juan Pablo Tosso 12:56:17 UTC
Ok last topic and a specific request from @Anuraag Agrawal
Juan Pablo Tosso 12:56:37 UTC
Let’s switch to only squash commits
Juan Pablo Tosso 12:57:13 UTC
It seems like otherwise, it breaks history for some PRs?
Anuraag Agrawal 12:57:36 UTC
It makes cherrypicking or picking a point in time in the repo’s history difficult mostly
JC 12:58:14 UTC
Also polutes the history with: wip update file xxxx wip 2 chore: lint etc etc
Juan Pablo Tosso 12:58:40 UTC
great, lgtm, lets enforce squash commits
Anuraag Agrawal 12:58:45 UTC
Unfortunately I’m not sure there is an org-wide setting for it so we would need to tweak it in each repo IIUC
Juan Pablo Tosso 12:58:55 UTC
we will take a look at the repo configuration
Juan Pablo Tosso 12:59:11 UTC
Ok so we are done
Juan Pablo Tosso 12:59:14 UTC
any other thing to discuss?
Matteo Pace 12:59:41 UTC
If you guys have still some minutes, could we elaborate a little the default behaviour of request body processing (corazawaf/coraza#438)?
↳ Juan Pablo Tosso 13:00:02 UTC
sure, so this is a behavior inherited from modsecurity
↳ Juan Pablo Tosso 13:00:12 UTC
It’s based on performance
↳ Juan Pablo Tosso 13:00:40 UTC
We only explicitly evaluate request bodies if the configuration matches
↳ Juan Pablo Tosso 13:01:03 UTC
also some body processors might fill REQUEST_BODY and others won’t
↳ Juan Pablo Tosso 13:01:22 UTC
you can enforce REQUEST_BODY in JSON by using the force request body ctl
↳ Matteo Pace 13:01:58 UTC
What sounds odds to me is that the same payload with text/plain or any other manipulated content-type will just bypass the waf
↳ Juan Pablo Tosso 13:02:19 UTC
you might force processing on text/plain
↳ Matteo Pace 13:02:29 UTC
This is basically the difference between what ModSec v2 and v3 (REQUEST_BODY is always populated) do
↳ Juan Pablo Tosso 13:03:11 UTC
SecRule REQUEST_HEADERS:content-type “!@rx ……” “…ctl:forceRequestBodyVariable=On,ctl:setReqBodyProcessor=urlencoded”
↳ JC 13:03:21 UTC
Jus tone thing here: in https://github.com/corazawaf/coraza/pull/441/files#diff-63ab601287b8b9c040760fe0bdd288f55b73f37cd7e4f1e519bea2bd43a18bbaL568 we say // We force URLENCODED if mime is x-www... or we have an empty RBP and ForceRequestBodyVariable but that is not what code does.
↳ Matteo Pace 13:03:32 UTC
From a user perspective I would expect that by enabling SecRequestBodyAccess the bodies are indeed accessed and analysed. Just as a second step I would fine tune my waf in order to improve performance, reduce dos problems and so on
↳ Juan Pablo Tosso 13:04:04 UTC
lgtm, we can make that change. We must just try to maintain regression with CRS
↳ Juan Pablo Tosso 13:04:37 UTC
but this is a huge problem for most wafs, look at AWS
↳ Matteo Pace 13:04:48 UTC
SecRule REQUEST_HEADERS:content-type “!@rx ……” “…ctl:forceRequestBodyVariable=On,ctl:setReqBodyProcessor=urlencoded”Yep, I see, thanks. It was more about what, as a user, I would expect as the default behaviour. Besides that we can actually enfroce it with your example
↳ Juan Pablo Tosso 13:04:48 UTC
https://kloudle.com/blog/the-infamous-8kb-aws-waf-request-body-inspection-limitation
↳ Juan Pablo Tosso 13:06:00 UTC
Ok lets evaluate in your issue if we can implement this without breaking default CRS compatibility
↳ Matteo Pace 13:06:06 UTC
It’s a spiky problem 😅
↳ Juan Pablo Tosso 13:06:26 UTC
but it does make sense
↳ Juan Pablo Tosso 13:06:56 UTC
btw in modsecurity 2 it always populates the body because it’s attached to the apache buffer, in libmodsecurity it’s not
↳ Juan Pablo Tosso 13:07:20 UTC
body is a copy of the buffer
↳ Matteo Pace 13:09:45 UTC
Just tested yesterday against libmodsec and in the scenario both text/plain and malformed content types have been analyzed :thinking_face:
↳ Matteo Pace 13:11:12 UTC
https://github.com/SpiderLabs/ModSecurity/wiki/Reference-Manual-(v3.x)#ctl:~:text=REQUEST_BODY%20is%20always%20populated%20in%20v3 (but well, no surprise if the doc is outdated)
↳ Matteo Pace 13:13:29 UTC
The point is, the only way I see to do not give a false sense of security is by default parsing all the bodies. But the solution ends up to be not okay for other reasons
↳ Juan Pablo Tosso 13:16:56 UTC
I remember the documentation was outdated and I based my results in my labs and crs integration
↳ Juan Pablo Tosso 13:17:05 UTC
But let’s make your changes and see what happens
↳ Matteo Pace 13:19:55 UTC
Let’s see! Thanks for you time @Juan Pablo Tosso, see you on Github!
Juan Pablo Tosso 13:01:55 UTC
Ok thank you everyone for joining !