* [pve-devel] [PATCH v2 container] fix #3313: recover unprivileged bit from old config during pct restore
@ 2021-02-23 12:29 Oguz Bektas
2021-02-23 14:19 ` Fabian Ebner
2021-02-23 14:21 ` Thomas Lamprecht
0 siblings, 2 replies; 5+ messages in thread
From: Oguz Bektas @ 2021-02-23 12:29 UTC (permalink / raw)
To: pve-devel
since pct defaults to privileged containers, it restores the container
as privileged when `--unprivileged 1` is not passed.
instead we should check the old configuration and retrieve it
from there.
this way, when one creates an unprivileged container on GUI, it will be
still restored as unprivileged via pct (without having to pass
`--unprivileged 1` parameter)
Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v1->v2:
* move the $is_root guard
* wrap line to make it shorter
* shorten comment
* use () around defined
* also check defined($orig_conf->{unprivileged})
src/PVE/API2/LXC.pm | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 8ce462f..3d3dbb0 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -352,7 +352,7 @@ __PACKAGE__->register_method({
my $orig_mp_param; # only used if $restore
if ($restore) {
die "can't overwrite running container\n" if PVE::LXC::check_running($vmid);
- if ($is_root && $archive ne '-') {
+ if ($archive ne '-') {
my $orig_conf;
print "recovering backed-up configuration from '$archive'\n";
($orig_conf, $orig_mp_param) = PVE::LXC::Create::recover_config($storage_cfg, $archive, $vmid);
@@ -361,7 +361,11 @@ __PACKAGE__->register_method({
# causing it to restore the raw lxc entries, among which there may be
# 'lxc.idmap' entries. We need to make sure that the extracted contents
# of the container match up with the restored configuration afterwards:
- $conf->{lxc} = $orig_conf->{lxc};
+ $conf->{lxc} = $orig_conf->{lxc} if $is_root;
+
+ # make sure to retrieve the privilege level of container if not specified
+ $conf->{unprivileged} = $orig_conf->{unprivileged} if !defined($unprivileged)
+ && defined($orig_conf->{unprivileged});
}
}
if ($storage_only_mode) {
--
2.20.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pve-devel] [PATCH v2 container] fix #3313: recover unprivileged bit from old config during pct restore
2021-02-23 12:29 [pve-devel] [PATCH v2 container] fix #3313: recover unprivileged bit from old config during pct restore Oguz Bektas
@ 2021-02-23 14:19 ` Fabian Ebner
2021-02-23 14:21 ` Thomas Lamprecht
1 sibling, 0 replies; 5+ messages in thread
From: Fabian Ebner @ 2021-02-23 14:19 UTC (permalink / raw)
To: pve-devel, o.bektas
Works for me and also fixes the "template restore as non-root" issue
that apparently nobody complained about.
It was a bit confusing to think about $orig_mp_param, whose assignment
is now also not guarded by $is_root anymore. But if $orig_mp_param is
used, i.e. if we enter the
if ($storage_only_mode) {
if ($restore) {
block, it's set by a second recover_config() call anyways, so that
should be fine too.
Reviewed-by: Fabian Ebner <f.ebner@proxmox.com>
Tested-by: Fabian Ebner <f.ebner@proxmox.com>
Am 23.02.21 um 13:29 schrieb Oguz Bektas:
> since pct defaults to privileged containers, it restores the container
> as privileged when `--unprivileged 1` is not passed.
>
> instead we should check the old configuration and retrieve it
> from there.
>
> this way, when one creates an unprivileged container on GUI, it will be
> still restored as unprivileged via pct (without having to pass
> `--unprivileged 1` parameter)
>
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
> v1->v2:
> * move the $is_root guard
> * wrap line to make it shorter
> * shorten comment
> * use () around defined
> * also check defined($orig_conf->{unprivileged})
>
>
> src/PVE/API2/LXC.pm | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 8ce462f..3d3dbb0 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -352,7 +352,7 @@ __PACKAGE__->register_method({
> my $orig_mp_param; # only used if $restore
> if ($restore) {
> die "can't overwrite running container\n" if PVE::LXC::check_running($vmid);
> - if ($is_root && $archive ne '-') {
> + if ($archive ne '-') {
> my $orig_conf;
> print "recovering backed-up configuration from '$archive'\n";
> ($orig_conf, $orig_mp_param) = PVE::LXC::Create::recover_config($storage_cfg, $archive, $vmid);
> @@ -361,7 +361,11 @@ __PACKAGE__->register_method({
> # causing it to restore the raw lxc entries, among which there may be
> # 'lxc.idmap' entries. We need to make sure that the extracted contents
> # of the container match up with the restored configuration afterwards:
> - $conf->{lxc} = $orig_conf->{lxc};
> + $conf->{lxc} = $orig_conf->{lxc} if $is_root;
> +
> + # make sure to retrieve the privilege level of container if not specified
> + $conf->{unprivileged} = $orig_conf->{unprivileged} if !defined($unprivileged)
> + && defined($orig_conf->{unprivileged});
> }
> }
> if ($storage_only_mode) {
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pve-devel] [PATCH v2 container] fix #3313: recover unprivileged bit from old config during pct restore
2021-02-23 12:29 [pve-devel] [PATCH v2 container] fix #3313: recover unprivileged bit from old config during pct restore Oguz Bektas
2021-02-23 14:19 ` Fabian Ebner
@ 2021-02-23 14:21 ` Thomas Lamprecht
2021-02-23 14:36 ` Oguz Bektas
1 sibling, 1 reply; 5+ messages in thread
From: Thomas Lamprecht @ 2021-02-23 14:21 UTC (permalink / raw)
To: Proxmox VE development discussion, Oguz Bektas
On 23.02.21 13:29, Oguz Bektas wrote:
> since pct defaults to privileged containers, it restores the container
> as privileged when `--unprivileged 1` is not passed.
>
> instead we should check the old configuration and retrieve it
> from there.
>
> this way, when one creates an unprivileged container on GUI, it will be
> still restored as unprivileged via pct (without having to pass
> `--unprivileged 1` parameter)
>
please note the effects of your change to `if ($is_root && $archive ne '-') {`
Fabi describes, pick up his R-b/T-b tag and send a v3 with the style comments
below addressed.
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
> v1->v2:
> * move the $is_root guard
> * wrap line to make it shorter
> * shorten comment
> * use () around defined
> * also check defined($orig_conf->{unprivileged})
>
>
> src/PVE/API2/LXC.pm | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 8ce462f..3d3dbb0 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -352,7 +352,7 @@ __PACKAGE__->register_method({
> my $orig_mp_param; # only used if $restore
> if ($restore) {
> die "can't overwrite running container\n" if PVE::LXC::check_running($vmid);
> - if ($is_root && $archive ne '-') {
> + if ($archive ne '-') {
> my $orig_conf;
> print "recovering backed-up configuration from '$archive'\n";
> ($orig_conf, $orig_mp_param) = PVE::LXC::Create::recover_config($storage_cfg, $archive, $vmid);
> @@ -361,7 +361,11 @@ __PACKAGE__->register_method({
> # causing it to restore the raw lxc entries, among which there may be
> # 'lxc.idmap' entries. We need to make sure that the extracted contents
> # of the container match up with the restored configuration afterwards:
> - $conf->{lxc} = $orig_conf->{lxc};
> + $conf->{lxc} = $orig_conf->{lxc} if $is_root;
> +
> + # make sure to retrieve the privilege level of container if not specified
Does this really adds any value in your opinion? It IMO adds even some confusion
as its not clear where has to be "not specified"... I'd really just drop it.
> + $conf->{unprivileged} = $orig_conf->{unprivileged} if !defined($unprivileged)
> + && defined($orig_conf->{unprivileged});
that's not how we wrap lines for post ifs, as you can se from looking at any code
of ours...
Wrote it now also down as more definite rule in the Perl Style Guide
https://pve.proxmox.com/wiki/Perl_Style_Guide#Wrapping_Post-If
> }
> }
> if ($storage_only_mode) {
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pve-devel] [PATCH v2 container] fix #3313: recover unprivileged bit from old config during pct restore
2021-02-23 14:21 ` Thomas Lamprecht
@ 2021-02-23 14:36 ` Oguz Bektas
2021-02-23 14:49 ` Thomas Lamprecht
0 siblings, 1 reply; 5+ messages in thread
From: Oguz Bektas @ 2021-02-23 14:36 UTC (permalink / raw)
To: Thomas Lamprecht; +Cc: Proxmox VE development discussion
hi,
On Tue, Feb 23, 2021 at 03:21:28PM +0100, Thomas Lamprecht wrote:
> On 23.02.21 13:29, Oguz Bektas wrote:
> > since pct defaults to privileged containers, it restores the container
> > as privileged when `--unprivileged 1` is not passed.
> >
> > instead we should check the old configuration and retrieve it
> > from there.
> >
> > this way, when one creates an unprivileged container on GUI, it will be
> > still restored as unprivileged via pct (without having to pass
> > `--unprivileged 1` parameter)
> >
>
> please note the effects of your change to `if ($is_root && $archive ne '-') {`
> Fabi describes, pick up his R-b/T-b tag and send a v3 with the style comments
> below addressed.
will do
>
> > Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> > ---
> > v1->v2:
> > * move the $is_root guard
> > * wrap line to make it shorter
> > * shorten comment
> > * use () around defined
> > * also check defined($orig_conf->{unprivileged})
> >
> >
> > src/PVE/API2/LXC.pm | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> > index 8ce462f..3d3dbb0 100644
> > --- a/src/PVE/API2/LXC.pm
> > +++ b/src/PVE/API2/LXC.pm
> > @@ -352,7 +352,7 @@ __PACKAGE__->register_method({
> > my $orig_mp_param; # only used if $restore
> > if ($restore) {
> > die "can't overwrite running container\n" if PVE::LXC::check_running($vmid);
> > - if ($is_root && $archive ne '-') {
> > + if ($archive ne '-') {
> > my $orig_conf;
> > print "recovering backed-up configuration from '$archive'\n";
> > ($orig_conf, $orig_mp_param) = PVE::LXC::Create::recover_config($storage_cfg, $archive, $vmid);
> > @@ -361,7 +361,11 @@ __PACKAGE__->register_method({
> > # causing it to restore the raw lxc entries, among which there may be
> > # 'lxc.idmap' entries. We need to make sure that the extracted contents
> > # of the container match up with the restored configuration afterwards:
> > - $conf->{lxc} = $orig_conf->{lxc};
> > + $conf->{lxc} = $orig_conf->{lxc} if $is_root;
> > +
> > + # make sure to retrieve the privilege level of container if not specified
>
> Does this really adds any value in your opinion? It IMO adds even some confusion
> as its not clear where has to be "not specified"... I'd really just drop it.
i adapt it to:
# retrieve the privilege level of container if cli parameter was not passed
otherwise i think it's not super obvious what's going on, since this
part of the code has a lot of special cases
>
>
> > + $conf->{unprivileged} = $orig_conf->{unprivileged} if !defined($unprivileged)
> > + && defined($orig_conf->{unprivileged});
>
> that's not how we wrap lines for post ifs, as you can se from looking at any code
> of ours...
>
> Wrote it now also down as more definite rule in the Perl Style Guide
> https://pve.proxmox.com/wiki/Perl_Style_Guide#Wrapping_Post-If
ok thank you
>
> > }
> > }
> > if ($storage_only_mode) {
> >
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pve-devel] [PATCH v2 container] fix #3313: recover unprivileged bit from old config during pct restore
2021-02-23 14:36 ` Oguz Bektas
@ 2021-02-23 14:49 ` Thomas Lamprecht
0 siblings, 0 replies; 5+ messages in thread
From: Thomas Lamprecht @ 2021-02-23 14:49 UTC (permalink / raw)
To: Oguz Bektas, Proxmox VE development discussion
On 23.02.21 15:36, Oguz Bektas wrote:
> hi,
>
> On Tue, Feb 23, 2021 at 03:21:28PM +0100, Thomas Lamprecht wrote:
>> On 23.02.21 13:29, Oguz Bektas wrote:
>>> since pct defaults to privileged containers, it restores the container
>>> as privileged when `--unprivileged 1` is not passed.
>>>
>>> instead we should check the old configuration and retrieve it
>>> from there.
>>>
>>> this way, when one creates an unprivileged container on GUI, it will be
>>> still restored as unprivileged via pct (without having to pass
>>> `--unprivileged 1` parameter)
>>>
>>
>> please note the effects of your change to `if ($is_root && $archive ne '-') {`
>> Fabi describes, pick up his R-b/T-b tag and send a v3 with the style comments
>> below addressed.
>
> will do
>
>>
>>> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
>>> ---
>>> v1->v2:
>>> * move the $is_root guard
>>> * wrap line to make it shorter
>>> * shorten comment
>>> * use () around defined
>>> * also check defined($orig_conf->{unprivileged})
>>>
>>>
>>> src/PVE/API2/LXC.pm | 8 ++++++--
>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
>>> index 8ce462f..3d3dbb0 100644
>>> --- a/src/PVE/API2/LXC.pm
>>> +++ b/src/PVE/API2/LXC.pm
>>> @@ -352,7 +352,7 @@ __PACKAGE__->register_method({
>>> my $orig_mp_param; # only used if $restore
>>> if ($restore) {
>>> die "can't overwrite running container\n" if PVE::LXC::check_running($vmid);
>>> - if ($is_root && $archive ne '-') {
>>> + if ($archive ne '-') {
>>> my $orig_conf;
>>> print "recovering backed-up configuration from '$archive'\n";
>>> ($orig_conf, $orig_mp_param) = PVE::LXC::Create::recover_config($storage_cfg, $archive, $vmid);
>>> @@ -361,7 +361,11 @@ __PACKAGE__->register_method({
>>> # causing it to restore the raw lxc entries, among which there may be
>>> # 'lxc.idmap' entries. We need to make sure that the extracted contents
>>> # of the container match up with the restored configuration afterwards:
>>> - $conf->{lxc} = $orig_conf->{lxc};
>>> + $conf->{lxc} = $orig_conf->{lxc} if $is_root;
>>> +
>>> + # make sure to retrieve the privilege level of container if not specified
>>
>> Does this really adds any value in your opinion? It IMO adds even some confusion
>> as its not clear where has to be "not specified"... I'd really just drop it.
>
> i adapt it to:
> # retrieve the privilege level of container if cli parameter was not passed
CLI is wrong, it's API (CLI is just derived from API).
>
> otherwise i think it's not super obvious what's going on, since this
> part of the code has a lot of special cases
>
the "whats going on" is obvious, and a why is rather uncontroversial here, missing this
was a clear bug.
To me this is like commenting a
# add orig comment if not api comment
$comment = $orig_comment if !defined($api_comment) && defined($orig_comment);
or almost like
# set a to 1 if $b is true
$a = 1 if $b;
which gets one to commenting every da+#!n value assignment? no thanks!
>>
>>
>>> + $conf->{unprivileged} = $orig_conf->{unprivileged} if !defined($unprivileged)
>>> + && defined($orig_conf->{unprivileged});
>>
>> that's not how we wrap lines for post ifs, as you can se from looking at any code
>> of ours...
>>
>> Wrote it now also down as more definite rule in the Perl Style Guide
>> https://pve.proxmox.com/wiki/Perl_Style_Guide#Wrapping_Post-If
>
> ok thank you
>>
>>> }
>>> }
>>> if ($storage_only_mode) {
>>>
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-02-23 14:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-23 12:29 [pve-devel] [PATCH v2 container] fix #3313: recover unprivileged bit from old config during pct restore Oguz Bektas
2021-02-23 14:19 ` Fabian Ebner
2021-02-23 14:21 ` Thomas Lamprecht
2021-02-23 14:36 ` Oguz Bektas
2021-02-23 14:49 ` Thomas Lamprecht
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox