* [pve-devel] [PATCH access-control/docs/manager/perl-rs/proxmox-openid v2 0/5] Make OIDC userinfo endpoint optional
@ 2024-12-16 4:14 Thomas Skinner
2024-12-16 4:14 ` [pve-devel] [PATCH access-control v2 1/5] fix #4234: add library functions for openid optional userinfo request Thomas Skinner
` (5 more replies)
0 siblings, 6 replies; 13+ messages in thread
From: Thomas Skinner @ 2024-12-16 4:14 UTC (permalink / raw)
To: pve-devel; +Cc: 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pve-devel] [PATCH access-control v2 1/5] fix #4234: add library functions for openid optional userinfo request
2024-12-16 4:14 [pve-devel] [PATCH access-control/docs/manager/perl-rs/proxmox-openid v2 0/5] Make OIDC userinfo endpoint optional Thomas Skinner
@ 2024-12-16 4:14 ` Thomas Skinner
2025-01-24 9:17 ` Fabian Grünbichler
2024-12-16 4:14 ` [pve-devel] [PATCH docs v2 2/5] fix #4234: add docs " Thomas Skinner
` (4 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Thomas Skinner @ 2024-12-16 4:14 UTC (permalink / raw)
To: pve-devel; +Cc: 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pve-devel] [PATCH docs v2 2/5] fix #4234: add docs for openid optional userinfo request
2024-12-16 4:14 [pve-devel] [PATCH access-control/docs/manager/perl-rs/proxmox-openid v2 0/5] Make OIDC userinfo endpoint optional Thomas Skinner
2024-12-16 4:14 ` [pve-devel] [PATCH access-control v2 1/5] fix #4234: add library functions for openid optional userinfo request Thomas Skinner
@ 2024-12-16 4:14 ` Thomas Skinner
2024-12-16 4:14 ` [pve-devel] [PATCH manager v2 3/5] fix #4234: add GUI option " Thomas Skinner
` (3 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Thomas Skinner @ 2024-12-16 4:14 UTC (permalink / raw)
To: pve-devel; +Cc: 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pve-devel] [PATCH manager v2 3/5] fix #4234: add GUI option for openid optional userinfo request
2024-12-16 4:14 [pve-devel] [PATCH access-control/docs/manager/perl-rs/proxmox-openid v2 0/5] Make OIDC userinfo endpoint optional Thomas Skinner
2024-12-16 4:14 ` [pve-devel] [PATCH access-control v2 1/5] fix #4234: add library functions for openid optional userinfo request Thomas Skinner
2024-12-16 4:14 ` [pve-devel] [PATCH docs v2 2/5] fix #4234: add docs " Thomas Skinner
@ 2024-12-16 4:14 ` Thomas Skinner
2024-12-16 4:14 ` [pve-devel] [PATCH perl-rs v2 4/5] fix #4234: openid: adjust openid verification function for userinfo option Thomas Skinner
` (2 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Thomas Skinner @ 2024-12-16 4:14 UTC (permalink / raw)
To: pve-devel; +Cc: 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pve-devel] [PATCH perl-rs v2 4/5] fix #4234: openid: adjust openid verification function for userinfo option
2024-12-16 4:14 [pve-devel] [PATCH access-control/docs/manager/perl-rs/proxmox-openid v2 0/5] Make OIDC userinfo endpoint optional Thomas Skinner
` (2 preceding siblings ...)
2024-12-16 4:14 ` [pve-devel] [PATCH manager v2 3/5] fix #4234: add GUI option " Thomas Skinner
@ 2024-12-16 4:14 ` Thomas Skinner
2025-01-24 9:17 ` Fabian Grünbichler
2024-12-16 4:14 ` [pve-devel] [PATCH proxmox v2 5/5] fix #4234: openid: add library functions for optional userinfo endpoint Thomas Skinner
2025-01-24 9:18 ` [pve-devel] [PATCH access-control/docs/manager/perl-rs/proxmox-openid v2 0/5] Make OIDC userinfo endpoint optional Fabian Grünbichler
5 siblings, 1 reply; 13+ messages in thread
From: Thomas Skinner @ 2024-12-16 4:14 UTC (permalink / raw)
To: pve-devel; +Cc: 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pve-devel] [PATCH proxmox v2 5/5] fix #4234: openid: add library functions for optional userinfo endpoint
2024-12-16 4:14 [pve-devel] [PATCH access-control/docs/manager/perl-rs/proxmox-openid v2 0/5] Make OIDC userinfo endpoint optional Thomas Skinner
` (3 preceding siblings ...)
2024-12-16 4:14 ` [pve-devel] [PATCH perl-rs v2 4/5] fix #4234: openid: adjust openid verification function for userinfo option Thomas Skinner
@ 2024-12-16 4:14 ` Thomas Skinner
2025-01-24 9:17 ` Fabian Grünbichler
2025-01-24 9:18 ` [pve-devel] [PATCH access-control/docs/manager/perl-rs/proxmox-openid v2 0/5] Make OIDC userinfo endpoint optional Fabian Grünbichler
5 siblings, 1 reply; 13+ messages in thread
From: Thomas Skinner @ 2024-12-16 4:14 UTC (permalink / raw)
To: pve-devel; +Cc: 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [pve-devel] [PATCH access-control v2 1/5] fix #4234: add library functions for openid optional userinfo request
2024-12-16 4:14 ` [pve-devel] [PATCH access-control v2 1/5] fix #4234: add library functions for openid optional userinfo request Thomas Skinner
@ 2025-01-24 9:17 ` Fabian Grünbichler
0 siblings, 0 replies; 13+ messages in thread
From: Fabian Grünbichler @ 2025-01-24 9:17 UTC (permalink / raw)
To: Proxmox VE development discussion; +Cc: Thomas Skinner
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [pve-devel] [PATCH perl-rs v2 4/5] fix #4234: openid: adjust openid verification function for userinfo option
2024-12-16 4:14 ` [pve-devel] [PATCH perl-rs v2 4/5] fix #4234: openid: adjust openid verification function for userinfo option Thomas Skinner
@ 2025-01-24 9:17 ` Fabian Grünbichler
2025-01-29 3:35 ` Thomas Skinner
0 siblings, 1 reply; 13+ messages in thread
From: Fabian Grünbichler @ 2025-01-24 9:17 UTC (permalink / raw)
To: Proxmox VE development discussion; +Cc: Thomas Skinner
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [pve-devel] [PATCH proxmox v2 5/5] fix #4234: openid: add library functions for optional userinfo endpoint
2024-12-16 4:14 ` [pve-devel] [PATCH proxmox v2 5/5] fix #4234: openid: add library functions for optional userinfo endpoint Thomas Skinner
@ 2025-01-24 9:17 ` Fabian Grünbichler
2025-01-29 3:30 ` Thomas Skinner
0 siblings, 1 reply; 13+ messages in thread
From: Fabian Grünbichler @ 2025-01-24 9:17 UTC (permalink / raw)
To: Proxmox VE development discussion; +Cc: Thomas Skinner
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [pve-devel] [PATCH access-control/docs/manager/perl-rs/proxmox-openid v2 0/5] Make OIDC userinfo endpoint optional
2024-12-16 4:14 [pve-devel] [PATCH access-control/docs/manager/perl-rs/proxmox-openid v2 0/5] Make OIDC userinfo endpoint optional Thomas Skinner
` (4 preceding siblings ...)
2024-12-16 4:14 ` [pve-devel] [PATCH proxmox v2 5/5] fix #4234: openid: add library functions for optional userinfo endpoint Thomas Skinner
@ 2025-01-24 9:18 ` Fabian Grünbichler
5 siblings, 0 replies; 13+ messages in thread
From: Fabian Grünbichler @ 2025-01-24 9:18 UTC (permalink / raw)
To: Proxmox VE development discussion; +Cc: Thomas Skinner
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [pve-devel] [PATCH proxmox v2 5/5] fix #4234: openid: add library functions for optional userinfo endpoint
2025-01-24 9:17 ` Fabian Grünbichler
@ 2025-01-29 3:30 ` Thomas Skinner
0 siblings, 0 replies; 13+ messages in thread
From: Thomas Skinner @ 2025-01-29 3:30 UTC (permalink / raw)
To: Fabian Grünbichler; +Cc: Proxmox VE development discussion
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [pve-devel] [PATCH perl-rs v2 4/5] fix #4234: openid: adjust openid verification function for userinfo option
2025-01-24 9:17 ` Fabian Grünbichler
@ 2025-01-29 3:35 ` Thomas Skinner
2025-01-29 12:24 ` Fabian Grünbichler
0 siblings, 1 reply; 13+ messages in thread
From: Thomas Skinner @ 2025-01-29 3:35 UTC (permalink / raw)
To: Fabian Grünbichler; +Cc: Proxmox VE development discussion
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [pve-devel] [PATCH perl-rs v2 4/5] fix #4234: openid: adjust openid verification function for userinfo option
2025-01-29 3:35 ` Thomas Skinner
@ 2025-01-29 12:24 ` Fabian Grünbichler
0 siblings, 0 replies; 13+ messages in thread
From: Fabian Grünbichler @ 2025-01-29 12:24 UTC (permalink / raw)
To: Thomas Skinner; +Cc: Proxmox VE development discussion
> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2025-01-29 12:25 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-12-16 4:14 [pve-devel] [PATCH access-control/docs/manager/perl-rs/proxmox-openid v2 0/5] Make OIDC userinfo endpoint optional Thomas Skinner
2024-12-16 4:14 ` [pve-devel] [PATCH access-control v2 1/5] fix #4234: add library functions for openid optional userinfo request Thomas Skinner
2025-01-24 9:17 ` Fabian Grünbichler
2024-12-16 4:14 ` [pve-devel] [PATCH docs v2 2/5] fix #4234: add docs " Thomas Skinner
2024-12-16 4:14 ` [pve-devel] [PATCH manager v2 3/5] fix #4234: add GUI option " Thomas Skinner
2024-12-16 4:14 ` [pve-devel] [PATCH perl-rs v2 4/5] fix #4234: openid: adjust openid verification function for userinfo option Thomas Skinner
2025-01-24 9:17 ` Fabian Grünbichler
2025-01-29 3:35 ` Thomas Skinner
2025-01-29 12:24 ` Fabian Grünbichler
2024-12-16 4:14 ` [pve-devel] [PATCH proxmox v2 5/5] fix #4234: openid: add library functions for optional userinfo endpoint Thomas Skinner
2025-01-24 9:17 ` Fabian Grünbichler
2025-01-29 3:30 ` Thomas Skinner
2025-01-24 9:18 ` [pve-devel] [PATCH access-control/docs/manager/perl-rs/proxmox-openid v2 0/5] Make OIDC userinfo endpoint optional Fabian Grünbichler
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox