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

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

[Thomas Skinner]

Continued work on adding support for OIDC groups. 

Changes since v1:
- incorporate changes in style requested by Fabian
- adjust logic to only add/remove groups as needed instead of clearing all groups
- added helper function to remove a single group
- added option to create groups in PVE automatically on login
- update commit message for proxmox-openid
- move docs for replacement character to "Advanced Settings" section


pve-access-control:

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

 src/PVE/API2/OpenId.pm   | 68 ++++++++++++++++++++++++++++++++++++++++
 src/PVE/AccessControl.pm | 13 +++++---
 src/PVE/Auth/OpenId.pm   | 30 ++++++++++++++++++
 3 files changed, 107 insertions(+), 4 deletions(-)


pve-docs:

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

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


pve-manager:

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

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


proxmox-openid:

Thomas Skinner (1):
  fix #4411: openid: add library code for generic id token claim support

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

-- 
2.39.5


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

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

[Thomas Skinner]

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

diff --git a/pveum.adoc b/pveum.adoc
index 81565ab..36b7560 100644
--- a/pveum.adoc
+++ b/pveum.adoc
@@ -456,6 +456,15 @@ 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`).
 
+* `Autocreate Groups` (`groups-autocreate`): Create all groups in the claim
+instead of using existing PVE groups (default behavior).
+
+* `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).
+
 Username mapping
 ^^^^^^^^^^^^^^^^
 
@@ -479,6 +488,36 @@ 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. If all groups reported by the OpenID provider should exist in {pve}, 
+the `groups-autocreate` option may be used.
+
+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: `'-'`,
+`'_'`, `'.'`).
+
+Advanced settings
+^^^^^^^^^^^^^^^^^
+
+* `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 (`'_'`).
+
 Examples
 ^^^^^^^^
 
-- 
2.39.5


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

[pve-devel] [PATCH proxmox v2 1/1] fix #4411: openid: add library code for generic id token claim 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.5


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

[pve-devel] [PATCH access-control v2 1/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   | 68 ++++++++++++++++++++++++++++++++++++++++
 src/PVE/AccessControl.pm | 13 +++++---
 src/PVE/Auth/OpenId.pm   | 30 ++++++++++++++++++
 3 files changed, 107 insertions(+), 4 deletions(-)

diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
index 77410e6..5cfe5a1 100644
--- a/src/PVE/API2/OpenId.pm
+++ b/src/PVE/API2/OpenId.pm
@@ -13,6 +13,7 @@ use PVE::Cluster qw(cfs_read_file cfs_write_file);
 use PVE::AccessControl;
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::Auth::Plugin;
+use PVE::Auth::OpenId;
 
 use PVE::RESTHandler;
 
@@ -220,6 +221,73 @@ __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 (ref($groups_list) eq 'ARRAY') {
+			PVE::AccessControl::lock_user_config(sub {
+			    my $usercfg = cfs_read_file("user.cfg");
+
+			    # replace any invalid characters with
+			    my $replace_character = $config->{'groups-replace-character'} // '_';
+			    my @oidc_groups_list = map { 
+				$_ =~ s/[^$PVE::Auth::OpenId::groupname_regex_chars]/$replace_character/gr 
+			    } $groups_list->@*;
+
+			    # list groups that exist in pve
+			    my @existing_groups_list = keys %{$usercfg->{groups}};
+
+			    my @groups_intersect;
+			    if ( $config->{'groups-autocreate'} ) {
+				# populate all groups in claim
+				@groups_intersect = @oidc_groups_list;
+			    }
+			    else {
+				# only populate groups that are in the oidc list and exist in pve
+				@groups_intersect = @{PVE::Tools::array_intersect(
+				    \@oidc_groups_list,
+				    \@existing_groups_list,
+				)};
+			    }
+
+			    # if groups should be overwritten, find and delete the ones to remove
+			    if ( $config->{'groups-overwrite'} ) {
+				# get the groups that need to be removed from the user
+				my %groups_remove_user;
+				$groups_remove_user{ $_ } = undef 
+				    for keys %{$usercfg->{users}->{$username}->{groups}};
+				delete $groups_remove_user{ $_ } for @groups_intersect;
+				
+				# ensure user is not a member of these groups
+				PVE::AccessControl::delete_user_group_single(
+				    $username, 
+				    $usercfg, 
+				    $_,
+				) for keys %groups_remove_user;
+			    }
+
+			    # get the groups that need to be added to the user
+			    my %groups_add_user;
+			    $groups_add_user{ $_ } = undef for @groups_intersect;
+			    delete $groups_add_user{ $_ } 
+			        for keys %{$usercfg->{users}->{$username}->{groups}};
+			    
+			    # ensure user is a member of these groups
+			    PVE::AccessControl::add_user_group(
+				$username,
+			    	$usercfg,
+				$_
+			    ) for keys %groups_add_user;
+
+			    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/AccessControl.pm b/src/PVE/AccessControl.pm
index 47f2d38..d643a00 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -990,12 +990,17 @@ sub delete_user_group {
     my ($username, $usercfg) = @_;
 
     foreach my $group (keys %{$usercfg->{groups}}) {
-
-	delete ($usercfg->{groups}->{$group}->{users}->{$username})
-	    if $usercfg->{groups}->{$group}->{users}->{$username};
+	delete_user_group_single($username, $usercfg, $group);
     }
 }
 
+sub delete_user_group_single {
+    my ($username, $usercfg, $group) = @_;
+
+    delete ($usercfg->{groups}->{$group}->{users}->{$username})
+	if $usercfg->{groups}->{$group}->{users}->{$username};
+}
+
 sub delete_user_acl {
     my ($username, $usercfg) = @_;
 
@@ -1293,7 +1298,7 @@ PVE::JSONSchema::register_format('pve-groupid', \&verify_groupname);
 sub verify_groupname {
     my ($groupname, $noerr) = @_;
 
-    if ($groupname !~ m/^[A-Za-z0-9\.\-_]+$/) {
+    if ($groupname !~ m/^[$PVE::Auth::OpenId::groupname_regex_chars]+$/) {
 
 	die "group name '$groupname' contains invalid characters\n" if !$noerr;
 
diff --git a/src/PVE/Auth/OpenId.pm b/src/PVE/Auth/OpenId.pm
index c8e4db9..d7b5574 100755
--- a/src/PVE/Auth/OpenId.pm
+++ b/src/PVE/Auth/OpenId.pm
@@ -9,6 +9,8 @@ use PVE::Cluster qw(cfs_register_file cfs_read_file cfs_write_file cfs_lock_file
 
 use base qw(PVE::Auth::Plugin);
 
+our $groupname_regex_chars = qr/A-Za-z0-9\.\-_/;
+
 sub type {
     return 'openid';
 }
@@ -42,6 +44,30 @@ sub properties {
 	    type => 'string',
 	    optional => 1,
 	},
+	"groups-claim" => {
+	    description => "OpenID claim used to retrieve groups with.",
+	    type => 'string',
+	    optional => 1,
+	},
+	"groups-autocreate" => {
+	    description => "Automatically create groups if they do not exist.",
+	    optional => 1,
+	    type => 'boolean',
+	    default => 0,
+	},
+	"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 => qr/^[$groupname_regex_chars]$/,
+	    default => '_',
+	    optional => 1,
+	},
 	prompt => {
 	    description => "Specifies whether the Authorization Server prompts the End-User for"
 	        ." reauthentication and consent.",
@@ -73,6 +99,10 @@ sub options {
 	"client-key" => { optional => 1 },
 	autocreate => { optional => 1 },
 	"username-claim" => { optional => 1, fixed => 1 },
+	"groups-claim" => { optional => 1 },
+	"groups-autocreate" => { optional => 1 },
+	"groups-overwrite" => { optional => 1 },
+	"groups-replace-character" => { optional => 1},
 	prompt => { optional => 1 },
 	scopes => { optional => 1 },
 	"acr-values" => { optional => 1 },
-- 
2.39.5


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

[pve-devel] [PATCH manager v2 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 | 44 ++++++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 3 deletions(-)

diff --git a/www/manager6/dc/AuthEditOpenId.js b/www/manager6/dc/AuthEditOpenId.js
index 544c0de5..7a578c36 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: [
@@ -72,16 +82,34 @@ Ext.define('PVE.panel.OpenIDInputPanel', {
 		editable: '{isCreate}',
 	    },
 	},
+	{
+	    xtype: 'proxmoxcheckbox',
+	    fieldLabel: gettext('Autocreate Groups'),
+	    name: 'groups-autocreate',
+	    value: 0,
+	    cbind: {
+		deleteEmpty: '{!isCreate}',
+	    },
+	},
 	{
 	    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 +139,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.5


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