[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

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

[Fabian Grünbichler]

On December 24, 2024 9:24 pm, Thomas Skinner wrote:
> 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 { 

style nit: we mostly use array references in our code, i.e.

my $oidc_group_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}};

same here

> +
> +			    my @groups_intersect;

and here

> +			    if ( $config->{'groups-autocreate'} ) {
> +				# populate all groups in claim
> +				@groups_intersect = @oidc_groups_list;

and here ;)

> +			    }
> +			    else {

style nit: the '}' and 'else {' should be on a single line

> +				# 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,
> +				)};

then this simply becomes

$groups_intersect = PVE::Tools::array_intersect($oidc_groups_list, $existing_groups_list);

but since the existing groups are already a hash, you could also do the
intersection + conversion to a hash in a single go here (see below for
why a hash is nicer anyway):

			    if ($config->{'groups-autocreate'}) {
				# populate all groups in claim
				$groups_intersect = { map { $_ => 1 } $oidc_groups_list->@* };
			    } else {
			        # populate only existing groups
			        for my $group ($oidc_groups_list->@*) {
			            $groups_intersect->{$group} = 1 if $usercfg->{groups}->{$group};
			        }
			    }


> +			    }
> +
> +			    # 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;
> +


we don't do post-for either ;)

you can just do

# converts group intersection to hash (but see above ;))
my $groups_intersect_hash = { map { $_ => 1 } $groups_intersect->@ };

if ($config->{'groups-overwrite'}) {
    my $user_groups = $usercfg->{users}->{$username}->{groups};
    for my $group (keys $user_groups->%*) {
        delete $user_groups->{$group} if !groups_intersect_hash->{$group};
    }
}

and similarly for the addition, although with addition, you could even
do it unconditionally since it is both cheap and idempotent (it just
sets two booleans in the $usercfg hash).

or you could make it a bit easier, and just delete all groups and then
add all the groups in the intersecting set, which would be similar to
how we do it when directly overwriting a user's group via the API/pveum:

https://git.proxmox.com/?p=pve-access-control.git;a=blob;f=src/PVE/API2/User.pm;h=535e58e0b3dbd4c8d8c882b7845173489803d8d0;hb=HEAD#l417

the changes will only persisted on-disk once you hit the cfs_write in the
next line, so this shouldn't be an issue.

some higher-level questions:

do we want to have a bit more logging here, although it would only end
up in syslog/journal.. maybe at least for groups which are created?

do we want to mangle the group names to include the OIDC-realm name,
like we do for LDAP/AD syncing? that way it is more clear that those
groups originated from OIDC.. downside is that you can't use a group
shared between OIDC and other realms..

> +			    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);

to be honest I am not sure why that post-if is there in the first place..

when parsing the config, the group is initialized with an empty 'users'
hash as members before filling using the member list, so there is no
risk of auto-vivifying something here..

if you want to delete a user's group membership, you can just

delete $usercfg->{groups}->{$group}->{users}->{$username};

and we don't need this refactoring

>      }
>  }
>  
> +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]+$/) {

see below ;) if the RE moves there, we could actually move the whole
helper and format registration (maybe as a separate commit/patch?)

>  
>  	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\.\-_/;

this should probably be in PVE::Auth::Plugin, where the user and realm
REs are as well.

> +
>  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',

this should probably be restricted (our usual safeid might be too
strict (I know some OIDC providers like to user things like @ and : in
attributes, would have to check what the spec says here?), but some
sensible set of characters at least would be nice ;))

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

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

[Thomas Skinner]

On Fri, Jan 24, 2025 at 4:18 AM Fabian Grünbichler
<f.gruenbichler@proxmox.com> wrote:
>
> On December 24, 2024 9:24 pm, Thomas Skinner wrote:
> > 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 {
>
> style nit: we mostly use array references in our code, i.e.
>
> my $oidc_group_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}};
>
> same here
>
> > +
> > +                         my @groups_intersect;
>
> and here
>
> > +                         if ( $config->{'groups-autocreate'} ) {
> > +                             # populate all groups in claim
> > +                             @groups_intersect = @oidc_groups_list;
>
> and here ;)
>
> > +                         }
> > +                         else {
>
> style nit: the '}' and 'else {' should be on a single line
>
> > +                             # 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,
> > +                             )};
>
> then this simply becomes
>
> $groups_intersect = PVE::Tools::array_intersect($oidc_groups_list, $existing_groups_list);

Understood for all the above and will fix. Apologies for missing them
when they are clearly in the style guide. Is there any linting setup
that you'd recommend for my dev environment that would help catch
these for future patches before I send them? I don't want to recreate
the wheel if the Proxmox team already has something in-place that
could be shared.

> but since the existing groups are already a hash, you could also do the
> intersection + conversion to a hash in a single go here (see below for
> why a hash is nicer anyway):
>
>                             if ($config->{'groups-autocreate'}) {
>                                 # populate all groups in claim
>                                 $groups_intersect = { map { $_ => 1 } $oidc_groups_list->@* };
>                             } else {
>                                 # populate only existing groups
>                                 for my $group ($oidc_groups_list->@*) {
>                                     $groups_intersect->{$group} = 1 if $usercfg->{groups}->{$group};
>                                 }
>                             }
>
>
> > +                         }
> > +
> > +                         # 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;
> > +
>
>
> we don't do post-for either ;)
>
> you can just do
>
> # converts group intersection to hash (but see above ;))
> my $groups_intersect_hash = { map { $_ => 1 } $groups_intersect->@ };
>
> if ($config->{'groups-overwrite'}) {
>     my $user_groups = $usercfg->{users}->{$username}->{groups};
>     for my $group (keys $user_groups->%*) {
>         delete $user_groups->{$group} if !groups_intersect_hash->{$group};
>     }
> }
>
> and similarly for the addition, although with addition, you could even
> do it unconditionally since it is both cheap and idempotent (it just
> sets two booleans in the $usercfg hash).
>
> or you could make it a bit easier, and just delete all groups and then
> add all the groups in the intersecting set, which would be similar to
> how we do it when directly overwriting a user's group via the API/pveum:
>
> https://git.proxmox.com/?p=pve-access-control.git;a=blob;f=src/PVE/API2/User.pm;h=535e58e0b3dbd4c8d8c882b7845173489803d8d0;hb=HEAD#l417
>
> the changes will only persisted on-disk once you hit the cfs_write in the
> next line, so this shouldn't be an issue.

The reason that I made an adjustment to the behavior was purely for
future auditing (only report groups added/removed), which speaks to
the below comments about logging. However, even just having logging of
group membership caused by OIDC would be sufficient. Plus, simplifying
this makes a lot more sense to me in hindsight.

> some higher-level questions:
>
> do we want to have a bit more logging here, although it would only end
> up in syslog/journal.. maybe at least for groups which are created?

I don't mind adding more logging at all. I'm all for supporting more
auditable actions. Do you have any other suggestions of where logging
would be useful? My current thoughts are: groups auto created, user
removed from groups, user added to groups.

> do we want to mangle the group names to include the OIDC-realm name,
> like we do for LDAP/AD syncing? that way it is more clear that those
> groups originated from OIDC.. downside is that you can't use a group
> shared between OIDC and other realms..

Yes, this was what went through my mind in consideration. Adding the
prefix definitely would make them nearly unique across providers. On
the flipside, the groups names should be able to be controlled at the
IdP. What do you think about adding an advanced option for adding a
prefix to the front of the group name as the realm name, that way the
user could have both options (I'm thinking a boolean that adds what
you suggested above, default opted out)? Actually might not be a bad
idea for other auth types, too.

> > +                         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);
>
> to be honest I am not sure why that post-if is there in the first place..
>
> when parsing the config, the group is initialized with an empty 'users'
> hash as members before filling using the member list, so there is no
> risk of auto-vivifying something here..
>
> if you want to delete a user's group membership, you can just
>
> delete $usercfg->{groups}->{$group}->{users}->{$username};
>
> and we don't need this refactoring

Will update and remove the refactor. Thanks for pointing that out.

> >      }
> >  }
> >
> > +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]+$/) {
>
> see below ;) if the RE moves there, we could actually move the whole
> helper and format registration (maybe as a separate commit/patch?)

Would a separate patch for this be preferred? I don't mind working it
into this one as well.

> >
> >       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\.\-_/;
>
> this should probably be in PVE::Auth::Plugin, where the user and realm
> REs are as well.

I'm just recently diving into plugins in another patch I'm working on
and definitely see where this makes more sense.

> > +
> >  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',
>
> this should probably be restricted (our usual safeid might be too
> strict (I know some OIDC providers like to user things like @ and : in
> attributes, would have to check what the spec says here?), but some
> sensible set of characters at least would be nice ;))

If this field must be restricted, then I'd suggest we start with
printable ASCII characters as a reasonable limitation. That would
cover everything that would likely end up in a JWT. If anything
outside of that is needed, a patch could be submitted. Could start
with a 128 character limit on length as well. Thoughts?


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

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

[Fabian Grünbichler]

On February 6, 2025 6:06 am, Thomas Skinner wrote:
> On Fri, Jan 24, 2025 at 4:18 AM Fabian Grünbichler
> <f.gruenbichler@proxmox.com> wrote:
>>
>> On December 24, 2024 9:24 pm, Thomas Skinner wrote:
>> > 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 {
>>
>> style nit: we mostly use array references in our code, i.e.
>>
>> my $oidc_group_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}};
>>
>> same here
>>
>> > +
>> > +                         my @groups_intersect;
>>
>> and here
>>
>> > +                         if ( $config->{'groups-autocreate'} ) {
>> > +                             # populate all groups in claim
>> > +                             @groups_intersect = @oidc_groups_list;
>>
>> and here ;)
>>
>> > +                         }
>> > +                         else {
>>
>> style nit: the '}' and 'else {' should be on a single line
>>
>> > +                             # 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,
>> > +                             )};
>>
>> then this simply becomes
>>
>> $groups_intersect = PVE::Tools::array_intersect($oidc_groups_list, $existing_groups_list);
> 
> Understood for all the above and will fix. Apologies for missing them
> when they are clearly in the style guide. Is there any linting setup
> that you'd recommend for my dev environment that would help catch
> these for future patches before I send them? I don't want to recreate
> the wheel if the Proxmox team already has something in-place that
> could be shared.

there's perlcritic that can help with this (see bottom part of the perl
style guide), but given perl's nature, there isn't really a proper tool
that really handles everything..

> 
>> but since the existing groups are already a hash, you could also do the
>> intersection + conversion to a hash in a single go here (see below for
>> why a hash is nicer anyway):
>>
>>                             if ($config->{'groups-autocreate'}) {
>>                                 # populate all groups in claim
>>                                 $groups_intersect = { map { $_ => 1 } $oidc_groups_list->@* };
>>                             } else {
>>                                 # populate only existing groups
>>                                 for my $group ($oidc_groups_list->@*) {
>>                                     $groups_intersect->{$group} = 1 if $usercfg->{groups}->{$group};
>>                                 }
>>                             }
>>
>>
>> > +                         }
>> > +
>> > +                         # 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;
>> > +
>>
>>
>> we don't do post-for either ;)
>>
>> you can just do
>>
>> # converts group intersection to hash (but see above ;))
>> my $groups_intersect_hash = { map { $_ => 1 } $groups_intersect->@ };
>>
>> if ($config->{'groups-overwrite'}) {
>>     my $user_groups = $usercfg->{users}->{$username}->{groups};
>>     for my $group (keys $user_groups->%*) {
>>         delete $user_groups->{$group} if !groups_intersect_hash->{$group};
>>     }
>> }
>>
>> and similarly for the addition, although with addition, you could even
>> do it unconditionally since it is both cheap and idempotent (it just
>> sets two booleans in the $usercfg hash).
>>
>> or you could make it a bit easier, and just delete all groups and then
>> add all the groups in the intersecting set, which would be similar to
>> how we do it when directly overwriting a user's group via the API/pveum:
>>
>> https://git.proxmox.com/?p=pve-access-control.git;a=blob;f=src/PVE/API2/User.pm;h=535e58e0b3dbd4c8d8c882b7845173489803d8d0;hb=HEAD#l417
>>
>> the changes will only persisted on-disk once you hit the cfs_write in the
>> next line, so this shouldn't be an issue.
> 
> The reason that I made an adjustment to the behavior was purely for
> future auditing (only report groups added/removed), which speaks to
> the below comments about logging. However, even just having logging of
> group membership caused by OIDC would be sufficient. Plus, simplifying
> this makes a lot more sense to me in hindsight.
> 
>> some higher-level questions:
>>
>> do we want to have a bit more logging here, although it would only end
>> up in syslog/journal.. maybe at least for groups which are created?
> 
> I don't mind adding more logging at all. I'm all for supporting more
> auditable actions. Do you have any other suggestions of where logging
> would be useful? My current thoughts are: groups auto created, user
> removed from groups, user added to groups.

I think for groups being created it makes sense, user group
modifications maybe as well, if you want to structure the code in a way
that gives you that information ;)

> 
>> do we want to mangle the group names to include the OIDC-realm name,
>> like we do for LDAP/AD syncing? that way it is more clear that those
>> groups originated from OIDC.. downside is that you can't use a group
>> shared between OIDC and other realms..
> 
> Yes, this was what went through my mind in consideration. Adding the
> prefix definitely would make them nearly unique across providers. On
> the flipside, the groups names should be able to be controlled at the
> IdP. What do you think about adding an advanced option for adding a
> prefix to the front of the group name as the realm name, that way the
> user could have both options (I'm thinking a boolean that adds what
> you suggested above, default opted out)? Actually might not be a bad
> idea for other auth types, too.

the main reason to "namespace" the groups is to prevent accidents (group
already exists with different semantics and has ACLs, OIDC adds user to
group, accidentally gave access to some resource as a result)

I don't think there is a huge use case for the non-namespaced variant -
if you want to pre-configure a group that is referenced via OIDC, you
can still do that after 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/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);
>>
>> to be honest I am not sure why that post-if is there in the first place..
>>
>> when parsing the config, the group is initialized with an empty 'users'
>> hash as members before filling using the member list, so there is no
>> risk of auto-vivifying something here..
>>
>> if you want to delete a user's group membership, you can just
>>
>> delete $usercfg->{groups}->{$group}->{users}->{$username};
>>
>> and we don't need this refactoring
> 
> Will update and remove the refactor. Thanks for pointing that out.
> 
>> >      }
>> >  }
>> >
>> > +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]+$/) {
>>
>> see below ;) if the RE moves there, we could actually move the whole
>> helper and format registration (maybe as a separate commit/patch?)
> 
> Would a separate patch for this be preferred? I don't mind working it
> into this one as well.

yes, this makes sense as a standalone patch IMHO.

>> >
>> >       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\.\-_/;
>>
>> this should probably be in PVE::Auth::Plugin, where the user and realm
>> REs are as well.
> 
> I'm just recently diving into plugins in another patch I'm working on
> and definitely see where this makes more sense.
> 
>> > +
>> >  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',
>>
>> this should probably be restricted (our usual safeid might be too
>> strict (I know some OIDC providers like to user things like @ and : in
>> attributes, would have to check what the spec says here?), but some
>> sensible set of characters at least would be nice ;))
> 
> If this field must be restricted, then I'd suggest we start with
> printable ASCII characters as a reasonable limitation. That would
> cover everything that would likely end up in a JWT. If anything
> outside of that is needed, a patch could be submitted. Could start
> with a 128 character limit on length as well. Thoughts?

sounds sensible.

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

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

[Thomas Skinner]

On Mon, Feb 10, 2025 at 4:43=E2=80=AFAM Fabian Gr=C3=BCnbichler
<f.gruenbichler@proxmox.com> wrote:
>
> On February 6, 2025 6:06 am, Thomas Skinner wrote:
> > On Fri, Jan 24, 2025 at 4:18=E2=80=AFAM Fabian Gr=C3=BCnbichler
> > <f.gruenbichler@proxmox.com> wrote:
> >>
> >> On December 24, 2024 9:24 pm, Thomas Skinner wrote:
> >> > 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 =3D $config->{'groups-claim'}=
)) {
> >> > +             if (defined(my $groups_list =3D $info->{$groups_claim}=
)) {
> >> > +                 if (ref($groups_list) eq 'ARRAY') {
> >> > +                     PVE::AccessControl::lock_user_config(sub {
> >> > +                         my $usercfg =3D cfs_read_file("user.cfg");
> >> > +
> >> > +                         # replace any invalid characters with
> >> > +                         my $replace_character =3D $config->{'group=
s-replace-character'} // '_';
> >> > +                         my @oidc_groups_list =3D map {
> >>
> >> style nit: we mostly use array references in our code, i.e.
> >>
> >> my $oidc_group_list =3D [ map ... ];
> >>
> >> > +                             $_ =3D~ s/[^$PVE::Auth::OpenId::groupn=
ame_regex_chars]/$replace_character/gr
> >> > +                         } $groups_list->@*;
> >> > +
> >> > +                         # list groups that exist in pve
> >> > +                         my @existing_groups_list =3D keys %{$userc=
fg->{groups}};
> >>
> >> same here
> >>
> >> > +
> >> > +                         my @groups_intersect;
> >>
> >> and here
> >>
> >> > +                         if ( $config->{'groups-autocreate'} ) {
> >> > +                             # populate all groups in claim
> >> > +                             @groups_intersect =3D @oidc_groups_lis=
t;
> >>
> >> and here ;)
> >>
> >> > +                         }
> >> > +                         else {
> >>
> >> style nit: the '}' and 'else {' should be on a single line
> >>
> >> > +                             # only populate groups that are in the=
 oidc list and exist in pve
> >> > +                             @groups_intersect =3D @{PVE::Tools::ar=
ray_intersect(
> >> > +                                 \@oidc_groups_list,
> >> > +                                 \@existing_groups_list,
> >> > +                             )};
> >>
> >> then this simply becomes
> >>
> >> $groups_intersect =3D PVE::Tools::array_intersect($oidc_groups_list, $=
existing_groups_list);
> >
> > Understood for all the above and will fix. Apologies for missing them
> > when they are clearly in the style guide. Is there any linting setup
> > that you'd recommend for my dev environment that would help catch
> > these for future patches before I send them? I don't want to recreate
> > the wheel if the Proxmox team already has something in-place that
> > could be shared.
>
> there's perlcritic that can help with this (see bottom part of the perl
> style guide), but given perl's nature, there isn't really a proper tool
> that really handles everything..

Okay, that's what I was figuring. I've been using the perlcritic, and
it's worked for syntax pretty well. I tried perltidy, too, but it's
not nearly perfect at formatting sections of code. Thanks!

> >
> >> but since the existing groups are already a hash, you could also do th=
e
> >> intersection + conversion to a hash in a single go here (see below for
> >> why a hash is nicer anyway):
> >>
> >>                             if ($config->{'groups-autocreate'}) {
> >>                                 # populate all groups in claim
> >>                                 $groups_intersect =3D { map { $_ =3D> =
1 } $oidc_groups_list->@* };
> >>                             } else {
> >>                                 # populate only existing groups
> >>                                 for my $group ($oidc_groups_list->@*) =
{
> >>                                     $groups_intersect->{$group} =3D 1 =
if $usercfg->{groups}->{$group};
> >>                                 }
> >>                             }
> >>
> >>
> >> > +                         }
> >> > +
> >> > +                         # if groups should be overwritten, find an=
d delete the ones to remove
> >> > +                         if ( $config->{'groups-overwrite'} ) {
> >> > +                             # get the groups that need to be remov=
ed from the user
> >> > +                             my %groups_remove_user;
> >> > +                             $groups_remove_user{ $_ } =3D undef
> >> > +                                 for keys %{$usercfg->{users}->{$us=
ername}->{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{ $_ } =3D undef for @grou=
ps_intersect;
> >> > +                         delete $groups_add_user{ $_ }
> >> > +                             for keys %{$usercfg->{users}->{$userna=
me}->{groups}};
> >> > +
> >> > +                         # ensure user is a member of these groups
> >> > +                         PVE::AccessControl::add_user_group(
> >> > +                             $username,
> >> > +                             $usercfg,
> >> > +                             $_
> >> > +                         ) for keys %groups_add_user;
> >> > +
> >>
> >>
> >> we don't do post-for either ;)
> >>
> >> you can just do
> >>
> >> # converts group intersection to hash (but see above ;))
> >> my $groups_intersect_hash =3D { map { $_ =3D> 1 } $groups_intersect->@=
 };
> >>
> >> if ($config->{'groups-overwrite'}) {
> >>     my $user_groups =3D $usercfg->{users}->{$username}->{groups};
> >>     for my $group (keys $user_groups->%*) {
> >>         delete $user_groups->{$group} if !groups_intersect_hash->{$gro=
up};
> >>     }
> >> }
> >>
> >> and similarly for the addition, although with addition, you could even
> >> do it unconditionally since it is both cheap and idempotent (it just
> >> sets two booleans in the $usercfg hash).
> >>
> >> or you could make it a bit easier, and just delete all groups and then
> >> add all the groups in the intersecting set, which would be similar to
> >> how we do it when directly overwriting a user's group via the API/pveu=
m:
> >>
> >> https://git.proxmox.com/?p=3Dpve-access-control.git;a=3Dblob;f=3Dsrc/P=
VE/API2/User.pm;h=3D535e58e0b3dbd4c8d8c882b7845173489803d8d0;hb=3DHEAD#l417
> >>
> >> the changes will only persisted on-disk once you hit the cfs_write in =
the
> >> next line, so this shouldn't be an issue.
> >
> > The reason that I made an adjustment to the behavior was purely for
> > future auditing (only report groups added/removed), which speaks to
> > the below comments about logging. However, even just having logging of
> > group membership caused by OIDC would be sufficient. Plus, simplifying
> > this makes a lot more sense to me in hindsight.
> >
> >> some higher-level questions:
> >>
> >> do we want to have a bit more logging here, although it would only end
> >> up in syslog/journal.. maybe at least for groups which are created?
> >
> > I don't mind adding more logging at all. I'm all for supporting more
> > auditable actions. Do you have any other suggestions of where logging
> > would be useful? My current thoughts are: groups auto created, user
> > removed from groups, user added to groups.
>
> I think for groups being created it makes sense, user group
> modifications maybe as well, if you want to structure the code in a way
> that gives you that information ;)

Me, too. I put in the following info messages:
- Message with all groups auto created from OIDC (only happens when
they are created the first time)
- Message if all groups were removed (only happens when override box is che=
cked)
- Message with groups assigned by OIDC (happens on every login)

> >
> >> do we want to mangle the group names to include the OIDC-realm name,
> >> like we do for LDAP/AD syncing? that way it is more clear that those
> >> groups originated from OIDC.. downside is that you can't use a group
> >> shared between OIDC and other realms..
> >
> > Yes, this was what went through my mind in consideration. Adding the
> > prefix definitely would make them nearly unique across providers. On
> > the flipside, the groups names should be able to be controlled at the
> > IdP. What do you think about adding an advanced option for adding a
> > prefix to the front of the group name as the realm name, that way the
> > user could have both options (I'm thinking a boolean that adds what
> > you suggested above, default opted out)? Actually might not be a bad
> > idea for other auth types, too.
>
> the main reason to "namespace" the groups is to prevent accidents (group
> already exists with different semantics and has ACLs, OIDC adds user to
> group, accidentally gave access to some resource as a result)
>
> I don't think there is a huge use case for the non-namespaced variant -
> if you want to pre-configure a group that is referenced via OIDC, you
> can still do that after all..

Yeah, I agree that it's got to be an all or nothing if "namespacing"
is going to happen with characters that could exist in the group name.
I will suffix with the realm like the AD/LDAP for consistency (bonus:
this would help if someone migrates from AD/LDAP to OIDC and uses the
same realm name).

> >
> >> > +                         cfs_write_file("user.cfg", $usercfg);
> >> > +                     }, "openid group mapping failed");
> >> > +                 } else {
> >> > +                     syslog('err', "groups list is not an array; gr=
oups will not be updated");
> >> > +                 }
> >> > +             } else {
> >> > +                 syslog('err', "groups claim '$groups_claim' is not=
 found in claims");
> >> > +             }
> >> > +         }
> >> > +
> >> >           my $ticket =3D PVE::AccessControl::assemble_ticket($userna=
me);
> >> >           my $csrftoken =3D PVE::AccessControl::assemble_csrf_preven=
tion_token($username);
> >> >           my $cap =3D $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) =3D @_;
> >> >
> >> >      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);
> >>
> >> to be honest I am not sure why that post-if is there in the first plac=
e..
> >>
> >> when parsing the config, the group is initialized with an empty 'users=
'
> >> hash as members before filling using the member list, so there is no
> >> risk of auto-vivifying something here..
> >>
> >> if you want to delete a user's group membership, you can just
> >>
> >> delete $usercfg->{groups}->{$group}->{users}->{$username};
> >>
> >> and we don't need this refactoring
> >
> > Will update and remove the refactor. Thanks for pointing that out.
> >
> >> >      }
> >> >  }
> >> >
> >> > +sub delete_user_group_single {
> >> > +    my ($username, $usercfg, $group) =3D @_;
> >> > +
> >> > +    delete ($usercfg->{groups}->{$group}->{users}->{$username})
> >> > +     if $usercfg->{groups}->{$group}->{users}->{$username};
> >> > +}
> >> > +
> >> >  sub delete_user_acl {
> >> >      my ($username, $usercfg) =3D @_;
> >> >
> >> > @@ -1293,7 +1298,7 @@ PVE::JSONSchema::register_format('pve-groupid'=
, \&verify_groupname);
> >> >  sub verify_groupname {
> >> >      my ($groupname, $noerr) =3D @_;
> >> >
> >> > -    if ($groupname !~ m/^[A-Za-z0-9\.\-_]+$/) {
> >> > +    if ($groupname !~ m/^[$PVE::Auth::OpenId::groupname_regex_chars=
]+$/) {
> >>
> >> see below ;) if the RE moves there, we could actually move the whole
> >> helper and format registration (maybe as a separate commit/patch?)
> >
> > Would a separate patch for this be preferred? I don't mind working it
> > into this one as well.
>
> yes, this makes sense as a standalone patch IMHO.

Looking at the registrations, it looks like the group format is
registered in src/PVE/AccessControl.pm. I've put the RE there for
consistency. Can submit another patch for moving the registrations
from AccessControl.pm if desired, but at least this seems like the
sensible place for this change.

> >> >
> >> >       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 =3D qr/A-Za-z0-9\.\-_/;
> >>
> >> this should probably be in PVE::Auth::Plugin, where the user and realm
> >> REs are as well.
> >
> > I'm just recently diving into plugins in another patch I'm working on
> > and definitely see where this makes more sense.
> >
> >> > +
> >> >  sub type {
> >> >      return 'openid';
> >> >  }
> >> > @@ -42,6 +44,30 @@ sub properties {
> >> >           type =3D> 'string',
> >> >           optional =3D> 1,
> >> >       },
> >> > +     "groups-claim" =3D> {
> >> > +         description =3D> "OpenID claim used to retrieve groups wit=
h.",
> >> > +         type =3D> 'string',
> >>
> >> this should probably be restricted (our usual safeid might be too
> >> strict (I know some OIDC providers like to user things like @ and : in
> >> attributes, would have to check what the spec says here?), but some
> >> sensible set of characters at least would be nice ;))
> >
> > If this field must be restricted, then I'd suggest we start with
> > printable ASCII characters as a reasonable limitation. That would
> > cover everything that would likely end up in a JWT. If anything
> > outside of that is needed, a patch could be submitted. Could start
> > with a 128 character limit on length as well. Thoughts?
>
> sounds sensible.

I ended up going with 256 characters to match the other fields that
were already there. Unlikely to be that long, but at least it'll
handle nearly anything that's thrown at it.

> >
> >> > +         optional =3D> 1,
> >> > +     },
> >> > +     "groups-autocreate" =3D> {
> >> > +         description =3D> "Automatically create groups if they do n=
ot exist.",
> >> > +         optional =3D> 1,
> >> > +         type =3D> 'boolean',
> >> > +         default =3D> 0,
> >> > +     },
> >> > +     "groups-overwrite" =3D> {
> >> > +         description =3D> "All groups will be overwritten for the u=
ser on login.",
> >> > +         type =3D> 'boolean',
> >> > +         default =3D> 0,
> >> > +         optional =3D> 1,
> >> > +     },
> >> > +     "groups-replace-character" =3D> {
> >> > +         description =3D> "Character used to replace any invalid ch=
aracters in groups from provider.",
> >> > +         type =3D> 'string',
> >> > +         pattern =3D> qr/^[$groupname_regex_chars]$/,
> >> > +         default =3D> '_',
> >> > +         optional =3D> 1,
> >> > +     },
> >> >       prompt =3D> {
> >> >           description =3D> "Specifies whether the Authorization Serv=
er prompts the End-User for"
> >> >               ." reauthentication and consent.",
> >> > @@ -73,6 +99,10 @@ sub options {
> >> >       "client-key" =3D> { optional =3D> 1 },
> >> >       autocreate =3D> { optional =3D> 1 },
> >> >       "username-claim" =3D> { optional =3D> 1, fixed =3D> 1 },
> >> > +     "groups-claim" =3D> { optional =3D> 1 },
> >> > +     "groups-autocreate" =3D> { optional =3D> 1 },
> >> > +     "groups-overwrite" =3D> { optional =3D> 1 },
> >> > +     "groups-replace-character" =3D> { optional =3D> 1},
> >> >       prompt =3D> { optional =3D> 1 },
> >> >       scopes =3D> { optional =3D> 1 },
> >> >       "acr-values" =3D> { optional =3D> 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

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

[Thomas Skinner]

> do we want to mangle the group names to include the OIDC-realm name,
> like we do for LDAP/AD syncing? that way it is more clear that those
> groups originated from OIDC.. downside is that you can't use a group
> shared between OIDC and other realms..

More on this: it looks like in LDAP/AD sync, the group is suffixed
with `-$realm`, which does meet the requirements of the existing regex
character requirements for groups. We could do the same thing with the
OIDC groups. I will add that suffix in as the option to be consistent.

-----

On a similar but separate note, I think that it may be more clear of
where groups come from to have them optionally suffixed with something
like `@$realm` like is already done for users. The verify function for
groups could be adjusted to validate on a regex that makes the suffix
optional and validates it as `group_regex@realm_regex`. The downside
of this change is that it would break any existing groups or the
module would need to be adjusted to continue to support or migrate the
groups with the old suffix. An upside of this change is eliminating
inadvertent group collisions. For example:

Consider we have 2 realms: AD/LDAP "ad-admins" and OIDC "oidc". Realm
"ad-admins" suffixes groups automatically and realm "oidc" is
configured to not suffix. Realm "ad-admins" contains a group
"privileged" and a realm "oidc" claim sends a group
"privileged-ad-admins".

With the existing configuration, the group "privileged" from realm
"ad-admins" becomes "privileged-ad-admins" in PVE. The user with the
group "privileged-ad-admins" in the OIDC claim is then granted access
to this group on login. This could lead to potential erroneous group
membership because the group names could overlap.

If the suffix was changed to include a character that would not exist
in the group name (e.g. "@"), then it would be impossible to have this
overlap. Additionally, this suffixing option could be extended to any
other realm type that could introduce groups into PVE. I can see
scenarios where having the option to enable/disable group suffixes for
all realms would be useful. If the admin controls all the
authentication services (or at least can ensure no inadvertent
collisions would occur), the suffix is not needed. Non-suffixed group
names could also simplify ACL delegation. If the admin cannot
guarantee inadvertent collisions in group names, then suffixes that do
not include a valid group character would be necessary to prevent
collisions.

If PVE is interested in moving towards this, I'd be happy to start
authoring a patch to support it. If nothing else, I can file a bug and
we can continue discussion there.

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

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

[Fabian Grünbichler]

On February 8, 2025 7:40 am, Thomas Skinner wrote:
>> do we want to mangle the group names to include the OIDC-realm name,
>> like we do for LDAP/AD syncing? that way it is more clear that those
>> groups originated from OIDC.. downside is that you can't use a group
>> shared between OIDC and other realms..
> 
> More on this: it looks like in LDAP/AD sync, the group is suffixed
> with `-$realm`, which does meet the requirements of the existing regex
> character requirements for groups. We could do the same thing with the
> OIDC groups. I will add that suffix in as the option to be consistent.
> 
> -----
> 
> On a similar but separate note, I think that it may be more clear of
> where groups come from to have them optionally suffixed with something
> like `@$realm` like is already done for users. The verify function for
> groups could be adjusted to validate on a regex that makes the suffix
> optional and validates it as `group_regex@realm_regex`. The downside
> of this change is that it would break any existing groups or the
> module would need to be adjusted to continue to support or migrate the
> groups with the old suffix. An upside of this change is eliminating
> inadvertent group collisions. For example:

groups are not really scoped to a realm though, we only did this
"soft-namespace" for the sync feature to avoid accidents (similar to the
one you describe below).

> Consider we have 2 realms: AD/LDAP "ad-admins" and OIDC "oidc". Realm
> "ad-admins" suffixes groups automatically and realm "oidc" is
> configured to not suffix. Realm "ad-admins" contains a group
> "privileged" and a realm "oidc" claim sends a group
> "privileged-ad-admins".
> 
> With the existing configuration, the group "privileged" from realm
> "ad-admins" becomes "privileged-ad-admins" in PVE. The user with the
> group "privileged-ad-admins" in the OIDC claim is then granted access
> to this group on login. This could lead to potential erroneous group
> membership because the group names could overlap.

yes, this is why I think OIDC groups should also always add the suffix,
just like LDAP/AD. that way, any group name collision must stem from a
manually created group containing the suffix (which can still happen,
but is a lot less likely than without).

> 
> If the suffix was changed to include a character that would not exist
> in the group name (e.g. "@"), then it would be impossible to have this
> overlap. Additionally, this suffixing option could be extended to any
> other realm type that could introduce groups into PVE. I can see
> scenarios where having the option to enable/disable group suffixes for
> all realms would be useful. If the admin controls all the
> authentication services (or at least can ensure no inadvertent
> collisions would occur), the suffix is not needed. Non-suffixed group
> names could also simplify ACL delegation. If the admin cannot
> guarantee inadvertent collisions in group names, then suffixes that do
> not include a valid group character would be necessary to prevent
> collisions.
> 
> If PVE is interested in moving towards this, I'd be happy to start
> authoring a patch to support it. If nothing else, I can file a bug and
> we can continue discussion there.

I think this would be a very hard transition given that user.cfg
contents are at the very core of PVE - and it would break valid use
cases like pre-configuring groups that are part of a AD/LDAP-sync or
OIDC-autocreation scope. but if other people think this is worth
exploring, I am of course not objecting to further discussing it :)


_______________________________________________
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