-
Notifications
You must be signed in to change notification settings - Fork 603
Global Whitelist and JSON output of events/blocks #488
Conversation
…and JSON output of logged events/blocks
Any update on this feature ? |
@mhf-ir You can achieve most of it by configuration changes, other than JSON log output. Having JSON log output would be great, especially to easily parse events in i.e DataDog. See linked issue discussions and my opened issues. To maintainers: Can JSON output be added? |
i would love to merge this, but seems that you broke some tests. |
@wargio @mhf-ir Actually tests are broken since they expect not JSON there: T13 and T28 can be ignored or need to be adjusted.
No idea why T23 does not pass. Will look some more when I have time
These fail as I understood:
@wargio You can definitely add this to your project and somehow make it configurable. Looking for hints from you how to make it as an option: https://github.com/nbs-system/naxsi/pull/488/files#diff-c94873dd7eede49a9d55290d07f66ef1R981 The rest can be achieved by config changes also, so for me it looks kinda optional. Unless users think it is easier via file also, I can then look into it again. Not sure which way could faster and if config option supports netmask/CIDR etc |
@wargio OK, my bad. Indeed this was a bug. Thanks for having tests. Fixed it. Output is now in both Formats (NAXSI_FMT and JSON). So user can see both and tests pass. Not sure if Multiline Logs will display correctly with JSON, if the length is too big, JSON will be empty. Had a hard time with them. Will chew some more on this, unless you see the solution. Please review it and let me know your comments. |
FYI This PR is experimental, but since Tests passed, should be fine. YMMV (Your Mileage May Vary) @wargio Looking for your review and tips how to improve it and maybe merge in master |
@wargio Multi line is pretty complex. Seems like JSON on multiline will not work. Will cap it to 1948 and if it is more, will return empry. Will update PR as I get it. |
@wargio Multiline will not make sense with JSON. So if a multiline event is made, I send empty JSON. You cannot connect JSON across logs lines, this would be very difficult for JSON parsing. I think this will help in most of the cases. |
See this simple tools, naxsi added also much more parsing inside nginx error log. |
Does your solution work also with a Multiline log???? I guess not. Multiline log example:
As you can see they're connected by "seed_start" and "seed_end". I will just return empty JSON with my PR:
My use case is DataDog (which parser cannot connect multiple JSON with "seed_start" and "seed_end" values, this would be also very complex to connect and parse in DataDog) The typical case should look like this (no Multiline log)
|
I'll review this tomorrow. |
@marcinguy No multiline not support i may work on that. one simple solution is increase nginx I donno why is really exist and so small? |
OK, sorry for the huge delay on this. i want to merge this and looks good to me. |
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.
Ok, is missing only a few pointer checks.
Maybe hardcoding a path for the whitelist is not the best idea?
Wanted to just say this. @mhf-ir Interesting ... could be also done. Nice! @wargio Great. One thing to consider is making /etc/nginx/whitelist.txt file name and location configurable via Naxsi config file. Don't know how to do it from the top of my head, would need to dig into this. Don't have time now. It also does not support now CIDR, could be added. Wondering if this would be faster than fast hashmaps, also I can assume NGINX config uses also hashmaps so making this via runtime modifiers, should/could have the same speed. Not sure about it. Definitely fewer lines and stuff is in one place with /etc/nginx/whitelist.txt approach.
|
@wargio Also the output format can be specified also in Naxsi config. If you want JSON or standard etc I think I output now both. So you can pick what you want. |
… to enable JSON output (set $naxsi_json_log 1)
Added Whitelist file as an option in Naxsi configuration file and also a dynamic modifier to enable JSON logging format also in Naxsi configuration file. Can update the Docs later on, also with my DataDog Setup instructions. @wargio |
I mean that you are not checking for null pointers @marcinguy there are some memcpy like functions after that malloc from the pool. |
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.
it looks really good. awesome job!
@wargio Cool. Modified also Wiki a little bit to add those new configuration options. |
* Added global whitelist in /etc/nginx/whitelist.txt (one IP per line) and JSON output of logged events/blocks * Fix in whitelist size * Reformatted to fit original formatting. * Changed loggin to include both formats * Formatting * Fix code * Fix pesky newlines with hashmap * Fix bug with uninitialization * Fix * Cleanup * Whitespaces * Whitespaces * Update naxsi.h * Fixes for failed tests * Fix * Undo and comment * Fixed JSON on Multiline * Correct Makefile * Remove debugs * Added whitelist first as a conf variable (WhitelistFile) and modifier to enable JSON output (set $naxsi_json_log 1) * Reformatting * Resolved comments from @wargio * Resolved change requests from @wargio
Added global whitelist in /etc/nginx/whitelist.txt (one IP per line), meaning these IPs will never be blocked and JSON output of logged events/blocks
Can you check my code edits (beginner to NGINX and NGINX modules).
If this or similar feature could be merged or added to NAXSI, it would be great.