[pve-devel] [PATCH v2 common] REST environment: default to root@pam in forked workers if no user was specified

[pve-devel] [PATCH v2 common] REST environment: default to root@pam in forked workers if no user was specified

[Oguz Bektas]

first call $rpcenv->get_user() if user was 'undef'. if that doesn't
return then we set it to root@pam.

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v1->v2:
* do get_user() first, set to 'root@pam' as fallback
* drop first patch for pve-container (not needed anymore)

 src/PVE/RESTEnvironment.pm | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/PVE/RESTEnvironment.pm b/src/PVE/RESTEnvironment.pm
index 1b2af08..bc5b8b5 100644
--- a/src/PVE/RESTEnvironment.pm
+++ b/src/PVE/RESTEnvironment.pm
@@ -492,7 +492,12 @@ sub fork_worker {
     $dtype = 'unknown' if !defined ($dtype);
     $id = '' if !defined ($id);
 
-    $user = 'root@pve' if !defined ($user);
+    $user = $self->get_user() if !defined($user);
+
+    if (!defined($user)) {
+	warn 'internal error: Worker user was not specified, defaulting to "root@pam"!';
+	$user = 'root@pam';
+    }
 
     my $sync = ($self->{type} eq 'cli' && !$background) ? 1 : 0;
 
-- 
2.30.2

Re: [pve-devel] [PATCH v2 common] REST environment: default to root@pam in forked workers if no user was specified

[Fabian Ebner]

Am 14.03.22 um 14:50 schrieb Oguz Bektas:
> first call $rpcenv->get_user() if user was 'undef'. if that doesn't
> return then we set it to root@pam.
> 
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
> v1->v2:
> * do get_user() first, set to 'root@pam' as fallback
> * drop first patch for pve-container (not needed anymore)
> 
>  src/PVE/RESTEnvironment.pm | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/RESTEnvironment.pm b/src/PVE/RESTEnvironment.pm
> index 1b2af08..bc5b8b5 100644
> --- a/src/PVE/RESTEnvironment.pm
> +++ b/src/PVE/RESTEnvironment.pm
> @@ -492,7 +492,12 @@ sub fork_worker {
>      $dtype = 'unknown' if !defined ($dtype);
>      $id = '' if !defined ($id);
>  
> -    $user = 'root@pve' if !defined ($user);
> +    $user = $self->get_user() if !defined($user);

If you don't set $noerr when calling get_user(), the below if block is
dead code.

> +
> +    if (!defined($user)) {
> +	warn 'internal error: Worker user was not specified, defaulting to "root@pam"!';
> +	$user = 'root@pam';
> +    }
>  
>      my $sync = ($self->{type} eq 'cli' && !$background) ? 1 : 0;
>  

Re: [pve-devel] [PATCH v2 common] REST environment: default to root@pam in forked workers if no user was specified

[Thomas Lamprecht]

On 15.03.22 08:31, Fabian Ebner wrote:
> Am 14.03.22 um 14:50 schrieb Oguz Bektas:
>> first call $rpcenv->get_user() if user was 'undef'. if that doesn't
>> return then we set it to root@pam.

this is just the "whats done" description, that's not really interesting for
such a short patch, as it can be read from the code change directly without
much effort. A sentence about why (original code reason, change reason to the
new behavior) and impact (what are the call sites that could be affected)
would be more helpful.

>>
>> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
>> ---
>> v1->v2:
>> * do get_user() first, set to 'root@pam' as fallback
>> * drop first patch for pve-container (not needed anymore)
>>
>>  src/PVE/RESTEnvironment.pm | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/PVE/RESTEnvironment.pm b/src/PVE/RESTEnvironment.pm
>> index 1b2af08..bc5b8b5 100644
>> --- a/src/PVE/RESTEnvironment.pm
>> +++ b/src/PVE/RESTEnvironment.pm
>> @@ -492,7 +492,12 @@ sub fork_worker {
>>      $dtype = 'unknown' if !defined ($dtype);
>>      $id = '' if !defined ($id);
>>  
>> -    $user = 'root@pve' if !defined ($user);
>> +    $user = $self->get_user() if !defined($user);
> 
> If you don't set $noerr when calling get_user(), the below if block is
> dead code.
> 
>> +
>> +    if (!defined($user)) {
>> +	warn 'internal error: Worker user was not specified, defaulting to "root@pam"!';

missing newline at the ends means spamming the log more with internal perl module
file/line location and I don't really get the warning in the first place, either
it's OK to fallback or not.

The REST-env should have a valid user for most (all?) API/CLI-handler derived use
cases, as only the public API calls have no user set but there we don't use fork_worker
at all.

So, I'd examine possible call sites that won't have a user passed nor available via
get_user(), and dependening from if they even exist I'd

* check if we could set a sensible user-id, if possible, there already
* make this either a no-warn or just drop the if-block and avoid passing the $noerr
  to $self->get_user(), making this a usage error. The latter would be cleaner, but
  has some theoretic breakage potential. As call-site evaluation should be done in any
  case, as neither breakage nor defaulting to root@pam is something that should be done
  "blindly" (I mean, root fallback was probably intended originally, just avoided due
   to the realm typo, but still).

Re: [pve-devel] [PATCH v2 common] REST environment: default to root@pam in forked workers if no user was specified

[Oguz Bektas]

hi,

On Tue, Mar 15, 2022 at 09:57:34AM +0100, Thomas Lamprecht wrote:
> On 15.03.22 08:31, Fabian Ebner wrote:
> > Am 14.03.22 um 14:50 schrieb Oguz Bektas:
> >> first call $rpcenv->get_user() if user was 'undef'. if that doesn't
> >> return then we set it to root@pam.
> 
> this is just the "whats done" description, that's not really interesting for
> such a short patch, as it can be read from the code change directly without
> much effort. A sentence about why (original code reason, change reason to the
> new behavior) and impact (what are the call sites that could be affected)
> would be more helpful.

those were all in the v1 patch for pve-container (which had the only call
sites that i could find where user was undefined, namely push_file and
pull_file) but got dropped with the v2 according to fabi's
recommendation. i can send that back if needed :)

> 
> >>
> >> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> >> ---
> >> v1->v2:
> >> * do get_user() first, set to 'root@pam' as fallback
> >> * drop first patch for pve-container (not needed anymore)
> >>
> >>  src/PVE/RESTEnvironment.pm | 7 ++++++-
> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/PVE/RESTEnvironment.pm b/src/PVE/RESTEnvironment.pm
> >> index 1b2af08..bc5b8b5 100644
> >> --- a/src/PVE/RESTEnvironment.pm
> >> +++ b/src/PVE/RESTEnvironment.pm
> >> @@ -492,7 +492,12 @@ sub fork_worker {
> >>      $dtype = 'unknown' if !defined ($dtype);
> >>      $id = '' if !defined ($id);
> >>  
> >> -    $user = 'root@pve' if !defined ($user);
> >> +    $user = $self->get_user() if !defined($user);
> > 
> > If you don't set $noerr when calling get_user(), the below if block is
> > dead code.


oops, you're right ^^

> > 
> >> +
> >> +    if (!defined($user)) {
> >> +	warn 'internal error: Worker user was not specified, defaulting to "root@pam"!';
> 
> missing newline at the ends means spamming the log more with internal perl module
> file/line location and I don't really get the warning in the first place, either
> it's OK to fallback or not.
> 
> The REST-env should have a valid user for most (all?) API/CLI-handler derived use
> cases, as only the public API calls have no user set but there we don't use fork_worker
> at all.
> 
> So, I'd examine possible call sites that won't have a user passed nor available via
> get_user(), and dependening from if they even exist I'd
> 
> * check if we could set a sensible user-id, if possible, there already
> * make this either a no-warn or just drop the if-block and avoid passing the $noerr
>   to $self->get_user(), making this a usage error. The latter would be cleaner, but
>   has some theoretic breakage potential. As call-site evaluation should be done in any
>   case, as neither breakage nor defaulting to root@pam is something that should be done
>   "blindly" (I mean, root fallback was probably intended originally, just avoided due
>    to the realm typo, but still).

see the v1 patch for pve-container. i didn't find any other call sites, the
warning was in case we forgot any other call sites somewhere.

defaulting to 'root@pam' in this case actually has no effect but it's
only in the logging for tasks.

i talked to dietmar about this off-list before sending the v1, since
this bit of code was originally imported from the svn repositories way
back, and he told me that it was intended for the log.

Re: [pve-devel] [PATCH v2 common] REST environment: default to root@pam in forked workers if no user was specified

[Thomas Lamprecht]

On 15.03.22 12:21, Oguz Bektas wrote:
> On Tue, Mar 15, 2022 at 09:57:34AM +0100, Thomas Lamprecht wrote:
>> On 15.03.22 08:31, Fabian Ebner wrote:
>>> Am 14.03.22 um 14:50 schrieb Oguz Bektas:
>>>> first call $rpcenv->get_user() if user was 'undef'. if that doesn't
>>>> return then we set it to root@pam.
>>
>> this is just the "whats done" description, that's not really interesting for
>> such a short patch, as it can be read from the code change directly without
>> much effort. A sentence about why (original code reason, change reason to the
>> new behavior) and impact (what are the call sites that could be affected)
>> would be more helpful.
> 
> those were all in the v1 patch for pve-container (which had the only call

but there no reference whatsoever to that, so someone checking pve-common's
history and stumbling over this commit is not aware at all of any such info.

So please either copy (for common) relevant info here or at least reference
the repo and commit id of the container patch for context, thx.

>>>> +
>>>> +    if (!defined($user)) {
>>>> +	warn 'internal error: Worker user was not specified, defaulting to "root@pam"!';
>>
>> missing newline at the ends means spamming the log more with internal perl module
>> file/line location and I don't really get the warning in the first place, either
>> it's OK to fallback or not.
>>
>> The REST-env should have a valid user for most (all?) API/CLI-handler derived use
>> cases, as only the public API calls have no user set but there we don't use fork_worker
>> at all.
>>
>> So, I'd examine possible call sites that won't have a user passed nor available via
>> get_user(), and dependening from if they even exist I'd
>>
>> * check if we could set a sensible user-id, if possible, there already
>> * make this either a no-warn or just drop the if-block and avoid passing the $noerr
>>   to $self->get_user(), making this a usage error. The latter would be cleaner, but
>>   has some theoretic breakage potential. As call-site evaluation should be done in any
>>   case, as neither breakage nor defaulting to root@pam is something that should be done
>>   "blindly" (I mean, root fallback was probably intended originally, just avoided due
>>    to the realm typo, but still).
> 
> see the v1 patch for pve-container. i didn't find any other call sites, the
> warning was in case we forgot any other call sites somewhere.
> 

that's exactly why I'd always like to have some context, allows to safe some
typing/research time ^^

> defaulting to 'root@pam' in this case actually has no effect but it's
> only in the logging for tasks.

if this is only ever used for logging (I did not get that from the commit message) then
warning makes not much sense, set $noerr and just fallback, i.e. something like

# note: below is for logging purpose only:
$user = $self->get_user(1) // 'root@pam' if !defined($user);

> 
> i talked to dietmar about this off-list before sending the v1, since
> this bit of code was originally imported from the svn repositories way
> back, and he told me that it was intended for the log.

k