[pve-devel] [PATCH v2 container] fix #3313: recover unprivileged bit from old config during pct restore

[pve-devel] [PATCH v2 container] fix #3313: recover unprivileged bit from old config during pct restore

[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) {
-- 
2.20.1

Re: [pve-devel] [PATCH v2 container] fix #3313: recover unprivileged bit from old config during pct restore

[Fabian Ebner]

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) {
> 

Re: [pve-devel] [PATCH v2 container] fix #3313: recover unprivileged bit from old config during pct restore

[Thomas Lamprecht]

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) {
> 

Re: [pve-devel] [PATCH v2 container] fix #3313: recover unprivileged bit from old config during pct restore

[Oguz Bektas]

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) {
> > 
> 

Re: [pve-devel] [PATCH v2 container] fix #3313: recover unprivileged bit from old config during pct restore

[Thomas Lamprecht]

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) {
>>>
>>