[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

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

[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

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

[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