feat: ✨ Add caddy client_ip variable parsing #184
Conversation
|
@fzipi |
|
@fzipi |
adrienyhuel
force-pushed
the
fix-client-ip
branch
3 times, most recently
from
c95c1bc to
2fe46bc
Compare
|
I think it is way more clear now. @M4tteoP can you help me with the review? |
adrienyhuel
force-pushed
the
fix-client-ip
branch
from
2badad2 to
50c5701
Compare
|
Thanks for this @adrienyhuel. I think the unit tests here aren't enough to cover this. Any chance you can create an integration test using |
|
@jcchavezs I added a simple integration test, is it sufficient ? |
|
Please fix the lint and we can merge it. |
|
@jcchavezs my bad, I always forget to lint as my IDE always replace tabs with 4 spaces... |
|
Maybe adding an |
| @@ -3,6 +3,9 @@ | |||
| debug | |||
| auto_https off | |||
| order coraza_waf first | |||
| servers { | |||
| trusted_proxies static private_ranges | |||
There was a problem hiding this comment.
Would it be possible to add a comment on why this line is needed? Also, is it then something that is strictly required to have the client_ip properly working? If so, should we document it in the readme: https://github.com/corazawaf/coraza-caddy?tab=readme-ov-file#plugin-syntax?
There was a problem hiding this comment.
@M4tteoP
This line is needed is you want Caddy to populate client_ip from X-Forwarded-For header, in the case Caddy is behind a reverse proxy.
If you don't provide this config, Caddy will populate client_ip from request.RemoteAddr :
- If client connect directly, it will be the real IP
- if client connect through another proxy (like Cloudflare, or an Ingress in K8s), it will the proxy IP
See : https://caddyserver.com/docs/caddyfile/options#trusted-proxies
I think all users who want real client IP in their access logs know that config.
There was a problem hiding this comment.
Awesome, thanks for the explanation!
There was a problem hiding this comment.
Btw, with this pull request, coraza-caddy will use client_ip variable provided by caddy to use in WAF rules and in WAF logs, regardless of how Caddy populate this variable.
There was a problem hiding this comment.
@adrienyhuel please add this explanation as a comment in the config.
There was a problem hiding this comment.
@jcchavezs done :)
|
Resolve #150