all lists on lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH container v2 1/2] fix #5194: pct: delete environment variables set by pve
@ 2024-01-29 15:43 Folke Gleumes
  2024-01-29 15:43 ` [pve-devel] [PATCH container v2 2/2] pct: add keep-env option Folke Gleumes
  2024-02-02 16:32 ` [pve-devel] applied: [PATCH container v2 1/2] fix #5194: pct: delete environment variables set by pve Thomas Lamprecht
  0 siblings, 2 replies; 6+ messages in thread
From: Folke Gleumes @ 2024-01-29 15:43 UTC (permalink / raw)
  To: pve-devel

proxmox-perl-rs set's SSL_CERT_{DIR,FILE}, which can break ssl in
containers if their certificate store can't be found in the same spot.
This patch explicitly unsets those variables before starting the
container.

Signed-off-by: Folke Gleumes <f.gleumes@proxmox.com>
---
Changes since v1:
* Add reevaluation notice for pve9

 src/PVE/CLI/pct.pm | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
index a0b9bce..091ac8e 100755
--- a/src/PVE/CLI/pct.pm
+++ b/src/PVE/CLI/pct.pm
@@ -143,6 +143,16 @@ __PACKAGE__->register_method ({
 	exec(@$cmd);
     }});
 
+# TODO: Evaluate if still needed with PVE9
+sub clean_environment {
+    # These env variables are currently needed by PVE to work correctly with rust libraries,
+    # but can break ssl inside of containers.
+    # An explanation why they are needed and the code that sets them can be found here:
+    # https://git.proxmox.com/?p=proxmox-perl-rs.git;a=blob;f=common/pkg/Proxmox/Lib/SslProbe.pm
+    delete $ENV{SSL_CERT_FILE};
+    delete $ENV{SSL_CERT_DIR};
+};
+
 __PACKAGE__->register_method ({
     name => 'enter',
     path => 'enter',
@@ -164,6 +174,7 @@ __PACKAGE__->register_method ({
 	PVE::LXC::Config->load_config($vmid); # test if container exists on this node
 	die "container '$vmid' not running!\n" if !PVE::LXC::check_running($vmid);
 
+	clean_environment();
 	exec('lxc-attach', '-n',  $vmid);
     }});
 
@@ -189,6 +200,7 @@ __PACKAGE__->register_method ({
 
 	die "missing command" if !@{$param->{'extra-args'}};
 
+	clean_environment();
 	exec('lxc-attach', '-n', $vmid, '--', @{$param->{'extra-args'}});
     }});
 
-- 
2.39.2





^ permalink raw reply	[flat|nested] 6+ messages in thread

* [pve-devel] [PATCH container v2 2/2] pct: add keep-env option
  2024-01-29 15:43 [pve-devel] [PATCH container v2 1/2] fix #5194: pct: delete environment variables set by pve Folke Gleumes
@ 2024-01-29 15:43 ` Folke Gleumes
  2024-02-02 16:53   ` Thomas Lamprecht
  2024-02-02 16:32 ` [pve-devel] applied: [PATCH container v2 1/2] fix #5194: pct: delete environment variables set by pve Thomas Lamprecht
  1 sibling, 1 reply; 6+ messages in thread
From: Folke Gleumes @ 2024-01-29 15:43 UTC (permalink / raw)
  To: pve-devel

The keep-env option allows the user to define if the current environment
should be kept when running 'pct enter/exec'. pct will now always set
'--keep-env' or '--discard-env' when calling lxc-attach to anticipate
the upcoming change in default behavior.

Signed-off-by: Folke Gleumes <f.gleumes@proxmox.com>
---
This wasn't present in v1

 src/PVE/CLI/pct.pm | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
index 091ac8e..7ce0de0 100755
--- a/src/PVE/CLI/pct.pm
+++ b/src/PVE/CLI/pct.pm
@@ -162,12 +162,21 @@ __PACKAGE__->register_method ({
 	additionalProperties => 0,
 	properties => {
 	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::LXC::complete_ctid_running }),
+	    # TODO: set keep-env to default false with PVE 9
+	    'keep-env' => {
+		type => 'boolean',
+		description => "Keep the current environment. This option will disabled by default with PVE 9. "
+		    ."If you rely on a preserved environment, please use this option to be future-proof.",
+		optional => 1,
+		default => 1,
+	    },
 	},
     },
     returns => { type => 'null' },
 
     code => sub {
 	my ($param) = @_;
+	my $keep_env = $param->{'keep-env'} // 1; # TODO: toggle with pve 9
 
 	my $vmid = $param->{vmid};
 
@@ -175,7 +184,7 @@ __PACKAGE__->register_method ({
 	die "container '$vmid' not running!\n" if !PVE::LXC::check_running($vmid);
 
 	clean_environment();
-	exec('lxc-attach', '-n',  $vmid);
+	exec('lxc-attach', $keep_env ? '--keep-env' : '--clear-env', '-n', $vmid);
     }});
 
 __PACKAGE__->register_method ({
@@ -187,12 +196,21 @@ __PACKAGE__->register_method ({
 	additionalProperties => 0,
 	properties => {
 	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::LXC::complete_ctid_running }),
+	    # TODO: set keep-env to default false with PVE 9
+	    'keep-env' => {
+		type => 'boolean',
+		description => "Keep the current environment. This option will disabled by default with PVE 9. "
+		    ."If you rely on a preserved environment, please use this option to be future-proof.",
+		optional => 1,
+		default => 1,
+	    },
 	    'extra-args' => get_standard_option('extra-args'),
 	},
     },
     returns => { type => 'null' },
     code => sub {
 	my ($param) = @_;
+	my $keep_env = $param->{'keep-env'} // 1; # TODO: toggle with pve 9
 
 	my $vmid = $param->{vmid};
 	PVE::LXC::Config->load_config($vmid); # test if container exists on this node
@@ -201,7 +219,7 @@ __PACKAGE__->register_method ({
 	die "missing command" if !@{$param->{'extra-args'}};
 
 	clean_environment();
-	exec('lxc-attach', '-n', $vmid, '--', @{$param->{'extra-args'}});
+	exec('lxc-attach', $keep_env ? '--keep-env' : '--clear-env', '-n', $vmid, '--', @{$param->{'extra-args'}});
     }});
 
 __PACKAGE__->register_method ({
-- 
2.39.2





^ permalink raw reply	[flat|nested] 6+ messages in thread

* [pve-devel] applied: [PATCH container v2 1/2] fix #5194: pct: delete environment variables set by pve
  2024-01-29 15:43 [pve-devel] [PATCH container v2 1/2] fix #5194: pct: delete environment variables set by pve Folke Gleumes
  2024-01-29 15:43 ` [pve-devel] [PATCH container v2 2/2] pct: add keep-env option Folke Gleumes
@ 2024-02-02 16:32 ` Thomas Lamprecht
  1 sibling, 0 replies; 6+ messages in thread
From: Thomas Lamprecht @ 2024-02-02 16:32 UTC (permalink / raw)
  To: Proxmox VE development discussion, Folke Gleumes

Am 29/01/2024 um 16:43 schrieb Folke Gleumes:
> proxmox-perl-rs set's SSL_CERT_{DIR,FILE}, which can break ssl in
> containers if their certificate store can't be found in the same spot.
> This patch explicitly unsets those variables before starting the
> container.
> 
> Signed-off-by: Folke Gleumes <f.gleumes@proxmox.com>
> ---
> Changes since v1:
> * Add reevaluation notice for pve9
> 
>  src/PVE/CLI/pct.pm | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
>

applied, thanks!




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [pve-devel] [PATCH container v2 2/2] pct: add keep-env option
  2024-01-29 15:43 ` [pve-devel] [PATCH container v2 2/2] pct: add keep-env option Folke Gleumes
@ 2024-02-02 16:53   ` Thomas Lamprecht
  2024-02-09 13:17     ` [pve-devel] [PATCH container v3] " Folke Gleumes
  2024-02-09 19:07     ` [pve-devel] applied: [PATCH container v2 2/2] " Thomas Lamprecht
  0 siblings, 2 replies; 6+ messages in thread
From: Thomas Lamprecht @ 2024-02-02 16:53 UTC (permalink / raw)
  To: Proxmox VE development discussion, Folke Gleumes

Am 29/01/2024 um 16:43 schrieb Folke Gleumes:
> The keep-env option allows the user to define if the current environment
> should be kept when running 'pct enter/exec'. pct will now always set
> '--keep-env' or '--discard-env' when calling lxc-attach to anticipate
> the upcoming change in default behavior.

seems fine in general, but I saw a two code style nits, of which one I'd
really like to avoid, see comment in line.

> 
> Signed-off-by: Folke Gleumes <f.gleumes@proxmox.com>
> ---
> This wasn't present in v1
> 
>  src/PVE/CLI/pct.pm | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
> index 091ac8e..7ce0de0 100755
> --- a/src/PVE/CLI/pct.pm
> +++ b/src/PVE/CLI/pct.pm
> @@ -162,12 +162,21 @@ __PACKAGE__->register_method ({
>  	additionalProperties => 0,
>  	properties => {
>  	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::LXC::complete_ctid_running }),
> +	    # TODO: set keep-env to default false with PVE 9

no hard feelings for thiss one, but could be a better fit if located in the
default line, which would be the one that will be changed, e.g.:

default => 1, # TODO: make keep-env opt-out here and in the code with PVE 9

or before with even more context, something like:

# FIXME: passing the environment in CT is really not ideal, it can leak secrets or cause
# programs to do behave weird, so change to opt-in in PVE 9

Having some more context about the reasoning can be good for such comments
(or at least the commit message), even if it should be relatively clear,
like here.


> +	    'keep-env' => {> +		type => 'boolean',
> +		description => "Keep the current environment. This option will disabled by default with PVE 9. "

nit: for line continuations the next line should hold the space, that way
one can add or remove sentences more likely without touching other unrelated
lines. As example for here:

description => "Keep the current environment. This option will disabled by default with PVE 9."
    ." If you rely on a preserved environment, please use this option to be future-proof."

But would not have blocked applying just because of this, so mostly mentioning it
due to writing a reply anyway.

> +		    ."If you rely on a preserved environment, please use this option to be future-proof.",


> @@ -175,7 +184,7 @@ __PACKAGE__->register_method ({
>  	die "container '$vmid' not running!\n" if !PVE::LXC::check_running($vmid);
>  
>  	clean_environment();
> -	exec('lxc-attach', '-n',  $vmid);
> +	exec('lxc-attach', $keep_env ? '--keep-env' : '--clear-env', '-n', $vmid);

I'd prefer to avoid such "hidden" ternaries inside other statements, that makes
the code unnecessarily hard to read without much gain.
While it makes it longer, having this split over a few line seems a bit nicer, e.g.
something like (untested):

my @lxc_attach_cmd = ('lxc-attach', '-n',  $vmid);

push @lxc_attach_cmd, $keep_env ? '--keep-env' : '--clear-env';

exec(@lxc_attach_cmd);

>      }});
>  
>  __PACKAGE__->register_method ({
> @@ -187,12 +196,21 @@ __PACKAGE__->register_method ({
>  	additionalProperties => 0,
>  	properties => {
>  	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::LXC::complete_ctid_running }),
> +	    # TODO: set keep-env to default false with PVE 9
> +	    'keep-env' => {
> +		type => 'boolean',
> +		description => "Keep the current environment. This option will disabled by default with PVE 9. "

same here w.r.t. trailing space for literal string continuation

> +		    ."If you rely on a preserved environment, please use this option to be future-proof.",
> +		optional => 1,
> +		default => 1,
> +	    },
>  	    'extra-args' => get_standard_option('extra-args'),
>  	},
>      },
>      returns => { type => 'null' },
>      code => sub {
>  	my ($param) = @_;
> +	my $keep_env = $param->{'keep-env'} // 1; # TODO: toggle with pve 9
>  
>  	my $vmid = $param->{vmid};
>  	PVE::LXC::Config->load_config($vmid); # test if container exists on this node
> @@ -201,7 +219,7 @@ __PACKAGE__->register_method ({
>  	die "missing command" if !@{$param->{'extra-args'}};
>  
>  	clean_environment();
> -	exec('lxc-attach', '-n', $vmid, '--', @{$param->{'extra-args'}});
> +	exec('lxc-attach', $keep_env ? '--keep-env' : '--clear-env', '-n', $vmid, '--', @{$param->{'extra-args'}});

same here w.r.t. ternary, gets a bit unwieldy

>      }});
>  
>  __PACKAGE__->register_method ({





^ permalink raw reply	[flat|nested] 6+ messages in thread

* [pve-devel] [PATCH container v3] pct: add keep-env option
  2024-02-02 16:53   ` Thomas Lamprecht
@ 2024-02-09 13:17     ` Folke Gleumes
  2024-02-09 19:07     ` [pve-devel] applied: [PATCH container v2 2/2] " Thomas Lamprecht
  1 sibling, 0 replies; 6+ messages in thread
From: Folke Gleumes @ 2024-02-09 13:17 UTC (permalink / raw)
  To: pve-devel

The keep-env option allows the user to define if the current environment
should be kept when running 'pct enter/exec'. pct will now always set
'--keep-env' or '--clear-env' when calling lxc-attach to anticipate
the upcoming change in default behavior.

Signed-off-by: Folke Gleumes <f.gleumes@proxmox.com>
---

changes in v3:
* fix code style nits

changes in v2:
* add this patch

 src/PVE/CLI/pct.pm | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
index 091ac8e..325178f 100755
--- a/src/PVE/CLI/pct.pm
+++ b/src/PVE/CLI/pct.pm
@@ -162,12 +162,22 @@ __PACKAGE__->register_method ({
 	additionalProperties => 0,
 	properties => {
 	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::LXC::complete_ctid_running }),
+	    # FIXME: passing the environment into the container potentially leaks hosts secrets, or causes
+	    # unexpected behavior. Change to opt-in for pve 9
+	    'keep-env' => {
+		type => 'boolean',
+		description => "Keep the current environment. This option will disabled by default with PVE 9."
+		    ." If you rely on a preserved environment, please use this option to be future-proof.",
+		optional => 1,
+		default => 1,
+	    },
 	},
     },
     returns => { type => 'null' },
 
     code => sub {
 	my ($param) = @_;
+	my $keep_env = $param->{'keep-env'} // 1; # FIXME: switch to default 0 with pve 9, see above
 
 	my $vmid = $param->{vmid};
 
@@ -175,7 +185,10 @@ __PACKAGE__->register_method ({
 	die "container '$vmid' not running!\n" if !PVE::LXC::check_running($vmid);
 
 	clean_environment();
-	exec('lxc-attach', '-n',  $vmid);
+
+	my @lxc_attach_cmd =  ('lxc-attach', '-n',  $vmid);
+	push @lxc_attach_cmd, $keep_env ? '--keep-env' : '--clear-env';
+	exec(@lxc_attach_cmd);
     }});
 
 __PACKAGE__->register_method ({
@@ -187,12 +200,20 @@ __PACKAGE__->register_method ({
 	additionalProperties => 0,
 	properties => {
 	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::LXC::complete_ctid_running }),
+	    'keep-env' => {
+		type => 'boolean',
+		description => "Keep the current environment. This option will disabled by default with PVE 9."
+		    ." If you rely on a preserved environment, please use this option to be future-proof.",
+		optional => 1,
+		default => 1, # FIXME: switch to default 0 with pve 9, see enter method
+	    },
 	    'extra-args' => get_standard_option('extra-args'),
 	},
     },
     returns => { type => 'null' },
     code => sub {
 	my ($param) = @_;
+	my $keep_env = $param->{'keep-env'} // 1; # FIXME: switch to default 0 with pve 9, see enter method
 
 	my $vmid = $param->{vmid};
 	PVE::LXC::Config->load_config($vmid); # test if container exists on this node
@@ -201,7 +222,11 @@ __PACKAGE__->register_method ({
 	die "missing command" if !@{$param->{'extra-args'}};
 
 	clean_environment();
-	exec('lxc-attach', '-n', $vmid, '--', @{$param->{'extra-args'}});
+
+	my @lxc_attach_cmd =  ('lxc-attach', '-n',  $vmid);
+	push @lxc_attach_cmd, $keep_env ? '--keep-env' : '--clear-env';
+	push @lxc_attach_cmd, '--', @{$param->{'extra-args'}};
+	exec(@lxc_attach_cmd);
     }});
 
 __PACKAGE__->register_method ({
-- 
2.39.2





^ permalink raw reply	[flat|nested] 6+ messages in thread

* [pve-devel] applied: [PATCH container v2 2/2] pct: add keep-env option
  2024-02-02 16:53   ` Thomas Lamprecht
  2024-02-09 13:17     ` [pve-devel] [PATCH container v3] " Folke Gleumes
@ 2024-02-09 19:07     ` Thomas Lamprecht
  1 sibling, 0 replies; 6+ messages in thread
From: Thomas Lamprecht @ 2024-02-09 19:07 UTC (permalink / raw)
  To: Proxmox VE development discussion, Folke Gleumes

Am 02/02/2024 um 17:53 schrieb Thomas Lamprecht:
> Am 29/01/2024 um 16:43 schrieb Folke Gleumes:
>> The keep-env option allows the user to define if the current environment
>> should be kept when running 'pct enter/exec'. pct will now always set
>> '--keep-env' or '--discard-env' when calling lxc-attach to anticipate
>> the upcoming change in default behavior.
> 
> seems fine in general, but I saw a two code style nits, of which one I'd
> really like to avoid, see comment in line.
> 
>>
>> Signed-off-by: Folke Gleumes <f.gleumes@proxmox.com>
>> ---
>> This wasn't present in v1
>>
>>  src/PVE/CLI/pct.pm | 22 ++++++++++++++++++++--
>>  1 file changed, 20 insertions(+), 2 deletions(-)
>>
>>

applied, thanks!

Two small things though:
- there where two instances with double whitespace, e.g. after the
  `my @lxc_attach_cmd = `and before `$vmid`, and that then copied over
  to the other command too. It really did not seemed to be due to
  some vertical alignment, so I ameded the commit and dropped them.

- in general it's preferred to send new patches, single or series, as new
  thread, i.e., without in-reply too. I know some open source projects
  sometimes are fine with sending those as in-reply-to, but IMO that
  hides them more. It's naturally fine to propose ad-hock diffs/code
  in a discussion in thread, but if there's a full patch with a new
  revision, then one is basically always better off when sending that
  as new thread.

Oh, and while at it, w.r.t perl array-refs you nowadays can write
`@{$foo->{bar}}` as `$foo->{bar}->@*`, and you can concat lists,
so the final "add base command + user params then exec" part could
look like:

exec(@lxc_attach_cmd, '--', $foo->{bar}->@*);

I kept that as is for now, it's IMO really not worse if done explicitly.




^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-02-09 19:07 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-29 15:43 [pve-devel] [PATCH container v2 1/2] fix #5194: pct: delete environment variables set by pve Folke Gleumes
2024-01-29 15:43 ` [pve-devel] [PATCH container v2 2/2] pct: add keep-env option Folke Gleumes
2024-02-02 16:53   ` Thomas Lamprecht
2024-02-09 13:17     ` [pve-devel] [PATCH container v3] " Folke Gleumes
2024-02-09 19:07     ` [pve-devel] applied: [PATCH container v2 2/2] " Thomas Lamprecht
2024-02-02 16:32 ` [pve-devel] applied: [PATCH container v2 1/2] fix #5194: pct: delete environment variables set by pve Thomas Lamprecht

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal