[pve-devel] [PATCH container] fix #5194: delete environment variables set by pve

[pve-devel] [PATCH container] fix #5194: delete environment variables set by pve

[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>
---
 src/PVE/CLI/pct.pm | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
index a0b9bce..53519e4 100755
--- a/src/PVE/CLI/pct.pm
+++ b/src/PVE/CLI/pct.pm
@@ -143,6 +143,15 @@ __PACKAGE__->register_method ({
 	exec(@$cmd);
     }});
 
+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 +173,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 +199,7 @@ __PACKAGE__->register_method ({
 
 	die "missing command" if !@{$param->{'extra-args'}};
 
+	clean_environment();
 	exec('lxc-attach', '-n', $vmid, '--', @{$param->{'extra-args'}});
     }});
 
-- 
2.39.2

Re: [pve-devel] [PATCH container] fix #5194: delete environment variables set by pve

[Fabian Grünbichler]

On January 22, 2024 11:12 am, Folke Gleumes wrote:
> 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.

after a short talk with Wolfgang - this patch is probably an okay
stop-gap to fix the particular regression.

but it might be nice to switch to `--clear-env` for lxc-attach with
corresponding options for pct to either preserve the whole env, or
particular variables? might be 9.0 material since it is a semantic
change that possibly breaks scripted use cases that rely on env
variables to pass along things from host to whatever they run inside the
container.. we could introduce the options now though and also have a
`--keep-env` that is the default for 8.x, and flip it to default to
`--clear-env` with 9.0.

> 
> Signed-off-by: Folke Gleumes <f.gleumes@proxmox.com>
> ---
>  src/PVE/CLI/pct.pm | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
> index a0b9bce..53519e4 100755
> --- a/src/PVE/CLI/pct.pm
> +++ b/src/PVE/CLI/pct.pm
> @@ -143,6 +143,15 @@ __PACKAGE__->register_method ({
>  	exec(@$cmd);
>      }});
>  
> +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 +173,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 +199,7 @@ __PACKAGE__->register_method ({
>  
>  	die "missing command" if !@{$param->{'extra-args'}};
>  
> +	clean_environment();
>  	exec('lxc-attach', '-n', $vmid, '--', @{$param->{'extra-args'}});
>      }});
>  
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH container] fix #5194: delete environment variables set by pve

[Folke Gleumes]

On Tue, 2024-01-23 at 10:51 +0100, Fabian Grünbichler wrote:
> On January 22, 2024 11:12 am, Folke Gleumes wrote:
> > 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.
> 
> after a short talk with Wolfgang - this patch is probably an okay
> stop-gap to fix the particular regression.
If I understood things correctly, setting the env variables won't be
necessary with the next Debian major release, so I'll add a notice to
remove the workaround with pve 9 [0].
> 
> but it might be nice to switch to `--clear-env` for lxc-attach with
> corresponding options for pct to either preserve the whole env, or
> particular variables? might be 9.0 material since it is a semantic
> change that possibly breaks scripted use cases that rely on env
> variables to pass along things from host to whatever they run inside
> the
> container.. we could introduce the options now though and also have a
> `--keep-env` that is the default for 8.x, and flip it to default to
> `--clear-env` with 9.0.
Seems like a good idea. I also noticed that the lxc-attach man page
currently states "[keep-env] is the current default behaviour  (as  of
version  0.9),  but is is likely to change in the future". By defining
it explicitly, we would be free to decide when to introduce the change.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=4979
> 
> > 
> > Signed-off-by: Folke Gleumes <f.gleumes@proxmox.com>
> > ---
> >  src/PVE/CLI/pct.pm | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
> > index a0b9bce..53519e4 100755
> > --- a/src/PVE/CLI/pct.pm
> > +++ b/src/PVE/CLI/pct.pm
> > @@ -143,6 +143,15 @@ __PACKAGE__->register_method ({
> >         exec(@$cmd);
> >      }});
> >  
> > +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 +173,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 +199,7 @@ __PACKAGE__->register_method ({
> >  
> >         die "missing command" if !@{$param->{'extra-args'}};
> >  
> > +       clean_environment();
> >         exec('lxc-attach', '-n', $vmid, '--', @{$param->{'extra-
> > args'}});
> >      }});
> >  
> > -- 
> > 2.39.2
> > 
> > 
> > 
> > _______________________________________________
> > pve-devel mailing list
> > pve-devel@lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> > 
> > 
> > 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

Re: [pve-devel] [PATCH container] fix #5194: delete environment variables set by pve

[Wolfgang Bumiller]

On Fri, Jan 26, 2024 at 12:39:17PM +0100, Folke Gleumes wrote:
> On Tue, 2024-01-23 at 10:51 +0100, Fabian Grünbichler wrote:
> > On January 22, 2024 11:12 am, Folke Gleumes wrote:
> > > 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.
> > 
> > after a short talk with Wolfgang - this patch is probably an okay
> > stop-gap to fix the particular regression.
> If I understood things correctly, setting the env variables won't be
> necessary with the next Debian major release, so I'll add a notice to
> remove the workaround with pve 9 [0].

Just note that while it won't be necessary for us to set them *early*,
they will still be set by the `openssl-probe` if any rust code calling
into something related to that from the `openssl` crate is called.
This is already kind of annoying, but nonetheless, at host/container
boundaries we should always deal with the environment anyway.

> > 
> > but it might be nice to switch to `--clear-env` for lxc-attach with
> > corresponding options for pct to either preserve the whole env, or
> > particular variables? might be 9.0 material since it is a semantic
> > change that possibly breaks scripted use cases that rely on env
> > variables to pass along things from host to whatever they run inside
> > the
> > container.. we could introduce the options now though and also have a
> > `--keep-env` that is the default for 8.x, and flip it to default to
> > `--clear-env` with 9.0.
> Seems like a good idea. I also noticed that the lxc-attach man page
> currently states "[keep-env] is the current default behaviour  (as  of
> version  0.9),  but is is likely to change in the future". By defining
> it explicitly, we would be free to decide when to introduce the change.

Exactly.