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

[pve-devel] [PATCH access-control/docs/manager/perl-rs/proxmox-openid v2 0/5] 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 v1:
- Adjust to add option in the UI to enable the functionality
- Add documentation for the option
- Adjust API back to previous behavior


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 | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)


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 access-control v2 1/5] 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..ea1de16 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_userinfo(
+		$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

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

[Fabian Grünbichler]

On December 16, 2024 5:14 am, 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..ea1de16 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_userinfo(
> +		$param->{code},
> +		$private_auth_state,
> +		$config->{'disable-userinfo'} // 0,
> +	    );

see comments on the libpve-rs-perl patch, but otherwise this LGTM!

>  	    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 mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH docs v2 2/5] 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 v2 3/5] 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 perl-rs v2 4/5] 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 | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/pve-rs/src/openid/mod.rs b/pve-rs/src/openid/mod.rs
index 1fa7572..cd573ee 100644
--- a/pve-rs/src/openid/mod.rs
+++ b/pve-rs/src/openid/mod.rs
@@ -50,13 +50,18 @@ mod export {
     }
 
     #[export(raw_return)]
-    pub fn verify_authorization_code(
+    pub fn verify_authorization_code_userinfo(
         #[try_from_ref] this: &OpenId,
         code: &str,
         private_auth_state: PrivateAuthState,
+        disable_userinfo: 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,
+        )?;
 
         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 perl-rs v2 4/5] fix #4234: openid: adjust openid verification function for userinfo option

[Fabian Grünbichler]

On December 16, 2024 5:14 am, Thomas Skinner wrote:
> Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> ---
>  pve-rs/src/openid/mod.rs | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/pve-rs/src/openid/mod.rs b/pve-rs/src/openid/mod.rs
> index 1fa7572..cd573ee 100644
> --- a/pve-rs/src/openid/mod.rs
> +++ b/pve-rs/src/openid/mod.rs
> @@ -50,13 +50,18 @@ mod export {
>      }
>  
>      #[export(raw_return)]
> -    pub fn verify_authorization_code(
> +    pub fn verify_authorization_code_userinfo(

we could either add a new wrapper like in proxmox-openid, keeping the
old one around (until PVE 9.0)

>          #[try_from_ref] this: &OpenId,
>          code: &str,
>          private_auth_state: PrivateAuthState,
> +        disable_userinfo: bool,

or make this an Option<bool> and not rename the fn so existing callers
are not broken

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

if we go the Option route, we could either select which proxmox-openid
fn to call here, or unwrap the Option to false and just call the new
one.

the reason I'd prefer a backwards-compat implementation here is that
without it, we need to have the following relations:

pve-access-control: depends on new libpve-rs-perl
libpve-rs-perl: breaks old pve-access-control
libpve-rs-perl: dpends on new librust-proxmox-openid-dev

whereas with a backwards-compat implementation in libpve-rs-perl we just
need:
pve-access-control depends on new libpve-rs-perl
libpve-rs-perl: depends on new librust-proxmox-openid-dev

which is much less entangled as all the version constraints go in the
same direction.

> +            code,
> +            &private_auth_state,
> +            disable_userinfo,
> +        )?;
>  
>          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
> 
> 
> 


_______________________________________________
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 v2 4/5] fix #4234: openid: adjust openid verification function for userinfo option

[Thomas Skinner]

On Fri, Jan 24, 2025 at 3:17 AM Fabian Grünbichler
<f.gruenbichler@proxmox.com> wrote:
>
> On December 16, 2024 5:14 am, Thomas Skinner wrote:
> > Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> > ---
> >  pve-rs/src/openid/mod.rs | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/pve-rs/src/openid/mod.rs b/pve-rs/src/openid/mod.rs
> > index 1fa7572..cd573ee 100644
> > --- a/pve-rs/src/openid/mod.rs
> > +++ b/pve-rs/src/openid/mod.rs
> > @@ -50,13 +50,18 @@ mod export {
> >      }
> >
> >      #[export(raw_return)]
> > -    pub fn verify_authorization_code(
> > +    pub fn verify_authorization_code_userinfo(
>
> we could either add a new wrapper like in proxmox-openid, keeping the
> old one around (until PVE 9.0)
>
> >          #[try_from_ref] this: &OpenId,
> >          code: &str,
> >          private_auth_state: PrivateAuthState,
> > +        disable_userinfo: bool,
>
> or make this an Option<bool> and not rename the fn so existing callers
> are not broken

Is this a backwards compatible fix? This would seem more preferable to
me and provide a reasonable default. If not, I can definitely keep the
old one around to provide backwards compat.

> >      ) -> 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(
>
> if we go the Option route, we could either select which proxmox-openid
> fn to call here, or unwrap the Option to false and just call the new
> one.

Unwrapping to false and calling the new one does make sense to me.

> the reason I'd prefer a backwards-compat implementation here is that
> without it, we need to have the following relations:
>
> pve-access-control: depends on new libpve-rs-perl
> libpve-rs-perl: breaks old pve-access-control
> libpve-rs-perl: dpends on new librust-proxmox-openid-dev
>
> whereas with a backwards-compat implementation in libpve-rs-perl we just
> need:
> pve-access-control depends on new libpve-rs-perl
> libpve-rs-perl: depends on new librust-proxmox-openid-dev
>
> which is much less entangled as all the version constraints go in the
> same direction.

Completely agree with not wanting to break backwards compat. If the
Option method is sufficient for this and we can just call the new
function with the unwrapped value, that works for me.

> > +            code,
> > +            &private_auth_state,
> > +            disable_userinfo,
> > +        )?;
> >
> >          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
> >
> >
> >
>

_______________________________________________
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 v2 4/5] fix #4234: openid: adjust openid verification function for userinfo option

[Fabian Grünbichler]

> Thomas Skinner <thomas@atskinner.net> hat am 29.01.2025 04:35 CET geschrieben:
> 
>  
> On Fri, Jan 24, 2025 at 3:17 AM Fabian Grünbichler
> <f.gruenbichler@proxmox.com> wrote:
> >
> > On December 16, 2024 5:14 am, Thomas Skinner wrote:
> > > Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> > > ---
> > >  pve-rs/src/openid/mod.rs | 9 +++++++--
> > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/pve-rs/src/openid/mod.rs b/pve-rs/src/openid/mod.rs
> > > index 1fa7572..cd573ee 100644
> > > --- a/pve-rs/src/openid/mod.rs
> > > +++ b/pve-rs/src/openid/mod.rs
> > > @@ -50,13 +50,18 @@ mod export {
> > >      }
> > >
> > >      #[export(raw_return)]
> > > -    pub fn verify_authorization_code(
> > > +    pub fn verify_authorization_code_userinfo(
> >
> > we could either add a new wrapper like in proxmox-openid, keeping the
> > old one around (until PVE 9.0)
> >
> > >          #[try_from_ref] this: &OpenId,
> > >          code: &str,
> > >          private_auth_state: PrivateAuthState,
> > > +        disable_userinfo: bool,
> >
> > or make this an Option<bool> and not rename the fn so existing callers
> > are not broken
> 
> Is this a backwards compatible fix? This would seem more preferable to
> me and provide a reasonable default. If not, I can definitely keep the
> old one around to provide backwards compat.

AFAIK it should work, perl undef / parameter not being set (old pve-access-control) should be translated to None via perlmod (and then unwrapped to false ;)), and the parameter being set (new pve-access-control) should be translated to Some(..).


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

[pve-devel] [PATCH proxmox v2 5/5] 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..87be1c8a 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, true)
+    }
+
+    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, true)
+    }
+
+    /// 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

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

[Fabian Grünbichler]

On December 16, 2024 5:14 am, Thomas Skinner wrote:
> 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..87be1c8a 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, true)

this default here is the wrong way round (to preserve the old behaviour,
we should pass in `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, true)

same here

> +    }
> +
> +    /// 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 mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

[Thomas Skinner]

On Fri, Jan 24, 2025 at 3:17 AM Fabian Grünbichler
<f.gruenbichler@proxmox.com> wrote:
>
> On December 16, 2024 5:14 am, Thomas Skinner wrote:
> > 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..87be1c8a 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, true)
>
> this default here is the wrong way round (to preserve the old behaviour,
> we should pass in `false`).

Good catch! I missed this in my debugging. Will get this fixed in v3.

> > +    }
> > +
> > +    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, true)
>
> same here

Ditto above.

> > +    }
> > +
> > +    /// 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 mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

[Fabian Grünbichler]

some small comments on individual patches, and sorry that it took so
long to get back to you with the holidays! next round will be faster :)

On December 16, 2024 5:14 am, Thomas Skinner wrote:
> Continues work on adding an option to disable querying the userinfo endpoint for an
> OIDC provider.
> 
> Changes since v1:
> - Adjust to add option in the UI to enable the functionality
> - Add documentation for the option
> - Adjust API back to previous behavior
> 
> 
> 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 | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> 
> 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 mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel