[pve-devel] [PATCH SERIES manager/http-server/docs] fix #5699: add support for real IP

[pve-devel] [PATCH SERIES manager/http-server/docs] fix #5699: add support for real IP

[Thomas Skinner]

This patch series adds support for setting a HTTP header for passing through a client IP
address for logging purposes. Two settings are introduced for /etc/default/pveproxy:

PROXY_REAL_IP_HEADER: defines a HTTP header to extract a client IP address from in the request. 
If the IP address is invalid, it is ignored. Otherwise, it is logged in addition to
the sending peer IP address.

TRUSTED_PROXY_IPS: defines a list of IP addresses or ranges that are allowed to set
the header defined in PROXY_REAL_IP_HEADER. If this setting is not set, any requests
with the PROXY_REAL_IP_HEADER set will have the extracted IP address logged.

pve-docs:

Thomas Skinner (1):
  fix #5699: pveproxy: add docs for real IP support

 pveproxy.adoc | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)


pve-http-server:

Thomas Skinner (1):
  fix #5699: pveproxy: add library methods for real IP support

 src/PVE/APIServer/AnyEvent.pm | 43 ++++++++++++++++++++++++++++++++---
 src/PVE/APIServer/Utils.pm    | 15 ++++++++++++
 2 files changed, 55 insertions(+), 3 deletions(-)


pve-manager:

Thomas Skinner (1):
  fix #5699: pveproxy: add settings for real IP support

 PVE/Service/pveproxy.pm | 2 ++
 1 file changed, 2 insertions(+)

-- 
2.39.2


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

[pve-devel] [PATCH docs 1/1] fix #5699: pveproxy: add docs for real IP support

[Thomas Skinner]

---
 pveproxy.adoc | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/pveproxy.adoc b/pveproxy.adoc
index 4b5dac0..f0ae0f7 100644
--- a/pveproxy.adoc
+++ b/pveproxy.adoc
@@ -198,6 +198,35 @@ content, if the client supports it. This can disabled in `/etc/default/pveproxy`
 
  COMPRESSION=0
 
+[[pveproxy_real_ip]]
+Real Client IP Logging
+----------------------
+
+By default, `pveproxy` logs the IP address of the client that sent the request.
+In cases where a proxy server is in front of `pveproxy`, it may be desirable to
+log the IP of the client making the request instead of the proxy IP.
+
+To enable processing of a HTTP header set by the proxy for logging purposes, set 
+`PROXY_REAL_IP_HEADER` to the name of the header to retrieve the client IP from. For
+example:
+
+ PROXY_REAL_IP_HEADER="X-Forwarded-For"
+
+Any invalid values passed in this header will be ignored.
+
+The default behavior is log the value in this header on all incoming requests. 
+To define a list of proxy servers that should be trusted to set the above HTTP
+header, set `TRUSTED_PROXY_IPS`, for example:
+
+ TRUSTED_PROXY_IPS="192.168.0.2"
+
+The `TRUSTED_PROXY_IPS` setting also supports values similar to the `ALLOW_FROM`
+and `DENY_FROM` settings.
+
+IP addresses can be specified using any syntax understood by `Net::IP`. The
+name `all` is an alias for `0/0` and `::/0` (meaning all IPv4 and IPv6
+addresses).
+
 ifdef::manvolnum[]
 include::pve-copyright.adoc[]
 endif::manvolnum[]
-- 
2.39.2


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

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

[Thomas Skinner]

---
 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\""
 
     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});
+    }
+
     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;
+
+    if ($self->{trusted_proxy_ips}) {
+	foreach my $t (@{$self->{trusted_proxy_ips}}) {
+	    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] [PATCH manager 1/1] fix #5699: pveproxy: add settings for real IP support

[Thomas Skinner]

---
 PVE/Service/pveproxy.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/PVE/Service/pveproxy.pm b/PVE/Service/pveproxy.pm
index ac108545..66db7a73 100755
--- a/PVE/Service/pveproxy.pm
+++ b/PVE/Service/pveproxy.pm
@@ -115,6 +115,8 @@ sub init {
 	    honor_cipher_order => $proxyconf->{HONOR_CIPHER_ORDER},
 	},
 	compression => $proxyconf->{COMPRESSION},
+	proxy_real_ip_header => $proxyconf->{PROXY_REAL_IP_HEADER},
+	trusted_proxy_ips => $proxyconf->{TRUSTED_PROXY_IPS},
 	# Note: there is no authentication for those pages and dirs!
 	pages => {
 	    '/' => sub { get_index($self->{nodename}, @_) },
-- 
2.39.2


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

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

[Fabian Grünbichler]

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

>  
>      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(),
            );
        }
    }
}

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

> +
> +    if ($self->{trusted_proxy_ips}) {
> +	foreach my $t (@{$self->{trusted_proxy_ips}}) {

this line doesn't really match our perl style ;)

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

Re: [pve-devel] [PATCH docs 1/1] fix #5699: pveproxy: add docs for real IP support

[Fabian Grünbichler]

thanks for sending docs along with your feature patch! this one looks
good to me, but see my comments on the feature patch itself.

On September 10, 2024 2:30 am, Thomas Skinner wrote:
> ---
>  pveproxy.adoc | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/pveproxy.adoc b/pveproxy.adoc
> index 4b5dac0..f0ae0f7 100644
> --- a/pveproxy.adoc
> +++ b/pveproxy.adoc
> @@ -198,6 +198,35 @@ content, if the client supports it. This can disabled in `/etc/default/pveproxy`
>  
>   COMPRESSION=0
>  
> +[[pveproxy_real_ip]]
> +Real Client IP Logging
> +----------------------
> +
> +By default, `pveproxy` logs the IP address of the client that sent the request.
> +In cases where a proxy server is in front of `pveproxy`, it may be desirable to
> +log the IP of the client making the request instead of the proxy IP.
> +
> +To enable processing of a HTTP header set by the proxy for logging purposes, set 
> +`PROXY_REAL_IP_HEADER` to the name of the header to retrieve the client IP from. For
> +example:
> +
> + PROXY_REAL_IP_HEADER="X-Forwarded-For"
> +
> +Any invalid values passed in this header will be ignored.
> +
> +The default behavior is log the value in this header on all incoming requests. 
> +To define a list of proxy servers that should be trusted to set the above HTTP
> +header, set `TRUSTED_PROXY_IPS`, for example:
> +
> + TRUSTED_PROXY_IPS="192.168.0.2"
> +
> +The `TRUSTED_PROXY_IPS` setting also supports values similar to the `ALLOW_FROM`
> +and `DENY_FROM` settings.
> +
> +IP addresses can be specified using any syntax understood by `Net::IP`. The
> +name `all` is an alias for `0/0` and `::/0` (meaning all IPv4 and IPv6
> +addresses).
> +
>  ifdef::manvolnum[]
>  include::pve-copyright.adoc[]
>  endif::manvolnum[]
> -- 
> 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

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

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

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

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

[Thomas Lamprecht]

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

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

[Fabian Grünbichler]

> Thomas Lamprecht <t.lamprecht@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..


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

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

[Thomas Lamprecht]

Am 25.11.24 um 10:05 schrieb Fabian Grünbichler:
> 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..

Sounds fine to me. Albeit for some this still might break, if they already
use a reverse proxy now – but these people at least could not have any
(working) fail2ban, as they just would have banned the IP of their reverse
proxy, so it should be fine I think.

btw., and sorry if I just missed this on reading, how do others log this?
I.e., is there any somewhat common format and does this patch already
adheres to that format?


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

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

[Fabian Grünbichler]

> Thomas Lamprecht <t.lamprecht@proxmox.com> hat am 25.11.2024 12:17 CET geschrieben:
> 
>  
> Am 25.11.24 um 10:05 schrieb Fabian Grünbichler:
> > 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..
> 
> Sounds fine to me. Albeit for some this still might break, if they already
> use a reverse proxy now – but these people at least could not have any
> (working) fail2ban, as they just would have banned the IP of their reverse
> proxy, so it should be fine I think.

it would still require enabling the new feature on the pveproxy side (that's what I meant with "header option", not that the default header is set on the HTTP request), so it's completely opt-in?

> btw., and sorry if I just missed this on reading, how do others log this?
> I.e., is there any somewhat common format and does this patch already
> adheres to that format?

that would indeed be nice to know! :)


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

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

[Thomas Skinner]

On Mon, Nov 25, 2024 at 5:31 AM Fabian Grünbichler
<f.gruenbichler@proxmox.com> wrote:
>
>
> > Thomas Lamprecht <t.lamprecht@proxmox.com> hat am 25.11.2024 12:17 CET geschrieben:
> >
> >
> > Am 25.11.24 um 10:05 schrieb Fabian Grünbichler:
> > > 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..
> >
> > Sounds fine to me. Albeit for some this still might break, if they already
> > use a reverse proxy now – but these people at least could not have any
> > (working) fail2ban, as they just would have banned the IP of their reverse
> > proxy, so it should be fine I think.
>
> it would still require enabling the new feature on the pveproxy side (that's what I meant with "header option", not that the default header is set on the HTTP request), so it's completely opt-in?

Yes, to log the real client IP from the proxy, the header would need
to be set in the PROXY_REAL_IP_HEADER option anyway to even get the
data. If anything, having the option would help those with the
"broken" fail2ban have the real client IP data instead of the proxy
IP.

I don't have any issues with keeping the format as-is and logging the
real IP instead of the peer IP. This can be covered in the docs and
would have to be explicitly enabled to change the current logging
behavior. This way the patch doesn't break any existing log analyzer
but the feature is there available for the user. The TRUSTED_PROXY_IPS
helps with the risk of not logging the proxy/peer IP by limiting the
list of sources that can set the header.

> > btw., and sorry if I just missed this on reading, how do others log this?
> > I.e., is there any somewhat common format and does this patch already
> > adheres to that format?
>
> that would indeed be nice to know! :)
>

I'm not sure that there is or is not a standard for logging the client
IP and peer/proxy IP information together. In my experience, it has
been a custom log format (specifically in Apache httpd) to output both
of them. Across products, for web-based logs, I've seen Apache/NCSA
style, semi-structured key/value text, or structured JSON objects. I'd
argue that the JSON is the most compatible since it explicitly labels
the data fields, but that would be a breaking change from what PVE is
doing now for logging, and it's not friendly for human readability.

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