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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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.

>>> +
>>> +    if ($self->{trusted_proxy_ips}) {
>>> +	foreach my $t (@{$self->{trusted_proxy_ips}}) {
>>
>> this line doesn't really match our perl style ;)
> 
> I'm not sure what you're referring to here. I copied and modified existing code
> that did nearly what I needed from the check_host_access subroutine. Could you 
> elaborate or give an example of what needs to be fixed here?
> 

Yeah, our existing code is not all that holy, we're moving that over slowly,
e.g. when touching some code parts anyway. The goal would be for all code to
follow this document giving some basic guidelines:

https://pve.proxmox.com/wiki/Perl_Style_Guide

One of the important one is to avoid single character variables and needles
abbreviation (cip vs client_ip), as that has _no_ gain and just makes
everyone's life harder (and yeah, our existing code base has a few of those).
Then there are some more minor style issues.

Here you could write:

if (my $trusted_proxy_ips = $self->{trusted_proxy_ips}) {
    for my $trusted_net ($trusted_proxy_ips->@*) {
        if ($trusted_net->overlaps($client_ip)) {
             $self->dprint("client IP '$client_ip' in allowed proxy network '". $t->print()."'");
            return 1;
        }
    }
}

return 0;

and maybe change the name, e.g. like:

s/trusted_proxy_ips/proxy_cidr_allowlist/




_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel