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

[Thomas Skinner <thomas@atskinner.net>]

> 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.

>>  
>>      return if $loginfo->{written}; # avoid duplicate logs
>>      $loginfo->{written} = 1;
>>  
>>      my $peerip = $reqstate->{peer_host} || '-';
>> +    my $realip = $loginfo->{real_ip} || $peerip;
>>      my $userid = $loginfo->{userid} || '-';
>>      my $content_length = defined($loginfo->{content_length}) ? $loginfo->{content_length} : '-';
>>      my $code =  $loginfo->{code} || 500;
>>      my $requestline = $loginfo->{requestline} || '-';
>>      my $timestr = strftime("%d/%m/%Y:%H:%M:%S %z", localtime());
>>  
>> -    my $msg = "$peerip - $userid [$timestr] \"$requestline\" $code $content_length\n";
>> +    my $msg = "$realip $peerip - $userid [$timestr] \"$requestline\" $code $content_length\n";
>>  
>>      $self->write_log($msg);
>>  }
>> @@ -1462,6 +1463,14 @@ sub authenticate_and_handle_request {
>>  
>>      my $auth = {};
>>  
>> +    if ($self->{proxy_real_ip_header} && $request->header($self->{proxy_real_ip_header})) {
>> +	my $real_ip = Net::IP->new($request->header($self->{proxy_real_ip_header})) || undef;
>> +	$reqstate->{log}->{real_ip} = Net::IP::ip_compress_address(
>> +    	    $real_ip->ip(), 
>> +	    $real_ip->version(),
>> +	) if defined($real_ip) && $self->check_trusted_proxy($reqstate->{peer_host});
>
> the alignment/indentation is off there, and the mixing of ifs and
> postifs is also not easily readable.. maybe:
>
> if (my $hdr = $self->{proxy_real_ip_header}) {
>     if (my $value = $request->header($hdr})) {
>         my $real_ip = Net::IP->new($value);
>         if (defined($real_ip) && $self->check_trusted_proxy($peerip)) {
>             $reqstate->{log}->{real_ip} = Net::IP::ip_compress_address(
>                 $real_ip->ip(), 
>                 $real_ip->version(),
>             );
>         }
>     }
> }

I agree. After looking at my code compared to yours, mine is rather messy. Will 
fix with a combination of the two. 

>> +    }
>> +
>>      if ($self->{spiceproxy}) {
>>  	my $connect_str = $request->header('Host');
>>  	my ($vmid, $node, $port) = $self->verify_spice_connect_url($connect_str);
>> @@ -1801,6 +1810,34 @@ sub check_host_access {
>>      return $match_allow;
>>  }
>>  
>> +sub check_trusted_proxy {
>> +    my ($self, $clientip) = @_;
>> +
>> +    $clientip = PVE::APIServer::Utils::normalize_v4_in_v6($clientip);
>> +    my $cip = Net::IP->new($clientip);
>> +
>> +    if (!$cip) {
>> +	$self->dprint("client IP not parsable: $@");
>> +	return 0;
>> +    }
>> +
>> +    my $match_trusted_proxy = 0;
>
> this is not needed, you can just return directly below..

Yep, that's accurate. I had it there to match the style of the check_host_access
subroutine for consistency. Will adjust accordingly.

>> +
>> +    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?

>> +	    if ($t->overlaps($cip)) {
>> +		$match_trusted_proxy = 1;
>> +		$self->dprint("client IP in trusted proxies: ". $t->print());
>> +		last;
>> +	    }
>> +	}
>> +    } else {
>> +	$match_trusted_proxy = 1;
>> +    }
>> +
>> +    return $match_trusted_proxy;
>> +}
>> +
>>  sub accept_connections {
>>      my ($self) = @_;
>>  
>> diff --git a/src/PVE/APIServer/Utils.pm b/src/PVE/APIServer/Utils.pm
>> index 5728d97..f7cff29 100644
>> --- a/src/PVE/APIServer/Utils.pm
>> +++ b/src/PVE/APIServer/Utils.pm
>> @@ -26,6 +26,8 @@ sub read_proxy_config {
>>      $shcmd .= 'echo \"COMPRESSION:\$COMPRESSION\";';
>>      $shcmd .= 'echo \"DISABLE_TLS_1_2:\$DISABLE_TLS_1_2\";';
>>      $shcmd .= 'echo \"DISABLE_TLS_1_3:\$DISABLE_TLS_1_3\";';
>> +	$shcmd .= 'echo \"PROXY_REAL_IP_HEADER:\$PROXY_REAL_IP_HEADER\";';
>> +	$shcmd .= 'echo \"TRUSTED_PROXY_IPS:\$TRUSTED_PROXY_IPS\";';
>>  
>>      my $data = -f $conffile ? `bash -c "$shcmd"` : '';
>>  
>> @@ -65,6 +67,19 @@ sub read_proxy_config {
>>  	    $res->{$key} = $value;
>>  	} elsif ($key eq 'TLS_KEY_FILE') {
>>  	    $res->{$key} = $value;
>> +	} elsif ($key eq 'PROXY_REAL_IP_HEADER') {
>> +		$res->{$key} = $value;
>> +	} elsif ($key eq 'TRUSTED_PROXY_IPS') {
>> +		my $ips = [];
>> +	    foreach my $ip (split(/,/, $value)) {
>> +		if ($ip eq 'all') {
>> +		    push @$ips, Net::IP->new('0/0') || die Net::IP::Error() . "\n";
>> +		    push @$ips, Net::IP->new('::/0') || die Net::IP::Error() . "\n";
>> +		    next;
>> +		}
>> +		push @$ips, Net::IP->new(normalize_v4_in_v6($ip)) || die Net::IP::Error() . "\n";
>> +	    }
>> +	    $res->{$key} = $ips;
>>  	} elsif (grep { $key eq $_ } @$boolean_options) {
>>  	    die "unknown value '$value' - use 0 or 1\n" if $value !~ m/^(0|1)$/;
>>  	    $res->{$key} = $value;
>> -- 
>> 2.39.2
>> 
>> 
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>> 
>> 
>> 
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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