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 B8B841FF173 for ; Mon, 25 Nov 2024 00:56:31 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 92FB7733F; Mon, 25 Nov 2024 00:56:40 +0100 (CET) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1732492589; x=1733097389; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=9yi+HWu+SEC6VsIxQT2iFnw/BEbnsvmJddUwPm7qLQs=; b=VgL8vhi5m9MyVIF6AK7Ua93ojYfYgN7KA2PQuQjs09nM3zbLFdx8IB39iUP+OsTEmo m7Rhwq3EX9FiCc7IN2qJlqj76UpnsQQHud/91MV6y3PxffblB7gOAV6or/AcnEFiXv9v BmsSD5r4wj7nQlz29IiGo+055rtgxPkUhmxqBKwwO+zK6IuekUPbBAz6D1Z18ycMQfxw YQyGL1xRUzcZjsekiaJYfnwQsN1yDzEWV8VQ1PPIqaTPcAsZPMZcuYZV9wWUDJMQRLrN 2PE3VZgvknXvLrej6tmDCmG263FaEYWPKeJYkokVSjvsQxtj5gvS+RPdZ4xH1ifNVt/D 9W9g== X-Gm-Message-State: AOJu0Yxa1OdvNyk4Xkd6cwHJD40q8/s5PR2eARXfVVY2RGIOQDhzgL8L wfN1UccTZvQ0jvdtK0K/xZ11vL+3MWSyd37CsrtcaOQTztuU42AUD2/RM7Hn X-Gm-Gg: ASbGnctzLbwL0xB6wfxrxQWDeUmGywCvxOukIC+rsm9S3mYdx5E2QfNTA46Uxm/xHPW EoYo7/8Uc8hf5UD5RJ6HCENTeQM4nrR5GF6Sd1Y092tHmwMywI+vzim4LGyGhqIZY82hEFFvgdY YVNikEydoYzpxktlNfnxUwBRMXdDySjO0dEwjUQtBqJm4/rK3BBJKHhjJ85/wTUY+O6r+bVSj0a v8yeNFiTYEnH8mjtXM0DoWd4bykLavQ0iI1jBzSlQCfUud8KrGx+tAZgg5ExXoK+TgZzNp6IvXg uH0hQ/wRoSnh8dfRB/nYWhAyYQS04EvMR1RdcqgtTpxJjvS6cniE46knGxK9ll4= X-Google-Smtp-Source: AGHT+IF/KcoBcfZdJgyRDtPdxGI9xOeGipQfNFar4Xyj9WzFb3pnkCAqiRj+X0p4+wo8/PjVb0mojA== X-Received: by 2002:a05:6902:2087:b0:e35:e0c4:a06b with SMTP id 3f1490d57ef6-e38f8b5b77cmr8183277276.25.1732492181411; Sun, 24 Nov 2024 15:49:41 -0800 (PST) From: Thomas Skinner To: f.gruenbichler@proxmox.com Date: Sun, 24 Nov 2024 17:49:38 -0600 Message-Id: <20241124234938.2668814-1-thomas@atskinner.net> X-Mailer: git-send-email 2.39.5 In-Reply-To: <1731496954.sd63rzj5wa.astroid@yuna.none> References: <1731496954.sd63rzj5wa.astroid@yuna.none> MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.404 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 FREEMAIL_FORGED_FROMDOMAIN 0.001 2nd level domains in From and EnvelopeFrom freemail headers are different FREEMAIL_FROM 0.001 Sender email is commonly abused enduser mail provider HEADER_FROM_DIFFERENT_DOMAINS 0.25 From and EnvelopeFrom 2nd level mail domains are different KAM_ASCII_DIVIDERS 0.8 Email that uses ascii formatting dividers and possible spam tricks KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_NONE -0.0001 Sender listed at https://www.dnswl.org/, no trust RCVD_IN_MSPIKE_H2 -0.001 Average reputation (+2) RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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 Cc: pve-devel@lists.proxmox.com 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)? 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