[pmg-devel] [PATCH pmg-api 1/2] cluster config: restrict slurp scope to avoid issue parsing network interfaces

[pmg-devel] [PATCH pmg-api 1/2] cluster config: restrict slurp scope to avoid issue parsing network interfaces

[Fiona Ebner]

As reported in the community forum [0], there is an edge case, where
querying the network interfaces would not work. In particular, this
could happen if the hostname cannot be resolved to a non-loopback IP
(when installing PMG on Debian and forgetting to adapt /etc/hosts for
example).

The issue manifested as follows:
- When setting up the RESTEnvironemnt, the cluster config is read.
- This reader uses slurp mode by setting the line ending to undef
  locally.
- But the subroutine call PVE::Network::get_local_ip() is still part
  of that local context.
- When resolving the hostname to a non-loopback IP address failed, the
  function would read (via the PVE::INotify module) the network
  interfaces file.
- As part of that, /proc/net/dev was read all at once, while the
  interface parsing code expects it line-by-line.
- The result for reading network interfaces was cached without having
  detected the interfaces in /proc/net/dev.
- When a new request came in, the cached result was used (even
  changing the file to invalidate the cache would only work as long
  as the cluster config file exists, because otherwise, there would be
  an attempt to read the cluster config which would read the updated
  version of the interfaces file while slurping again).

[0]: https://forum.proxmox.com/threads/129958/

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PMG/ClusterConfig.pm | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/PMG/ClusterConfig.pm b/src/PMG/ClusterConfig.pm
index 77b9e60..c52508d 100644
--- a/src/PMG/ClusterConfig.pm
+++ b/src/PMG/ClusterConfig.pm
@@ -170,9 +170,7 @@ sub lock_config {
 sub read_cluster_conf {
     my ($filename, $fh) = @_;
 
-    local $/ = undef; # slurp mode
-
-    my $raw = defined($fh) ? <$fh> : undef;
+    my $raw = defined($fh) ? do { local $/ = undef; <$fh> } : undef;
 
     my $cinfo = PMG::ClusterConfig::Base->parse_config($filename, $raw);
 
-- 
2.39.2

[pmg-devel] [PATCH pmg-api 2/2] tree-wide: make slurp mode as local as possible for future-proofing

[Fiona Ebner]

similar to what PMG/TFAConfig.pm already does.

Otherwise, sub-routine calls would still be affected leading to
unexpected results, like the issue fixed by commit "cluster config:
restrict slurp scope to avoid issue parsing network interfaces".

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PMG/API2/ACMEPlugin.pm |  3 +--
 src/PMG/Config.pm          |  4 +---
 src/PMG/LDAPConfig.pm      |  4 +---
 src/PMG/NodeConfig.pm      |  3 +--
 src/PMG/PBSConfig.pm       |  4 +---
 src/PMG/Ticket.pm          | 12 +++---------
 6 files changed, 8 insertions(+), 22 deletions(-)

diff --git a/src/PMG/API2/ACMEPlugin.pm b/src/PMG/API2/ACMEPlugin.pm
index e2004bf..25d3a04 100644
--- a/src/PMG/API2/ACMEPlugin.pm
+++ b/src/PMG/API2/ACMEPlugin.pm
@@ -30,8 +30,7 @@ PVE::JSONSchema::register_standard_option('pmg-acme-pluginid', {
 
 sub read_pmg_acme_challenge_config {
     my ($filename, $fh) = @_;
-    local $/ = undef; # slurp mode
-    my $raw = defined($fh) ? <$fh> : '';
+    my $raw = defined($fh) ? do { local $/ = undef; <$fh> } : '';
     return PVE::ACME::Challenge->parse_config($filename, $raw);
 }
 
diff --git a/src/PMG/Config.pm b/src/PMG/Config.pm
index fe89e11..7339e0d 100644
--- a/src/PMG/Config.pm
+++ b/src/PMG/Config.pm
@@ -939,10 +939,8 @@ sub get_config {
 sub read_pmg_conf {
     my ($filename, $fh) = @_;
 
-    local $/ = undef; # slurp mode
-
     my $raw;
-    $raw = <$fh> if defined($fh);
+    $raw = do { local $/ = undef; <$fh> } if defined($fh);
 
     return  PMG::Config::Base->parse_config($filename, $raw);
 }
diff --git a/src/PMG/LDAPConfig.pm b/src/PMG/LDAPConfig.pm
index a6cd6ef..e5b3388 100644
--- a/src/PMG/LDAPConfig.pm
+++ b/src/PMG/LDAPConfig.pm
@@ -221,9 +221,7 @@ __PACKAGE__->init();
 sub read_pmg_ldap_conf {
     my ($filename, $fh) = @_;
 
-    local $/ = undef; # slurp mode
-
-    my $raw = defined($fh) ? <$fh> : '';
+    my $raw = defined($fh) ? do { local $/ = undef; <$fh> } : '';
 
     return __PACKAGE__->parse_config($filename, $raw);
 }
diff --git a/src/PMG/NodeConfig.pm b/src/PMG/NodeConfig.pm
index 42139e4..6303979 100644
--- a/src/PMG/NodeConfig.pm
+++ b/src/PMG/NodeConfig.pm
@@ -120,8 +120,7 @@ sub print_domain : prototype($) {
 
 sub read_pmg_node_config {
     my ($filename, $fh) = @_;
-    local $/ = undef; # slurp mode
-    my $raw = defined($fh) ? <$fh> : '';
+    my $raw = defined($fh) ? do { local $/ = undef; <$fh> } : '';
     my $digest = Digest::SHA::sha1_hex($raw);
     my $conf = PVE::JSONSchema::parse_config($config_schema, $filename, $raw);
     $conf->{digest} = $digest;
diff --git a/src/PMG/PBSConfig.pm b/src/PMG/PBSConfig.pm
index 3417123..ee506f1 100644
--- a/src/PMG/PBSConfig.pm
+++ b/src/PMG/PBSConfig.pm
@@ -194,9 +194,7 @@ __PACKAGE__->init();
 sub read_pmg_pbs_conf {
     my ($filename, $fh) = @_;
 
-    local $/ = undef; # slurp mode
-
-    my $raw = defined($fh) ? <$fh> : '';
+    my $raw = defined($fh) ? do { local $/ = undef; <$fh> } : '';
 
     return __PACKAGE__->parse_config($filename, $raw);
 }
diff --git a/src/PMG/Ticket.pm b/src/PMG/Ticket.pm
index 0c2ec0b..fc2ac77 100644
--- a/src/PMG/Ticket.pm
+++ b/src/PMG/Ticket.pm
@@ -106,9 +106,7 @@ sub generate_auth_key {
 my $read_rsa_priv_key = sub {
    my ($filename, $fh) = @_;
 
-   local $/ = undef; # slurp mode
-
-   my $input = <$fh>;
+   my $input = do { local $/ = undef; <$fh> };
 
    return Crypt::OpenSSL::RSA->new_private_key($input);
 
@@ -121,9 +119,7 @@ PVE::INotify::register_file('auth_priv_key', $authprivkeyfn,
 my $read_rsa_pub_key = sub {
    my ($filename, $fh) = @_;
 
-   local $/ = undef; # slurp mode
-
-   my $input = <$fh>;
+   my $input = do { local $/ = undef; <$fh> };
 
    return Crypt::OpenSSL::RSA->new_public_key($input);
 };
@@ -135,9 +131,7 @@ PVE::INotify::register_file('auth_pub_key', $authpubkeyfn,
 my $read_csrf_secret = sub {
    my ($filename, $fh) = @_;
 
-   local $/ = undef; # slurp mode
-
-   my $input = <$fh>;
+   my $input = do { local $/ = undef; <$fh> };
 
    return Digest::SHA::hmac_sha256_base64($input);
 };
-- 
2.39.2

[pmg-devel] applied-series: [PATCH pmg-api 1/2] cluster config: restrict slurp scope to avoid issue parsing network interfaces

[Stoiko Ivanov]

hi,

huge thanks for your persistence and patience while digging into this -
great find!

applied both patches to master and stable-7 (after verifying that it's
affected as well :)


On Mon, 10 Jul 2023 13:36:46 +0200
Fiona Ebner <f.ebner@proxmox.com> wrote:

> As reported in the community forum [0], there is an edge case, where
> querying the network interfaces would not work. In particular, this
> could happen if the hostname cannot be resolved to a non-loopback IP
> (when installing PMG on Debian and forgetting to adapt /etc/hosts for
> example).
> 
> The issue manifested as follows:
> - When setting up the RESTEnvironemnt, the cluster config is read.
> - This reader uses slurp mode by setting the line ending to undef
>   locally.
> - But the subroutine call PVE::Network::get_local_ip() is still part
>   of that local context.
> - When resolving the hostname to a non-loopback IP address failed, the
>   function would read (via the PVE::INotify module) the network
>   interfaces file.
> - As part of that, /proc/net/dev was read all at once, while the
>   interface parsing code expects it line-by-line.
> - The result for reading network interfaces was cached without having
>   detected the interfaces in /proc/net/dev.
> - When a new request came in, the cached result was used (even
>   changing the file to invalidate the cache would only work as long
>   as the cluster config file exists, because otherwise, there would be
>   an attempt to read the cluster config which would read the updated
>   version of the interfaces file while slurping again).
> 
> [0]: https://forum.proxmox.com/threads/129958/
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  src/PMG/ClusterConfig.pm | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/src/PMG/ClusterConfig.pm b/src/PMG/ClusterConfig.pm
> index 77b9e60..c52508d 100644
> --- a/src/PMG/ClusterConfig.pm
> +++ b/src/PMG/ClusterConfig.pm
> @@ -170,9 +170,7 @@ sub lock_config {
>  sub read_cluster_conf {
>      my ($filename, $fh) = @_;
>  
> -    local $/ = undef; # slurp mode
> -
> -    my $raw = defined($fh) ? <$fh> : undef;
> +    my $raw = defined($fh) ? do { local $/ = undef; <$fh> } : undef;
>  
>      my $cinfo = PMG::ClusterConfig::Base->parse_config($filename, $raw);
>