[pve-devel] [PATCH access-control] fix #3513: pass configured proxy to OpenID

[pve-devel] [PATCH access-control] fix #3513: pass configured proxy to OpenID

[Fabian Grünbichler]

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
seemed like the easiest way to fix this - but we could also change the
proxmox-openid-rs API to take the proxy as parameter..

 src/PVE/API2/OpenId.pm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
index 22423ba..9080865 100644
--- a/src/PVE/API2/OpenId.pm
+++ b/src/PVE/API2/OpenId.pm
@@ -97,6 +97,9 @@ __PACKAGE__->register_method ({
     code => sub {
 	my ($param) = @_;
 
+	my $dcconf = PVE::Cluster::cfs_read_file('datacenter.cfg');
+	local $ENV{all_proxy} = $dcconf->{http_proxy};
+
 	my $realm = extract_param($param, 'realm');
 	my $redirect_url = extract_param($param, 'redirect-url');
 
@@ -149,6 +152,9 @@ __PACKAGE__->register_method ({
 
 	my $res;
 	eval {
+	    my $dcconf = PVE::Cluster::cfs_read_file('datacenter.cfg');
+	    local $ENV{all_proxy} = $dcconf->{http_proxy};
+
 	    my ($realm, $private_auth_state) = PVE::RS::OpenId::verify_public_auth_state(
 		$openid_state_path, $param->{'state'});
 
-- 
2.30.2

Re: [pve-devel] [PATCH access-control] fix #3513: pass configured proxy to OpenID

[Thomas Lamprecht]

On 13/07/2021 10:09, Fabian Grünbichler wrote:
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> seemed like the easiest way to fix this - but we could also change the
> proxmox-openid-rs API to take the proxy as parameter..
> 

seems OK in general, but do we only want to set it in case it actually has
a value? Not sure if non-existing and existing but empty makes any difference
here - e.g., a behavior that one could possibly imagine is that it would override
another source/default for a proxy to force no-proxy...

Mostly just asking if you thought about that, it's probably just a very vague and
theoretical issue..

>  src/PVE/API2/OpenId.pm | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
> index 22423ba..9080865 100644
> --- a/src/PVE/API2/OpenId.pm
> +++ b/src/PVE/API2/OpenId.pm
> @@ -97,6 +97,9 @@ __PACKAGE__->register_method ({
>      code => sub {
>  	my ($param) = @_;
>  
> +	my $dcconf = PVE::Cluster::cfs_read_file('datacenter.cfg');
> +	local $ENV{all_proxy} = $dcconf->{http_proxy};
> +
>  	my $realm = extract_param($param, 'realm');
>  	my $redirect_url = extract_param($param, 'redirect-url');
>  
> @@ -149,6 +152,9 @@ __PACKAGE__->register_method ({
>  
>  	my $res;
>  	eval {
> +	    my $dcconf = PVE::Cluster::cfs_read_file('datacenter.cfg');
> +	    local $ENV{all_proxy} = $dcconf->{http_proxy};
> +
>  	    my ($realm, $private_auth_state) = PVE::RS::OpenId::verify_public_auth_state(
>  		$openid_state_path, $param->{'state'});
>  
> 

Re: [pve-devel] [PATCH access-control] fix #3513: pass configured proxy to OpenID

[Fabian Grünbichler]

On August 25, 2021 6:31 pm, Thomas Lamprecht wrote:
> On 13/07/2021 10:09, Fabian Grünbichler wrote:
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>> seemed like the easiest way to fix this - but we could also change the
>> proxmox-openid-rs API to take the proxy as parameter..
>> 
> 
> seems OK in general, but do we only want to set it in case it actually has
> a value? Not sure if non-existing and existing but empty makes any difference
> here - e.g., a behavior that one could possibly imagine is that it would override
> another source/default for a proxy to force no-proxy...
> 
> Mostly just asking if you thought about that, it's probably just a very vague and
> theoretical issue..

Yeah I think this would be rather theoretical, as our API services run 
without that env variable set by default, so a user would have to 
edit/override the systemd unit to set the env variable instead of using 
our built-in mechanism..

but alas,

local %ENV;
if (my $http_proxy = $dcconf->{http_proxy}) {
  $ENV{all_proxy} = $http_proxy;
}

should work just as well but still preserve a fallback to pre-existing 
env variable

> 
>>  src/PVE/API2/OpenId.pm | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
>> index 22423ba..9080865 100644
>> --- a/src/PVE/API2/OpenId.pm
>> +++ b/src/PVE/API2/OpenId.pm
>> @@ -97,6 +97,9 @@ __PACKAGE__->register_method ({
>>      code => sub {
>>  	my ($param) = @_;
>>  
>> +	my $dcconf = PVE::Cluster::cfs_read_file('datacenter.cfg');
>> +	local $ENV{all_proxy} = $dcconf->{http_proxy};
>> +
>>  	my $realm = extract_param($param, 'realm');
>>  	my $redirect_url = extract_param($param, 'redirect-url');
>>  
>> @@ -149,6 +152,9 @@ __PACKAGE__->register_method ({
>>  
>>  	my $res;
>>  	eval {
>> +	    my $dcconf = PVE::Cluster::cfs_read_file('datacenter.cfg');
>> +	    local $ENV{all_proxy} = $dcconf->{http_proxy};
>> +
>>  	    my ($realm, $private_auth_state) = PVE::RS::OpenId::verify_public_auth_state(
>>  		$openid_state_path, $param->{'state'});
>>  
>> 
> 
> 

[pve-devel] applied: [PATCH access-control] fix #3513: pass configured proxy to OpenID

[Thomas Lamprecht]

On 13.07.21 10:09, Fabian Grünbichler wrote:
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> seemed like the easiest way to fix this - but we could also change the
> proxmox-openid-rs API to take the proxy as parameter..
> 
>  src/PVE/API2/OpenId.pm | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
>

applied, thanks! just to be sure I made a followup to only change the env if the config
variable actually exists:

--8<--
commit afda4f1a83a5c0bbd798facd27c0cb74759968ff
Author: Thomas Lamprecht <t.lamprecht@proxmox.com>
Date:   Wed Nov 3 11:30:05 2021 +0100

    openid: proxy: only set env var if DC-config property exists
    
    Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>

diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
index 9080865..4fc0be8 100644
--- a/src/PVE/API2/OpenId.pm
+++ b/src/PVE/API2/OpenId.pm
@@ -98,7 +98,7 @@ __PACKAGE__->register_method ({
 	my ($param) = @_;
 
 	my $dcconf = PVE::Cluster::cfs_read_file('datacenter.cfg');
-	local $ENV{all_proxy} = $dcconf->{http_proxy};
+	local $ENV{all_proxy} = $dcconf->{http_proxy} if exists $dcconf->{http_proxy};
 
 	my $realm = extract_param($param, 'realm');
 	my $redirect_url = extract_param($param, 'redirect-url');
@@ -153,7 +153,7 @@ __PACKAGE__->register_method ({
 	my $res;
 	eval {
 	    my $dcconf = PVE::Cluster::cfs_read_file('datacenter.cfg');
-	    local $ENV{all_proxy} = $dcconf->{http_proxy};
+	    local $ENV{all_proxy} = $dcconf->{http_proxy} if exists $dcconf->{http_proxy};
 
 	    my ($realm, $private_auth_state) = PVE::RS::OpenId::verify_public_auth_state(
 		$openid_state_path, $param->{'state'});