[pve-devel] [PATCH SERIES openid/access-control/docs/manager] fix #4411: add support for openid groups

[pve-devel] [PATCH SERIES openid/access-control/docs/manager] fix #4411: add support for openid groups

[Thomas Skinner]

This patch series adds support for groups for OpenID logins. 

The following options are implemented:
  - Configurable claim for retrieving a list of groups and adding them to the 
    user in PVE
  - Allowing "synchronization" of groups on login by overriding groups assigned
    to the user in PVE (this option is off by default)
  - Replacing invalid characters in group names with a configurable valid characters
    (by default, this is an underscore '_')

The logic implemented by this patch expects the groups claim in the ID token/userinfo
endpoint to send a list of groups.

This patch also adds support for using additional claims from the OpenID provider
by exposing all additional claims and their values from the ID token (previously
only available for the userinfo endpoint). This is necessary for OpenID providers
that do not support additional information in the userinfo endpoint.


proxmox/proxmox-openid:

Thomas Skinner (1):
  fix #4411: openid: add library code for openid groups support

 proxmox-openid/src/lib.rs | 55 +++++++++++++++++++++++++++++++++------
 1 file changed, 47 insertions(+), 8 deletions(-)


pve-access-control:

Thomas Skinner (1):
  fix #4411: openid: add logic for openid groups support

 src/PVE/API2/OpenId.pm | 32 ++++++++++++++++++++++++++++++++
 src/PVE/Auth/OpenId.pm | 21 +++++++++++++++++++++
 2 files changed, 53 insertions(+)


pve-docs:

Thomas Skinner (1):
  fix #4411: openid: add docs for openid groups support

 api-viewer/apidata.js | 40 ++++++++++++++++++++++++++++++++++++++++
 pveum.adoc            | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)


pve-manager:

Thomas Skinner (1):
  fix #4411: openid: add ui config for openid groups support

 www/manager6/dc/AuthEditOpenId.js | 35 ++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

-- 
2.39.2


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

[pve-devel] [PATCH docs 1/1] fix #4411: openid: add docs for openid groups support

[Thomas Skinner]

Signed-off-by: Thomas Skinner <thomas@atskinner.net>
---
 api-viewer/apidata.js | 40 ++++++++++++++++++++++++++++++++++++++++
 pveum.adoc            | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/api-viewer/apidata.js b/api-viewer/apidata.js
index 8ba94e4..0edafd7 100644
--- a/api-viewer/apidata.js
+++ b/api-viewer/apidata.js
@@ -53895,6 +53895,26 @@ const apiSchema = [
                                  "type" : "string",
                                  "typetext" : "<string>"
                               },
+                              "groups-claim" : {
+                                 "description" : "OpenID claim used to retrieve groups with.",
+                                 "optional" : 1,
+                                 "type" : "string",
+                                 "typetext" : "<string>"
+                              },
+                              "groups-overwrite" : {
+                                 "default" : 0,
+                                 "description" : "All groups will be overwritten for the user on login.",
+                                 "optional" : 1,
+                                 "type" : "boolean",
+                                 "typetext" : "<boolean>"
+                              },
+                              "groups-replace-character" : {
+                                 "default" : "_",
+                                 "description" : "Character used to replace any invalid characters in groups from provider.",
+                                 "optional" : 1,
+                                 "pattern" : "^[A-Za-z0-9\\.\\-_]$",
+                                 "type" : "string"
+                              },
                               "issuer-url" : {
                                  "description" : "OpenID Issuer Url",
                                  "maxLength" : 256,
@@ -54233,6 +54253,26 @@ const apiSchema = [
                            "type" : "string",
                            "typetext" : "<string>"
                         },
+                        "groups-claim" : {
+                           "description" : "OpenID claim used to retrieve groups with.",
+                           "optional" : 1,
+                           "type" : "string",
+                           "typetext" : "<string>"
+                        },
+                        "groups-overwrite" : {
+                           "default" : 0,
+                           "description" : "All groups will be overwritten for the user on login.",
+                           "optional" : 1,
+                           "type" : "boolean",
+                           "typetext" : "<boolean>"
+                        },
+                        "groups-replace-character" : {
+                           "default" : "_",
+                           "description" : "Character used to replace any invalid characters in groups from provider.",
+                           "optional" : 1,
+                           "pattern" : "^[A-Za-z0-9\\.\\-_]$",
+                           "type" : "string"
+                        },
                         "issuer-url" : {
                            "description" : "OpenID Issuer Url",
                            "maxLength" : 256,
diff --git a/pveum.adoc b/pveum.adoc
index c115866..0a031cc 100644
--- a/pveum.adoc
+++ b/pveum.adoc
@@ -456,6 +456,16 @@ use the `autocreate` option to automatically add new users.
 * `Username Claim` (`username-claim`): OpenID claim used to generate the unique
 username (`subject`, `username` or `email`).
 
+* `Groups Claim` (`groups-claim`): OpenID claim used to retrieve the groups from
+the ID token or userinfo endpoint.
+
+* `Overwrite Groups` (`groups-overwrite`): Overwrite all groups assigned to user
+instead of appending to existing groups (default behavior).
+
+* `Groups Replacement Character` (`groups-replace-character`): Character to
+replace invalid characters in groups names from the OpenID provider. Default
+behavior is to replace each invalid character with an underscore (`'_'`).
+
 Username mapping
 ^^^^^^^^^^^^^^^^
 
@@ -479,6 +489,28 @@ 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.
 
+Groups mapping
+^^^^^^^^^^^^^^
+
+Specifying the `groups-claim` setting in the OpenID configuration enables group
+mapping functionality. The data provided in the `groups-claim` should be
+a list of strings that correspond to groups that a user should be a member of in
+{pve}. Any groups reported by the OpenID provider that do not exist in {pve} are
+ignored.
+
+By default, groups are appended to the user's existing groups. It may be 
+desirable to overwrite any groups that the user is already a member in {pve}
+with those from the OpenID provider. Enabling the `groups-overwrite` setting
+removes all groups from the user in {pve} before adding the groups reported by
+the OpenID provider.
+
+In some cases, OpenID servers may send groups claims which include invalid
+characters for {pve} group IDs. By default, each invalid character is replaced
+with an underscore (`'_'`). To adjust the character used for replacement, adjust 
+the `groups-replace-character` setting to any valid character for a {pve} group
+ID (i.e. includes alphanumeric characters and any of the following: `'-'`,
+`'_'`, `'.'`).
+
 Examples
 ^^^^^^^^
 
-- 
2.39.2


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

[pve-devel] [PATCH openid 1/1] fix #4411: openid: add library code for openid groups support

[Thomas Skinner]

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

diff --git a/proxmox-openid/src/lib.rs b/proxmox-openid/src/lib.rs
index fe65fded..bf8c650b 100644
--- a/proxmox-openid/src/lib.rs
+++ b/proxmox-openid/src/lib.rs
@@ -15,8 +15,11 @@ pub use auth_state::*;
 use openidconnect::{
     //curl::http_client,
     core::{
-        CoreAuthDisplay, CoreAuthPrompt, CoreAuthenticationFlow, CoreClient, CoreGenderClaim,
-        CoreIdTokenClaims, CoreIdTokenVerifier, CoreProviderMetadata,
+        CoreAuthDisplay, CoreAuthPrompt, CoreAuthenticationFlow, CoreErrorResponseType,
+        CoreGenderClaim, CoreIdTokenVerifier, CoreJsonWebKey, CoreJsonWebKeyType,
+        CoreJsonWebKeyUse, CoreJweContentEncryptionAlgorithm, CoreJwsSigningAlgorithm,
+        CoreProviderMetadata, CoreRevocableToken, CoreRevocationErrorResponse,
+        CoreTokenIntrospectionResponse, CoreTokenType,
     },
     AdditionalClaims,
     AuthenticationContextClass,
@@ -24,6 +27,9 @@ use openidconnect::{
     ClientId,
     ClientSecret,
     CsrfToken,
+    EmptyExtraTokenFields,
+    IdTokenClaims,
+    IdTokenFields,
     IssuerUrl,
     Nonce,
     OAuth2TokenResponse,
@@ -31,15 +37,47 @@ use openidconnect::{
     PkceCodeVerifier,
     RedirectUrl,
     Scope,
+    StandardErrorResponse,
+    StandardTokenResponse,
     UserInfoClaims,
 };
 
 /// Stores Additional Claims into a serde_json::Value;
-#[derive(Debug, Deserialize, Serialize)]
+#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize)]
 pub struct GenericClaims(Value);
 impl AdditionalClaims for GenericClaims {}
 
 pub type GenericUserInfoClaims = UserInfoClaims<GenericClaims, CoreGenderClaim>;
+pub type GenericIdTokenClaims = IdTokenClaims<GenericClaims, CoreGenderClaim>;
+
+pub type GenericIdTokenFields = IdTokenFields<
+    GenericClaims,
+    EmptyExtraTokenFields,
+    CoreGenderClaim,
+    CoreJweContentEncryptionAlgorithm,
+    CoreJwsSigningAlgorithm,
+    CoreJsonWebKeyType,
+>;
+
+pub type GenericTokenResponse = StandardTokenResponse<GenericIdTokenFields, CoreTokenType>;
+
+pub type GenericClient = openidconnect::Client<
+    GenericClaims,
+    CoreAuthDisplay,
+    CoreGenderClaim,
+    CoreJweContentEncryptionAlgorithm,
+    CoreJwsSigningAlgorithm,
+    CoreJsonWebKeyType,
+    CoreJsonWebKeyUse,
+    CoreJsonWebKey,
+    CoreAuthPrompt,
+    StandardErrorResponse<CoreErrorResponseType>,
+    GenericTokenResponse,
+    CoreTokenType,
+    CoreTokenIntrospectionResponse,
+    CoreRevocableToken,
+    CoreRevocationErrorResponse,
+>;
 
 #[derive(Debug, Deserialize, Serialize, Clone)]
 pub struct OpenIdConfig {
@@ -56,7 +94,7 @@ pub struct OpenIdConfig {
 }
 
 pub struct OpenIdAuthenticator {
-    client: CoreClient,
+    client: GenericClient,
     config: OpenIdConfig,
 }
 
@@ -120,8 +158,9 @@ impl OpenIdAuthenticator {
 
         let provider_metadata = CoreProviderMetadata::discover(&issuer_url, http_client)?;
 
-        let client = CoreClient::from_provider_metadata(provider_metadata, client_id, client_key)
-            .set_redirect_uri(RedirectUrl::new(String::from(redirect_url))?);
+        let client =
+            GenericClient::from_provider_metadata(provider_metadata, client_id, client_key)
+                .set_redirect_uri(RedirectUrl::new(String::from(redirect_url))?);
 
         Ok(Self {
             client,
@@ -195,7 +234,7 @@ impl OpenIdAuthenticator {
         &self,
         code: &str,
         private_auth_state: &PrivateAuthState,
-    ) -> Result<(CoreIdTokenClaims, GenericUserInfoClaims), Error> {
+    ) -> Result<(GenericIdTokenClaims, GenericUserInfoClaims), Error> {
         let code = AuthorizationCode::new(code.to_string());
         // Exchange the code with a token.
         let token_response = self
@@ -206,7 +245,7 @@ impl OpenIdAuthenticator {
             .map_err(|err| format_err!("Failed to contact token endpoint: {}", err))?;
 
         let id_token_verifier: CoreIdTokenVerifier = self.client.id_token_verifier();
-        let id_token_claims: &CoreIdTokenClaims = token_response
+        let id_token_claims: &GenericIdTokenClaims = token_response
             .extra_fields()
             .id_token()
             .expect("Server did not return an ID token")
-- 
2.39.2


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

[pve-devel] [PATCH access-control 1/1] fix #4411: openid: add logic for openid groups support

[Thomas Skinner]

Signed-off-by: Thomas Skinner <thomas@atskinner.net>
---
 src/PVE/API2/OpenId.pm | 32 ++++++++++++++++++++++++++++++++
 src/PVE/Auth/OpenId.pm | 21 +++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
index 77410e6..22a2188 100644
--- a/src/PVE/API2/OpenId.pm
+++ b/src/PVE/API2/OpenId.pm
@@ -220,6 +220,38 @@ __PACKAGE__->register_method ({
 		$rpcenv->check_user_enabled($username);
 	    }
 
+		if (defined(my $groups_claim = $config->{'groups-claim'})) {
+			if (defined(my $groups_list = $info->{$groups_claim})) {
+				if (UNIVERSAL::isa($groups_list, 'ARRAY')) {
+					PVE::AccessControl::lock_user_config(sub {
+						my $usercfg = cfs_read_file("user.cfg");
+						
+						# if groups should be overwritten, delete them first
+						if ( $config->{'groups-overwrite'}) {
+							PVE::AccessControl::delete_user_group($username, $usercfg);
+						}
+						
+						# replace any invalid characters with
+						my $replace_character = $config->{'groups-replace-character'};
+						my @oidc_groups_list = map { $_ =~ s/[^A-Za-z0-9\.\-_]/$replace_character/gr } @{ $groups_list };
+						
+						# only populate groups that are in the oidc list and exist in pve
+						my @existing_groups_list = keys %{$usercfg->{groups}};
+						my @groups_intersect = grep { my $g = $_; grep $_ eq $g, @oidc_groups_list } @existing_groups_list;
+
+						# ensure user is a member of these groups
+						map { PVE::AccessControl::add_user_group($username, $usercfg, $_) } @groups_intersect;
+
+						cfs_write_file("user.cfg", $usercfg);
+					}, "openid group mapping failed");
+				} else {
+					syslog('err', "groups list is not an array; groups will not be updated");
+				}
+			} else {
+				syslog('err', "groups claim '$groups_claim' is not found in claims");
+			}
+		}
+
 	    my $ticket = PVE::AccessControl::assemble_ticket($username);
 	    my $csrftoken = PVE::AccessControl::assemble_csrf_prevention_token($username);
 	    my $cap = $rpcenv->compute_api_permission($username);
diff --git a/src/PVE/Auth/OpenId.pm b/src/PVE/Auth/OpenId.pm
index c8e4db9..0e3fdc4 100755
--- a/src/PVE/Auth/OpenId.pm
+++ b/src/PVE/Auth/OpenId.pm
@@ -42,6 +42,24 @@ sub properties {
 	    type => 'string',
 	    optional => 1,
 	},
+	"groups-claim" => {
+	    description => "OpenID claim used to retrieve groups with.",
+	    type => 'string',
+	    optional => 1,
+	},
+	"groups-overwrite" => {
+		description => "All groups will be overwritten for the user on login.",
+	    type => 'boolean',
+		default => 0,
+	    optional => 1,
+	},
+	"groups-replace-character" => {
+	    description => "Character used to replace any invalid characters in groups from provider.",
+	    type => 'string',
+		pattern => '^[A-Za-z0-9\.\-_]$',
+		default => '_',
+	    optional => 1,
+	},
 	prompt => {
 	    description => "Specifies whether the Authorization Server prompts the End-User for"
 	        ." reauthentication and consent.",
@@ -73,6 +91,9 @@ sub options {
 	"client-key" => { optional => 1 },
 	autocreate => { optional => 1 },
 	"username-claim" => { optional => 1, fixed => 1 },
+	"groups-claim" => { optional => 1 },
+	"groups-overwrite" => { optional => 1 },
+	"groups-replace-character" => { optional => 1},
 	prompt => { optional => 1 },
 	scopes => { optional => 1 },
 	"acr-values" => { optional => 1 },
-- 
2.39.2


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

[pve-devel] [PATCH manager 1/1] fix #4411: openid: add ui config for openid groups support

[Thomas Skinner]

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

diff --git a/www/manager6/dc/AuthEditOpenId.js b/www/manager6/dc/AuthEditOpenId.js
index 544c0de5..30ee050a 100644
--- a/www/manager6/dc/AuthEditOpenId.js
+++ b/www/manager6/dc/AuthEditOpenId.js
@@ -40,6 +40,16 @@ Ext.define('PVE.panel.OpenIDInputPanel', {
 	    },
 	    name: 'client-key',
 	},
+	{
+	    xtype: 'proxmoxtextfield',
+	    name: 'scopes',
+	    fieldLabel: gettext('Scopes'),
+	    emptyText: `${Proxmox.Utils.defaultText} (email profile)`,
+	    submitEmpty: false,
+	    cbind: {
+		deleteEmpty: '{!isCreate}',
+	    },
+	},
     ],
 
     column2: [
@@ -74,14 +84,23 @@ Ext.define('PVE.panel.OpenIDInputPanel', {
 	},
 	{
 	    xtype: 'proxmoxtextfield',
-	    name: 'scopes',
-	    fieldLabel: gettext('Scopes'),
-	    emptyText: `${Proxmox.Utils.defaultText} (email profile)`,
+	    name: 'groups-claim',
+	    fieldLabel: gettext('Groups Claim'),
+	    emptyText: `${Proxmox.Utils.defaultText} ${gettext('(none)')}`,
 	    submitEmpty: false,
 	    cbind: {
 		deleteEmpty: '{!isCreate}',
 	    },
 	},
+	{
+	    xtype: 'proxmoxcheckbox',
+	    fieldLabel: gettext('Overwrite Groups'),
+	    name: 'groups-overwrite',
+	    value: 0,
+	    cbind: {
+		deleteEmpty: '{!isCreate}',
+	    },
+	},
 	{
 	    xtype: 'proxmoxKVComboBox',
 	    name: 'prompt',
@@ -111,6 +130,16 @@ Ext.define('PVE.panel.OpenIDInputPanel', {
 		deleteEmpty: '{!isCreate}',
 	    },
 	},
+	{
+	    xtype: 'proxmoxtextfield',
+	    name: 'groups-replace-character',
+	    fieldLabel: gettext('Groups Replacement Character'),
+		emptyText: `${Proxmox.Utils.defaultText} '_'`,
+	    submitEmpty: false,
+	    cbind: {
+		deleteEmpty: '{!isCreate}',
+	    },
+	},
     ],
 
     initComponent: function() {
-- 
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 SERIES openid/access-control/docs/manager] fix #4411: add support for openid groups

[Thomas Skinner]

This is still applicable to the latest master for the referenced
repositories. Any movement?

On Sun, Sep 1, 2024, 11:55 AM Thomas Skinner <thomas@atskinner.net> wrote:

> This patch series adds support for groups for OpenID logins.
>
> The following options are implemented:
>   - Configurable claim for retrieving a list of groups and adding them to
> the
>     user in PVE
>   - Allowing "synchronization" of groups on login by overriding groups
> assigned
>     to the user in PVE (this option is off by default)
>   - Replacing invalid characters in group names with a configurable valid
> characters
>     (by default, this is an underscore '_')
>
> The logic implemented by this patch expects the groups claim in the ID
> token/userinfo
> endpoint to send a list of groups.
>
> This patch also adds support for using additional claims from the OpenID
> provider
> by exposing all additional claims and their values from the ID token
> (previously
> only available for the userinfo endpoint). This is necessary for OpenID
> providers
> that do not support additional information in the userinfo endpoint.
>
>
> proxmox/proxmox-openid:
>
> Thomas Skinner (1):
>   fix #4411: openid: add library code for openid groups support
>
>  proxmox-openid/src/lib.rs | 55 +++++++++++++++++++++++++++++++++------
>  1 file changed, 47 insertions(+), 8 deletions(-)
>
>
> pve-access-control:
>
> Thomas Skinner (1):
>   fix #4411: openid: add logic for openid groups support
>
>  src/PVE/API2/OpenId.pm | 32 ++++++++++++++++++++++++++++++++
>  src/PVE/Auth/OpenId.pm | 21 +++++++++++++++++++++
>  2 files changed, 53 insertions(+)
>
>
> pve-docs:
>
> Thomas Skinner (1):
>   fix #4411: openid: add docs for openid groups support
>
>  api-viewer/apidata.js | 40 ++++++++++++++++++++++++++++++++++++++++
>  pveum.adoc            | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 72 insertions(+)
>
>
> pve-manager:
>
> Thomas Skinner (1):
>   fix #4411: openid: add ui config for openid groups support
>
>  www/manager6/dc/AuthEditOpenId.js | 35 ++++++++++++++++++++++++++++---
>  1 file changed, 32 insertions(+), 3 deletions(-)
>
> --
> 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 SERIES openid/access-control/docs/manager] fix #4411: add support for openid groups

[DERUMIER, Alexandre via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 16672 bytes --]

From: "DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>
To: "pve-devel@lists.proxmox.com" <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH SERIES openid/access-control/docs/manager] fix #4411: add support for openid groups
Date: Sun, 6 Oct 2024 17:27:57 +0000
Message-ID: <1c15fbd6029c9b9f85c3da9dd00f885908d3885f.camel@groupe-cyllene.com>

Looking for this feature too for my production :)

-------- Message initial --------
De: Thomas Skinner <thomas@atskinner.net>
Répondre à: Proxmox VE development discussion <pve-
devel@lists.proxmox.com>
À: pve-devel@lists.proxmox.com
Objet: Re: [pve-devel] [PATCH SERIES openid/access-
control/docs/manager] fix #4411: add support for openid groups
Date: 03/10/2024 03:45:15

This is still applicable to the latest master for the referenced
repositories. Any movement?

On Sun, Sep 1, 2024, 11:55 AM Thomas Skinner <thomas@atskinner.net>
wrote:

> This patch series adds support for groups for OpenID logins.
> 
> The following options are implemented:
>   - Configurable claim for retrieving a list of groups and adding
> them to
> the
>     user in PVE
>   - Allowing "synchronization" of groups on login by overriding
> groups
> assigned
>     to the user in PVE (this option is off by default)
>   - Replacing invalid characters in group names with a configurable
> valid
> characters
>     (by default, this is an underscore '_')
> 
> The logic implemented by this patch expects the groups claim in the
> ID
> token/userinfo
> endpoint to send a list of groups.
> 
> This patch also adds support for using additional claims from the
> OpenID
> provider
> by exposing all additional claims and their values from the ID token
> (previously
> only available for the userinfo endpoint). This is necessary for
> OpenID
> providers
> that do not support additional information in the userinfo endpoint.
> 
> 
> proxmox/proxmox-openid:
> 
> Thomas Skinner (1):
>   fix #4411: openid: add library code for openid groups support
> 
>  proxmox-openid/src/lib.rs | 55 +++++++++++++++++++++++++++++++++----
> --
>  1 file changed, 47 insertions(+), 8 deletions(-)
> 
> 
> pve-access-control:
> 
> Thomas Skinner (1):
>   fix #4411: openid: add logic for openid groups support
> 
>  src/PVE/API2/OpenId.pm | 32 ++++++++++++++++++++++++++++++++
>  src/PVE/Auth/OpenId.pm | 21 +++++++++++++++++++++
>  2 files changed, 53 insertions(+)
> 
> 
> pve-docs:
> 
> Thomas Skinner (1):
>   fix #4411: openid: add docs for openid groups support
> 
>  api-viewer/apidata.js | 40 ++++++++++++++++++++++++++++++++++++++++
>  pveum.adoc            | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 72 insertions(+)
> 
> 
> pve-manager:
> 
> Thomas Skinner (1):
>   fix #4411: openid: add ui config for openid groups support
> 
>  www/manager6/dc/AuthEditOpenId.js | 35 ++++++++++++++++++++++++++++-
> --
>  1 file changed, 32 insertions(+), 3 deletions(-)
> 
> --
> 2.39.2
> 
> 
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://antiphishing.vadesecure.com/v4?f=c2xMdVN4Smh2R2tOZDdIRKCk7WEocH
pTPMerT1Q-
Aq5qwr8l2xvAWuOGvFsV3frp2oSAgxNUQCpJDHp2iUmTWg&i=d1l4NXNNaWE4SWZqU0dLWe
iyW515SOU2RVoj_9OQ7Ew&k=fjzS&r=MXJUa0FrUVJqc1UwYWxNZ-
tmXdGQOM0bQR6kYEgvrmGZrgAumkB5XEgd10kSzvIx&s=8cd24c2f90b250765f2ceb8891
4e45da75f5223e1030847b6919a9863d0e2f09&u=https%3A%2F%2Flists.proxmox.co
m%2Fcgi-bin%2Fmailman%2Flistinfo%2Fpve-devel


[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
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 1/1] fix #4411: openid: add logic for openid groups support

[Fabian Grünbichler]

a few nits, mostly style related below

On September 1, 2024 6:55 pm, Thomas Skinner wrote:
> Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> ---
>  src/PVE/API2/OpenId.pm | 32 ++++++++++++++++++++++++++++++++
>  src/PVE/Auth/OpenId.pm | 21 +++++++++++++++++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
> index 77410e6..22a2188 100644
> --- a/src/PVE/API2/OpenId.pm
> +++ b/src/PVE/API2/OpenId.pm
> @@ -220,6 +220,38 @@ __PACKAGE__->register_method ({
>  		$rpcenv->check_user_enabled($username);
>  	    }
>  
> +		if (defined(my $groups_claim = $config->{'groups-claim'})) {
> +			if (defined(my $groups_list = $info->{$groups_claim})) {
> +				if (UNIVERSAL::isa($groups_list, 'ARRAY')) {

we normally use `ref($groups_list) eq 'ARRAY'`

> +					PVE::AccessControl::lock_user_config(sub {
> +						my $usercfg = cfs_read_file("user.cfg");
> +						
> +						# if groups should be overwritten, delete them first
> +						if ( $config->{'groups-overwrite'}) {
> +							PVE::AccessControl::delete_user_group($username, $usercfg);
> +						}
> +						
> +						# replace any invalid characters with
> +						my $replace_character = $config->{'groups-replace-character'};
> +						my @oidc_groups_list = map { $_ =~ s/[^A-Za-z0-9\.\-_]/$replace_character/gr } @{ $groups_list };

we normally use array references, and (for new code) dereferencing via
->

this RE (continued below)

> +						
> +						# only populate groups that are in the oidc list and exist in pve
> +						my @existing_groups_list = keys %{$usercfg->{groups}};
> +						my @groups_intersect = grep { my $g = $_; grep $_ eq $g, @oidc_groups_list } @existing_groups_list;

we do have PVE::Tools:array_intersect which does this for N array
references..

> +
> +						# ensure user is a member of these groups
> +						map { PVE::AccessControl::add_user_group($username, $usercfg, $_) } @groups_intersect;

this could be a `for` loop, since the map result is not used at all..

> +
> +						cfs_write_file("user.cfg", $usercfg);
> +					}, "openid group mapping failed");
> +				} else {
> +					syslog('err', "groups list is not an array; groups will not be updated");
> +				}
> +			} else {
> +				syslog('err', "groups claim '$groups_claim' is not found in claims");
> +			}
> +		}
> +
>  	    my $ticket = PVE::AccessControl::assemble_ticket($username);
>  	    my $csrftoken = PVE::AccessControl::assemble_csrf_prevention_token($username);
>  	    my $cap = $rpcenv->compute_api_permission($username);
> diff --git a/src/PVE/Auth/OpenId.pm b/src/PVE/Auth/OpenId.pm
> index c8e4db9..0e3fdc4 100755
> --- a/src/PVE/Auth/OpenId.pm
> +++ b/src/PVE/Auth/OpenId.pm
> @@ -42,6 +42,24 @@ sub properties {
>  	    type => 'string',
>  	    optional => 1,
>  	},
> +	"groups-claim" => {
> +	    description => "OpenID claim used to retrieve groups with.",
> +	    type => 'string',
> +	    optional => 1,
> +	},
> +	"groups-overwrite" => {
> +		description => "All groups will be overwritten for the user on login.",
> +	    type => 'boolean',
> +		default => 0,
> +	    optional => 1,
> +	},
> +	"groups-replace-character" => {
> +	    description => "Character used to replace any invalid characters in groups from provider.",
> +	    type => 'string',
> +		pattern => '^[A-Za-z0-9\.\-_]$',

and this RE are inverses of eachother - should we define them in one
place in case we ever need to update it?

> +		default => '_',
> +	    optional => 1,
> +	},
>  	prompt => {
>  	    description => "Specifies whether the Authorization Server prompts the End-User for"
>  	        ." reauthentication and consent.",
> @@ -73,6 +91,9 @@ sub options {
>  	"client-key" => { optional => 1 },
>  	autocreate => { optional => 1 },
>  	"username-claim" => { optional => 1, fixed => 1 },
> +	"groups-claim" => { optional => 1 },
> +	"groups-overwrite" => { optional => 1 },
> +	"groups-replace-character" => { optional => 1},
>  	prompt => { optional => 1 },
>  	scopes => { optional => 1 },
>  	"acr-values" => { optional => 1 },
> -- 
> 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 openid 1/1] fix #4411: openid: add library code for openid groups support

[Fabian Grünbichler]

this change actually does a lot more, right? it returns *all* claims,
and also doesn't do anything groups specific at all, so the patch
subject is not quite correct..

I haven't tested this yet, but since all it does is allow arbitrary
values instead of just empty ones, it should be fine. definitely would
require some testing with different IdP implementations to ensure no
regressions..

On September 1, 2024 6:55 pm, Thomas Skinner wrote:
> Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> ---
>  proxmox-openid/src/lib.rs | 55 +++++++++++++++++++++++++++++++++------
>  1 file changed, 47 insertions(+), 8 deletions(-)
> 
> diff --git a/proxmox-openid/src/lib.rs b/proxmox-openid/src/lib.rs
> index fe65fded..bf8c650b 100644
> --- a/proxmox-openid/src/lib.rs
> +++ b/proxmox-openid/src/lib.rs
> @@ -15,8 +15,11 @@ pub use auth_state::*;
>  use openidconnect::{
>      //curl::http_client,
>      core::{
> -        CoreAuthDisplay, CoreAuthPrompt, CoreAuthenticationFlow, CoreClient, CoreGenderClaim,
> -        CoreIdTokenClaims, CoreIdTokenVerifier, CoreProviderMetadata,
> +        CoreAuthDisplay, CoreAuthPrompt, CoreAuthenticationFlow, CoreErrorResponseType,
> +        CoreGenderClaim, CoreIdTokenVerifier, CoreJsonWebKey, CoreJsonWebKeyType,
> +        CoreJsonWebKeyUse, CoreJweContentEncryptionAlgorithm, CoreJwsSigningAlgorithm,
> +        CoreProviderMetadata, CoreRevocableToken, CoreRevocationErrorResponse,
> +        CoreTokenIntrospectionResponse, CoreTokenType,
>      },
>      AdditionalClaims,
>      AuthenticationContextClass,
> @@ -24,6 +27,9 @@ use openidconnect::{
>      ClientId,
>      ClientSecret,
>      CsrfToken,
> +    EmptyExtraTokenFields,
> +    IdTokenClaims,
> +    IdTokenFields,
>      IssuerUrl,
>      Nonce,
>      OAuth2TokenResponse,
> @@ -31,15 +37,47 @@ use openidconnect::{
>      PkceCodeVerifier,
>      RedirectUrl,
>      Scope,
> +    StandardErrorResponse,
> +    StandardTokenResponse,
>      UserInfoClaims,
>  };
>  
>  /// Stores Additional Claims into a serde_json::Value;
> -#[derive(Debug, Deserialize, Serialize)]
> +#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize)]
>  pub struct GenericClaims(Value);
>  impl AdditionalClaims for GenericClaims {}
>  
>  pub type GenericUserInfoClaims = UserInfoClaims<GenericClaims, CoreGenderClaim>;
> +pub type GenericIdTokenClaims = IdTokenClaims<GenericClaims, CoreGenderClaim>;
> +
> +pub type GenericIdTokenFields = IdTokenFields<
> +    GenericClaims,
> +    EmptyExtraTokenFields,
> +    CoreGenderClaim,
> +    CoreJweContentEncryptionAlgorithm,
> +    CoreJwsSigningAlgorithm,
> +    CoreJsonWebKeyType,
> +>;
> +
> +pub type GenericTokenResponse = StandardTokenResponse<GenericIdTokenFields, CoreTokenType>;
> +
> +pub type GenericClient = openidconnect::Client<
> +    GenericClaims,
> +    CoreAuthDisplay,
> +    CoreGenderClaim,
> +    CoreJweContentEncryptionAlgorithm,
> +    CoreJwsSigningAlgorithm,
> +    CoreJsonWebKeyType,
> +    CoreJsonWebKeyUse,
> +    CoreJsonWebKey,
> +    CoreAuthPrompt,
> +    StandardErrorResponse<CoreErrorResponseType>,
> +    GenericTokenResponse,
> +    CoreTokenType,
> +    CoreTokenIntrospectionResponse,
> +    CoreRevocableToken,
> +    CoreRevocationErrorResponse,
> +>;
>  
>  #[derive(Debug, Deserialize, Serialize, Clone)]
>  pub struct OpenIdConfig {
> @@ -56,7 +94,7 @@ pub struct OpenIdConfig {
>  }
>  
>  pub struct OpenIdAuthenticator {
> -    client: CoreClient,
> +    client: GenericClient,
>      config: OpenIdConfig,
>  }
>  
> @@ -120,8 +158,9 @@ impl OpenIdAuthenticator {
>  
>          let provider_metadata = CoreProviderMetadata::discover(&issuer_url, http_client)?;
>  
> -        let client = CoreClient::from_provider_metadata(provider_metadata, client_id, client_key)
> -            .set_redirect_uri(RedirectUrl::new(String::from(redirect_url))?);
> +        let client =
> +            GenericClient::from_provider_metadata(provider_metadata, client_id, client_key)
> +                .set_redirect_uri(RedirectUrl::new(String::from(redirect_url))?);
>  
>          Ok(Self {
>              client,
> @@ -195,7 +234,7 @@ impl OpenIdAuthenticator {
>          &self,
>          code: &str,
>          private_auth_state: &PrivateAuthState,
> -    ) -> Result<(CoreIdTokenClaims, GenericUserInfoClaims), Error> {
> +    ) -> Result<(GenericIdTokenClaims, GenericUserInfoClaims), Error> {
>          let code = AuthorizationCode::new(code.to_string());
>          // Exchange the code with a token.
>          let token_response = self
> @@ -206,7 +245,7 @@ impl OpenIdAuthenticator {
>              .map_err(|err| format_err!("Failed to contact token endpoint: {}", err))?;
>  
>          let id_token_verifier: CoreIdTokenVerifier = self.client.id_token_verifier();
> -        let id_token_claims: &CoreIdTokenClaims = token_response
> +        let id_token_claims: &GenericIdTokenClaims = token_response
>              .extra_fields()
>              .id_token()
>              .expect("Server did not return an ID token")
> -- 
> 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 access-control 1/1] fix #4411: openid: add logic for openid groups support

[Fabian Grünbichler]

On September 1, 2024 6:55 pm, Thomas Skinner wrote:
> Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> ---
>  src/PVE/API2/OpenId.pm | 32 ++++++++++++++++++++++++++++++++
>  src/PVE/Auth/OpenId.pm | 21 +++++++++++++++++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/src/PVE/Auth/OpenId.pm b/src/PVE/Auth/OpenId.pm
> index c8e4db9..0e3fdc4 100755
> --- a/src/PVE/Auth/OpenId.pm
> +++ b/src/PVE/Auth/OpenId.pm
> @@ -42,6 +42,24 @@ sub properties {
>  	    type => 'string',
>  	    optional => 1,
>  	},
> +	"groups-claim" => {
> +	    description => "OpenID claim used to retrieve groups with.",
> +	    type => 'string',

forgot this part: this should probably have a format to limit valid
values..

> +	    optional => 1,
> +	},
> +	"groups-overwrite" => {
> +		description => "All groups will be overwritten for the user on login.",
> +	    type => 'boolean',
> +		default => 0,
> +	    optional => 1,
> +	},
> +	"groups-replace-character" => {
> +	    description => "Character used to replace any invalid characters in groups from provider.",
> +	    type => 'string',
> +		pattern => '^[A-Za-z0-9\.\-_]$',
> +		default => '_',
> +	    optional => 1,
> +	},
>  	prompt => {
>  	    description => "Specifies whether the Authorization Server prompts the End-User for"
>  	        ." reauthentication and consent.",
> @@ -73,6 +91,9 @@ sub options {
>  	"client-key" => { optional => 1 },
>  	autocreate => { optional => 1 },
>  	"username-claim" => { optional => 1, fixed => 1 },
> +	"groups-claim" => { optional => 1 },
> +	"groups-overwrite" => { optional => 1 },
> +	"groups-replace-character" => { optional => 1},
>  	prompt => { optional => 1 },
>  	scopes => { optional => 1 },
>  	"acr-values" => { optional => 1 },
> -- 
> 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 SERIES openid/access-control/docs/manager] fix #4411: add support for openid groups

[Mira Limbeck]

On 9/1/24 18:55, Thomas Skinner wrote:
> This patch series adds support for groups for OpenID logins. 
> 
> The following options are implemented:
>   - Configurable claim for retrieving a list of groups and adding them to the 
>     user in PVE
>   - Allowing "synchronization" of groups on login by overriding groups assigned
>     to the user in PVE (this option is off by default)
>   - Replacing invalid characters in group names with a configurable valid characters
>     (by default, this is an underscore '_')
> 
> The logic implemented by this patch expects the groups claim in the ID token/userinfo
> endpoint to send a list of groups.
> 
> This patch also adds support for using additional claims from the OpenID provider
> by exposing all additional claims and their values from the ID token (previously
> only available for the userinfo endpoint). This is necessary for OpenID providers
> that do not support additional information in the userinfo endpoint.

I did a small test of this patch series with a Keycloak 17 as provider.

For the test I added a `Client Scope` in the realm with the predefined
mapper `groups'. The user info can then be checked under Realm -> Client
Scope -> Evaluate -> (select user, evaluate) -> Generated User Info


Tests done:

1)
* no groups defined in PVE
* realm configured with:
  - Groups Claim: `groups`
  - Overwrite Groups: true
* user in keycloak configured with groups: group1, group2, group3
* login with user
* no groups created and/or assigned to the user (missing 3 groups)

2)
* group1 defined in PVE
* realm configured with:
  - Groups Claim: `groups`
  - Overwrite Groups: true
* user in keycloak configured with groups: group1, group2, group3
* login with user
* user was assigned the group1 (missing 2 others)

3)
* group2 defined in PVE
* realm configured with:
  - Groups Claim: `groups`
  - Overwrite Groups: true
* user in keycloak configured with groups: group1, group2, group3
* login with user
* group was switched from group1 to group2

4)
* group1, group2 defined in PVE
* realm configured with:
  - Groups Claim: `groups`
  - Overwrite Groups: false
* user in keycloak configured with groups: group1, group3
* login with user
* group1 was added, group2 was kept

5)
* group1, group2 defined in PVE
* realm configured with:
  - Groups Claim: none (default)
  - Overwrite Groups: <any>
* user in keycloak configured with groups: group1, group2, group3
* login with user
* no changes


It seemed to work reliably once Keycloak was configured correctly. One
thing that was confusing, even with `Overwrite Groups` no groups are set
if they aren't already configured on the PVE cluster.

I haven't tested the character replacement yet.


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

Re: [pve-devel] [PATCH openid 1/1] fix #4411: openid: add library code for openid groups support

[Thomas Skinner]

On Wed, Nov 13, 2024 at 6:46 AM Fabian Grünbichler
<f.gruenbichler@proxmox.com> wrote:
>
> this change actually does a lot more, right? it returns *all* claims,
> and also doesn't do anything groups specific at all, so the patch
> subject is not quite correct..

That's correct. I will update the commit message appropriately. It's
critical to return all of the claims so that the user can specify an
arbitrary claim name in the UI since a claim is not standardized
across IdPs for group names. This also allows Proxmox to be more
flexible in the future when supporting OIDC capabilities.

> I haven't tested this yet, but since all it does is allow arbitrary
> values instead of just empty ones, it should be fine. definitely would
> require some testing with different IdP implementations to ensure no
> regressions..

This change allows the ID token to contain more than just the core
claims prescribed in the openid CoreClaims struct. More importantly,
it allows any claims fields in the ID token to come through. I believe
this is not a breaking change because this is not removing any claims
from the ID token and any values that would have been passed through
OIDC would already be in some serializable format.

> On September 1, 2024 6:55 pm, Thomas Skinner wrote:
> > Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> > ---
> >  proxmox-openid/src/lib.rs | 55 +++++++++++++++++++++++++++++++++------
> >  1 file changed, 47 insertions(+), 8 deletions(-)
> >
> > diff --git a/proxmox-openid/src/lib.rs b/proxmox-openid/src/lib.rs
> > index fe65fded..bf8c650b 100644
> > --- a/proxmox-openid/src/lib.rs
> > +++ b/proxmox-openid/src/lib.rs
> > @@ -15,8 +15,11 @@ pub use auth_state::*;
> >  use openidconnect::{
> >      //curl::http_client,
> >      core::{
> > -        CoreAuthDisplay, CoreAuthPrompt, CoreAuthenticationFlow, CoreClient, CoreGenderClaim,
> > -        CoreIdTokenClaims, CoreIdTokenVerifier, CoreProviderMetadata,
> > +        CoreAuthDisplay, CoreAuthPrompt, CoreAuthenticationFlow, CoreErrorResponseType,
> > +        CoreGenderClaim, CoreIdTokenVerifier, CoreJsonWebKey, CoreJsonWebKeyType,
> > +        CoreJsonWebKeyUse, CoreJweContentEncryptionAlgorithm, CoreJwsSigningAlgorithm,
> > +        CoreProviderMetadata, CoreRevocableToken, CoreRevocationErrorResponse,
> > +        CoreTokenIntrospectionResponse, CoreTokenType,
> >      },
> >      AdditionalClaims,
> >      AuthenticationContextClass,
> > @@ -24,6 +27,9 @@ use openidconnect::{
> >      ClientId,
> >      ClientSecret,
> >      CsrfToken,
> > +    EmptyExtraTokenFields,
> > +    IdTokenClaims,
> > +    IdTokenFields,
> >      IssuerUrl,
> >      Nonce,
> >      OAuth2TokenResponse,
> > @@ -31,15 +37,47 @@ use openidconnect::{
> >      PkceCodeVerifier,
> >      RedirectUrl,
> >      Scope,
> > +    StandardErrorResponse,
> > +    StandardTokenResponse,
> >      UserInfoClaims,
> >  };
> >
> >  /// Stores Additional Claims into a serde_json::Value;
> > -#[derive(Debug, Deserialize, Serialize)]
> > +#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize)]
> >  pub struct GenericClaims(Value);
> >  impl AdditionalClaims for GenericClaims {}
> >
> >  pub type GenericUserInfoClaims = UserInfoClaims<GenericClaims, CoreGenderClaim>;
> > +pub type GenericIdTokenClaims = IdTokenClaims<GenericClaims, CoreGenderClaim>;
> > +
> > +pub type GenericIdTokenFields = IdTokenFields<
> > +    GenericClaims,
> > +    EmptyExtraTokenFields,
> > +    CoreGenderClaim,
> > +    CoreJweContentEncryptionAlgorithm,
> > +    CoreJwsSigningAlgorithm,
> > +    CoreJsonWebKeyType,
> > +>;
> > +
> > +pub type GenericTokenResponse = StandardTokenResponse<GenericIdTokenFields, CoreTokenType>;
> > +
> > +pub type GenericClient = openidconnect::Client<
> > +    GenericClaims,
> > +    CoreAuthDisplay,
> > +    CoreGenderClaim,
> > +    CoreJweContentEncryptionAlgorithm,
> > +    CoreJwsSigningAlgorithm,
> > +    CoreJsonWebKeyType,
> > +    CoreJsonWebKeyUse,
> > +    CoreJsonWebKey,
> > +    CoreAuthPrompt,
> > +    StandardErrorResponse<CoreErrorResponseType>,
> > +    GenericTokenResponse,
> > +    CoreTokenType,
> > +    CoreTokenIntrospectionResponse,
> > +    CoreRevocableToken,
> > +    CoreRevocationErrorResponse,
> > +>;
> >
> >  #[derive(Debug, Deserialize, Serialize, Clone)]
> >  pub struct OpenIdConfig {
> > @@ -56,7 +94,7 @@ pub struct OpenIdConfig {
> >  }
> >
> >  pub struct OpenIdAuthenticator {
> > -    client: CoreClient,
> > +    client: GenericClient,
> >      config: OpenIdConfig,
> >  }
> >
> > @@ -120,8 +158,9 @@ impl OpenIdAuthenticator {
> >
> >          let provider_metadata = CoreProviderMetadata::discover(&issuer_url, http_client)?;
> >
> > -        let client = CoreClient::from_provider_metadata(provider_metadata, client_id, client_key)
> > -            .set_redirect_uri(RedirectUrl::new(String::from(redirect_url))?);
> > +        let client =
> > +            GenericClient::from_provider_metadata(provider_metadata, client_id, client_key)
> > +                .set_redirect_uri(RedirectUrl::new(String::from(redirect_url))?);
> >
> >          Ok(Self {
> >              client,
> > @@ -195,7 +234,7 @@ impl OpenIdAuthenticator {
> >          &self,
> >          code: &str,
> >          private_auth_state: &PrivateAuthState,
> > -    ) -> Result<(CoreIdTokenClaims, GenericUserInfoClaims), Error> {
> > +    ) -> Result<(GenericIdTokenClaims, GenericUserInfoClaims), Error> {
> >          let code = AuthorizationCode::new(code.to_string());
> >          // Exchange the code with a token.
> >          let token_response = self
> > @@ -206,7 +245,7 @@ impl OpenIdAuthenticator {
> >              .map_err(|err| format_err!("Failed to contact token endpoint: {}", err))?;
> >
> >          let id_token_verifier: CoreIdTokenVerifier = self.client.id_token_verifier();
> > -        let id_token_claims: &CoreIdTokenClaims = token_response
> > +        let id_token_claims: &GenericIdTokenClaims = token_response
> >              .extra_fields()
> >              .id_token()
> >              .expect("Server did not return an ID token")
> > --
> > 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 access-control 1/1] fix #4411: openid: add logic for openid groups support

[Thomas Skinner]

On Wed, Nov 13, 2024 at 6:46 AM Fabian Grünbichler
<f.gruenbichler@proxmox.com> wrote:
>
> a few nits, mostly style related below

Will get these fixed up and submit in a v2 patch.

> On September 1, 2024 6:55 pm, Thomas Skinner wrote:
> > Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> > ---
> >  src/PVE/API2/OpenId.pm | 32 ++++++++++++++++++++++++++++++++
> >  src/PVE/Auth/OpenId.pm | 21 +++++++++++++++++++++
> >  2 files changed, 53 insertions(+)
> >
> > diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
> > index 77410e6..22a2188 100644
> > --- a/src/PVE/API2/OpenId.pm
> > +++ b/src/PVE/API2/OpenId.pm
> > @@ -220,6 +220,38 @@ __PACKAGE__->register_method ({
> >               $rpcenv->check_user_enabled($username);
> >           }
> >
> > +             if (defined(my $groups_claim = $config->{'groups-claim'})) {
> > +                     if (defined(my $groups_list = $info->{$groups_claim})) {
> > +                             if (UNIVERSAL::isa($groups_list, 'ARRAY')) {
>
> we normally use `ref($groups_list) eq 'ARRAY'`
>
> > +                                     PVE::AccessControl::lock_user_config(sub {
> > +                                             my $usercfg = cfs_read_file("user.cfg");
> > +
> > +                                             # if groups should be overwritten, delete them first
> > +                                             if ( $config->{'groups-overwrite'}) {
> > +                                                     PVE::AccessControl::delete_user_group($username, $usercfg);
> > +                                             }
> > +
> > +                                             # replace any invalid characters with
> > +                                             my $replace_character = $config->{'groups-replace-character'};
> > +                                             my @oidc_groups_list = map { $_ =~ s/[^A-Za-z0-9\.\-_]/$replace_character/gr } @{ $groups_list };
>
> we normally use array references, and (for new code) dereferencing via
> ->
>
> this RE (continued below)
>
> > +
> > +                                             # only populate groups that are in the oidc list and exist in pve
> > +                                             my @existing_groups_list = keys %{$usercfg->{groups}};
> > +                                             my @groups_intersect = grep { my $g = $_; grep $_ eq $g, @oidc_groups_list } @existing_groups_list;
>
> we do have PVE::Tools:array_intersect which does this for N array
> references..
>
> > +
> > +                                             # ensure user is a member of these groups
> > +                                             map { PVE::AccessControl::add_user_group($username, $usercfg, $_) } @groups_intersect;
>
> this could be a `for` loop, since the map result is not used at all..
>
> > +
> > +                                             cfs_write_file("user.cfg", $usercfg);
> > +                                     }, "openid group mapping failed");
> > +                             } else {
> > +                                     syslog('err', "groups list is not an array; groups will not be updated");
> > +                             }
> > +                     } else {
> > +                             syslog('err', "groups claim '$groups_claim' is not found in claims");
> > +                     }
> > +             }
> > +
> >           my $ticket = PVE::AccessControl::assemble_ticket($username);
> >           my $csrftoken = PVE::AccessControl::assemble_csrf_prevention_token($username);
> >           my $cap = $rpcenv->compute_api_permission($username);
> > diff --git a/src/PVE/Auth/OpenId.pm b/src/PVE/Auth/OpenId.pm
> > index c8e4db9..0e3fdc4 100755
> > --- a/src/PVE/Auth/OpenId.pm
> > +++ b/src/PVE/Auth/OpenId.pm
> > @@ -42,6 +42,24 @@ sub properties {
> >           type => 'string',
> >           optional => 1,
> >       },
> > +     "groups-claim" => {
> > +         description => "OpenID claim used to retrieve groups with.",
> > +         type => 'string',
>
> forgot this part: this should probably have a format to limit valid
> values..

I would tend to agree, but there doesn't seem to be a specification
that I can find that requires certain characters as part of the claim
name. However, if Proxmox wants to have one, I would suggest the same
RE used for the invalid characters for groups replacement to start off
with and document appropriately what characters are allowed.

> > +         optional => 1,
> > +     },
> > +     "groups-overwrite" => {
> > +             description => "All groups will be overwritten for the user on login.",
> > +         type => 'boolean',
> > +             default => 0,
> > +         optional => 1,
> > +     },
> > +     "groups-replace-character" => {
> > +         description => "Character used to replace any invalid characters in groups from provider.",
> > +         type => 'string',
> > +             pattern => '^[A-Za-z0-9\.\-_]$',
>
> and this RE are inverses of eachother - should we define them in one
> place in case we ever need to update it?

Yes, that would probably be ideal. I'll work it into the v2. It uses
the same RE as the groupid validator, so maybe it can reference that
as well.

> > +             default => '_',
> > +         optional => 1,
> > +     },
> >       prompt => {
> >           description => "Specifies whether the Authorization Server prompts the End-User for"
> >               ." reauthentication and consent.",
> > @@ -73,6 +91,9 @@ sub options {
> >       "client-key" => { optional => 1 },
> >       autocreate => { optional => 1 },
> >       "username-claim" => { optional => 1, fixed => 1 },
> > +     "groups-claim" => { optional => 1 },
> > +     "groups-overwrite" => { optional => 1 },
> > +     "groups-replace-character" => { optional => 1},
> >       prompt => { optional => 1 },
> >       scopes => { optional => 1 },
> >       "acr-values" => { optional => 1 },
> > --
> > 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