[pve-devel] [PATCH http-server 1/1] fix #5699: pveproxy: add library methods for real IP support

[Fabian Grünbichler]

> Thomas Lamprecht <t.lamprecht at proxmox.com> hat am 25.11.2024 09:31 CET geschrieben:
> 
>  
> Am 25.11.24 um 00:49 schrieb Thomas Skinner:
> >> On September 10, 2024 2:30 am, Thomas Skinner wrote:
> >>> ---
> >>>  src/PVE/APIServer/AnyEvent.pm | 43 ++++++++++++++++++++++++++++++++---
> >>>  src/PVE/APIServer/Utils.pm    | 15 ++++++++++++
> >>>  2 files changed, 55 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
> >>> index a8d60c1..c2afb4d 100644
> >>> --- a/src/PVE/APIServer/AnyEvent.pm
> >>> +++ b/src/PVE/APIServer/AnyEvent.pm
> >>> @@ -85,20 +85,21 @@ sub log_request {
> >>>  
> >>>      my $loginfo = $reqstate->{log};
> >>>  
> >>> -    # like apache2 common log format
> >>> -    # LogFormat "%h %l %u %t \"%r\" %>s %b \"%{Referer}i\" \"%{User-agent}i\""
> >>> +    # like apache2 common log format + client IP address
> >>> +    # LogFormat "%a %h %l %u %t \"%r\" %>s %b \"%{Referer}i\" \"%{User-agent}i\""
> >>
> >> this has the potential to break a lot of things analysing this log
> >> file.. would it make sense to only do it conditionally (if a "real IP" is set)?
> >>
> >> or *just* log the "real IP" instead of the peerip (which would be
> >> backwards-compatible as well)?
> > 
> > I agree that it would break existing log analyzers. I also believe it's important
> > to have the option to log the peer/proxy IP as well (in the event there are multiple
> > proxies or that the proxy itself is the source). I'll change the formatting back to
> > what was there previously and add an option to log the peer IP as well.
> > 
> 
> Note that people have fail2ban setup on these logs, among other things,
> and that breaking suddenly is _far_ from ideal. If always done it should
> be on a major release, that avoids the headache of keeping (work for devs)
> and detecting the existence of that option (work for the user).
> 
> But not a must, we can also guard this with some option or the like now.

yeah, we could switch to the new format *only* if the header option is set? as else, the two IPs are identical anyway, so logging the same one twice while breaking the format provides no benefit?

and then maybe with 9.0 switch the format unconditionally, so that parsers/fail2ban configs only need to handle one of them going forward..