* [pve-devel] [PATCH docs v3 1/1] fix #4411: openid: add docs for openid groups support
2025-02-11 5:40 [pve-devel] [PATCH SERIES access-control/docs/manager/proxmox-openid v3] fix #4411: add support for openid groups Thomas Skinner
@ 2025-02-11 5:40 ` Thomas Skinner
2025-02-11 5:40 ` [pve-devel] [PATCH proxmox-openid v3 1/1] fix #4411: openid: add library code for generic id token claim support Thomas Skinner
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Thomas Skinner @ 2025-02-11 5:40 UTC (permalink / raw)
To: pve-devel; +Cc: Thomas Skinner
Signed-off-by: Thomas Skinner <thomas@atskinner.net>
---
pveum.adoc | 44 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 44 insertions(+)
diff --git a/pveum.adoc b/pveum.adoc
index 81565ab..1166f17 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,41 @@ 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}. To prevent collisions, group names from the OpenID claim are suffixed
+with `-<realm name>` (e.g. for the OpenID group name `my-openid-group` in the
+realm `oidc`, the group name in {pve} would be `my-openid-group-oidc`).
+
+Any groups reported by the OpenID provider that do not exist in {pve} are
+ignored by default. If all groups reported by the OpenID provider should exist
+in {pve}, the `groups-autocreate` option may be used to automatically create
+these groups on user logins.
+
+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
^ permalink raw reply [flat|nested] 9+ messages in thread* [pve-devel] [PATCH proxmox-openid v3 1/1] fix #4411: openid: add library code for generic id token claim support
2025-02-11 5:40 [pve-devel] [PATCH SERIES access-control/docs/manager/proxmox-openid v3] fix #4411: add support for openid groups Thomas Skinner
2025-02-11 5:40 ` [pve-devel] [PATCH docs v3 1/1] fix #4411: openid: add docs for openid groups support Thomas Skinner
@ 2025-02-11 5:40 ` Thomas Skinner
2025-02-11 5:40 ` [pve-devel] [PATCH access-control v3 1/1] fix #4411: openid: add logic for openid groups support Thomas Skinner
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Thomas Skinner @ 2025-02-11 5:40 UTC (permalink / raw)
To: pve-devel; +Cc: 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
^ permalink raw reply [flat|nested] 9+ messages in thread* [pve-devel] [PATCH access-control v3 1/1] fix #4411: openid: add logic for openid groups support
2025-02-11 5:40 [pve-devel] [PATCH SERIES access-control/docs/manager/proxmox-openid v3] fix #4411: add support for openid groups Thomas Skinner
2025-02-11 5:40 ` [pve-devel] [PATCH docs v3 1/1] fix #4411: openid: add docs for openid groups support Thomas Skinner
2025-02-11 5:40 ` [pve-devel] [PATCH proxmox-openid v3 1/1] fix #4411: openid: add library code for generic id token claim support Thomas Skinner
@ 2025-02-11 5:40 ` Thomas Skinner
2025-02-12 14:51 ` Mira Limbeck
2025-02-11 5:40 ` [pve-devel] [PATCH manager v3 1/1] fix #4411: openid: add ui config " Thomas Skinner
2025-02-12 14:47 ` [pve-devel] [PATCH SERIES access-control/docs/manager/proxmox-openid v3] fix #4411: add support for openid groups Mira Limbeck
4 siblings, 1 reply; 9+ messages in thread
From: Thomas Skinner @ 2025-02-11 5:40 UTC (permalink / raw)
To: pve-devel; +Cc: Thomas Skinner
Signed-off-by: Thomas Skinner <thomas@atskinner.net>
---
src/PVE/API2/OpenId.pm | 79 ++++++++++++++++++++++++++++++++++++++++
src/PVE/AccessControl.pm | 2 +-
src/PVE/Auth/OpenId.pm | 33 +++++++++++++++++
src/PVE/Auth/Plugin.pm | 1 +
4 files changed, 114 insertions(+), 1 deletion(-)
diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
index 77410e6..818175e 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,84 @@ __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 = { map {
+ $_ =~ s/[^$PVE::Auth::Plugin::groupname_regex_chars]/$replace_character/gr => 1
+ } $groups_list->@* };
+
+ # add realm name as suffix to group
+ my $suffixed_oidc_groups;
+ for my $group (keys %$oidc_groups) {
+ $suffixed_oidc_groups->{"$group-$realm"} = 1;
+ }
+ $oidc_groups = $suffixed_oidc_groups;
+
+ # get groups that exist in OIDC and PVE
+ my $groups_intersect;
+ for my $group (keys %$oidc_groups) {
+ $groups_intersect->{$group} = 1 if $usercfg->{groups}->{$group};
+ }
+
+ if ($config->{'groups-autocreate'}) {
+ # populate all groups in claim
+ $groups_intersect = $oidc_groups;
+ my $groups_to_create;
+ for my $group (keys %$oidc_groups) {
+ $groups_to_create->{$group} = 1 if !$usercfg->{groups}->{$group};
+ }
+ if ($groups_to_create) {
+ # log a messages about created groups here
+ my $groups_to_create_string = join(', ', sort keys %$groups_to_create);
+ syslog(
+ 'info',
+ "groups created automatically from openid claim: $groups_to_create_string"
+ );
+ }
+ }
+
+ # if groups should be overwritten, delete all the users groups first
+ if ( $config->{'groups-overwrite'} ) {
+ PVE::AccessControl::delete_user_group(
+ $username,
+ $usercfg,
+ );
+ syslog(
+ 'info',
+ "openid overwrite groups enabled; user '$username' removed from all groups"
+ );
+ }
+
+ # ensure user is a member of the groups
+ for my $group (keys %$groups_intersect) {
+ PVE::AccessControl::add_user_group(
+ $username,
+ $usercfg,
+ $group
+ );
+ }
+ my $groups_intersect_string = join(', ', sort keys %$groups_intersect);
+ syslog(
+ 'info',
+ "openid user '$username' added to groups: $groups_intersect_string"
+ );
+
+ cfs_write_file("user.cfg", $usercfg);
+ }, "openid group mapping failed");
+ } else {
+ syslog('err', "openid groups list is not an array; groups will not be updated");
+ }
+ } else {
+ syslog('err', "openid 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..7493c57 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -1293,7 +1293,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::Plugin::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..fd1cd6f 100755
--- a/src/PVE/Auth/OpenId.pm
+++ b/src/PVE/Auth/OpenId.pm
@@ -9,6 +9,9 @@ use PVE::Cluster qw(cfs_register_file cfs_read_file cfs_write_file cfs_lock_file
use base qw(PVE::Auth::Plugin);
+# include all printable ascii characters
+my $openid_claim_regex = qr/[ -~]+/;
+
sub type {
return 'openid';
}
@@ -42,6 +45,32 @@ sub properties {
type => 'string',
optional => 1,
},
+ "groups-claim" => {
+ description => "OpenID claim used to retrieve groups with.",
+ type => 'string',
+ pattern => $openid_claim_regex,
+ maxLength => 256,
+ 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/^[$PVE::Auth::Plugin::groupname_regex_chars]$/,
+ default => '_',
+ optional => 1,
+ },
prompt => {
description => "Specifies whether the Authorization Server prompts the End-User for"
." reauthentication and consent.",
@@ -73,6 +102,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 },
diff --git a/src/PVE/Auth/Plugin.pm b/src/PVE/Auth/Plugin.pm
index 763239f..7617044 100755
--- a/src/PVE/Auth/Plugin.pm
+++ b/src/PVE/Auth/Plugin.pm
@@ -31,6 +31,7 @@ sub lock_domain_config {
our $realm_regex = qr/[A-Za-z][A-Za-z0-9\.\-_]+/;
our $user_regex = qr![^\s:/]+!;
+our $groupname_regex_chars = qr/A-Za-z0-9\.\-_/;
PVE::JSONSchema::register_format('pve-realm', \&pve_verify_realm);
sub pve_verify_realm {
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [pve-devel] [PATCH access-control v3 1/1] fix #4411: openid: add logic for openid groups support
2025-02-11 5:40 ` [pve-devel] [PATCH access-control v3 1/1] fix #4411: openid: add logic for openid groups support Thomas Skinner
@ 2025-02-12 14:51 ` Mira Limbeck
2025-02-13 11:03 ` Fabian Grünbichler
0 siblings, 1 reply; 9+ messages in thread
From: Mira Limbeck @ 2025-02-12 14:51 UTC (permalink / raw)
To: pve-devel
On 2/11/25 06:40, Thomas Skinner wrote:
> Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> ---
> src/PVE/API2/OpenId.pm | 79 ++++++++++++++++++++++++++++++++++++++++
> src/PVE/AccessControl.pm | 2 +-
> src/PVE/Auth/OpenId.pm | 33 +++++++++++++++++
> src/PVE/Auth/Plugin.pm | 1 +
> 4 files changed, 114 insertions(+), 1 deletion(-)
>
> diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
> index 77410e6..818175e 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,84 @@ __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 = { map {
> + $_ =~ s/[^$PVE::Auth::Plugin::groupname_regex_chars]/$replace_character/gr => 1
> + } $groups_list->@* };
maybe we could log any of those replacements here? doing this silently
may lead to confusion when groups don't match
> +
> + # add realm name as suffix to group
> + my $suffixed_oidc_groups;
> + for my $group (keys %$oidc_groups) {
> + $suffixed_oidc_groups->{"$group-$realm"} = 1;
> + }
> + $oidc_groups = $suffixed_oidc_groups;
> +
> + # get groups that exist in OIDC and PVE
> + my $groups_intersect;
> + for my $group (keys %$oidc_groups) {
> + $groups_intersect->{$group} = 1 if $usercfg->{groups}->{$group};
> + }
> +
> + if ($config->{'groups-autocreate'}) {
> + # populate all groups in claim
> + $groups_intersect = $oidc_groups;
> + my $groups_to_create;
> + for my $group (keys %$oidc_groups) {
> + $groups_to_create->{$group} = 1 if !$usercfg->{groups}->{$group};
> + }
> + if ($groups_to_create) {
> + # log a messages about created groups here
> + my $groups_to_create_string = join(', ', sort keys %$groups_to_create);
> + syslog(
> + 'info',
> + "groups created automatically from openid claim: $groups_to_create_string"
> + );
> + }
> + }
> +
> + # if groups should be overwritten, delete all the users groups first
> + if ( $config->{'groups-overwrite'} ) {
> + PVE::AccessControl::delete_user_group(
> + $username,
> + $usercfg,
> + );
> + syslog(
> + 'info',
> + "openid overwrite groups enabled; user '$username' removed from all groups"
> + );
> + }
> +
> + # ensure user is a member of the groups
> + for my $group (keys %$groups_intersect) {
> + PVE::AccessControl::add_user_group(
> + $username,
> + $usercfg,
> + $group
> + );
> + }
> + my $groups_intersect_string = join(', ', sort keys %$groups_intersect);
> + syslog(
> + 'info',
> + "openid user '$username' added to groups: $groups_intersect_string"
> + );
> +
> + cfs_write_file("user.cfg", $usercfg);
> + }, "openid group mapping failed");
> + } else {
> + syslog('err', "openid groups list is not an array; groups will not be updated");
> + }
> + } else {
> + syslog('err', "openid 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..7493c57 100644
> --- a/src/PVE/AccessControl.pm
> +++ b/src/PVE/AccessControl.pm
> @@ -1293,7 +1293,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::Plugin::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..fd1cd6f 100755
> --- a/src/PVE/Auth/OpenId.pm
> +++ b/src/PVE/Auth/OpenId.pm
> @@ -9,6 +9,9 @@ use PVE::Cluster qw(cfs_register_file cfs_read_file cfs_write_file cfs_lock_file
>
> use base qw(PVE::Auth::Plugin);
>
> +# include all printable ascii characters
> +my $openid_claim_regex = qr/[ -~]+/;
> +
> sub type {
> return 'openid';
> }
> @@ -42,6 +45,32 @@ sub properties {
> type => 'string',
> optional => 1,
> },
> + "groups-claim" => {
> + description => "OpenID claim used to retrieve groups with.",
> + type => 'string',
> + pattern => $openid_claim_regex,
> + maxLength => 256,
> + 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/^[$PVE::Auth::Plugin::groupname_regex_chars]$/,
> + default => '_',
> + optional => 1,
> + },
> prompt => {
> description => "Specifies whether the Authorization Server prompts the End-User for"
> ." reauthentication and consent.",
> @@ -73,6 +102,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 },
> diff --git a/src/PVE/Auth/Plugin.pm b/src/PVE/Auth/Plugin.pm
> index 763239f..7617044 100755
> --- a/src/PVE/Auth/Plugin.pm
> +++ b/src/PVE/Auth/Plugin.pm
> @@ -31,6 +31,7 @@ sub lock_domain_config {
>
> our $realm_regex = qr/[A-Za-z][A-Za-z0-9\.\-_]+/;
> our $user_regex = qr![^\s:/]+!;
> +our $groupname_regex_chars = qr/A-Za-z0-9\.\-_/;
>
> PVE::JSONSchema::register_format('pve-realm', \&pve_verify_realm);
> sub pve_verify_realm {
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [pve-devel] [PATCH access-control v3 1/1] fix #4411: openid: add logic for openid groups support
2025-02-12 14:51 ` Mira Limbeck
@ 2025-02-13 11:03 ` Fabian Grünbichler
2025-03-17 12:18 ` Fabian Grünbichler
0 siblings, 1 reply; 9+ messages in thread
From: Fabian Grünbichler @ 2025-02-13 11:03 UTC (permalink / raw)
To: Proxmox VE development discussion, Mira Limbeck
> Mira Limbeck <m.limbeck@proxmox.com> hat am 12.02.2025 15:51 CET geschrieben:
>
>
> On 2/11/25 06:40, Thomas Skinner wrote:
> > Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> > ---
> > src/PVE/API2/OpenId.pm | 79 ++++++++++++++++++++++++++++++++++++++++
> > src/PVE/AccessControl.pm | 2 +-
> > src/PVE/Auth/OpenId.pm | 33 +++++++++++++++++
> > src/PVE/Auth/Plugin.pm | 1 +
> > 4 files changed, 114 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
> > index 77410e6..818175e 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,84 @@ __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 = { map {
> > + $_ =~ s/[^$PVE::Auth::Plugin::groupname_regex_chars]/$replace_character/gr => 1
> > + } $groups_list->@* };
> maybe we could log any of those replacements here? doing this silently
> may lead to confusion when groups don't match
a similar issue is filed for LDAP/AD sync as well - and I now wonder based on the discussion there - do we really want to make this configurable? how do we want to handle conflicts? while it's a bit less critical for two or more OIDC groups to be mapped to the same PVE-side group (compared to the same happening with users ;)), if it's possible to avoid it that would still be great..
groups currently allow .-_ as special characters, so we could designate one of them as escape character and then have a unique mapping for each character that isn't allowed on the PVE side (including that escape character ;))
e.g., an OIDC group called "foo bar" could be encoded as "foo_32_bar" (where 32 is hex for ASCII-" "). correspondingly, a group called "foo_bar" would need to be encoded as "foo_5F_bar". (the second '_' could of course be left off if desired).
unfortunately, adding an entirely new escape character is not really possible unless we want to wait for 9.0, as that would then break parsing of user.cfg in a mixed cluster which can have really dangerous side-effects..
or we could live with such a potentially lossy mapping, but then I am not sure whether a single, hard-coded, documented value wouldn't be better? the main issue with that is if you allow (unprivileged) creation and joining of groups on the OIDC side, as then if there already is a group called "System Administrators" that got mapped to "System_Adminstrators" on the PVE side, a user could create and join "System!Administrators" on the OIDC side and get mapped to the existing, probably privileged "System_Administrators" group..
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [pve-devel] [PATCH access-control v3 1/1] fix #4411: openid: add logic for openid groups support
2025-02-13 11:03 ` Fabian Grünbichler
@ 2025-03-17 12:18 ` Fabian Grünbichler
0 siblings, 0 replies; 9+ messages in thread
From: Fabian Grünbichler @ 2025-03-17 12:18 UTC (permalink / raw)
To: Mira Limbeck, Proxmox VE development discussion; +Cc: Thomas Skinner
On February 13, 2025 12:03 pm, Fabian Grünbichler wrote:
>
>> Mira Limbeck <m.limbeck@proxmox.com> hat am 12.02.2025 15:51 CET geschrieben:
>>
>>
>> On 2/11/25 06:40, Thomas Skinner wrote:
>> > Signed-off-by: Thomas Skinner <thomas@atskinner.net>
>> > ---
>> > src/PVE/API2/OpenId.pm | 79 ++++++++++++++++++++++++++++++++++++++++
>> > src/PVE/AccessControl.pm | 2 +-
>> > src/PVE/Auth/OpenId.pm | 33 +++++++++++++++++
>> > src/PVE/Auth/Plugin.pm | 1 +
>> > 4 files changed, 114 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
>> > index 77410e6..818175e 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,84 @@ __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 = { map {
>> > + $_ =~ s/[^$PVE::Auth::Plugin::groupname_regex_chars]/$replace_character/gr => 1
>> > + } $groups_list->@* };
>> maybe we could log any of those replacements here? doing this silently
>> may lead to confusion when groups don't match
>
> a similar issue is filed for LDAP/AD sync as well - and I now wonder based on the discussion there - do we really want to make this configurable? how do we want to handle conflicts? while it's a bit less critical for two or more OIDC groups to be mapped to the same PVE-side group (compared to the same happening with users ;)), if it's possible to avoid it that would still be great..
>
> groups currently allow .-_ as special characters, so we could designate one of them as escape character and then have a unique mapping for each character that isn't allowed on the PVE side (including that escape character ;))
>
> e.g., an OIDC group called "foo bar" could be encoded as "foo_32_bar" (where 32 is hex for ASCII-" "). correspondingly, a group called "foo_bar" would need to be encoded as "foo_5F_bar". (the second '_' could of course be left off if desired).
>
> unfortunately, adding an entirely new escape character is not really possible unless we want to wait for 9.0, as that would then break parsing of user.cfg in a mixed cluster which can have really dangerous side-effects..
>
> or we could live with such a potentially lossy mapping, but then I am not sure whether a single, hard-coded, documented value wouldn't be better? the main issue with that is if you allow (unprivileged) creation and joining of groups on the OIDC side, as then if there already is a group called "System Administrators" that got mapped to "System_Adminstrators" on the PVE side, a user could create and join "System!Administrators" on the OIDC side and get mapped to the existing, probably privileged "System_Administrators" group..
this part now got split out into its own discussion:
https://lore.proxmox.com/pve-devel/b8fba9f6-6c83-4846-923f-2f7b93856bcf@proxmox.com/T/#u
what do you think about the following to not keep this blocked longer:
- rebase this series
- drop the name mangling/.. part for now, and only allow groups that
work with the PVE constraints for the time being
we can implement it when we've decided how to handle the name
mangling/collision/.. issue, and ensure we get a consistent
implementation for both LDAP/AD and OIDC.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* [pve-devel] [PATCH manager v3 1/1] fix #4411: openid: add ui config for openid groups support
2025-02-11 5:40 [pve-devel] [PATCH SERIES access-control/docs/manager/proxmox-openid v3] fix #4411: add support for openid groups Thomas Skinner
` (2 preceding siblings ...)
2025-02-11 5:40 ` [pve-devel] [PATCH access-control v3 1/1] fix #4411: openid: add logic for openid groups support Thomas Skinner
@ 2025-02-11 5:40 ` Thomas Skinner
2025-02-12 14:47 ` [pve-devel] [PATCH SERIES access-control/docs/manager/proxmox-openid v3] fix #4411: add support for openid groups Mira Limbeck
4 siblings, 0 replies; 9+ messages in thread
From: Thomas Skinner @ 2025-02-11 5:40 UTC (permalink / raw)
To: pve-devel; +Cc: 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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [pve-devel] [PATCH SERIES access-control/docs/manager/proxmox-openid v3] fix #4411: add support for openid groups
2025-02-11 5:40 [pve-devel] [PATCH SERIES access-control/docs/manager/proxmox-openid v3] fix #4411: add support for openid groups Thomas Skinner
` (3 preceding siblings ...)
2025-02-11 5:40 ` [pve-devel] [PATCH manager v3 1/1] fix #4411: openid: add ui config " Thomas Skinner
@ 2025-02-12 14:47 ` Mira Limbeck
4 siblings, 0 replies; 9+ messages in thread
From: Mira Limbeck @ 2025-02-12 14:47 UTC (permalink / raw)
To: pve-devel
On 2/11/25 06:40, Thomas Skinner wrote:
> Continued work on adding support for OIDC groups.
>
> changes since v2:
> - Move RE for group name characters to Plugin.pm
> - Undo refactoring of user group deletion
> - Refactor logic to use hashes instead of arrays
> - Cleanup code style
> - Add RE and length limit for group claim
> - Clarify docs on suffix and automatic group creation
>
>
> access-control:
>
> Thomas Skinner (1):
> fix #4411: openid: add logic for openid groups support
>
> src/PVE/API2/OpenId.pm | 79 ++++++++++++++++++++++++++++++++++++++++
> src/PVE/AccessControl.pm | 2 +-
> src/PVE/Auth/OpenId.pm | 33 +++++++++++++++++
> src/PVE/Auth/Plugin.pm | 1 +
> 4 files changed, 114 insertions(+), 1 deletion(-)
>
>
> docs:
>
> Thomas Skinner (1):
> fix #4411: openid: add docs for openid groups support
>
> pveum.adoc | 44 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 44 insertions(+)
>
>
> manager:
>
> Thomas Skinner (1):
> fix #4411: openid: add ui config for openid groups support
>
> www/manager6/dc/AuthEditOpenId.js | 44 ++++++++++++++++++++++++++++---
>
>
> proxmox-openid:
>
> Thomas Skinner (1):
> fix #4411: openid: add library code for generic id token claim support
>
> proxmox-openid/src/lib.rs | 55 +++++++++++++++++++++++++++++++++------
>
>
Tested this with Authentik for now. Logging looks good when groups are
created and when users have groups removed and assigned again.
It could be nice to also log when groups are renamed because of invalid
characters that are replaced?
Group claim, adding and overwriting groups looks good.
One test group was renamed because of a `!` in its name. When changing
the replacement character it created a new group and the old one still
existed. So you can end up with lots of leftover groups if you change
the replacement character later on.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 9+ messages in thread