[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