[pve-devel] [PATCH SERIES access-control/docs/manager/perl-rs/proxmox-openid v3] Make OIDC userinfo endpoint optional

[pve-devel] [PATCH SERIES access-control/docs/manager/perl-rs/proxmox-openid v3] Make OIDC userinfo endpoint optional

[Thomas Skinner]

Continues work on adding an option to disable querying the userinfo endpoint for an
OIDC provider.

Changes since v2:
- Adjust verify_authorization_code in pve-rs to be backwards compatible
- Fix defaults in wrapper functions

access-control:

Thomas Skinner (1):
  fix #4234: add library functions for openid optional userinfo request

 src/PVE/API2/OpenId.pm | 6 +++++-
 src/PVE/Auth/OpenId.pm | 7 +++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

 
docs:

Thomas Skinner (1):
  fix #4234: add docs for openid optional userinfo request

 pveum.adoc | 8 ++++++++
 1 file changed, 8 insertions(+)

 
manager:

Thomas Skinner (1):
  fix #4234: add GUI option for openid optional userinfo request

 www/manager6/dc/AuthEditOpenId.js | 9 +++++++++
 1 file changed, 9 insertions(+)

 
perl-rs:

Thomas Skinner (1):
  fix #4234: openid: adjust openid verification function for userinfo
    option

 pve-rs/src/openid/mod.rs | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

 
proxmox-openid:

Thomas Skinner (1):
  fix #4234: openid: add library functions for optional userinfo
    endpoint

 proxmox-openid/src/lib.rs | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

 
-- 
2.39.5


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH docs v3 1/1] fix #4234: add docs for openid optional userinfo request

[Thomas Skinner]

Signed-off-by: Thomas Skinner <thomas@atskinner.net>
---
 pveum.adoc | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/pveum.adoc b/pveum.adoc
index 81565ab..1d18d38 100644
--- a/pveum.adoc
+++ b/pveum.adoc
@@ -479,6 +479,14 @@ Another option is to use `email`, which also yields human readable
 usernames. Again, only use this setting if the server guarantees the
 uniqueness of this attribute.
 
+Advanced settings
+^^^^^^^^^^^^^^^^^
+
+* `Disable userinfo request` (`disable-userinfo`): Enabling this option prevents
+the OpenID Connect authenticator from querying the "userinfo" endpoint for claim
+values. This is useful for some identity providers that do not support the "userinfo"
+endpoint (e.g. ADFS).
+
 Examples
 ^^^^^^^^
 
-- 
2.39.5


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH manager v3 1/1] fix #4234: add GUI option for openid optional userinfo request

[Thomas Skinner]

Signed-off-by: Thomas Skinner <thomas@atskinner.net>
---
 www/manager6/dc/AuthEditOpenId.js | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/www/manager6/dc/AuthEditOpenId.js b/www/manager6/dc/AuthEditOpenId.js
index 544c0de5..904e508c 100644
--- a/www/manager6/dc/AuthEditOpenId.js
+++ b/www/manager6/dc/AuthEditOpenId.js
@@ -111,6 +111,15 @@ Ext.define('PVE.panel.OpenIDInputPanel', {
 		deleteEmpty: '{!isCreate}',
 	    },
 	},
+	{
+	    xtype: 'proxmoxcheckbox',
+	    fieldLabel: gettext('Disable userinfo request'),
+	    name: 'disable-userinfo',
+	    value: 0,
+	    cbind: {
+		deleteEmpty: '{!isCreate}',
+	    },
+	},
     ],
 
     initComponent: function() {
-- 
2.39.5


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH access-control v3 1/1] fix #4234: add library functions for openid optional userinfo request

[Thomas Skinner]

Signed-off-by: Thomas Skinner <thomas@atskinner.net>
---
 src/PVE/API2/OpenId.pm | 6 +++++-
 src/PVE/Auth/OpenId.pm | 7 +++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
index 77410e6..456e96a 100644
--- a/src/PVE/API2/OpenId.pm
+++ b/src/PVE/API2/OpenId.pm
@@ -170,7 +170,11 @@ __PACKAGE__->register_method ({
 
 	    my ($config, $openid) = $lookup_openid_auth->($realm, $redirect_url);
 
-	    my $info = $openid->verify_authorization_code($param->{code}, $private_auth_state);
+	    my $info = $openid->verify_authorization_code(
+		$param->{code},
+		$private_auth_state,
+		$config->{'disable-userinfo'} // 0,
+	    );
 	    my $subject = $info->{'sub'};
 
 	    my $unique_name;
diff --git a/src/PVE/Auth/OpenId.pm b/src/PVE/Auth/OpenId.pm
index c8e4db9..6efa696 100755
--- a/src/PVE/Auth/OpenId.pm
+++ b/src/PVE/Auth/OpenId.pm
@@ -63,6 +63,12 @@ sub properties {
 	    pattern => '^[^\x00-\x1F\x7F <>#"]*$', # Prohibit characters not allowed in URI RFC 2396.
 	    optional => 1,
 	},
+	"disable-userinfo" => {
+	    description => "Prevents querying the userinfo endpoint for claims values.",
+	    type => 'boolean',
+	    default => 0,
+	    optional => 1,
+	},
    };
 }
 
@@ -78,6 +84,7 @@ sub options {
 	"acr-values" => { optional => 1 },
 	default => { optional => 1 },
 	comment => { optional => 1 },
+	"disable-userinfo" => { optional => 1 },
     };
 }
 
-- 
2.39.5


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH proxmox-openid v3 1/1] fix #4234: openid: add library functions for optional userinfo endpoint

[Thomas Skinner]

Signed-off-by: Thomas Skinner <thomas@atskinner.net>
---
 proxmox-openid/src/lib.rs | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/proxmox-openid/src/lib.rs b/proxmox-openid/src/lib.rs
index fe65fded..d2a53d45 100644
--- a/proxmox-openid/src/lib.rs
+++ b/proxmox-openid/src/lib.rs
@@ -31,6 +31,7 @@ use openidconnect::{
     PkceCodeVerifier,
     RedirectUrl,
     Scope,
+    StandardClaims,
     UserInfoClaims,
 };
 
@@ -195,6 +196,15 @@ impl OpenIdAuthenticator {
         &self,
         code: &str,
         private_auth_state: &PrivateAuthState,
+    ) -> Result<(CoreIdTokenClaims, GenericUserInfoClaims), Error> {
+        self.verify_authorization_code_userinfo(code, private_auth_state, false)
+    }
+
+    pub fn verify_authorization_code_userinfo(
+        &self,
+        code: &str,
+        private_auth_state: &PrivateAuthState,
+        disable_userinfo: bool,
     ) -> Result<(CoreIdTokenClaims, GenericUserInfoClaims), Error> {
         let code = AuthorizationCode::new(code.to_string());
         // Exchange the code with a token.
@@ -213,6 +223,14 @@ impl OpenIdAuthenticator {
             .claims(&id_token_verifier, &private_auth_state.nonce)
             .map_err(|err| format_err!("Failed to verify ID token: {}", err))?;
 
+        if disable_userinfo {
+            let empty_userinfo_claims = UserInfoClaims::new(
+                StandardClaims::new(id_token_claims.subject().clone()),
+                GenericClaims(Value::Null),
+            );
+            return Ok((id_token_claims.clone(), empty_userinfo_claims));
+        }
+
         let userinfo_claims: GenericUserInfoClaims = self
             .client
             .user_info(token_response.access_token().to_owned(), None)?
@@ -227,9 +245,19 @@ impl OpenIdAuthenticator {
         &self,
         code: &str,
         private_auth_state: &PrivateAuthState,
+    ) -> Result<Value, Error> {
+        self.verify_authorization_code_simple_userinfo(code, private_auth_state, false)
+    }
+
+    /// Like verify_authorization_code_simple_userinfo(), but returns claims as serde_json::Value
+    pub fn verify_authorization_code_simple_userinfo(
+        &self,
+        code: &str,
+        private_auth_state: &PrivateAuthState,
+        disable_userinfo: bool,
     ) -> Result<Value, Error> {
         let (id_token_claims, userinfo_claims) =
-            self.verify_authorization_code(code, private_auth_state)?;
+            self.verify_authorization_code_userinfo(code, private_auth_state, disable_userinfo)?;
 
         let mut data = serde_json::to_value(id_token_claims)?;
 
-- 
2.39.5


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH perl-rs v3 1/1] fix #4234: openid: adjust openid verification function for userinfo option

[Thomas Skinner]

Signed-off-by: Thomas Skinner <thomas@atskinner.net>
---
 pve-rs/src/openid/mod.rs | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/pve-rs/src/openid/mod.rs b/pve-rs/src/openid/mod.rs
index 1fa7572..8f914ad 100644
--- a/pve-rs/src/openid/mod.rs
+++ b/pve-rs/src/openid/mod.rs
@@ -54,9 +54,14 @@ mod export {
         #[try_from_ref] this: &OpenId,
         code: &str,
         private_auth_state: PrivateAuthState,
+        disable_userinfo: Option<bool>,
     ) -> Result<Value, Error> {
         let open_id = this.inner.lock().unwrap();
-        let claims = open_id.verify_authorization_code_simple(code, &private_auth_state)?;
+        let claims = open_id.verify_authorization_code_simple_userinfo(
+            code,
+            &private_auth_state,
+            disable_userinfo.unwrap_or(false),
+        )?;
 
         Ok(to_value(&claims)?)
     }
-- 
2.39.5


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH SERIES access-control/docs/manager/perl-rs/proxmox-openid v3] Make OIDC userinfo endpoint optional

[Mira Limbeck]

On 2/8/25 06:42, Thomas Skinner wrote:
> Continues work on adding an option to disable querying the userinfo endpoint for an
> OIDC provider.
> 
> Changes since v2:
> - Adjust verify_authorization_code in pve-rs to be backwards compatible
> - Fix defaults in wrapper functions
> 
> access-control:
> 
> Thomas Skinner (1):
>   fix #4234: add library functions for openid optional userinfo request
> 
>  src/PVE/API2/OpenId.pm | 6 +++++-
>  src/PVE/Auth/OpenId.pm | 7 +++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
>  
> docs:
> 
> Thomas Skinner (1):
>   fix #4234: add docs for openid optional userinfo request
> 
>  pveum.adoc | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
>  
> manager:
> 
> Thomas Skinner (1):
>   fix #4234: add GUI option for openid optional userinfo request
> 
>  www/manager6/dc/AuthEditOpenId.js | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
>  
> perl-rs:
> 
> Thomas Skinner (1):
>   fix #4234: openid: adjust openid verification function for userinfo
>     option
> 
>  pve-rs/src/openid/mod.rs | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
>  
> proxmox-openid:
> 
> Thomas Skinner (1):
>   fix #4234: openid: add library functions for optional userinfo
>     endpoint
> 
>  proxmox-openid/src/lib.rs | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
>  

Thank you for the patch series!

I've tested it with Authentik, and checked with tcpdump to see if the
userinfo endpoint was queried. Works as I would expect.

I had to manually apply the pve-rs patch since code was moved around
since then. More information as reply to the patch itself.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH access-control v3 1/1] fix #4234: add library functions for openid optional userinfo request

[Mira Limbeck]

On 2/8/25 06:42, Thomas Skinner wrote:
> Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> ---
>  src/PVE/API2/OpenId.pm | 6 +++++-
>  src/PVE/Auth/OpenId.pm | 7 +++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
> index 77410e6..456e96a 100644
> --- a/src/PVE/API2/OpenId.pm
> +++ b/src/PVE/API2/OpenId.pm
> @@ -170,7 +170,11 @@ __PACKAGE__->register_method ({
>  
>  	    my ($config, $openid) = $lookup_openid_auth->($realm, $redirect_url);
>  
> -	    my $info = $openid->verify_authorization_code($param->{code}, $private_auth_state);
> +	    my $info = $openid->verify_authorization_code(
> +		$param->{code},
> +		$private_auth_state,
> +		$config->{'disable-userinfo'} // 0,
> +	    );
>  	    my $subject = $info->{'sub'};
>  
>  	    my $unique_name;
> diff --git a/src/PVE/Auth/OpenId.pm b/src/PVE/Auth/OpenId.pm
> index c8e4db9..6efa696 100755
> --- a/src/PVE/Auth/OpenId.pm
> +++ b/src/PVE/Auth/OpenId.pm
> @@ -63,6 +63,12 @@ sub properties {
>  	    pattern => '^[^\x00-\x1F\x7F <>#"]*$', # Prohibit characters not allowed in URI RFC 2396.
>  	    optional => 1,
>  	},
> +	"disable-userinfo" => {
> +	    description => "Prevents querying the userinfo endpoint for claims values.",
> +	    type => 'boolean',
> +	    default => 0,
> +	    optional => 1,
> +	},
I would prefer `query-userinfo` (or similar) and a default of `1` over
`disable-userinfo` and a default of `0`.

>     };
>  }
>  
> @@ -78,6 +84,7 @@ sub options {
>  	"acr-values" => { optional => 1 },
>  	default => { optional => 1 },
>  	comment => { optional => 1 },
> +	"disable-userinfo" => { optional => 1 },
>      };
>  }
>  



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH perl-rs v3 1/1] fix #4234: openid: adjust openid verification function for userinfo option

[Mira Limbeck]

On 2/8/25 06:42, Thomas Skinner wrote:
> Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> ---
>  pve-rs/src/openid/mod.rs | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/pve-rs/src/openid/mod.rs b/pve-rs/src/openid/mod.rs
> index 1fa7572..8f914ad 100644
> --- a/pve-rs/src/openid/mod.rs
> +++ b/pve-rs/src/openid/mod.rs
> @@ -54,9 +54,14 @@ mod export {
>          #[try_from_ref] this: &OpenId,
>          code: &str,
>          private_auth_state: PrivateAuthState,
> +        disable_userinfo: Option<bool>,
>      ) -> Result<Value, Error> {
>          let open_id = this.inner.lock().unwrap();
> -        let claims = open_id.verify_authorization_code_simple(code, &private_auth_state)?;
> +        let claims = open_id.verify_authorization_code_simple_userinfo(
> +            code,
> +            &private_auth_state,
> +            disable_userinfo.unwrap_or(false),
> +        )?;
>  
>          Ok(to_value(&claims)?)
>      }

Commit 9ee9ad4 moved the code to common/src/oidc. I had to apply those
manually at the new location, and update the forwarding call in
pve-rs/src/openid/mod.rs.

Other than that, worked as espected.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH access-control v3 1/1] fix #4234: add library functions for openid optional userinfo request

[Fabian Grünbichler]

On March 6, 2025 4:20 pm, Mira Limbeck wrote:
> 
> 
> On 2/8/25 06:42, Thomas Skinner wrote:
>> Signed-off-by: Thomas Skinner <thomas@atskinner.net>
>> ---
>>  src/PVE/API2/OpenId.pm | 6 +++++-
>>  src/PVE/Auth/OpenId.pm | 7 +++++++
>>  2 files changed, 12 insertions(+), 1 deletion(-)
>> 
>> diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
>> index 77410e6..456e96a 100644
>> --- a/src/PVE/API2/OpenId.pm
>> +++ b/src/PVE/API2/OpenId.pm
>> @@ -170,7 +170,11 @@ __PACKAGE__->register_method ({
>>  
>>  	    my ($config, $openid) = $lookup_openid_auth->($realm, $redirect_url);
>>  
>> -	    my $info = $openid->verify_authorization_code($param->{code}, $private_auth_state);
>> +	    my $info = $openid->verify_authorization_code(
>> +		$param->{code},
>> +		$private_auth_state,
>> +		$config->{'disable-userinfo'} // 0,
>> +	    );
>>  	    my $subject = $info->{'sub'};
>>  
>>  	    my $unique_name;
>> diff --git a/src/PVE/Auth/OpenId.pm b/src/PVE/Auth/OpenId.pm
>> index c8e4db9..6efa696 100755
>> --- a/src/PVE/Auth/OpenId.pm
>> +++ b/src/PVE/Auth/OpenId.pm
>> @@ -63,6 +63,12 @@ sub properties {
>>  	    pattern => '^[^\x00-\x1F\x7F <>#"]*$', # Prohibit characters not allowed in URI RFC 2396.
>>  	    optional => 1,
>>  	},
>> +	"disable-userinfo" => {
>> +	    description => "Prevents querying the userinfo endpoint for claims values.",
>> +	    type => 'boolean',
>> +	    default => 0,
>> +	    optional => 1,
>> +	},
> I would prefer `query-userinfo` (or similar) and a default of `1` over
> `disable-userinfo` and a default of `0`.

that probably makes sense..

@Thomas - would you mind rebasing this and incorporating this last bit
of feedback, I think this is otherwise ready to be applied :)


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel