From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 805B21FF15C for ; Wed, 13 Nov 2024 12:35:33 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 01AD914214; Wed, 13 Nov 2024 12:35:33 +0100 (CET) Date: Wed, 13 Nov 2024 12:34:55 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20240910003026.667328-1-thomas@atskinner.net> <20240910003026.667328-3-thomas@atskinner.net> In-Reply-To: <20240910003026.667328-3-thomas@atskinner.net> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1731496954.sd63rzj5wa.astroid@yuna.none> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.048 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH http-server 1/1] fix #5699: pveproxy: add library methods for real IP support X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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