public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH manager] pveproxy: create log directory when starting
@ 2025-04-15  9:38 Maximiliano Sandoval
  2025-04-16  8:23 ` Fabian Grünbichler
  0 siblings, 1 reply; 3+ messages in thread
From: Maximiliano Sandoval @ 2025-04-15  9:38 UTC (permalink / raw)
  To: pve-devel

We only create this directory while installing the package. If a user
deletes /var/log then they will lose access to the web UI.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 bin/pveproxy | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/bin/pveproxy b/bin/pveproxy
index 20e8f2ab2..1090df039 100755
--- a/bin/pveproxy
+++ b/bin/pveproxy
@@ -19,12 +19,24 @@ $SIG{'__WARN__'} = sub {
     $@ = $err;
 };
 
+sub chown_to_www_data {
+    my ($dir) = @_;
+
+    my $gid = getgrnam('www-data') || die "getgrnam failed - $!\n";
+    my $uid = getpwnam('www-data') || die "getpwnam failed - $!\n";
+
+    chown($uid, $gid, $dir);
+}
+
 my $prepare = sub {
     my $rundir="/var/run/pveproxy";
+    my $logdir = '/var/log/pveproxy';
+
     if (mkdir($rundir, 0700)) { # only works at first start if we are root)
-	my $gid = getgrnam('www-data') || die "getgrnam failed - $!\n";
-	my $uid = getpwnam('www-data') || die "getpwnam failed - $!\n";
-	chown($uid, $gid, $rundir);
+        chown_to_www_data($rundir);
+    }
+    if (mkdir($logdir, 0700)) {
+        chown_to_www_data($logdir);
     }
 };
 
-- 
2.39.5



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


^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [pve-devel] [PATCH manager] pveproxy: create log directory when starting
  2025-04-15  9:38 [pve-devel] [PATCH manager] pveproxy: create log directory when starting Maximiliano Sandoval
@ 2025-04-16  8:23 ` Fabian Grünbichler
  2025-04-16  9:21   ` Maximiliano Sandoval
  0 siblings, 1 reply; 3+ messages in thread
From: Fabian Grünbichler @ 2025-04-16  8:23 UTC (permalink / raw)
  To: Proxmox VE development discussion

On April 15, 2025 11:38 am, Maximiliano Sandoval wrote:
> We only create this directory while installing the package. If a user
> deletes /var/log then they will lose access to the web UI.

until the reinstall or upgrade the package?

> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>  bin/pveproxy | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/bin/pveproxy b/bin/pveproxy
> index 20e8f2ab2..1090df039 100755
> --- a/bin/pveproxy
> +++ b/bin/pveproxy
> @@ -19,12 +19,24 @@ $SIG{'__WARN__'} = sub {
>      $@ = $err;
>  };
>  
> +sub chown_to_www_data {
> +    my ($dir) = @_;
> +
> +    my $gid = getgrnam('www-data') || die "getgrnam failed - $!\n";
> +    my $uid = getpwnam('www-data') || die "getpwnam failed - $!\n";
> +
> +    chown($uid, $gid, $dir);
> +}

the thing that only works as root is the chown, not all of this..

> +
>  my $prepare = sub {
>      my $rundir="/var/run/pveproxy";
> +    my $logdir = '/var/log/pveproxy';
> +

but anyway, it would be much easier to read and have better semantics if
we switch to

if root {
  my $uid = ..
  my $gid = ..
  mkdir(..)
  chown(..) || warn ...
  mkdir(..)
  chown(..) || warn ...
}

which has the added benefit of resetting the ownership to the right
value on reboot/service start, so fixes another potential issue (at the
cost of two unconditional syscalls).

>      if (mkdir($rundir, 0700)) { # only works at first start if we are root)
> -	my $gid = getgrnam('www-data') || die "getgrnam failed - $!\n";
> -	my $uid = getpwnam('www-data') || die "getpwnam failed - $!\n";
> -	chown($uid, $gid, $rundir);
> +        chown_to_www_data($rundir);
> +    }
> +    if (mkdir($logdir, 0700)) {
> +        chown_to_www_data($logdir);
>      }
>  };
>  
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> 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


^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [pve-devel] [PATCH manager] pveproxy: create log directory when starting
  2025-04-16  8:23 ` Fabian Grünbichler
@ 2025-04-16  9:21   ` Maximiliano Sandoval
  0 siblings, 0 replies; 3+ messages in thread
From: Maximiliano Sandoval @ 2025-04-16  9:21 UTC (permalink / raw)
  To: Proxmox VE development discussion


Fabian Grünbichler <f.gruenbichler@proxmox.com> writes:

> On April 15, 2025 11:38 am, Maximiliano Sandoval wrote:
>> We only create this directory while installing the package. If a user
>> deletes /var/log then they will lose access to the web UI.
>
> until the reinstall or upgrade the package?
>
>> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
>> ---
>>  bin/pveproxy | 18 +++++++++++++++---
>>  1 file changed, 15 insertions(+), 3 deletions(-)
>> 
>> diff --git a/bin/pveproxy b/bin/pveproxy
>> index 20e8f2ab2..1090df039 100755
>> --- a/bin/pveproxy
>> +++ b/bin/pveproxy
>> @@ -19,12 +19,24 @@ $SIG{'__WARN__'} = sub {
>>      $@ = $err;
>>  };
>>  
>> +sub chown_to_www_data {
>> +    my ($dir) = @_;
>> +
>> +    my $gid = getgrnam('www-data') || die "getgrnam failed - $!\n";
>> +    my $uid = getpwnam('www-data') || die "getpwnam failed - $!\n";
>> +
>> +    chown($uid, $gid, $dir);
>> +}
>
> the thing that only works as root is the chown, not all of this..
>
>> +
>>  my $prepare = sub {
>>      my $rundir="/var/run/pveproxy";
>> +    my $logdir = '/var/log/pveproxy';
>> +
>
> but anyway, it would be much easier to read and have better semantics if
> we switch to
>
> if root {
>   my $uid = ..
>   my $gid = ..
>   mkdir(..)
>   chown(..) || warn ...
>   mkdir(..)
>   chown(..) || warn ...
> }
>
> which has the added benefit of resetting the ownership to the right
> value on reboot/service start, so fixes another potential issue (at the
> cost of two unconditional syscalls).
>
>>      if (mkdir($rundir, 0700)) { # only works at first start if we are root)
>> -	my $gid = getgrnam('www-data') || die "getgrnam failed - $!\n";
>> -	my $uid = getpwnam('www-data') || die "getpwnam failed - $!\n";
>> -	chown($uid, $gid, $rundir);
>> +        chown_to_www_data($rundir);
>> +    }
>> +    if (mkdir($logdir, 0700)) {
>> +        chown_to_www_data($logdir);
>>      }
>>  };
>>  
>> -- 
>> 2.39.5
>> 
>> 
>> 
>> _______________________________________________
>> 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

superseded by https://lore.proxmox.com/pve-devel/20250416091829.124366-1-m.sandoval@proxmox.com/T/#u.


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

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-04-16  9:23 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-04-15  9:38 [pve-devel] [PATCH manager] pveproxy: create log directory when starting Maximiliano Sandoval
2025-04-16  8:23 ` Fabian Grünbichler
2025-04-16  9:21   ` Maximiliano Sandoval

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal