From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Fabian Ebner <f.ebner@proxmox.com>,
Oguz Bektas <o.bektas@proxmox.com>
Subject: Re: [pve-devel] [PATCH v2 common] REST environment: default to root@pam in forked workers if no user was specified
Date: Tue, 15 Mar 2022 09:57:34 +0100 [thread overview]
Message-ID: <c116dfb2-0e34-a1de-b3dc-a5f3e48c64b5@proxmox.com> (raw)
In-Reply-To: <937a66e6-aa1f-5ef7-5f84-7814f8b6469f@proxmox.com>
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).
next prev parent reply other threads:[~2022-03-15 8:58 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-14 13:50 Oguz Bektas
2022-03-15 7:31 ` Fabian Ebner
2022-03-15 8:57 ` Thomas Lamprecht [this message]
2022-03-15 11:21 ` Oguz Bektas
2022-03-15 11:34 ` Thomas Lamprecht
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c116dfb2-0e34-a1de-b3dc-a5f3e48c64b5@proxmox.com \
--to=t.lamprecht@proxmox.com \
--cc=f.ebner@proxmox.com \
--cc=o.bektas@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox